[aptos-framework] Dispatchable Token Standard #12635

runtian-zhou · 2024-03-22T00:20:22Z

Description

This PR provides a reference implementation for AIP-73. See discussion thread: #12490.

The PR mainly have the following four parts:

MoveVM related change to:
- support native functions to return control flow change directive.
- Add runtime check for cyclic call graph that could be introduced by the dispatch logic
A function_info.move that mocks the function pointer
A overloadable_fungible_asset.move that utilizes the function info and dispatch native to allow for a custom withdraw operation
An example of how a overloadable withdraw function could be implemented as a test.
WIP: Compiler directive to check the dispatch target function has expected type so the users can get early warnings on type mismatch?

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Added tests for all logics added.

Key Areas to Review

A few areas we should focus on:

The runtime check implemented in MoveVM is indeed safe.
Make sure we charge gas for dispatch properly.
Work with ecosystem folks on the public interfaces of dispatchable token module.

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-03-22T00:20:25Z

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

Job	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	36m	🟥
run-tests-main-branch	36m	🟥 🟥 🟥 🟥 🟥 (+1 more)
rust-unit-tests	26m	🟥
windows-build	23m	🟩
rust-move-unit-coverage	18m	🟩
rust-targeted-unit-tests	6m	⬜
rust-lints	5m	🟥 ⬜
rust-move-tests	4m	🟥
check	4m	🟩
general-lints	2m	🟩
check-dynamic-deps	2m	🟩
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci	51s	🟩 🟩
permission-check	24s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	19s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	18s	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	16s	🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩 (+1 more)

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

Job	Duration	vs 7d avg	Delta
run-tests-main-branch	6m	4m

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

runtian-zhou · 2024-03-22T00:26:40Z

I tried to separate out the components into each individual commits. So reviewing by commit might be better.

codecov · 2024-03-22T00:39:50Z

Codecov Report

Attention: Patch coverage is 69.62233% with 185 lines in your changes are missing coverage. Please review.

Project coverage is 62.4%. Comparing base (3b3edf1) to head (984aa7e).
Report is 2 commits behind head on main.

Files	Patch %	Lines
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	94 Missing ⚠️
aptos-move/aptos-vm/src/transaction_validation.rs	0.0%	34 Missing ⚠️
...hird_party/move/move-vm/runtime/src/interpreter.rs	89.9%	20 Missing ⚠️
...ptos-vm/src/verifier/transaction_arg_validation.rs	0.0%	8 Missing ⚠️
aptos-move/framework/src/natives/function_info.rs	93.2%	8 Missing ⚠️
...s-gas-schedule/src/gas_schedule/aptos_framework.rs	0.0%	7 Missing ⚠️
aptos-move/aptos-vm/src/testing.rs	0.0%	4 Missing ⚠️
aptos-move/aptos-vm/src/validator_txns/dkg.rs	0.0%	2 Missing ⚠️
aptos-move/aptos-vm/src/validator_txns/jwk.rs	0.0%	2 Missing ⚠️
...party/move/move-vm/runtime/src/native_functions.rs	92.3%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #12635     +/-   ##
=========================================
+ Coverage    62.3%    62.4%   +0.1%     
=========================================
  Files         828      830      +2     
  Lines      185844   186363    +519     
=========================================
+ Hits       115858   116390    +532     
+ Misses      69986    69973     -13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alnoki

Is there any way that this support can be extended for primary fungible stores?

Asking since those will be the primary use case for FA transfers

alnoki · 2024-03-22T01:01:54Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+    /// A transfer with a fixed amount debited from the sender
+    public fun transfer_fixed_send<T: key>(
+        _sender: &signer,
+        from: Object<T>,
+        to: Object<T>,
+        send_amount: u64,
+    ) acquires OverloadFunctionStore {
+        let store_address = object::object_address(&from);
+        assert!(exists<OverloadFunctionStore>(store_address), error::not_found(EFUNCTION_STORE_NOT_FOUND));
+        let overloadable_store = borrow_global<OverloadFunctionStore>(store_address);
+        let fa = fungible_asset::withdraw_with_ref(&overloadable_store.transfer_ref, from, send_amount);
+        deposit(to, fa);
+    }


