-
Notifications
You must be signed in to change notification settings - Fork 86
fix: make get account proof retrieve latest known state #1422
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
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 but I was also involved in investigating a fix for this so ideally @drahnr can take a look as well
This reverts commit e8f74dd.
|
This PR as well as others are failing the MSRV check, attempted to fix it, to no avail. |
Yeah we have some speculation going on in #1418 but its unclear at the moment wtf is going on there :) |
Mirko-von-Leipzig
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.
I'll leave @drahnr to formally approve, but LGTM.
Some suggestions on returning to the original phrasing and removing the changelog entry.
proto/proto/store/rpc.proto
Outdated
| // If the specified block does not contain an update for the specified account, | ||
| // the latest available data will be returned. |
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.
I think this is a bit ambiguous and could mean that asking for block N could return data after N aka latest > N.
I think phrasing the entire comment as something like
// Optional block height at which to return the proof.
//
// Defaults to current chain tip if unspecified.
Co-authored-by: Mirko <[email protected]>
|
The change is sound, thank you! Note that #1394 will refactor the accounts table fully and introduce I'd hence ask to undo all the documentation comments, to minimize my merge + rebase pains 🫠 |
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 think we can ignore the MSRV check for this PR. I'll merge it as is.
As observed while updating the client with the latest node version, retrieval of account proof is prone to failure when the block number used for the request is different from the block where the last account update happened, as it returns a not found in those cases.
This PRs lighly refactors the logic of proof retrieval to return the account proof of an account up to the specified block.