Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrapped all ErrSendingRequest #409

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Wrapped all ErrSendingRequest #409

merged 5 commits into from
Nov 28, 2023

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Nov 23, 2023 •
edited
Loading

Wrapped all errors for more details on response, when all observers return error(previously, simply sending request error was returned).

Some testing scenarios:

  • /block/:shard/by-hash/:hash endpoint with an invalid hash for the provided shard. Same thing fot /block/:shard/by-nonce/:nonce with a huge nonce
  • /block/:shard/altered-accounts/by-hash/:hash endpoint with an invalid hash for the provided shard
  • /network/epoch-start/:shard/by-epoch/:epoch endpoint with a non existing epoch for the specified shard
  • or any other situation when all observers will return error
  • test as many routes as possible

@sstanculeanu sstanculeanu self-assigned this Nov 23, 2023
@ssd04 ssd04 self-requested a review November 24, 2023 13:01
ssd04
ssd04 reviewed Nov 24, 2023
View reviewed changes
process/nodeStatusProcessor.go Outdated
@@ -475,8 +475,8 @@ func (nsp *NodeStatusProcessor) GetGasConfigs() (*data.GenericAPIResponse, error
return nil, err
}

var responseGenesisNodesConfig data.GenericAPIResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to responseGasConfigs or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

process/nodeGroupProcessor.go Outdated
Comment on lines 90 to 91
return false, ErrSendingRequest
return false, fmt.Errorf("%w, %s", ErrSendingRequest, apiResponse.Error)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap also error in line 82?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

@bogdan-rosianu bogdan-rosianu self-requested a review November 24, 2023 14:04
bogdan-rosianu
bogdan-rosianu reviewed Nov 24, 2023
View reviewed changes
process/accountProcessor.go Outdated Show resolved Hide resolved
process/nodeStatusProcessor.go Outdated Show resolved Hide resolved
bogdan-rosianu
bogdan-rosianu reviewed Nov 27, 2023
View reviewed changes
process/economicMetrics.go Show resolved Hide resolved
process/nodeStatusProcessor.go Show resolved Hide resolved
process/nodeStatusProcessor.go Show resolved Hide resolved
process/nodeStatusProcessor.go Outdated Show resolved Hide resolved
process/nodeStatusProcessor.go Outdated Show resolved Hide resolved
process/nodeStatusProcessor.go Outdated Show resolved Hide resolved
@sstanculeanu sstanculeanu merged commit 202181b into rc/v1.7.0 Nov 28, 2023
4 checks passed
@sstanculeanu sstanculeanu deleted the wrap_ErrSendingRequest branch November 28, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssd04 ssd04 ssd04 approved these changes

@gabi-vuls gabi-vuls gabi-vuls approved these changes

@bogdan-rosianu bogdan-rosianu bogdan-rosianu approved these changes

Assignees

@sstanculeanu sstanculeanu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sstanculeanu @ssd04 @gabi-vuls @bogdan-rosianu