feat: Add account details endpoint #248

mFragaBA · 2024-04-09T19:04:20Z

This PR introduces a few things:

Adds a new param to the CLI for the account new command, --storage-mode=onchain,offchain(default) that enables the creation of onchain accounts
Adds to the node rpc client a get_account_updates trait function. This function takes an account ID and fetches the GetAccountDetails endpoint on the node added on #294 from miden-node (and previous PRs)
Updates the sync behavior
- When doing a sync we check the account updates in two ways (always doing so for tracked accounts).
  - First, validating that the updated offchain account hashes match the ones of our client
  - Second, checking the updated onchain accounts, if the hash does not match ours that means it's outdated and thus we'll use the get_account_updates function to fetch the most up to date account

Now during the state sync we check the updated account hashes for onchain accoints. if they don't match, then we fetch the current state from the node. The old validation for offchain accounts is left unchanged but I refactored it so the logic looks similar to the one for onchain accoints. I also left a few TODO's and panics that'll be removed in follow-up commits.

igamigo

Left some comments for now, please disregard if you were already planning changes for any of them (since it's still a draft)

src/client/sync.rs

src/store/sqlite_store/accounts.rs

src/store/sqlite_store/mod.rs

src/client/sync.rs

igamigo · 2024-04-10T17:43:30Z

src/client/sync.rs

+        let updated_onchain_accounts = self
+            .get_updated_onchain_accounts(&response.account_hash_updates, &onchain_accounts)
+            .await?;
+        self.check_local_account_hashes(&response.account_hash_updates, &offchain_accounts)


Let's take the opportunity of the refactor to rename this to validate_local_account_hashes

Also, I would leave the comment that was taken out (or something similar that takes your changes into account)

src/client/sync.rs

clippy.toml

src/store/sqlite_store/sync.rs

src/client/sync.rs

…response

…ients

igamigo

LGTM. ~~Fixed the CI issue I believe~~ seems I didn't. I left some comments which should be simple to address, but in any case they can be deferred to a follow up PR or issue.

There are also a couple of TODOs that need addressing but I'm guessing those were planned

igamigo · 2024-04-10T22:47:48Z

src/cli/account.rs

-    NonFungibleFaucet,
+    NonFungibleFaucet {
+        #[clap(short, long, value_enum, default_value_t = AccountStorageMode::OffChain)]
+        storage_mode: AccountStorageMode,


I think we can add this to the AccountCmd::New since it's common to all variants.

igamigo · 2024-04-10T22:48:30Z

src/cli/account.rs

+pub enum AccountStorageMode {
+    OffChain,
+    OnChain,
+}
+
+impl From<AccountStorageMode> for accounts::AccountStorageMode {
+    fn from(value: AccountStorageMode) -> Self {
+        match value {
+            AccountStorageMode::OffChain => accounts::AccountStorageMode::Local,
+            AccountStorageMode::OnChain => accounts::AccountStorageMode::OnChain,
+        }
+    }
+}
+
+impl From<&AccountStorageMode> for accounts::AccountStorageMode {
+    fn from(value: &AccountStorageMode) -> Self {
+        accounts::AccountStorageMode::from(*value)
+    }


Can we add a TODO to review these struct's names (and maybe the local vs offchain terms)?

yes! added it here and in the client/accounts.rs one

igamigo · 2024-04-10T22:50:30Z

src/client/rpc/tonic_client.rs

+        &mut self,
+        account_id: AccountId,
+    ) -> Result<Option<Account>, NodeRpcClientError> {
+        debug_assert!(account_id.is_on_chain());


This should probably be an error instead of a panic

igamigo · 2024-04-10T22:53:02Z

src/client/rpc/mod.rs

+    async fn get_account_update(
+        &mut self,
+        account_id: AccountId,
+    ) -> Result<Option<Account>, NodeRpcClientError>;


It seems that the return type should be Account instead of Option<Account> for this. I believe the node will return an error if the account is not found so an Account is always expected.

I went for Option since it's what the response comes with so I expected that there will eventually be some use for the summary but no details case. In any way we can take another look at it in the future and keep it simple for now

* feat: add GetAccountDetails endpoint to tonic rpc client * feat: update CLI to provide account storage mode as cli args * feat: handle updated onchain accounts during sync and refactor Now during the state sync we check the updated account hashes for onchain accoints. if they don't match, then we fetch the current state from the node. The old validation for offchain accounts is left unchanged but I refactored it so the logic looks similar to the one for onchain accoints. I also left a few TODO's and panics that'll be removed in follow-up commits. * use updated create_basic_wallet and faucet * use next for miden node proto * update order of create_basic_fungible_faucet * add small integration test for onchain accounts transactions * address comments * add missing sync * fix: read Account instead of AccountDetails from get_account_details response * remove dbg statement * re-enable other integration tests * extend mint integration test to reuse same on-chain account in two clients * Test fix * Test fix * Test fix * Update Makefile.toml * address review * extend onchain integration test to do transfer between accounts * update missing documentation * fix test * Follow ups * Stop test execution before transfer We stop the onchain accounts test before running the p2id transaction because there's an error on how inputs are inserted/extracted on the vm, so until that gets fixed we'll leave the test code but do an early return so the test still passes. * add rpc endpoints to trait functions' documentation * split test helper in two functions * fix tests * Update main.rs * dep version * re-add assert * remove unnecessary sleeps --------- Co-authored-by: Ignacio Amigo <ignacio.amigo@lambdaclass.com>

* Use next branches * Fix integration test build (still doesnt work accordingly) * Fix advice inputs? * Fix and fmt * Update script * Update script properly * fix integration test * point to next instead of custom branch * fix falcon seed size * update note script root * temp: clone the node on next branch * run tests in parallel * Fix path creation * Fix tests and small refactors * test * Fix tests's account id * Several refactors after updates * Tests fix * Mint script fix * Fix integration * Make compatible with newer ref * Fix test * feat: Add account details endpoint (#248) * feat: add GetAccountDetails endpoint to tonic rpc client * feat: update CLI to provide account storage mode as cli args * feat: handle updated onchain accounts during sync and refactor Now during the state sync we check the updated account hashes for onchain accoints. if they don't match, then we fetch the current state from the node. The old validation for offchain accounts is left unchanged but I refactored it so the logic looks similar to the one for onchain accoints. I also left a few TODO's and panics that'll be removed in follow-up commits. * use updated create_basic_wallet and faucet * use next for miden node proto * update order of create_basic_fungible_faucet * add small integration test for onchain accounts transactions * address comments * add missing sync * fix: read Account instead of AccountDetails from get_account_details response * remove dbg statement * re-enable other integration tests * extend mint integration test to reuse same on-chain account in two clients * Test fix * Test fix * Test fix * Update Makefile.toml * address review * fix compilation errors * add onchain accounts to docs --------- Co-authored-by: Ignacio Amigo <ignacio.amigo@lambdaclass.com> * feat: Implement `GetNotesById` RPC endpoint (#247) * On-chain note support. * Merge conflicts * Fixes * Update main.rs * Update sync.rs * Use correct dependencies * Remove underscore * Follow ups * Docs and fixes * doc&refactor: missing changes from #248 (#249) * feat: add GetAccountDetails endpoint to tonic rpc client * feat: update CLI to provide account storage mode as cli args * feat: handle updated onchain accounts during sync and refactor Now during the state sync we check the updated account hashes for onchain accoints. if they don't match, then we fetch the current state from the node. The old validation for offchain accounts is left unchanged but I refactored it so the logic looks similar to the one for onchain accoints. I also left a few TODO's and panics that'll be removed in follow-up commits. * use updated create_basic_wallet and faucet * use next for miden node proto * update order of create_basic_fungible_faucet * add small integration test for onchain accounts transactions * address comments * add missing sync * fix: read Account instead of AccountDetails from get_account_details response * remove dbg statement * re-enable other integration tests * extend mint integration test to reuse same on-chain account in two clients * Test fix * Test fix * Test fix * Update Makefile.toml * address review * extend onchain integration test to do transfer between accounts * update missing documentation * fix test * Follow ups * Stop test execution before transfer We stop the onchain accounts test before running the p2id transaction because there's an error on how inputs are inserted/extracted on the vm, so until that gets fixed we'll leave the test code but do an early return so the test still passes. * add rpc endpoints to trait functions' documentation * split test helper in two functions * fix tests * Update main.rs * dep version * re-add assert * remove unnecessary sleeps --------- Co-authored-by: Ignacio Amigo <ignacio.amigo@lambdaclass.com> * Cargo toml * Address reviews --------- Co-authored-by: Martin Fraga <martin.fraga@lambdaclass.com> Co-authored-by: Martin Fraga <fragamartine@gmail.com>

mFragaBA force-pushed the mFragaBA-add-account-details-endpoint branch 2 times, most recently from 27288fc to eaa7dd6 Compare April 9, 2024 20:34

mFragaBA added 6 commits April 10, 2024 14:22

feat: add GetAccountDetails endpoint to tonic rpc client

0625429

feat: update CLI to provide account storage mode as cli args

7bd407e

use updated create_basic_wallet and faucet

d29cdbd

use next for miden node proto

e4f7c2c

update order of create_basic_fungible_faucet

ba06e93

igamigo reviewed Apr 10, 2024

View reviewed changes

add small integration test for onchain accounts transactions

8d9c31d

mFragaBA force-pushed the mFragaBA-add-account-details-endpoint branch from eaa7dd6 to 8d9c31d Compare April 10, 2024 18:08

igamigo reviewed Apr 10, 2024

View reviewed changes

src/client/sync.rs Outdated Show resolved Hide resolved

mFragaBA and others added 8 commits April 10, 2024 15:30

address comments

738c342

add missing sync

c5be252

fix: read Account instead of AccountDetails from get_account_details …

6eceecb

…response

remove dbg statement

bae1f24

re-enable other integration tests

a5d0f68

extend mint integration test to reuse same on-chain account in two cl…

0bc83e6

…ients

Test fix

15f4eb6

Test fix

b65a8fa

igamigo changed the title ~~add account details endpoint~~ feat: Add account details endpoint Apr 10, 2024

igamigo marked this pull request as ready for review April 10, 2024 22:43

igamigo approved these changes Apr 10, 2024

View reviewed changes

igamigo and others added 5 commits April 10, 2024 20:15

Test fix

8a2f2a9

Update Makefile.toml

d60aa9b

address review

fc6ce6a

fix compilation errors

d8b6f97

add onchain accounts to docs

35be1fc

igamigo merged commit e298357 into igamigo-use-next Apr 11, 2024
4 of 5 checks passed

igamigo deleted the mFragaBA-add-account-details-endpoint branch April 11, 2024 14:44

This was referenced Apr 11, 2024

feat: Support on-chain entities (accounts and notes) #245

Merged

doc&refactor: missing changes from #248 #249

Merged

mFragaBA mentioned this pull request Apr 11, 2024

Add support for GetAccountDetails rpc endpoint for off-chain accounts #251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add account details endpoint #248

feat: Add account details endpoint #248

mFragaBA commented Apr 9, 2024 •

edited

Loading

igamigo left a comment

igamigo Apr 10, 2024

igamigo Apr 10, 2024

igamigo left a comment •

edited

Loading

igamigo Apr 10, 2024

igamigo Apr 10, 2024

mFragaBA Apr 11, 2024

igamigo Apr 10, 2024

mFragaBA Apr 11, 2024

igamigo Apr 10, 2024

mFragaBA Apr 11, 2024

feat: Add account details endpoint #248

feat: Add account details endpoint #248

Conversation

mFragaBA commented Apr 9, 2024 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mFragaBA commented Apr 9, 2024 •

edited

Loading

igamigo left a comment •

edited

Loading