refactor: [4/4] unify get_account_details and get_account_proof[s] into get_account
#1385
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drahnr
changed the title
get_account_details and get_account_proof[s]get_account_details and get_account_proof[s]
Collaborator
|
@igamigo could you weigh in on
|
Collaborator
At least a very simple version of replacing |
bobbinth
reviewed
Nov 29, 2025
bobbinth
reviewed
Nov 29, 2025
drahnr
force-pushed
the
bernhard-unify-get-details-and-get-proofs
branch
from
7e654d4 to
1755724
Compare
drahnr
force-pushed
the
bernhard-unify-get-details-and-get-proofs
branch
from
69e1952 to
977e30f
Compare
igamigo
reviewed
Dec 2, 2025
Collaborator
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM! Left a few comments, mostly nits. There is only one comment related to the functionality of the network monitor that I'm wondering about.
3 tasks
The fetch_wallet_account function was always returning None since the RPC API only returns commitments, not full account data. This made it completely useless - the code immediately replaced None with the file-based account using .unwrap_or(). Removed the dead code and simplified setup_increment_task to directly use the file-based account, which is what was happening anyway.
The previous commit incorrectly deleted the fetch_wallet_account function. This commit properly implements it to: 1. Request account details from RPC (including the header) 2. Extract the AccountHeader with the latest nonce 3. Reconstruct the wallet account with: - Code, vault, storage from the file-based account - Updated nonce from the chain 4. Fall back to file-based account if RPC fails or account not found This ensures the wallet account nonce stays synced with on-chain state, preventing nonce conflicts in transaction submission.
drahnr
force-pushed
the
bernhard-unify-get-details-and-get-proofs
branch
from
72d09a5 to
f000d5d
Compare
drahnr
changed the title
get_account_details and get_account_proof[s]get_account_details and get_account_proof[s]
drahnr
changed the base branch from
next
to
bernhard-partial-storage-map-queries
drahnr
changed the title
get_account_details and get_account_proof[s]get_account_details and get_account_proof[s] into get_account
bobbinth
reviewed
Dec 28, 2025
Contributor
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - most are pretty minor.
proto/proto/rpc.proto
Outdated
Comment on lines
33
to
34
| // Returns the latest state of an account with the specified ID. | ||
| rpc GetAccountDetails(account.AccountId) returns (account.AccountDetails) {} |
Contributor
There was a problem hiding this comment.
I thought we were removing GetAccountDetails as a part of this PR - but maybe I misunderstood?
Comment on lines
37
to
+38
| // Returns the latest state proof of the specified account. | ||
| rpc GetAccountProof(rpc.AccountProofRequest) returns (rpc.AccountProofResponse) {} | ||
| rpc GetAccount(rpc.AccountRequest) returns (rpc.AccountResponse) {} |
bin/network-monitor/src/counter.rs
Outdated
Comment on lines
295
to
305
| let entries = map_entries.get(&slot.slot_name).cloned().unwrap_or_default(); | ||
| if entries.is_empty() { | ||
| warn!( | ||
| account.id = %account_id, | ||
| slot_name = %slot.slot_name, | ||
| "storage map slot has no entries (not requested or empty)" | ||
| ); | ||
| } | ||
| let storage_map = miden_objects::account::StorageMap::with_entries(entries) | ||
| .context("failed to create storage map")?; | ||
| StorageSlot::with_map(slot_name, storage_map) |
Contributor
There was a problem hiding this comment.
Similar to the above comment: I think we can probably just error out if there are any storage maps in the account.
drahnr
changed the title
get_account_details and get_account_proof[s] into get_accountget_account_details and get_account_proof[s] into get_account
…fy-get-details-and-get-proofs
…fy-get-details-and-get-proofs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unify API calls
The new API is flexible enough to address all needs and avoid loading a lot of data from the DB.
Main changes in:
crates/proto/src/domain/account.rs:AccountProofRequest→AccountRequest,AccountProofResponse→AccountResponseGetAccountDetails*Caveat
Will require client changes!
Part 2
This is part 1 of two pieces. The second piece is in and does the actual DB changes, as well as populate the
SmtForest. This is NOT part of this PR.Open Questions
Do we want compatibility on the RPC level to give the client some more migration time, or is the change bounded enough?
Part of #1349
Part 1 of 2 of the remaining pieces in #1185