Change GetAccountProofs to a single GetAccountProof, cleanup#1211
Change GetAccountProofs to a single GetAccountProof, cleanup#1211
GetAccountProofs to a single GetAccountProof, cleanup#1211Conversation
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
Looks good overall, only one small comment for now (which you might have already been keeping in mind)
drahnr
force-pushed
the
bernhard-1185-acounts-part-1
branch
from
451ab1f to
0848b56
Compare
crates/store/src/server/rpc_api.rs
Outdated
| let proto::rpc_store::AccountProofRequest { | ||
| account_request, | ||
| include_headers, | ||
| code_commitments, | ||
| code_commitment, | ||
| } = request.into_inner(); | ||
|
|
||
| let include_headers = include_headers.unwrap_or_default(); | ||
| let request_code_commitments: BTreeSet<Word> = try_convert(code_commitments) | ||
| .collect::<Result<_, _>>() | ||
| .map_err(|err| Status::invalid_argument(format!("Invalid code commitment: {err}")))?; | ||
|
|
||
| let account_requests: Vec<AccountProofRequest> = | ||
| try_convert(account_requests).collect::<Result<_, _>>().map_err(|err| { | ||
| Status::invalid_argument(format!("Invalid account proofs request: {err}")) | ||
| })?; | ||
|
|
||
| let (block_num, infos) = self | ||
| let request_code_commitment = Word::try_from( | ||
| &(code_commitment.ok_or(Status::invalid_argument(stringify!(code_commitment)))?), | ||
| ) | ||
| .map_err(|err| Status::invalid_argument(format!("Invalid code commitment: {err}")))?; | ||
|
|
||
| let account_request = | ||
| account_request.ok_or(Status::invalid_argument(stringify!(account_request)))?; | ||
| let account_request = account_request.try_into().map_err(|err| { | ||
| Status::invalid_argument(format!("Invalid account proof request: {err}")) | ||
| })?; |
There was a problem hiding this comment.
If we update the request message as I mentioned in the other comment, I think the logic here and downstream would get simpler. Basically, we'd just need to pass account Id and an optional "account request" (maybe makes sense to rename it to "account details request").
drahnr
changed the title
GetAccountProofs -> GetAccountProofGetAccountProofs to a single GetAccountProof, cleanup
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - but they are pretty small.
| repeated StorageSlotMapProof storage_maps = 4; | ||
| message AccountProof { | ||
| // State header, available for public accounts only. | ||
| message AccountDetailsResponse { |
There was a problem hiding this comment.
Once we get rid of GetAccountDetails endpoint, we should rename AccountDetailsResponse into just AccountDetails.
There was a problem hiding this comment.
I think for clarity the *Response suffix does help, but it's also a bit weird if we'd use these message types in i.e. queries.
4 participants
Part one of #1185
If desired, can also change to adding rather than replacing the single account
GetAccountProofAPI