-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat(rpc): SyncNotes
endpoint
#424
Conversation
94b1475
to
bd05f7c
Compare
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 some comments inline.
Overall, I think this looks right - but there are two questions I have and probably @igamigo and @tomyrd are the ones best positioned to answer them:
- Do we need to include the
transactions
field in the response (similar to how we have it for state sync). It seems to me that we don't - but I also haven't thought it through completely. - Would the client be able to use the MMR info returned from this endpoint to update its MMR correctly? On the client side, this will probably work somewhat differently from how state sync works because this endpoint would be invoked at the time when the client's MMR is "in the future" compared to the requested data.
69d1f9d
to
0626e76
Compare
I don't think this is necessary.
After reviewing it a little bit, I think returning a Merkle path here is better and makes for a cleaner handling of the response on the client side. More so if we assume the client is going to call the endpoint after having synced past the block of the note's creation. |
Yes - I came to a similar conclusion. Basically, we just need the |
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 a few more comments inline.
0681702
to
2eba60d
Compare
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! I agree with what @igamigo said, we will need to test it in the client but I think it will work.
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 a few more minor comments inline. After these are addressed we can merge.
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.
All looks good! Thank you!
Closes #409.
The implementation is essentially a clone of SyncState with some parts left off. Which makes me believe I've misunderstood the assignment :)