Refactor fungible assets #7379

movekevin · 2023-03-24T22:38:12Z

See commits for the full list of changes.

Remaining work:

Add more unit test coverage for fungible_asset and managed_fungible_asset
Switch to OptionalAggregator for supply + balance
(Considering) Rename FungibleAssetMetadata => something else
(Considering) Remove generics in view and entry functions

davidiw · 2023-03-27T05:28:02Z

aptos-move/framework/aptos-framework/sources/object.move

+    /// Derives an object address from type and source account: sha3_256([source | type | seed | 0xFC]).
+    public fun create_typed_object_address<T>(source: &address, seed: vector<u8>): address {
+        let bytes = bcs::to_bytes(source);
+        let type_name_bytes = bcs::to_bytes(&type_info::type_name<T>());
+        vector::append(&mut bytes, type_name_bytes);
+        vector::append(&mut bytes, seed);
+        vector::push_back(&mut bytes, OBJECT_FROM_TYPE_SCHEME);
+        from_bcs::to_address(hash::sha3_256(bytes))
+    }
+


Shouldn't seed just be the typeinfo maybe bcs serialized? I don't understand why we want a seed here.

alternatively... could we make an object witness such that I could do the following:

create_deterministic_object(user: &signer, ref: &Witness): ConstructorRef

where Witness is a ref that can be accessed either via ConstructorRef or ExtendRef...

Then an object implementation can expose a function like:
fun generate_account_object(account: &signer): Object<T>

which would create a fully useful object of say a fungible asset

Seed is used to create a separate but deterministic wallet address for each fungible asset metadata object.

Yep, an object implementation such as fungible assets might still want to create multiple objects with the same address formula (i.e. type T). In a way, type T allows namespacing objects by module/type.

I don't think this is particularly needed. We should already be able to create arbitrary objects using the guid or named pattern. The disadvantage of having this is is it effectively becomes a new named variant.

If someone passes in a seed, it will be untrackable without tracking all possible seeds. At which point it doesn't seem different than what we started with... requiring off-chain or on-chain coordination to determine which of possibly any object is the one we're looking for.

Switched over to using a DeriveRef to generate deterministic object addresses of this form:

hash(owner addr | address of object to derive from | 0xFC) where 0xFC is a new domain separator in object.move

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

lightmark · 2023-03-27T04:56:32Z

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

    /// Get the current supply from `metadata`.
