Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rosetta staking balances #15246

Closed
wants to merge 6 commits into from
Closed

Conversation

gregnazario
Copy link
Contributor

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.

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

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (Rosetta)

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

junkil-park and others added 5 commits October 25, 2024 23:25
Co-authored-by: gedigi <[email protected]>
(cherry picked from commit 7097148)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ond the overall commit progress.

(cherry picked from commit a6dd53d)
(cherry picked from commit 46364e9)
Copy link

trunk-io bot commented Nov 9, 2024

⏱️ 21m total CI duration on this PR
Job Cumulative Duration Recent Runs
check-dynamic-deps 6m 🟩🟩🟩
rust-cargo-deny 5m 🟩🟩🟩
rust-move-tests 2m 🟩
rust-move-tests 2m 🟩
rust-move-tests 2m 🟩
semgrep/ci 2m 🟩🟩🟩
general-lints 1m 🟩🟩🟩
test-copy-images 1m 🟩🟩🟩
file_change_determinator 34s 🟩🟩🟩
permission-check 12s 🟩🟩🟩
permission-check 8s 🟩🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
check-dynamic-deps 4m 1m +264%

settingsfeedbackdocs ⋅ learn more about trunk.io

@gregnazario gregnazario force-pushed the fix-rosetta-staking-balances branch from 1e5c99e to 2b5dd27 Compare November 9, 2024 01:22
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 gregnazario force-pushed the fix-rosetta-staking-balances branch from 2b5dd27 to 5b12db7 Compare November 9, 2024 01:26
@gregnazario
Copy link
Contributor Author

Closed in favor of #15247

@gregnazario gregnazario closed this Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants