Skip to content

Conversation

@runtian-zhou
Copy link
Contributor

@runtian-zhou runtian-zhou commented Apr 9, 2025

Description

This PR removes the balance check in the dispatchable_fungible_asset module that was verifying the withdrawn amount matched the requested amount. The check was unnecessary and was causing tests to fail. Additionally, it enhances the ten_x_token test module by implementing and registering proper withdraw and deposit dispatch functions.

How Has This Been Tested?

The changes have been tested through the existing nil_op_token_tests which previously expected a failure but now passes correctly. The test verifies that withdrawal and deposit operations work properly without the balance check.

Key Areas to Review

  • Removal of the balance check in dispatchable_fungible_asset.move which was comparing start and end balances
  • Implementation of withdraw and deposit functions in the ten_x_token.move test module
  • Registration of these functions in the constructor of the test token

Type of Change

  • Bug fix
  • Refactoring

Which Components or Systems Does This Change Impact?

  • Aptos Framework

Checklist

  • I have performed a self-review of my own code
  • I have tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io
Copy link

trunk-io bot commented Apr 9, 2025

⏱️ 41m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 19m 🟩
check-dynamic-deps 7m 🟩🟩🟩
rust-cargo-deny 5m 🟩🟩🟩
rust-lints 4m 🟩
rust-check-merge-base 3m 🟩
general-lints 1m 🟩🟩🟩
semgrep/ci 1m 🟩🟩🟩
file_change_determinator 35s 🟩🟩🟩
permission-check 6s 🟩🟩
permission-check 5s 🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@runtian-zhou runtian-zhou changed the title [Transaction Filter] Compare standardized addresses (#16151) [aptos-framework] Remove balance checks in DFA Apr 9, 2025
@runtian-zhou runtian-zhou marked this pull request as ready for review April 9, 2025 17:06
@runtian-zhou runtian-zhou requested review from lightmark and removed request for davidiw and wrwg April 9, 2025 17:06
@runtian-zhou runtian-zhou force-pushed the runtianz/remove_checks branch from 94a1f9a to d67254f Compare April 9, 2025 17:31
@runtian-zhou
Copy link
Contributor Author

Drafted AIP for discussion and justification for this change: aptos-foundation/AIPs#582


#[test(creator = @0xcafe)]
#[expected_failure(abort_code=0x70002, location=aptos_framework::dispatchable_fungible_asset)]
fun test_nil_op_token(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the test name to record the new expected behavior now?

@movekevin
Copy link
Contributor

Can you add a test where the override function clamps down the amount to max amount users can withdraw?

Copy link
Contributor Author

Test added

@runtian-zhou runtian-zhou force-pushed the runtianz/remove_checks branch from d67254f to 12b0f38 Compare April 15, 2025 21:26
@@ -0,0 +1,89 @@
#[test_only]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does clamped mean here?
add some comments?

@runtian-zhou runtian-zhou enabled auto-merge (squash) April 17, 2025 05:35
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@runtian-zhou runtian-zhou force-pushed the runtianz/remove_checks branch from 0907b12 to edf8d8c Compare April 17, 2025 19:18
@runtian-zhou runtian-zhou force-pushed the runtianz/remove_checks branch from edf8d8c to cd8b091 Compare April 17, 2025 19:20
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4

two traffics test: inner traffic : committed: 12166.38 txn/s, submitted: 12170.53 txn/s, expired: 4.16 txn/s, latency: 3227.81 ms, (p50: 3000 ms, p70: 3300, p90: 3600 ms, p99: 5400 ms), latency samples: 4625940
two traffics test : committed: 99.99 txn/s, latency: 2121.30 ms, (p50: 1600 ms, p70: 2000, p90: 2400 ms, p99: 22200 ms), latency samples: 1760
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.938, avg: 1.666", "ConsensusProposalToOrdered: max: 0.316, avg: 0.310", "ConsensusOrderedToCommit: max: 0.495, avg: 0.420", "ConsensusProposalToCommit: max: 0.802, avg: 0.730"]
Max non-epoch-change gap was: 1 rounds at version 44021 (avg 0.00) [limit 4], 1.90s no progress at version 44021 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.94s no progress at version 4349872 (avg 0.86s) [limit 16].
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4

Forge report malformed: Expecting ',' delimiter: line 18 column 6 (char 480)
'{\n  "metrics": [\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "submitted_txn",\n      "value": 139702.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "expired_txn",\n      "value": 500.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_tps",\n      "value": 1573.2070955086501\n    },\n[2025-04-17T19:51:05Z INFO  aptos_forge::report] Test Ok\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_latency",\n      "value": 2487.119272711599\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p50_latency",\n      "value": 1800.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p90_latency",\n      "value": 3300.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p99_latency",\n      "value": 13100.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "submitted_txn",\n      "value": 155660.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "expired_txn",\n      "value": 753.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_tps",\n      "value": 1704.172712257373\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_latency",\n      "value": 1708.919028836657\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p50_latency",\n      "value": 1600.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p90_latency",\n      "value": 2400.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p99_latency",\n      "value": 3300.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "submitted_txn",\n      "value": 109782.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "expired_txn",\n      "value": 340.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_tps",\n      "value": 1213.650195735411\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "avg_latency",\n      "value": 2465.7776173681036\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p50_latency",\n      "value": 1500.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p90_latency",\n      "value": 3000.0\n    },\n    {\n      "test_name": "framework_upgrade::framework-upgrade::full-framework-upgrade",\n      "metric": "p99_latency",\n      "value": 12600.0\n    }\n  ],\n  "text": "Compatibility test results for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 (PR)\\nUpgrade the nodes to version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4\\nframework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1573.21 txn/s, submitted: 1578.86 txn/s, failed submission: 5.65 txn/s, expired: 5.65 txn/s, latency: 2487.12 ms, (p50: 1800 ms, p70: 2100, p90: 3300 ms, p99: 13100 ms), latency samples: 139202\\nframework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1704.17 txn/s, submitted: 1712.46 txn/s, failed submission: 8.14 txn/s, expired: 8.28 txn/s, latency: 1708.92 ms, (p50: 1600 ms, p70: 2000, p90: 2400 ms, p99: 3300 ms), latency samples: 154907\\n5. check swarm health\\nCompatibility test for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 passed\\nUpgrade the remaining nodes to version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4\\nframework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1213.65 txn/s, submitted: 1217.42 txn/s, failed submission: 3.77 txn/s, expired: 3.77 txn/s, latency: 2465.78 ms, (p50: 1500 ms, p70: 1900, p90: 3000 ms, p99: 12600 ms), latency samples: 109442\\nTest Ok"\n}'
Trailing Log Lines:
Compatibility test results for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 (PR)
Upgrade the nodes to version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1573.21 txn/s, submitted: 1578.86 txn/s, failed submission: 5.65 txn/s, expired: 5.65 txn/s, latency: 2487.12 ms, (p50: 1800 ms, p70: 2100, p90: 3300 ms, p99: 13100 ms), latency samples: 139202
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1704.17 txn/s, submitted: 1712.46 txn/s, failed submission: 8.14 txn/s, expired: 8.28 txn/s, latency: 1708.92 ms, (p50: 1600 ms, p70: 2000, p90: 2400 ms, p99: 3300 ms), latency samples: 154907
5. check swarm health
Compatibility test for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 passed
Upgrade the remaining nodes to version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1213.65 txn/s, submitted: 1217.42 txn/s, failed submission: 3.77 txn/s, expired: 3.77 txn/s, latency: 2465.78 ms, (p50: 1500 ms, p70: 1900, p90: 3000 ms, p99: 12600 ms), latency samples: 109442
Test Ok

=== BEGIN JUNIT ===
<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="forge" tests="1" failures="0" errors="0" uuid="707f8f11-8ee4-4d2c-9ea9-711e53c4d30f">
    <testsuite name="local" tests="1" disabled="0" errors="0" failures="0">
        <testcase name="framework_upgrade::framework-upgrade">
        </testcase>
    </testsuite>
</testsuites>
=== END JUNIT ===
[2025-04-17T19:51:05Z INFO  aptos_forge::backend::k8s::cluster_helper] Deleting namespace forge-framework-upgrade-pr-16313: Some(NamespaceStatus { conditions: None, phase: Some("Terminating") })
[2025-04-17T19:51:05Z INFO  aptos_forge::backend::k8s::cluster_helper] aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-16313

test result: ok. 1 passed; 0 failed; 0 filtered out

Debugging output:
NAME                                   READY   STATUS      RESTARTS   AGE
aptos-node-0-validator-0               1/1     Running     0          10m
aptos-node-1-validator-0               1/1     Running     0          10m
aptos-node-2-validator-0               1/1     Running     0          3m14s
aptos-node-3-validator-0               1/1     Running     0          2m40s
forge-testnet-deployer-xmj6h           0/1     Completed   0          13m
genesis-aptos-genesis-eforge84-5ztqz   0/1     Completed   0          12m

@github-actions
Copy link
Contributor

✅ Forge suite compat success on 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4

Compatibility test results for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 (PR)
1. Check liveness of validators at old version: 463dda939a7bdc1266946e824a1eb309f300eaf8
compatibility::simple-validator-upgrade::liveness-check : committed: 3655.09 txn/s, submitted: 3655.26 txn/s, expired: 0.17 txn/s, latency: 3573.09 ms, (p50: 3800 ms, p70: 4100, p90: 4400 ms, p99: 5000 ms), latency samples: 304786
2. Upgrading first Validator to new version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 2765.10 txn/s, latency: 11963.01 ms, (p50: 13600 ms, p70: 13800, p90: 13900 ms, p99: 14000 ms), latency samples: 103940
3. Upgrading rest of first batch to new version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2870.62 txn/s, latency: 11592.34 ms, (p50: 13100 ms, p70: 13500, p90: 13700 ms, p99: 13800 ms), latency samples: 108360
4. upgrading second batch to new version: cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5048.99 txn/s, latency: 6817.91 ms, (p50: 7300 ms, p70: 7700, p90: 7900 ms, p99: 8100 ms), latency samples: 177440
5. check swarm health
Compatibility test for 463dda939a7bdc1266946e824a1eb309f300eaf8 ==> cd8b0918c1bc2ab56285af45bf02f0fa69e6f5d4 passed
Test Ok

@runtian-zhou runtian-zhou merged commit b4af7e7 into main Apr 17, 2025
46 checks passed
@runtian-zhou runtian-zhou deleted the runtianz/remove_checks branch April 17, 2025 20:10
lwedge99 pushed a commit to sentioxyz/aptos-core that referenced this pull request May 11, 2025
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