[api] balance api #15755

lightmark · 2025-01-16T15:23:51Z

create endpoint /accounts/{ADDRESS}/balance/{TYPE_TAG | ADDRESS} to return the total balance of migrated coin or pure FA.

trunk-io · 2025-01-16T15:23:56Z

⏱️ 2h 49m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 12m	🟩 🟥 🟥 🟥 🟩
check-dynamic-deps	18m	🟩 🟩 🟩 🟩 🟩 (+1 more)
execution-performance / test-target-determinator	15m	🟩 🟩 🟩
test-target-determinator	14m	🟩 🟩 🟩
forge-e2e-test / forge	14m	🟩
rust-cargo-deny	9m	🟩 🟩 🟩 🟩 🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
fetch-last-released-docker-image-tag	5m	🟩 🟩 🟩
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)
general-lints	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	1m	🟩 ⬜ 🟩 🟩 🟩 (+1 more)
file_change_determinator	35s	🟩 🟩 🟩
permission-check	19s	🟩 🟩 🟩 🟩 🟩 (+1 more)

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
check-dynamic-deps	4m	2m
execution-performance / single-node-performance	25m	15m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

gregnazario

Please test with local testnet

gregnazario · 2025-01-16T15:26:18Z

api/src/accounts.rs

+    pub fn balance(&self, asset_type: AssetType, accept_type: &AcceptType) -> BasicResultWith404<u64> {
+        let (fa_metadata_address, mut balance) = match asset_type {
+            AssetType::Coin(move_struct_tag) => {
+                let coin_store_type_tag = StructTag::from_str(&format!("0x1::coin::CoinStore<{}>", move_struct_tag.to_string())).map_err(|err| {


Do you want to validate the move struct type first?

Ensure that we're not getting balances of say u8?

Also, ensure it's limited in parsing

api/src/accounts.rs

gregnazario · 2025-01-16T15:29:14Z

api/types/src/account.rs

+impl FromStr for AssetType {
+    type Err = anyhow::Error;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match Address::from_str(s) {
+            Ok(address) => Ok(AssetType::FungibleAsset(address)),
+            Err(_) => {
+                match MoveStructTag::from_str(s) {
+                    Ok(tag) => Ok(AssetType::Coin(tag)),
+                    Err(e) => Err(e),
+                }
+            }
+        }
+    }
+}


Oh, I see, you do one then the other

yes, I do FA first as it is shorter and we will migrate mostly eventually.

gregnazario · 2025-01-16T15:31:27Z

api/src/accounts.rs

+        let primary_fungible_store_address = get_paired_fa_primary_store_address(self.address.into(), fa_metadata_address);
+        if let Some(data_blob) = self.context.get_state_value_poem(&StateKey::resource_group(&primary_fungible_store_address, &ObjectGroupResource::struct_tag()), self.ledger_version, &self.latest_ledger_info)? {
+            if let Some(object_group) = bcs::from_bytes::<ObjectGroupResource>(&data_blob).ok() {
+                if let Some(fa_store) = object_group.group.get(&FungibleStoreResource::struct_tag()) {
+                    let fa_store_resource = bcs::from_bytes::<FungibleStoreResource>(fa_store).map_err(|err| {
+                        BasicErrorWith404::internal_with_code(
+                            err,
+                            AptosErrorCode::InternalError,
+                            &self.latest_ledger_info,
+                        )
+                    })?;
+                    if fa_store_resource.balance != 0 {
+                        balance += fa_store_resource.balance();
+                    } else if let Some(concurrent_fa_balance) = object_group.group.get(&ConcurrentFungibleBalanceResource::struct_tag()) {
+                        // query potential concurrent fa balance
+                        let concurrent_fa_balance_resource = bcs::from_bytes::<ConcurrentFungibleBalanceResource>(concurrent_fa_balance).map_err(|err| {
+                            BasicErrorWith404::internal_with_code(
+                                err,
+                                AptosErrorCode::InternalError,
+                                &self.latest_ledger_info,
+                            )
+                        })?;
+                        balance += concurrent_fa_balance_resource.balance();
+                    }


How's this compare in performance to the view function?

Also, if we're doing this... we probably want a supply query too

good question. it does not need move vm so I guess it should be much better.
Anyway to quickly test it?

I was just wondering if you had a comparison.

Otherwise, this should be much cheaper. I don't need you to prove it if you didn't compare.

graphite-app · 2025-01-16T23:46:46Z

api/src/accounts.rs

+                    coin_balance,
+                )
+            },
+            AssetType::FungibleAsset(fa_metadata_adddress) => (fa_metadata_adddress.into(), 0),


The variable fa_metadata_adddress contains a typo (extra 'd'). Please rename to fa_metadata_address to maintain consistent spelling throughout the codebase.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

github-actions · 2025-01-17T01:30:10Z

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `544ab77116d5e098e584fbe4d63ea5985527e90e`

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> 544ab77116d5e098e584fbe4d63ea5985527e90e (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 18007.62 txn/s, latency: 1946.02 ms, (p50: 2100 ms, p70: 2100, p90: 2200 ms, p99: 2700 ms), latency samples: 581020
2. Upgrading first Validator to new version: 544ab77116d5e098e584fbe4d63ea5985527e90e
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5283.88 txn/s, latency: 5779.99 ms, (p50: 6300 ms, p70: 7200, p90: 7400 ms, p99: 7500 ms), latency samples: 105660
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 5074.49 txn/s, latency: 6784.98 ms, (p50: 7400 ms, p70: 7500, p90: 7500 ms, p99: 7700 ms), latency samples: 178760
3. Upgrading rest of first batch to new version: 544ab77116d5e098e584fbe4d63ea5985527e90e
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 4006.20 txn/s, latency: 6698.96 ms, (p50: 7700 ms, p70: 10300, p90: 10600 ms, p99: 10700 ms), latency samples: 73100
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 4719.33 txn/s, latency: 7256.75 ms, (p50: 7900 ms, p70: 8000, p90: 8100 ms, p99: 8200 ms), latency samples: 171800
4. upgrading second batch to new version: 544ab77116d5e098e584fbe4d63ea5985527e90e
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 9293.03 txn/s, latency: 3314.06 ms, (p50: 3900 ms, p70: 4000, p90: 4100 ms, p99: 4200 ms), latency samples: 165360
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 9033.95 txn/s, latency: 3805.36 ms, (p50: 4100 ms, p70: 4200, p90: 4400 ms, p99: 4800 ms), latency samples: 296340
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> 544ab77116d5e098e584fbe4d63ea5985527e90e passed
Test Ok

github-actions · 2025-01-17T02:06:52Z

✅ Forge suite `realistic_env_max_load` success on `544ab77116d5e098e584fbe4d63ea5985527e90e`

two traffics test: inner traffic : committed: 14737.01 txn/s, latency: 2693.78 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5603320
two traffics test : committed: 99.96 txn/s, latency: 1386.21 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 2800 ms), latency samples: 1680
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.619, avg: 1.532", "ConsensusProposalToOrdered: max: 0.295, avg: 0.290", "ConsensusOrderedToCommit: max: 0.319, avg: 0.310", "ConsensusProposalToCommit: max: 0.610, avg: 0.601"]
Max non-epoch-change gap was: 1 rounds at version 28823 (avg 0.00) [limit 4], 1.94s no progress at version 28823 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.55s no progress at version 1639583 (avg 0.55s) [limit 16].
Test Ok

lightmark requested review from banool, gregnazario and 0xmaayan as code owners January 16, 2025 15:23

lightmark requested review from banool and removed request for banool January 16, 2025 15:23

gregnazario approved these changes Jan 16, 2025

View reviewed changes

rahxephon89 approved these changes Jan 16, 2025

View reviewed changes

lightmark force-pushed the lightmark/balance_api branch 2 times, most recently from d2a217b to 38d30b2 Compare January 16, 2025 23:46

lightmark enabled auto-merge (squash) January 16, 2025 23:46

graphite-app bot reviewed Jan 16, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

lightmark force-pushed the lightmark/balance_api branch from 38d30b2 to 55b8d69 Compare January 17, 2025 00:32

This comment has been minimized.

Sign in to view

[api] balance api

544ab77

lightmark force-pushed the lightmark/balance_api branch from 55b8d69 to 544ab77 Compare January 17, 2025 01:03

This comment has been minimized.

Sign in to view

lightmark merged commit f7cc847 into main Jan 17, 2025
43 of 46 checks passed

lightmark deleted the lightmark/balance_api branch January 17, 2025 02:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] balance api #15755

[api] balance api #15755

lightmark commented Jan 16, 2025

trunk-io bot commented Jan 16, 2025 •

edited

Loading

gregnazario left a comment

gregnazario Jan 16, 2025

gregnazario Jan 16, 2025

lightmark Jan 16, 2025

gregnazario Jan 16, 2025

lightmark Jan 16, 2025

gregnazario Jan 16, 2025

graphite-app bot Jan 16, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 17, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 17, 2025

[api] balance api #15755

[api] balance api #15755

Conversation

lightmark commented Jan 16, 2025

trunk-io bot commented Jan 16, 2025 • edited Loading

gregnazario left a comment

Choose a reason for hiding this comment

gregnazario Jan 16, 2025

Choose a reason for hiding this comment

gregnazario Jan 16, 2025

Choose a reason for hiding this comment

lightmark Jan 16, 2025

Choose a reason for hiding this comment

gregnazario Jan 16, 2025

Choose a reason for hiding this comment

lightmark Jan 16, 2025

Choose a reason for hiding this comment

gregnazario Jan 16, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 16, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 17, 2025

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> 544ab77116d5e098e584fbe4d63ea5985527e90e

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 17, 2025

✅ Forge suite realistic_env_max_load success on 544ab77116d5e098e584fbe4d63ea5985527e90e

trunk-io bot commented Jan 16, 2025 •

edited

Loading

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `544ab77116d5e098e584fbe4d63ea5985527e90e`

✅ Forge suite `realistic_env_max_load` success on `544ab77116d5e098e584fbe4d63ea5985527e90e`