[rosetta] Fix staking balances #15247

gregnazario · 2024-11-09T01:30:19Z

Description

This cleans up and ensures that Rosetta only returns either the base account balances, the staking balances, or the delegated staking balances. It will never return all three at the same time.

How Has This Been Tested?

Tested against existing accounts on mainnet with a local version. Prior version would return the account's balance and stake together, when requesting stake.

Now it only returns the balance

curl -s -X POST http://localhost:8082/account/balance -H 'Content-Type: application/json' -d '{ "network_identifier": { "blockchain": "aptos", "network": "mainnet" }, "account_identifier": { "address": "75d9ef170a1e8c4215084da639d626d75dfd8a6405e91928261160ed66aa748e", "sub_account": {"address":"stake" }}, "metadata": {} }' | jq
{
  "block_identifier": {
    "index": 249813200,
    "hash": "mainnet-249813200"
  },
  "balances": [
    {
      "value": "888062873502462",
      "currency": {
        "symbol": "APT",
        "decimals": 8,
        "metadata": {
          "move_type": "0x1::aptos_coin::AptosCoin"
        }
      }
    }
  ],
  "metadata": {
    "sequence_number": "50",
    "operators": [
      "e6a5cbbb9754a54af17909352ba96d2b46b379037a4bc94f6530596aaf5f2a13"
    ],
    "lockup_expiration_time_utc": "1731575845"
  }
}

Key Areas to Review

Logically, this should be the same as before. The main issue was the fallthrough behavior that would handle the base account path on every path. Instead it should only be handled on the base path, where staking should do the staking path.

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-11-09T01:30:21Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-smoke-tests	27m	🟩
test-target-determinator	14m	🟩 🟩 ⬜ 🟩
execution-performance / test-target-determinator	13m	🟩 🟩 🟩
check	11m	🟩 🟩 🟩
fetch-last-released-docker-image-tag	10m	🟩 🟩 ⬜ 🟩
rust-cargo-deny	8m	🟩 🟩 🟩 ⬜ 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩
rust-targeted-unit-tests	6m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	⬜ 🟩
rust-lints	4m	🟩
rust-build-cached-packages	2m	🟩
rust-check-merge-base	2m	🟩
general-lints	2m	🟩 🟩 🟩 ⬜ 🟩
rust-move-tests	2m	⬜ 🟩

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

GhostWalker562

Looks good, the only logical change I see is the if statements for base, pool staking, and account staking as stated in the description.

banool · 2024-11-12T21:27:22Z

crates/aptos-rosetta/src/account.rs

