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

refactor(jsonrpc): Structured errors from other JSON-RPC methods #4213

Merged
merged 18 commits into from
Apr 19, 2021
Merged

refactor(jsonrpc): Structured errors from other JSON-RPC methods #4213

merged 18 commits into from
Apr 19, 2021

Conversation

khorolets
Copy link
Member

@khorolets khorolets commented Apr 12, 2021 •
edited
Loading

  • status
  • health
  • EXPERIMENTAL_changes_in_block
  • EXPERIMENTAL_changes
  • EXPERIMENTAL_light_client_proof
  • EXPERIMENTAL_validators_ordered
  • light_client_proof

@khorolets khorolets requested a review from frol April 12, 2021 15:55
frol
frol approved these changes Apr 12, 2021
View reviewed changes
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Great job! Just a few notes on where we can improve even further.

chain/client-primitives/src/types.rs Outdated Show resolved Hide resolved
chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-rest-methods branch from 2097b89 to d116b09 Compare April 12, 2021 18:13
matklad
matklad reviewed Apr 13, 2021
View reviewed changes
chain/jsonrpc-primitives/src/errors.rs Outdated Show resolved Hide resolved
chain/jsonrpc-primitives/src/types/transactions.rs Outdated Show resolved Hide resolved
chain/jsonrpc-primitives/src/types/transactions.rs Outdated Show resolved Hide resolved
chain/jsonrpc-primitives/src/utils.rs Outdated Show resolved Hide resolved
chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved
chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved
neard/tests/rpc_nodes.rs Outdated Show resolved Hide resolved
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-rest-methods branch 2 times, most recently from d4e26e0 to ebe9e41 Compare April 14, 2021 13:29
@khorolets
Copy link
Member Author

@frol I've created RpcError::serialization_error method which created "Server error" and replaced usages of parse_error in handlers. Could you have a look and say about client's call_method method, should I replace usages of parse_error there as well?

@matklad appreciate your suggestions!

@khorolets khorolets marked this pull request as ready for review April 14, 2021 13:32
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-rest-methods branch from 81a2969 to 8b93f62 Compare April 16, 2021 07:58
frol
frol approved these changes Apr 16, 2021
View reviewed changes
Copy link
Collaborator

@frol frol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yay! 🎉

We did it! (For those who follows this: this is the last PR in the series of internal JSON RPC error handling refactoring, and the next step would be to expose those errors through JSON RPC with exhaustive and easy-to-reason error kinds)

@frol
Copy link
Collaborator

frol commented Apr 16, 2021

@bowenwang1996 your review is required, FYI

bowenwang1996
bowenwang1996 reviewed Apr 17, 2021
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Please run nayduck to make sure that there is no regression

@khorolets
Copy link
Member Author

@bowenwang1996 does it look ok http://nayduck.eastus.cloudapp.azure.com:3000/#/run/1521 ?

khorolets added 11 commits April 19, 2021 18:52
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-rest-methods branch from 368c9c4 to d54eb0f Compare April 19, 2021 16:18
@near-bulldozer near-bulldozer bot merged commit b335779 into master Apr 19, 2021
@near-bulldozer near-bulldozer bot deleted the refactor/structured-errors-view-client-jsonrpc-rest-methods branch April 19, 2021 19:43
@frol frol mentioned this pull request May 1, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

@bowenwang1996 bowenwang1996 bowenwang1996 approved these changes

@frol frol frol approved these changes

@chefsale chefsale Awaiting requested review from chefsale

@evgenykuzyakov evgenykuzyakov Awaiting requested review from evgenykuzyakov

@MaksymZavershynskyi MaksymZavershynskyi Awaiting requested review from MaksymZavershynskyi

@SkidanovAlex SkidanovAlex Awaiting requested review from SkidanovAlex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khorolets @frol @matklad @bowenwang1996