[fungible_assets] fix bugs, add tests, rename and change object api #7608

lightmark · 2023-04-06T17:38:02Z

Description

rename wallet to store. rename FungibleAssetSupport to DeriveRefPod.
add tests to primary_store. Also fixed a bug there.
change create_user_derived_object to be a friend function w/o signer but use address instead.

Test Plan

ut

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

aptos-move/framework/aptos-framework/sources/primary_store.move

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

movekevin · 2023-04-06T17:47:25Z

aptos-move/framework/aptos-framework/sources/primary_store.move

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


Why is DeriveRefPod a better name?

Because it can be used for other purpose. If you agree that function is friend function, I agree to change it back to the original name. Otherwise, it is just a holder of derived ref.

@gregnazario Any naming suggestion here? :P

I mean I would make it PrimaryWalletCapability, though I think we overloaded that word now...

PrimaryWalletDeriveRef

Tells you what it is, and what it's used for. Do you suspect it'll be used for anything else in the future? Then

FungibleAssetDeriveRef

aptos-move/framework/aptos-framework/sources/primary_store.move

gregnazario · 2023-04-11T15:33:04Z

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

@@ -471,6 +472,14 @@ module aptos_framework::object {
    }

    /// Accessors


This should be a // and not a ///

gregnazario · 2023-04-11T15:39:50Z

aptos-move/framework/aptos-framework/sources/primary_store.move

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


I mean I would make it PrimaryWalletCapability, though I think we overloaded that word now...

PrimaryWalletDeriveRef

Tells you what it is, and what it's used for. Do you suspect it'll be used for anything else in the future? Then

FungibleAssetDeriveRef

lightmark · 2023-04-11T20:53:23Z

@movekevin Can you take another look of fungible_asset.move for the aggregator?

movekevin · 2023-04-11T21:22:22Z

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

