-
Notifications
You must be signed in to change notification settings - Fork 40
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: Return block authentication data with GetBlockHeaderByNum
#345
Conversation
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.
crates/proto/proto/responses.proto
Outdated
@@ -18,6 +18,7 @@ message CheckNullifiersResponse { | |||
|
|||
message GetBlockHeaderByNumberResponse { | |||
block_header.BlockHeader block_header = 1; | |||
merkle.MerklePath proof = 2; |
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.
Shouldn't proof
be an optional field? (based on the flag in the request).
Also, would returning just the Merkle proof be enough? Or should we include forest
value as well?
Lastly, let's add comments to the fields here. I'd like to improve inline documentation (to be similar to SyncStateResponse
documentation)
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.
Added the comments, and the forest
which I had not taken into account.
For the optional field based on the flag, I wasn't sure so I was going to address it later, but I ended up adding the flag in the request to include authentication data. I think defaulting to omitting it is the better option out of the two (versus including it by default) since it incurs in less overhead (if your estimation of 2-4KB is correct, it probably dominates the response size). I don't have much of a strong opinion either though, so I'm open to changing it. I am also not sure if the bool
in the request should be optional
to enable the scenario of not even including it in the request.
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 couple more 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.
Looks good! Thank you! I left some comments inline - all related to updating the use of forest
in docs/comments.
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 #256