Added support for on-chain (public) accounts in store #293
Conversation
This file contains 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
# Conflicts: # .gitignore # Cargo.toml
hackaugusto
approved these changes
Apr 1, 2024
polydez
force-pushed
the
polydez-onchain-accounts-store
branch
from
b1b29e5 to
5167ec6
Compare
polydez
force-pushed
the
polydez-onchain-accounts-store
branch
from
614485d to
54d3527
Compare
polydez
force-pushed
the
polydez-onchain-accounts-store
branch
from
54d3527 to
63160a2
Compare
Co-authored-by: Augusto Hack <[email protected]>
polydez
force-pushed
the
polydez-onchain-accounts-store
branch
from
76504e0 to
d6b02c8
Compare
# Conflicts: # Cargo.lock # Cargo.toml # block-producer/src/block.rs # block-producer/src/block_builder/mod.rs # block-producer/src/test_utils/block.rs # block-producer/src/test_utils/proven_tx.rs # faucet/Cargo.toml # proto/src/errors.rs # store/src/db/migrations.rs # store/src/db/sql.rs # store/src/state.rs
# Conflicts: # block-producer/src/block_builder/prover/block_witness.rs # block-producer/src/test_utils/block.rs
polydez
force-pushed
the
polydez-onchain-accounts-store
branch
from
d6b02c8 to
09987a8
Compare
bobbinth
approved these changes
Apr 6, 2024
store/src/db/sql.rs
Outdated
Comment on lines
89
to
93
| SELECT | ||
| account_id, account_hash, block_num | ||
| account_id, | ||
| account_hash, | ||
| block_num, | ||
| details |
There was a problem hiding this comment.
This is probably fine for now, but this function is used to build a sync_state response where account details are not needed. So, we are doing potentially quite a bit of extra work here by fetching details from the database and parsing them.
Let's create an issue to address this in the future.
There was a problem hiding this comment.
I refactored this, now there are two different structures (AccountHashUpdate is reused), so details aren't provided here anymore.
# Conflicts: # Cargo.lock # block-producer/src/block_builder/prover/tests.rs # block-producer/src/block_builder/tests.rs # block-producer/src/test_utils/account.rs # store/src/db/tests.rs
bobbinth
reviewed
Apr 6, 2024
Comment on lines
+13
to
22
| message AccountHashUpdate { | ||
| account.AccountId account_id = 1; | ||
| digest.Digest account_hash = 2; | ||
| fixed32 block_num = 3; | ||
| uint32 block_num = 3; | ||
| } | ||
|
|
||
| message AccountInfo { | ||
| AccountHashUpdate update = 1; | ||
| optional bytes details = 2; | ||
| } |
There was a problem hiding this comment.
account.AccountId can be just AccountId - right?
Also, I wonder if this is the best naming. Specifically, AccountHashUpdate seems a bit confusing. Maybe the following would be better?
message AccountSummary {
AccountId account_id = 1;
digest.Digest account_hash = 2;
uint32 block_num = 3;
}
message AccountDetails {
AccountSummary summary = 1;
optional bytes details = 2;
}Or we can even expand the details like so:
message AccountDetails {
AccountId account_id = 1;
digest.Digest account_hash = 2;
uint32 block_num = 3;
optional bytes details = 4;
}There was a problem hiding this comment.
We already have AccountDetails structure in miden-objects, maybe need to end up with different name.
Comment on lines
+73
to
+74
| * `account`: `AccountInfo` – account state information. For public accounts there is also details describing current state, stored on-chain; | ||
| for private accounts only hash of the latest known state is returned. |
There was a problem hiding this comment.
nit: I would write this as:
account:AccountDetails- latest state of the account. For public accounts, this will include full details describing the current account state. For private accounts, only the hash of the latest state and the time of the last update is returned.
Also, we usually don't do manual line breaks in markdown files.
Comment on lines
+119
to
+120
| * `account`: `AccountInfo` – account state information. For public accounts there is also details describing current state, stored on-chain; | ||
| for private accounts only hash of the latest known state is returned. |
bobbinth
pushed a commit
that referenced
this pull request
Apr 12, 2024
* feat: add `account_details` table to the DB * refactor: rename `block_number` column in nullifiers table to `block_num` * refactor: use `BETWEEN` in interval comparison checks * feat: implement account details protobuf messages, domain objects and conversions * feat: (WIP) implement account details support * feat: (WIP) implement account details support * feat: (WIP) implement account details support * feat: (WIP) implement account details support * fix: db creation * docs: remove TODO * refactor: apply formatting * feat: implement endpoint for getting public account details * tests: add test for storage * feat: add rpc endpoint for getting account details * refactor: keep only domain object changes * fix: compilation errors * fix: use note tag conversion from `u64` * refactor: remove account details protobuf messages * fix: remove unused error invariants * refactor: introduce `UpdatedAccount` struct * fix: rollback details conversion * fix: compilation error * feat: account details in store * refactor: add constraint name for foreign key * refactor: small code improvement Co-authored-by: Augusto Hack <[email protected]> * feat: account id validation * refactor: rename `get_account_details` to `select_*` * feat: return serialized account details * feat: add requirement of account id to be public in RPC * fix: remove error message used in different PR * fix: union account details with account and process them together * docs: remove `GetAccountDetails` from README.md * fix: remove unused error invariants * fix: use `Account` instead of `AccountDetails` in store * wip * feat: implement `GetAccountDetails` endpoint * docs: document `GetAccountDetails` endpoint * refactor: simplify code, make account details optional * fix: clippy warning * fix: address review comments * fix: update code to the latest miden-base * refactor: little code improvement --------- Co-authored-by: Augusto Hack <[email protected]>
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.
In this PR database structure updated and only read-only access is implemented. Inserting of account details into the database is implemented in the next PR with block applying.