Nice!

So in this case the deposit function is the one that might implement a tax?

Asking because I don't see an assert for EAMOUNT_MISMATCH

So the assumption here is we use withdraw_with_ref to get the exact amount from the sender side. So we don't need EAMOUNT_MISMATCH

alnoki · 2024-03-22T01:03:31Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+        let fa = withdraw(sender, from, receive_amount);
+        let store_address = object::object_address(&from);
+        let overloadable_store = borrow_global<OverloadFunctionStore>(store_address);
+        assert!(fungible_asset::amount(&fa) == receive_amount, error::aborted(EAMOUNT_MISMATCH));


So here, the assumption is that once a hot potato FA has been withdrawn, the exact amount will necessarily end up in the receiver store

Correct?

Yes because we use deposit_with_ref so it has no custom behavior.

alnoki · 2024-03-22T01:04:38Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+    public fun withdraw<T: key>(
+        owner: &signer,
+        store: Object<T>,
+        amount: u64,
+    ): FungibleAsset acquires OverloadFunctionStore {
+        let metadata_addr = object::object_address(&fungible_asset::store_metadata(store));
+        let owner_address = signer::address_of(owner);
+        assert!(exists<OverloadFunctionStore>(metadata_addr), error::not_found(EFUNCTION_STORE_NOT_FOUND));
+        let overloadable_store = borrow_global<OverloadFunctionStore>(metadata_addr);
+        dispatchable_withdraw(
+            owner_address,
+            store,
+            amount,
+            &overloadable_store.transfer_ref,
+            &overloadable_store.withdraw_function,
+        )
+    }
+
+    /// Deposit `amount` of the fungible asset to `store`.
+    ///
+    /// The semantics of deposit will be governed by the function specified in OverloadFunctionStore.
+    public fun deposit<T: key>(
+        store: Object<T>,
+        fa: FungibleAsset
+    ) acquires OverloadFunctionStore {
+        let metadata_addr = object::object_address(&fungible_asset::store_metadata(store));
+        assert!(exists<OverloadFunctionStore>(metadata_addr), error::not_found(EFUNCTION_STORE_NOT_FOUND));
+        let overloadable_store = borrow_global<OverloadFunctionStore>(metadata_addr);
+        dispatchable_deposit(
+            store,
+            fa,
+            &overloadable_store.transfer_ref,
+            &overloadable_store.deposit_function,
+        );
+    }


What happens for the two-sided transfer functions in the case that

a. withdraw assesses a tax
b. deposit assesses a tax
c. both assess a tax

?

That's an interesting case. To avoid double assess maybe we should just use the transfer api so at most one side pays the tax?

What is the issue? I already pay two taxes in the US ... income and sales 😭

@davidiw my main concern here is ensuring that a transfer can have either a known amount sent or received

else the accounting for DeFi breaks, since you don't have a known amount deposited into a pool (known receive amount) or withdrawn from a pool (known send amount)