-    public fun supply<T: key>(metadata: &Object<T>): u64 acquires FungibleAssetMetadata {
-        borrow_fungible_metadata(metadata).supply
+    public fun supply<T: key>(metadata: Object<T>): u64 acquires FungibleAssetMetadata {


@gerben-stavenga would add reference support soon.

oh that would be nice

lightmark · 2023-03-27T04:58:25Z

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

    /// Get the maximum supply from `metadata`.
-    public fun maximum<T: key>(metadata: &Object<T>): Option<u64> acquires FungibleAssetMetadata {
-        borrow_fungible_metadata(metadata).maximum
+    public fun maximum<T: key>(metadata: Object<T>): Option<u64> acquires FungibleAssetMetadata {


I thought we chatted in slack about replacing T with FungibleAssetMetadata

Yeah but it's still controversial as there can be some improvements we can later on in the adapter layer that might make this reasonable. Let's discuss more in a wider setting

Ahh... so this is still up for discussion.

considering that we are doing a copy on all these functions anyway (we're passing in the value), it might not be the end of the world to expect that this is coerced to the appropriate type. Perhaps this also creates the opportunity for someone to validate it is of a given type before coercing. Let them have their own error handling if they want it.

Note, we will still need to re-validate exists<T>(object_address(obj)) since an Object<T> just implies it existed at some point.

Yeah this warrants a deeper discussion on whether the experience can be improved regarding using entry + view functions with Object if it's not clear by the callers what T should be. One example is the explorer's view UI where users can call the view functions of modules.

I wonder if we can have a default placeholder for Object if called by a view or entry function... i.e., ObjectCore

Yeah I was thinking exactly this too. We need to think about this a bit more about how to do this though

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

lightmark · 2023-03-27T05:26:10Z

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

-    inline fun assert_owner<T: key>(owner: &signer, metadata: &Object<T>) {
-        assert!(object::is_owner(*metadata, signer::address_of(owner)), error::permission_denied(ENOT_OWNER));
+    /// Create a new fungible asset with a value of 0.
+    public fun zero<T: key>(metadata: Object<T>): FungibleAsset {


I see similar things in current coin.move. But I don't understand the potential use case. Can you share an example in comment?

In the past, this is really useful because Coin is storable. zero() would allow people to initialize their module's key resources with empty state. It's less useful now that Coin is no longer storable, but it can make the following programming pattern easier:

let all_payments = fungible_asset::zero();
collect_payment_1(&mut all_payments, ...);
collect_payment_2(&mut all_payments, ...);
collect_payment_3(&mut all_payments, ...);

What do you think?

Then I would prefer a merge_all function in my old PR that takes a vector of fa and merge into 1 using vector::fold. We should make the invariance that zero fa will never be produced to simplify our model

I removed zero(). You can add merge_all in a follow up PR :)

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

aptos-move/move-examples/fungible_asset/sources/managed_fungible_asset.move

davidiw

this kludges together a primary wallet from an arbitrary wallet... could we please move the primary wallet outside of this context?

it'd be great to have a withdraw that extracts from a signer and another that expects a signer and an object pointer. Similarly a deposit that points to a user's address and another that points to an object.

davidiw · 2023-03-27T14:52:45Z

aptos-move/framework/aptos-framework/sources/object.move

+    /// Derives an object address from type and source account: sha3_256([source | type | seed | 0xFC]).
+    public fun create_typed_object_address<T>(source: &address, seed: vector<u8>): address {
+        let bytes = bcs::to_bytes(source);
+        let type_name_bytes = bcs::to_bytes(&type_info::type_name<T>());
+        vector::append(&mut bytes, type_name_bytes);
+        vector::append(&mut bytes, seed);
+        vector::push_back(&mut bytes, OBJECT_FROM_TYPE_SCHEME);
+        from_bcs::to_address(hash::sha3_256(bytes))
+    }
+


I don't think this is particularly needed. We should already be able to create arbitrary objects using the guid or named pattern. The disadvantage of having this is is it effectively becomes a new named variant.

If someone passes in a seed, it will be untrackable without tracking all possible seeds. At which point it doesn't seem different than what we started with... requiring off-chain or on-chain coordination to determine which of possibly any object is the one we're looking for.

davidiw · 2023-03-27T14:54:26Z

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

+
+    /// Emitted when fungible assets are deposited into a wallet.
+    struct DepositEvent has drop, store {
+        metadata: Object<FungibleAssetMetadata>,


I don't think we need this, the event is emitted from the associated object that can then be used to look up the metadata object. We should only add this if we don't want a layer of indirection.

Yes that's what I was thinking - saving one layer of indirection. It can make the indexing flow a lot simpler. Will check with @bowenyang007 later on this

let's clean this up and challenge Bowen.

Sure. I'll talk to Bowen later

davidiw · 2023-03-27T15:00:29Z

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

    /// Get the maximum supply from `metadata`.
-    public fun maximum<T: key>(metadata: &Object<T>): Option<u64> acquires FungibleAssetMetadata {
-        borrow_fungible_metadata(metadata).maximum
+    public fun maximum<T: key>(metadata: Object<T>): Option<u64> acquires FungibleAssetMetadata {


Ahh... so this is still up for discussion.

considering that we are doing a copy on all these functions anyway (we're passing in the value), it might not be the end of the world to expect that this is coerced to the appropriate type. Perhaps this also creates the opportunity for someone to validate it is of a given type before coercing. Let them have their own error handling if they want it.

Note, we will still need to re-validate exists<T>(object_address(obj)) since an Object<T> just implies it existed at some point.

davidiw · 2023-03-27T15:02:10Z

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

-    }
-
-    /// Withdarw `amount` of fungible metadata from `account` ignoring `allow_ungated_transfer`.
+    /// Withdraw `amount` of fungible metadata from an account ignoring `allow_ungated_transfer`.
    public fun withdraw_with_ref(


why do you need the ref and the address?

The ref points to the metadata obj while address is the account where the asset is withdrawn from. Address has now been updated to wallet reference (Object)

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

lightmark · 2023-03-27T22:59:35Z

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

        assert!(amount != 0, error::invalid_argument(EAMOUNT_CANNOT_BE_ZERO));
-        let wallet = borrow_fungible_asset_mut(wallet);
-        assert!(wallet.allow_ungated_transfer, error::invalid_argument(EUNGATED_TRANSFER_IS_NOT_ALLOWED));
+        let wallet = borrow_global_mut<FungibleAssetWallet>(wallet_addr);


but withdraw_with_ref doesn't check. why not put the check here instead?

aptos-move/move-examples/fungible_asset/sources/managed_fungible_asset.move

movekevin · 2023-03-28T06:06:50Z

this kludges together a primary wallet from an arbitrary wallet... could we please move the primary wallet outside of this context?

it'd be great to have a withdraw that extracts from a signer and another that expects a signer and an object pointer. Similarly a deposit that points to a user's address and another that points to an object.

I've updated the implementation to do exactly this - primary wallet operations are done via primary_wallet module (name's up for negotiation) while the operations in fungible_wallet deal with wallets (Object) directly

…pass Object<FungibleAssetMetadata>

… new_gas_curve

lightmark · 2023-04-04T21:08:08Z

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

        ref: &TransferRef,
-        wallet: &Object<FungibleAssetWallet>,
+        wallet: Object<T>,


why this is T?

Same as an earlier comment - I'm just keeping all functions that take Object references consistent and make them generic

lightmark · 2023-04-04T21:14:27Z

aptos-move/framework/aptos-framework/sources/object.move

+    ///
+    /// This 0xFC constant serves as a domain separation tag to prevent existing authentication key and resource account
+    /// derivation to produce an object address.
+    const OBJECT_DERIVED_SCHEME: u8 = 0xFC;


really need a centralized place for those domain separators.

Where else would they be outside of object?

resource acct scheme

Since constants are not public currently. We'll need to provide getter functions which is kinda annoying though

lightmark · 2023-04-04T21:15:50Z

aptos-move/framework/aptos-framework/sources/primary_wallet.move

+
+    #[resource_group_member(group = aptos_framework::object::ObjectGroup)]
+    /// Resource stored on the fungible asset metadata object to allow creating primary wallets for it.
+    struct PrimaryWalletSupport has key {


maybe rename as DeriveRefPod. Support as a resource looks weird.

Peas in a pod? Let's rename things all in one go in a followup PR

github-actions · 2023-04-05T02:15:29Z

✅ Forge suite `land_blocking` success on `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`

performance benchmark with full nodes : 5904 TPS, 6726 ms latency, 10600 ms p99 latency,(!) expired 840 out of 2521920 txns
Test Ok

github-actions · 2023-04-05T02:17:51Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7593 TPS, 5032 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce
compatibility::simple-validator-upgrade::single-validator-upgrade : 4224 TPS, 9814 ms latency, 12600 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce
compatibility::simple-validator-upgrade::half-validator-upgrade : 4440 TPS, 8961 ms latency, 11500 ms p99 latency,no expired txns
4. upgrading second batch to new version: dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6799 TPS, 5793 ms latency, 10400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce passed
Test Ok

github-actions · 2023-04-05T02:18:04Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60` ==> `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce (PR)
Upgrade the nodes to version: dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce
framework_upgrade::framework-upgrade::full-framework-upgrade : 6520 TPS, 5928 ms latency, 8600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce passed
Test Ok

davidiw

need some move-e2e-tests and maybe some api tests
missing tests for "primary wallet"
naming is hard

davidiw · 2023-04-04T21:28:58Z

aptos-move/framework/aptos-framework/sources/primary_wallet.move

+
+    /// Creators of fungible assets can call this to enable support for creating primary (deterministic) wallets for
+    /// their users.
+    public fun enable_primary_wallet(metadata_constructor_ref: &ConstructorRef) {


I thought the plan was the have another method that would wrap FungibleAssetMetadata?

What do you mean?

It seems what you'd really want is to use this to create the FungibleAssetMetadata

Huh? So move FungibleAssetMetadata to this module?

davidiw · 2023-04-05T04:13:28Z

aptos-move/framework/aptos-framework/sources/primary_wallet.move

+        let recipient_wallet = ensure_primary_wallet_exists(recipient, metadata);
+        fungible_asset::transfer(sender, sender_wallet, recipient_wallet, amount);
+    }
+}


the lack of test cases...

Yeah @lightmark will work on those

davidiw · 2023-04-05T04:14:11Z

aptos-move/framework/aptos-framework/sources/object.move

@@ -160,6 +188,14 @@ module aptos_framework::object {
        create_object_internal(creator_address, obj_addr, false)
    }

+    /// Create a new object whose address is derived based on the creator account address and another object.
+    /// Derivde objects, similar to named objects, cannot be deleted.
+    public fun create_derived_object(creator: &signer, derive_ref: &DeriveRef): ConstructorRef {


should this be user derived?

It's derived from the user addr + object addr. Do you mean something else?

create_user_derived_object

I'd make this friend only for now.

@davidiw , do we want users to call this to generate un-deletable objects?

They can't be deleted

davidiw · 2023-04-05T04:16:41Z

gah, please don't enable auto-merge in the future.

movekevin force-pushed the fungible-deter branch 4 times, most recently from 7132857 to e13fcd3 Compare March 25, 2023 04:50

movekevin changed the title ~~[Aptos Framework][Fungible Assets] Remove fungible_store and switch to deterministic object addresses for fungible asset wallet objects~~ Refactor fungible assets Mar 25, 2023

movekevin requested a review from lightmark March 25, 2023 04:57

movekevin force-pushed the fungible-deter branch 4 times, most recently from 2d861e7 to d8fc813 Compare March 26, 2023 20:45

davidiw reviewed Mar 27, 2023

View reviewed changes

lightmark reviewed Mar 27, 2023

View reviewed changes

movekevin force-pushed the fungible-deter branch from f027e88 to c91c148 Compare March 27, 2023 13:54

davidiw reviewed Mar 27, 2023

View reviewed changes

movekevin force-pushed the fungible-deter branch from c91c148 to f1c9e14 Compare March 27, 2023 17:24

lightmark reviewed Mar 27, 2023

View reviewed changes

movekevin force-pushed the fungible-deter branch 4 times, most recently from 1c9892f to 424bb20 Compare March 28, 2023 02:19

movekevin force-pushed the fungible-deter branch from 424bb20 to 0cfb6e7 Compare March 28, 2023 06:20

movekevin marked this pull request as ready for review March 28, 2023 06:21

movekevin requested review from clay-aptos, saharct, junkil-park and wrwg as code owners March 28, 2023 06:21

movekevin force-pushed the fungible-deter branch from 0cfb6e7 to fea8099 Compare March 28, 2023 06:22

movekevin requested review from davidiw and lightmark March 29, 2023 13:01

movekevin added 8 commits April 4, 2023 12:53

Add events to fungible_asset

1ea4bdb

Use init_module in managed_fungible_asset

dae6aa2

Return true for ungated_transfer_allowed if the wallet doesn't exist

0149853

Disable ungated transfers for primary wallets

4c49746

Simplify managed_fungible_asset example further to not need users to …

6afe2a9

…pass Object<FungibleAssetMetadata>

Add prover specs for fungible_asset and primary_wallet

c9b7f5e

Temporarily disable verification for storage_gas::validate_points and…

2f041ae

… new_gas_curve

Create fungible_asset's mint/burn/transfer separate from construction

2a4bc02

movekevin force-pushed the fungible-deter branch 2 times, most recently from f22dd39 to c07075b Compare April 4, 2023 20:06

Move deterministic (primary) wallet creation logic to primary_wallet

dc9d6ab

movekevin force-pushed the fungible-deter branch from c07075b to dc9d6ab Compare April 4, 2023 20:43

lightmark reviewed Apr 4, 2023

View reviewed changes

lightmark approved these changes Apr 5, 2023

View reviewed changes

movekevin requested a review from davidiw April 5, 2023 01:08

movekevin enabled auto-merge (rebase) April 5, 2023 01:45

This comment has been minimized.

Sign in to view

davidiw approved these changes Apr 5, 2023

View reviewed changes

movekevin merged commit c2bb1d4 into main Apr 5, 2023

movekevin deleted the fungible-deter branch April 5, 2023 04:16

lightmark mentioned this pull request Apr 11, 2023

[AIP-21][Discussion] Fungible Asset using objects aptos-foundation/AIPs#95

Closed

thepomeranian mentioned this pull request Apr 18, 2023

[AIP-22][Discussion] No-Code Token Objects aptos-foundation/AIPs#101

Closed

Refactor fungible assets #7379

Refactor fungible assets #7379

Conversation

movekevin commented Mar 24, 2023 • edited Loading

davidiw Mar 27, 2023 • 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

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

lightmark Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw 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

Choose a reason for hiding this comment

movekevin commented Mar 28, 2023

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.

github-actions bot commented Apr 5, 2023

✅ Forge suite land_blocking success on dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce

github-actions bot commented Apr 5, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce

github-actions bot commented Apr 5, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce

davidiw 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

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

davidiw commented Apr 5, 2023

movekevin commented Mar 24, 2023 •

edited

Loading

davidiw Mar 27, 2023 •

edited

Loading

lightmark Mar 27, 2023 •

edited

Loading

✅ Forge suite `land_blocking` success on `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60` ==> `dc9d6ab41dc743dc356282bb373f1cb1cc3f7bce`