Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Looks good! I left a few comments inline. The main one is about returning account code only if include_headers is set to true.
crates/store/src/state.rs
Outdated
|
|
||
| let state_headers = if !include_headers { | ||
| BTreeMap::<AccountId, AccountStateHeader>::default() | ||
| BTreeMap::<u64, AccountStateHeader>::new() |
There was a problem hiding this comment.
Why was this change necessary?
There was a problem hiding this comment.
It wasn't, I thought I was rolling it back correctly but I wasn't (AccountId here is a newtype for u64)
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple more comments inline. Once they are addressed, we should be good to merge.
| // Account code commitments corresponding to the last-known `AccountCode` for requested | ||
| // accounts. Responses will include only the ones that are not known to the caller. | ||
| repeated digest.Digest code_commitments = 2; | ||
| // Optional flag to include header in the response. `false` by default. | ||
| optional bool include_headers = 2; | ||
| optional bool include_headers = 3; |
There was a problem hiding this comment.
nit: I would move include_headers above code_commitments.
Also, we should update comments to make it clear that account code only is included if include_headers is true, and also that it is not associated with a specific account ID (i.e., we check code roots against all accounts).
crates/store/src/server/api.rs
Outdated
| let request = request.into_inner(); | ||
| if request.account_ids.len() >= request.code_commitments.len() { | ||
| return Err(Status::invalid_argument( | ||
| "Amount of code commitments should not be larger than amount of requested accounts.", |
There was a problem hiding this comment.
nit: I would use "number" instead of "amount" here.
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
3 participants
Closes #520