+                Ok(Some(balance_result)) => {
+                    if let Some(balance) = balance_result.balance {
+                        has_staking = true;
+                        total_balance += u64::from_str(&balance.value).unwrap_or_default();


Just to confirm, this isn't a u128?

User balance is a u64, but I can just change them all to u128 since they're being converted to and from strings

Actually nevermind it will always be a u64

the last refactor made it get staking balances and the account's balance this should fix it so it never cross contaminates between staking, delegated staking, and normal balances. It's much more straightforward and should be easier to understand.

github-actions · 2024-11-13T00:30:40Z

✅ Forge suite `realistic_env_max_load` success on `d69cd07e324a95974bfdb83787f55c4952fd59f0`

two traffics test: inner traffic : committed: 14588.98 txn/s, latency: 2725.49 ms, (p50: 2700 ms, p70: 2700, p90: 2700 ms, p99: 4800 ms), latency samples: 5547120
two traffics test : committed: 99.94 txn/s, latency: 1763.01 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 16900 ms), latency samples: 1700
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.779, avg: 1.484", "ConsensusProposalToOrdered: max: 0.326, avg: 0.291", "ConsensusOrderedToCommit: max: 0.365, avg: 0.353", "ConsensusProposalToCommit: max: 0.655, avg: 0.644"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.85s no progress at version 2249615 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.71s no progress at version 2249613 (avg 8.71s) [limit 15].
Test Ok

github-actions · 2024-11-13T00:32:23Z

✅ Forge suite `compat` success on `58c5c53eaffe680eb2fc4cfbddf05a867fecadb9` ==> `d69cd07e324a95974bfdb83787f55c4952fd59f0`

Compatibility test results for 58c5c53eaffe680eb2fc4cfbddf05a867fecadb9 ==> d69cd07e324a95974bfdb83787f55c4952fd59f0 (PR)
1. Check liveness of validators at old version: 58c5c53eaffe680eb2fc4cfbddf05a867fecadb9
compatibility::simple-validator-upgrade::liveness-check : committed: 15490.70 txn/s, latency: 1918.19 ms, (p50: 1900 ms, p70: 2100, p90: 2100 ms, p99: 2200 ms), latency samples: 577720
2. Upgrading first Validator to new version: d69cd07e324a95974bfdb83787f55c4952fd59f0
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7886.81 txn/s, latency: 3426.81 ms, (p50: 3600 ms, p70: 4000, p90: 4700 ms, p99: 4800 ms), latency samples: 143200
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7821.20 txn/s, latency: 4055.67 ms, (p50: 4100 ms, p70: 4200, p90: 6200 ms, p99: 6600 ms), latency samples: 258460
3. Upgrading rest of first batch to new version: d69cd07e324a95974bfdb83787f55c4952fd59f0
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7978.96 txn/s, latency: 3450.54 ms, (p50: 3400 ms, p70: 4100, p90: 4700 ms, p99: 4900 ms), latency samples: 145180
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7728.21 txn/s, latency: 4078.24 ms, (p50: 4100 ms, p70: 4200, p90: 6400 ms, p99: 6800 ms), latency samples: 255680
4. upgrading second batch to new version: d69cd07e324a95974bfdb83787f55c4952fd59f0
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12394.82 txn/s, latency: 2200.02 ms, (p50: 2400 ms, p70: 2500, p90: 2700 ms, p99: 2900 ms), latency samples: 213440
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12097.59 txn/s, latency: 2629.34 ms, (p50: 2500 ms, p70: 2700, p90: 2900 ms, p99: 4300 ms), latency samples: 388660
5. check swarm health
Compatibility test for 58c5c53eaffe680eb2fc4cfbddf05a867fecadb9 ==> d69cd07e324a95974bfdb83787f55c4952fd59f0 passed
Test Ok

the last refactor made it get staking balances and the account's balance this should fix it so it never cross contaminates between staking, delegated staking, and normal balances. It's much more straightforward and should be easier to understand. (cherry picked from commit 7d6d59c)

github-actions · 2024-11-13T00:33:22Z

💚 All backports created successfully

Status	Branch	Result
✅	aptos-release-v1.23

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

gregnazario mentioned this pull request Nov 9, 2024

Fix rosetta staking balances #15246

Closed

22 tasks

gregnazario requested review from davidiw, lightmark and banool November 9, 2024 01:30

gregnazario enabled auto-merge (squash) November 9, 2024 01:42

This comment has been minimized.

Sign in to view

vusirikala added the v1.23 label Nov 12, 2024

GhostWalker562 approved these changes Nov 12, 2024

View reviewed changes

banool approved these changes Nov 12, 2024

View reviewed changes

[rosetta] Fix staking balances

d69cd07

the last refactor made it get staking balances and the account's balance this should fix it so it never cross contaminates between staking, delegated staking, and normal balances. It's much more straightforward and should be easier to understand.

gregnazario force-pushed the pr-rosetta-staking-balances branch from 67b6773 to d69cd07 Compare November 12, 2024 21:50

This comment has been minimized.

Sign in to view

gregnazario merged commit 7d6d59c into main Nov 13, 2024
48 of 54 checks passed

gregnazario deleted the pr-rosetta-staking-balances branch November 13, 2024 00:32

github-actions bot mentioned this pull request Nov 13, 2024

[cp][aptos-release-v1.23] [rosetta] Fix staking balances #15262

Merged

gregnazario pushed a commit that referenced this pull request Nov 14, 2024

[rosetta] Fix staking balances (#15247) (#15262)

a0ec6ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rosetta] Fix staking balances #15247

[rosetta] Fix staking balances #15247

gregnazario commented Nov 9, 2024 •

edited

Loading

trunk-io bot commented Nov 9, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GhostWalker562 left a comment

banool Nov 12, 2024

gregnazario Nov 12, 2024

gregnazario Nov 12, 2024

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

[rosetta] Fix staking balances #15247

[rosetta] Fix staking balances #15247

Conversation

gregnazario commented Nov 9, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Nov 9, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GhostWalker562 left a comment

Choose a reason for hiding this comment

banool Nov 12, 2024

Choose a reason for hiding this comment

gregnazario Nov 12, 2024

Choose a reason for hiding this comment

gregnazario Nov 12, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 13, 2024

✅ Forge suite realistic_env_max_load success on d69cd07e324a95974bfdb83787f55c4952fd59f0

github-actions bot commented Nov 13, 2024

✅ Forge suite compat success on 58c5c53eaffe680eb2fc4cfbddf05a867fecadb9 ==> d69cd07e324a95974bfdb83787f55c4952fd59f0

github-actions bot commented Nov 13, 2024

💚 All backports created successfully

Questions ?

gregnazario commented Nov 9, 2024 •

edited

Loading

trunk-io bot commented Nov 9, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `d69cd07e324a95974bfdb83787f55c4952fd59f0`

✅ Forge suite `compat` success on `58c5c53eaffe680eb2fc4cfbddf05a867fecadb9` ==> `d69cd07e324a95974bfdb83787f55c4952fd59f0`