-
Notifications
You must be signed in to change notification settings - Fork 89
refactor: [4/4] unify get_account_details and get_account_proof[s] into get_account
#1385
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.
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
Conversation
get_account_details and get_account_proof[s]get_account_details and get_account_proof[s]
|
@igamigo could you weigh in on
|
At least a very simple version of replacing |
7e654d4 to
1755724
Compare
69e1952 to
977e30f
Compare
igamigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left a few comments, mostly nits. There is only one comment related to the functionality of the network monitor that I'm wondering about.
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left just a coupe of small comments inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not for this PR, but I'd try to break up this file into a couple of smaller files.
…to bernhard-partial-storage-map-queries
…fy-get-details-and-get-proofs
…tails-and-get-proofs
igamigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Unify API calls
The new API is flexible enough to address all needs and avoid loading a lot of data from the DB.
Main changes in:
crates/proto/src/domain/account.rs:AccountProofRequest→AccountRequest,AccountProofResponse→AccountResponseGetAccountDetails*Caveat
Will require client changes!
Part 2
This is part 1 of two pieces. The second piece is in and does the actual DB changes, as well as populate the
SmtForest. This is NOT part of this PR.Open Questions
Do we want compatibility on the RPC level to give the client some more migration time, or is the change bounded enough?
Part of #1349
Part 1 of 2 of the remaining pieces in #1185