+    /// Maximum possible coin supply.
+    const MAX_U128: u128 = 340282366920938463463374607431768211455;
+
+    struct Supply has store {


@georgemitenkov

movekevin · 2023-04-11T21:33:19Z

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

+        object::object_from_constructor_ref<FungibleStore>(constructor_ref)
+    }
+
+    public fun remove_store(delete_ref: &DeleteRef) acquires FungibleStore, FungibleAssetEvents {


Why do we want this?

movekevin · 2023-04-11T21:34:00Z

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

+    const MAX_U128: u128 = 340282366920938463463374607431768211455;
+
+    struct Supply has store {
+        current: OptionalAggregator,


Do we need a function to "activate" aggregator for this or can we add later?

movekevin · 2023-04-11T21:35:52Z

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

+    const ASSET_SYMBOL: vector<u8> = b"APT";
+
+    /// Initialize metadata object and store the refs.
+    fun init_module(admin: &signer) {


Why have two separate modules and not just one? In many cases people just deploy once and expect the fungible asset to be created

movekevin · 2023-04-12T19:20:09Z

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

        /// The address of the base metadata object.
        metadata: Object<Metadata>,
        /// The balance of the fungible metadata.
        balance: u64,
        /// Fungible Assets transferring is a common operation, this allows for freezing/unfreezing accounts.
-        allow_ungated_transfer: bool,
+        allow_ungated_balance_transfer: bool,


allow_owner_balance_transfer?

movekevin · 2023-04-12T19:20:52Z

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

+    // Constants
+    //
+
+    const MAX_NAME_LENGTH: u64 = 32;


How did you come up with these numbers?

github-actions · 2023-04-12T19:35:31Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `7ad895827c880f6c1602933a951d8d7d792379e8`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7ad895827c880f6c1602933a951d8d7d792379e8 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7795 TPS, 4908 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 7ad895827c880f6c1602933a951d8d7d792379e8
compatibility::simple-validator-upgrade::single-validator-upgrade : 4649 TPS, 8807 ms latency, 12000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 7ad895827c880f6c1602933a951d8d7d792379e8
compatibility::simple-validator-upgrade::half-validator-upgrade : 4846 TPS, 8462 ms latency, 11500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 7ad895827c880f6c1602933a951d8d7d792379e8
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6587 TPS, 5879 ms latency, 9600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7ad895827c880f6c1602933a951d8d7d792379e8 passed
Test Ok

github-actions · 2023-04-12T19:36:26Z

✅ Forge suite `land_blocking` success on `7ad895827c880f6c1602933a951d8d7d792379e8`

performance benchmark with full nodes : 5562 TPS, 7097 ms latency, 15100 ms p99 latency,(!) expired 20 out of 2375220 txns
Test Ok

github-actions · 2023-04-12T19:40:32Z

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

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

lightmark force-pushed the lightmark/fa_again branch 2 times, most recently from e86dbf6 to c9b05f1 Compare April 6, 2023 17:40

lightmark marked this pull request as ready for review April 6, 2023 17:40

lightmark requested review from clay-aptos, saharct, junkil-park, davidiw, movekevin and wrwg as code owners April 6, 2023 17:40

movekevin reviewed Apr 6, 2023

View reviewed changes

aptos-move/move-examples/fungible_asset/sources/managed_fungible_asset.move Outdated Show resolved Hide resolved

movekevin reviewed Apr 6, 2023

View reviewed changes

aptos-move/framework/aptos-framework/sources/primary_store.move Show resolved Hide resolved

movekevin reviewed Apr 6, 2023

View reviewed changes

aptos-move/framework/aptos-framework/sources/object.move Show resolved Hide resolved

movekevin reviewed Apr 6, 2023

View reviewed changes

rename

535f4f5

lightmark force-pushed the lightmark/fa_again branch from c9b05f1 to 889dd58 Compare April 6, 2023 18:24

[fungible_assets] fix bugs, add tests and change object api

c767194

lightmark force-pushed the lightmark/fa_again branch from 889dd58 to c767194 Compare April 6, 2023 20:34

lightmark added 4 commits April 10, 2023 10:44

rename fungible_asset_store to fungible_store

2edcdf9

rename fa ungated transfer name for ux purpose

9d1488d

add remove_store

b31c405

more rename

51a7ff7

movekevin approved these changes Apr 11, 2023

View reviewed changes

gregnazario approved these changes Apr 11, 2023

View reviewed changes

add optional_aggregator

80d26d9

lightmark mentioned this pull request Apr 11, 2023

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

Closed

add back managed_coin example

be71d3b

lightmark force-pushed the lightmark/fa_again branch 2 times, most recently from d7b03b5 to 15d8c88 Compare April 12, 2023 18:32

add limit to name/symbol length

7ad8958

lightmark force-pushed the lightmark/fa_again branch from 15d8c88 to 7ad8958 Compare April 12, 2023 18:59

lightmark enabled auto-merge (rebase) April 12, 2023 19:04

This comment has been minimized.

Sign in to view

movekevin reviewed Apr 12, 2023

View reviewed changes

lightmark merged commit 0e28fad into main Apr 12, 2023

lightmark deleted the lightmark/fa_again branch April 12, 2023 19:41

thepomeranian mentioned this pull request Apr 18, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fungible_assets] fix bugs, add tests, rename and change object api #7608

[fungible_assets] fix bugs, add tests, rename and change object api #7608

lightmark commented Apr 6, 2023

movekevin Apr 6, 2023

lightmark Apr 6, 2023

movekevin Apr 11, 2023

gregnazario Apr 11, 2023

gregnazario Apr 11, 2023

gregnazario Apr 11, 2023

lightmark commented Apr 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

movekevin Apr 11, 2023

movekevin Apr 11, 2023

movekevin Apr 11, 2023

movekevin Apr 11, 2023

movekevin Apr 12, 2023

movekevin Apr 12, 2023

github-actions bot commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

		@@ -471,6 +472,14 @@ module aptos_framework::object {
		}

		/// Accessors

[fungible_assets] fix bugs, add tests, rename and change object api #7608

[fungible_assets] fix bugs, add tests, rename and change object api #7608

Conversation

lightmark commented Apr 6, 2023

Description

Test Plan

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 commented Apr 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

github-actions bot commented Apr 12, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7ad895827c880f6c1602933a951d8d7d792379e8

github-actions bot commented Apr 12, 2023

✅ Forge suite land_blocking success on 7ad895827c880f6c1602933a951d8d7d792379e8

github-actions bot commented Apr 12, 2023

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

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `7ad895827c880f6c1602933a951d8d7d792379e8`

✅ Forge suite `land_blocking` success on `7ad895827c880f6c1602933a951d8d7d792379e8`

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