Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few small comments inline.
One thing I'm still not sure of is whether GetAccountStates is the right name for this endpoint since we not actually getting account states from it. Maybe GetAccountProofs or GetAccountHeaders would be better?
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Looks good! I left one comment inline. Also, let's rename the endpoint to GetAccountProofs.
crates/rpc/Cargo.toml
Outdated
| directories = { version = "5.0" } | ||
| figment = { version = "0.10", features = ["toml", "env"] } | ||
| hex = { version = "0.4" } | ||
| miden-lib = { workspace = true} |
There was a problem hiding this comment.
Is this dependency still needed?
crates/store/src/state.rs
Outdated
| // because changing one of them would lead to inconsistent state. | ||
| let inner_state = self.inner.read().await; | ||
|
|
||
| let infos = self.db.select_accounts_by_ids(account_ids.iter().copied().collect()).await?; |
There was a problem hiding this comment.
We need to go to the database only if include_headers is true - right? Otherwise, all the info we need should be already in memory.
There was a problem hiding this comment.
Good catch, thank you!
polydez
changed the title
GetAccountStates endpointGetAccountProofs endpoint
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some small comments inline - once these are addressed, we can merge.
Also, our docs for various endpoints (in README files) are getting out of date. Let's create an issue to update them. In the same issue we can discuss if there is some automated way to generate endpoint docs for rpc/store/block producer components.
proto/responses.proto
Outdated
| /// Values of all account storage slots (max 255). | ||
| bytes storage_header = 2; |
There was a problem hiding this comment.
nit: I think this should be just // rather than ///.
proto/responses.proto
Outdated
| /// State header for public accounts. Filled only if `include_headers` flag is set to `true`. | ||
| optional AccountStateHeader state_header = 4; |
There was a problem hiding this comment.
Similar nit as the one above.
proto/responses.proto
Outdated
| // List of account state infos for the requested account keys. | ||
| repeated AccountProofsResponse account_state_infos = 2; |
There was a problem hiding this comment.
Should the name of the field be account_proofs or something like that? (not sure if state_infos has much meaning in this context).
crates/rpc/src/server/api.rs
Outdated
|
|
||
| #[instrument( | ||
| target = "miden-rpc", | ||
| name = "rpc:get_account_states", |
There was a problem hiding this comment.
This should also be changed to rpc:get_account_proofs.
crates/store/src/server/api.rs
Outdated
|
|
||
| #[instrument( | ||
| target = "miden-store", | ||
| name = "store:get_account_state", |
There was a problem hiding this comment.
This should also be changed to store:get_account_proofs.
| pub async fn get_account_states( | ||
| &self, | ||
| account_ids: HashSet<AccountId>, | ||
| include_headers: bool, | ||
| ) -> Result<(BlockNumber, Vec<AccountProofsResponse>), DatabaseError> { |
There was a problem hiding this comment.
Question: why pass account_ids as a HashSet rather than a simple Vec?
There was a problem hiding this comment.
I wanted to reduce number of conversions but now I think it was better to pass vector here. Thank you!
crates/store/src/state.rs
Outdated
| let inner_state = self.inner.read().await; | ||
|
|
||
| let state_headers = if !include_headers { | ||
| HashMap::<AccountId, AccountStateHeader>::default() |
There was a problem hiding this comment.
Is there a reason to use HashMap rather than BTreeMap here? I don't really have a strong preference, but we usually use BTreeMpas everywhere.
There was a problem hiding this comment.
I also don't have strong preference here. Taking that this map will be pretty small, it shouldn't be any significant difference between them, so I will use BTreeMap here, thanks!
# Conflicts: # CHANGELOG.md
3 participants
Resolves #485
This PR adds
GetAccountProofsendpoint which returns latest state headers for given accounts. For public accounts it also returns account and storage headers.After discussion we considered to lock whole database and in-memory state for the processing of the whole request.