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): Refactor broadcast_tx jsonrpc handler to add structured errors #4180

Merged
merged 9 commits into from
Apr 13, 2021
Merged

refactor(jsonrpc): Refactor broadcast_tx jsonrpc handler to add structured errors #4180

merged 9 commits into from
Apr 13, 2021

Conversation

khorolets
Copy link
Member

@khorolets khorolets commented Mar 30, 2021 •
edited
Loading

This PR introduces structured errors to JSON RPC methods which deals with transaction broadcasting:

  • broadcast_tx_async
  • broadcast_tx_commit
  • EXPERIMENTAL_broadcast_tx_sync
  • EXPERIMENTAL_check_tx
  • EXPERIMENTAL_tx_status
  • tx

@khorolets khorolets requested a review from frol March 30, 2021 17:10
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from 04a553d to c7c6766 Compare March 30, 2021 17:20
@khorolets khorolets marked this pull request as draft March 31, 2021 10:05
@khorolets
Copy link
Member Author

khorolets commented Mar 31, 2021 •
edited by frol
Loading

@vgrichina

We have methods:

  • EXPERIMENTAL_broadcast_tx_sync
  • EXPERIMENTAL_check_tx

I assume they were created for Wallet needs (#2525). Currently EXPERIMENTAL_check_tx returns OK with an empty body in case if everything is fine and error in other cases. EXPERIMENTAL_broadcast_tx_sync returns OK with tx hash and is_routed: bool in case if tx is valid and in case if tx was routed.

@frol and I suggest returning an error in case tx was routed as well because from an end-user perspective it was unable to check if tx is valid because it was routed and the user needs to check tx to ensure it is valid. Any objections if we throw an error there?

Another suggestion to return the same structure for EXPERIMENTAL_check_tx as we return in EXPERIMENTAL_broadcast_tx_sync instead of empty body.

/cc @bowenwang1996

@bowenwang1996
Copy link
Collaborator

@frol and I suggest returning an error in case tx was routed as well because from an end-user perspective it was unable to check if tx is valid because it was routed and the user needs to check tx to ensure it is valid. Any objections if we throw an error there?

Another suggestion to return the same structure for EXPERIMENTAL_check_tx as we return in EXPERIMENTAL_broadcast_tx_sync instead of empty body.

Sounds good to me

@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from 7a4eacb to 7a6ceed Compare April 1, 2021 13:17
@khorolets khorolets mentioned this pull request Apr 7, 2021
Merged
near-bulldozer bot pushed a commit that referenced this pull request Apr 7, 2021
@khorolets
test(jsonrpc): Cover JSON-RPC transaction related methods with tests …
f783789 
…for error messages (to watch for backward compatibility after refactor) (#4200)

I've refactored transaction related methods on JSON-RPC in this PR #4180

Here I've added tests to check that error messages are the same, so we can ensure that refactor is backward compatible. We need to merge this PR before #4180
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from 7a6ceed to 7bd09bc Compare April 8, 2021 08:56
@khorolets khorolets marked this pull request as ready for review April 8, 2021 09:58
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from 9b8e174 to f003ac4 Compare April 8, 2021 13:14
frol
frol approved these changes Apr 9, 2021
View reviewed changes
chain/jsonrpc-primitives/src/types/transactions.rs 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
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from f003ac4 to aba2a29 Compare April 11, 2021 08:45
frol
frol reviewed 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.

  1. I have minor suggestions in the comments
  2. I want to ask you to add notable changes to the chain/jsonrpc/CHANGELOG

chain/jsonrpc-primitives/src/types/transactions.rs Outdated Show resolved Hide resolved
chain/jsonrpc-primitives/src/types/transactions.rs Outdated Show resolved Hide resolved
@khorolets khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch from 35bd7af to 9d080c2 Compare April 12, 2021 17:20
frol
frol reviewed Apr 12, 2021
View reviewed changes
chain/jsonrpc-primitives/src/types/transactions.rs Outdated Show resolved Hide resolved
chain/jsonrpc/CHANGELOG.md Outdated Show resolved Hide resolved
chain/jsonrpc/CHANGELOG.md 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
@khorolets khorolets merged commit 83968b6 into master Apr 13, 2021
@khorolets khorolets deleted the refactor/structured-errors-view-client-jsonrpc-query-broadcast-tx branch April 13, 2021 14:14
olonho pushed a commit that referenced this pull request Apr 13, 2021
@khorolets @olonho
refactor(jsonrpc): Refactor broadcast_tx jsonrpc handler to add struc…
67dbcbb 
…tured errors (#4180)

* Refactor broadcast_tx jsonrpc handler to add structurred errors

* Remove wip comments

* Refactor tx_status_common, move impl From<NetworkClientResponses> to jsonrpc to avoid near_network dependency in jsonrpc-primitives

* Return error in case of NetworkClientResponses::RequestRouted, return same result on check_tx and send_tx_sync

* Make backward compatible error messages to refactored methods. Make corresponding tests pass

* Move backward compatibility to jsonrpc-primitives from jsonrpc, move ServerError from jsonrpc to jsonrpc-primitives, fix dependencies

* Address review suggestions

* Address review suggestions. Update jsonrpc changelog, bump version

* Address review suggestions
@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

@bowenwang1996 bowenwang1996 bowenwang1996 left review comments

@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.
3 participants
@khorolets @bowenwang1996 @frol