junkil-park · 2024-03-26T04:55:15Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+        fungible_asset::deposit_with_ref(&overloadable_store.transfer_ref, to, fa);
+    }
+
+    native fun dispatchable_withdraw<T: key>(


Unfortunately, we cannot specify this function because, at the static time, function is unknown, and we do not know the exact behavior of this native function dispatchable_withdraw.

It would be useful to observe some instances of overloaded functions and see what properties we can specify for them.

davidiw · 2024-03-27T23:52:49Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+
+    #[resource_group_member(group = aptos_framework::object::ObjectGroup)]
+    struct OverloadFunctionStore has key {
+		withdraw_function: FunctionInfo,


maybe these should be optional and we need balance and something else, @movekevin

davidiw · 2024-03-27T23:54:19Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+    public fun withdraw<T: key>(
+        owner: &signer,
+        store: Object<T>,
+        amount: u64,
+    ): FungibleAsset acquires OverloadFunctionStore {


This should be a wrapper around withdraw, the happy path is that the FA isn't frozen and that this isn't used. We'll also need to see performance of the global freeze.

davidiw · 2024-03-27T23:55:16Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+    public fun withdraw<T: key>(
+        owner: &signer,
+        store: Object<T>,
+        amount: u64,
+    ): FungibleAsset acquires OverloadFunctionStore {
+        let metadata_addr = object::object_address(&fungible_asset::store_metadata(store));
+        let owner_address = signer::address_of(owner);
+        assert!(exists<OverloadFunctionStore>(metadata_addr), error::not_found(EFUNCTION_STORE_NOT_FOUND));
+        let overloadable_store = borrow_global<OverloadFunctionStore>(metadata_addr);
+        dispatchable_withdraw(
+            owner_address,
+            store,
+            amount,
+            &overloadable_store.transfer_ref,
+            &overloadable_store.withdraw_function,
+        )
+    }
+
+    /// Deposit `amount` of the fungible asset to `store`.
+    ///
+    /// The semantics of deposit will be governed by the function specified in OverloadFunctionStore.
+    public fun deposit<T: key>(
+        store: Object<T>,
+        fa: FungibleAsset
+    ) acquires OverloadFunctionStore {
+        let metadata_addr = object::object_address(&fungible_asset::store_metadata(store));
+        assert!(exists<OverloadFunctionStore>(metadata_addr), error::not_found(EFUNCTION_STORE_NOT_FOUND));
+        let overloadable_store = borrow_global<OverloadFunctionStore>(metadata_addr);
+        dispatchable_deposit(
+            store,
+            fa,
+            &overloadable_store.transfer_ref,
+            &overloadable_store.deposit_function,
+        );
+    }


What is the issue? I already pay two taxes in the US ... income and sales 😭

davidiw · 2024-03-27T23:56:41Z

aptos-move/framework/aptos-framework/sources/overloadable_fungible_asset.move

+    ) acquires OverloadFunctionStore {
+        let metadata_addr = object::object_address(&fungible_asset::store_metadata(from));


something seems off here where is the ownership check?

igor-aptos · 2024-03-28T18:06:51Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

@@ -100,6 +100,11 @@ module aptos_framework::fungible_asset {
        project_uri: String,
    }

+    #[resource_group_member(group = aptos_framework::object::ObjectGroup)]
+    struct GlobalFreeze has key {


freeze has a different meaning.

Let's call this differently / more descriptively, and not share error codes.

maybe DelegatedToOverloaded? ActionsOverloaded?
also, can only one of the withdraw/deposit be overloaded, or do both need to be simultanously? or if either is, we just ask them to go through the other api anyways?

zekun000

overall looks reasonable to me

zekun000 · 2024-03-30T13:42:45Z

third_party/move/move-vm/runtime/src/interpreter.rs

@@ -139,9 +142,14 @@ impl Interpreter {
                .map_err(|e| self.set_location(e))?;
        }

+        if let Some(module_id) = function.module_id() {


i guess it's impossible to re-enter a script?

Yea script has to be always on the top most frame only.

zekun000 · 2024-03-30T13:57:04Z

aptos-move/framework/src/natives/dispatchable_fungible_asset.rs

+    mut arguments: VecDeque<Value>,
+) -> SafeNativeResult<SmallVec<[Value; 1]>> {
+    let (module_name, func_name) = extract_function_info(&mut arguments)?;
+    Err(SafeNativeError::CallFunction {


this seems pretty hacky

This is indeed a hack, but it's due to a limitation of our current implementation -- native functions lack access to the real gas meter, so to do the metering properly, we exit from the native function and then instruct the Move VM to do the actual work.

We have considered various alternatives and non of them is trivial.

zekun000 · 2024-03-30T14:15:46Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

+            string::utf8(b"dispatchable_withdraw"),
+        );
+        // Verify that caller type matches callee type so wrongly typed function cannot be registered.
+        assert!(function_info::check_dispatch_type_compatibility(


do we ensure the function is public? otherwise it can be upgraded and even change the function signature

do we want to be even stricter, and require it to be un-upgradeable?

I think it needs to be a public function -- otherwise this check needs to be performed each transfer.

I think it ought to be upgradeable -- not sure why we'd restrict it that way.

aptos-move/framework/src/natives/function_info.rs

igor-aptos · 2024-04-02T21:39:29Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

+            string::utf8(b"dispatchable_withdraw"),
+        );
+        // Verify that caller type matches callee type so wrongly typed function cannot be registered.
+        assert!(function_info::check_dispatch_type_compatibility(


do we want to be even stricter, and require it to be un-upgradeable?

aptos-move/framework/aptos-framework/sources/fungible_asset.move

aptos-move/framework/aptos-framework/tests/deflation_token.move

aptos-move/framework/aptos-framework/sources/dispatchable_fungible_asset.move

sherry-x · 2024-04-25T17:38:53Z

aptos-move/aptos-release-builder/data/release.yaml

@@ -45,3 +45,13 @@ proposals:
            secrecy_threshold_in_percentage: 50
            reconstruct_threshold_in_percentage: 66
            fast_path_secrecy_threshold_in_percentage: 67
+  - name: step_5_dispatchable_token


we don't add features in the default release yaml file anymore, they will be enabled separately. if you want you can create a dispatchable.yaml file in the same folder

github-actions · 2024-04-25T19:30:17Z

✅ Forge suite `realistic_env_max_load` success on `984aa7efffb7fe269d458888ba4b9d1843c03e7c`

two traffics test: inner traffic : committed: 7732 txn/s, latency: 5070 ms, (p50: 4800 ms, p90: 6000 ms, p99: 11100 ms), latency samples: 3340340
two traffics test : committed: 100 txn/s, latency: 2041 ms, (p50: 1800 ms, p90: 2200 ms, p99: 8200 ms), latency samples: 1760
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.205, avg: 0.201", "QsPosToProposal: max: 0.281, avg: 0.248", "ConsensusProposalToOrdered: max: 0.472, avg: 0.436", "ConsensusOrderedToCommit: max: 0.406, avg: 0.383", "ConsensusProposalToCommit: max: 0.858, avg: 0.819"]
Max round gap was 1 [limit 4] at version 1467624. Max no progress secs was 6.476503 [limit 15] at version 1467624.
Test Ok

github-actions · 2024-04-25T19:30:19Z

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `984aa7efffb7fe269d458888ba4b9d1843c03e7c`

Compatibility test results for aptos-node-v1.10.1 ==> 984aa7efffb7fe269d458888ba4b9d1843c03e7c (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 5327 txn/s, latency: 5682 ms, (p50: 5000 ms, p90: 9900 ms, p99: 11700 ms), latency samples: 229100
2. Upgrading first Validator to new version: 984aa7efffb7fe269d458888ba4b9d1843c03e7c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1836 txn/s, latency: 15675 ms, (p50: 18300 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 91840
3. Upgrading rest of first batch to new version: 984aa7efffb7fe269d458888ba4b9d1843c03e7c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1846 txn/s, latency: 15672 ms, (p50: 19000 ms, p90: 22600 ms, p99: 23100 ms), latency samples: 90460
4. upgrading second batch to new version: 984aa7efffb7fe269d458888ba4b9d1843c03e7c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3481 txn/s, latency: 9219 ms, (p50: 9600 ms, p90: 12700 ms, p99: 13000 ms), latency samples: 146240
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 984aa7efffb7fe269d458888ba4b9d1843c03e7c passed
Test Ok

runtian-zhou · 2024-04-25T22:10:53Z

third_party/move/tools/move-unit-test/src/test_runner.rs

@@ -263,20 +267,25 @@ impl SharedTestingConfig {
        VMResult<Vec<Vec<u8>>>,
        TestRunInfo,
    ) {
-        let move_vm = MoveVM::new(self.native_function_table.clone()).unwrap();
+        let mut config = VMConfig::default();
+        config.paranoid_type_checks = true;


cc @brianrmurphy

This reverts commit bbf569a.

runtian-zhou requested review from davidiw, movekevin and wrwg as code owners March 22, 2024 00:20

alnoki reviewed Mar 22, 2024

View reviewed changes

vgao1996 self-requested a review March 25, 2024 17:24

runtian-zhou force-pushed the runtianz/dispatchable_token branch 2 times, most recently from ff16f49 to 8328a2c Compare March 25, 2024 19:26

junkil-park reviewed Mar 26, 2024

View reviewed changes

davidiw reviewed Mar 27, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/dispatchable_token branch 2 times, most recently from fda30ca to 637170e Compare March 28, 2024 00:55

igor-aptos reviewed Mar 28, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/dispatchable_token branch from 637170e to 4d5d593 Compare March 29, 2024 22:19

zekun000 reviewed Mar 30, 2024

View reviewed changes

aptos-move/framework/src/natives/function_info.rs Outdated Show resolved Hide resolved

igor-aptos previously requested changes Apr 2, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/dispatchable_token branch from 4d5d593 to 6d6a293 Compare April 4, 2024 08:52

runtian-zhou requested a review from georgemitenkov as a code owner April 4, 2024 08:52

runtian-zhou force-pushed the runtianz/dispatchable_token branch 8 times, most recently from 22c88ae to dd6450c Compare April 8, 2024 22:04

runtian-zhou requested a review from igor-aptos April 8, 2024 22:05

This comment has been minimized.

Sign in to view

sherry-x reviewed Apr 25, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/dispatchable_token branch 2 times, most recently from f5bec0c to 417e662 Compare April 25, 2024 18:02

igor-aptos removed the CICD:run-execution-performance-full-test Run execution performance test (full version) label Apr 25, 2024

runtian-zhou force-pushed the runtianz/dispatchable_token branch 2 times, most recently from 7d9b67b to 2e0f00c Compare April 25, 2024 18:17

This comment has been minimized.

Sign in to view

[aptos-framework] Dispatchable Token Standard

984aa7e

runtian-zhou force-pushed the runtianz/dispatchable_token branch from 2e0f00c to 984aa7e Compare April 25, 2024 19:01

This comment has been minimized.

Sign in to view

runtian-zhou merged commit bbf569a into main Apr 25, 2024
53 checks passed

runtian-zhou deleted the runtianz/dispatchable_token branch April 25, 2024 20:12

runtian-zhou commented Apr 25, 2024

View reviewed changes

runtian-zhou added a commit that referenced this pull request May 3, 2024

Revert "[aptos-framework] Dispatchable Token Standard (#12635)"

fbf25a3

This reverts commit bbf569a.

runtian-zhou mentioned this pull request May 3, 2024

Reverting dispatchable on 1.12 #13190

Merged

sherry-x pushed a commit that referenced this pull request May 3, 2024

Revert "[aptos-framework] Dispatchable Token Standard (#12635)" (#13190)

2505fce

This reverts commit bbf569a.

		) acquires OverloadFunctionStore {
		let metadata_addr = object::object_address(&fungible_asset::store_metadata(from));

[aptos-framework] Dispatchable Token Standard #12635

[aptos-framework] Dispatchable Token Standard #12635

Conversation

runtian-zhou commented Mar 22, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Mar 22, 2024 • edited Loading

runtian-zhou commented Mar 22, 2024

codecov bot commented Mar 22, 2024 • edited Loading

Codecov Report

alnoki left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junkil-park Mar 26, 2024 • 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

zekun000 left a 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

Choose a reason for hiding this comment

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.

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.

github-actions bot commented Apr 25, 2024

✅ Forge suite realistic_env_max_load success on 984aa7efffb7fe269d458888ba4b9d1843c03e7c

github-actions bot commented Apr 25, 2024

✅ Forge suite compat success on aptos-node-v1.10.1 ==> 984aa7efffb7fe269d458888ba4b9d1843c03e7c

Choose a reason for hiding this comment

runtian-zhou commented Mar 22, 2024 •

edited

Loading

trunk-io bot commented Mar 22, 2024 •

edited

Loading

codecov bot commented Mar 22, 2024 •

edited

Loading

junkil-park Mar 26, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `984aa7efffb7fe269d458888ba4b9d1843c03e7c`

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `984aa7efffb7fe269d458888ba4b9d1843c03e7c`