Migrate token objects into the framework at 0x4 #7277

davidiw · 2023-03-18T22:31:36Z

Will start working immediately on writing / updating the appropriate AIPs.

movekevin · 2023-03-20T14:23:19Z

aptos-move/framework/aptos-token-objects/Move.toml

@@ -0,0 +1,13 @@
+[package]


Why make this a new package vs merging into aptos-token? This would require making sure aptos-token-objects is supported as a new package in many places (genesis, release-tooling, etc.)

I think this is the better approach... the constructs at 0x3 share names and would all be but confusing to co-exist.

Good call though... I need to actually run a local node and ensure that it is taken in otherwise this change won't impact devnet.

I'd suggest ensuring both genesis and framework upgrade work. Otherwise, this would potentially bypass tests, compat checks, etc. which can somehow break the release process down the line

A new package just because of shared names?

I worry users would end up even more confused.

movekevin · 2023-03-24T00:41:40Z

aptos-move/move-examples/token_objects/sources/aptos_token.move

+        object::address_to_object<AptosCollection>(collection_addr)
+    }
+
+    entry fun set_collection_description_call(


This is no longer required now that #7090 is getting merged soon. set_collection_description can become public fun. Similar comment for other functions here

movekevin · 2023-03-24T00:42:27Z

aptos-move/move-examples/token_objects/sources/aptos_token.move

+    }
+
+    #[test(creator = @0x123)]
+    entry fun est_property_remove(creator: &signer) acquires AptosCollection, AptosToken {


nit: missing t at the beginning of test name

movekevin · 2023-03-24T01:06:50Z

aptos-move/framework/aptos-token-objects/sources/collection.move

@@ -192,48 +192,6 @@ module aptos_token_objects::collection {
        }
    }

-    /// Entry function for creating a collection
-    public entry fun create_collection(


Makes sense to remove

movekevin · 2023-03-24T01:09:07Z

aptos-move/framework/aptos-token-objects/sources/property_map.move

@@ -247,7 +247,7 @@ module aptos_token_objects::property_map {
        let (type, value) = read(object, key);
        assert!(
            type == type_info::type_name<V>(),
-            error::invalid_argument(ETYPE_INVALID),


Can we use 2 different errors for the 2 remaining uses of ETYPE_INVALID as well?

I think it is the right error and they appear in different contexts. This was intended to be mismatch, but copypasta, I guess

movekevin · 2023-03-24T15:11:52Z

aptos-move/framework/aptos-token-objects/sources/collection.move

@@ -151,6 +151,9 @@ module aptos_token_objects::collection {
            royalty::init(&constructor_ref, option::extract(&mut royalty))
        };

+        let transfer_ref = object::generate_transfer_ref(&constructor_ref);


Awesome! Can you add a comment here explaining that transferring collection objects is disabled as it's meaningless - the new owner wouldn't gain any control over the collection.

lightmark · 2023-03-26T04:26:23Z

aptos-move/framework/aptos-token-objects/sources/aptos_token.move

@@ -562,11 +546,11 @@ module token_objects::aptos_token {
        collection: String,
        name: String,
        key: String,
-        type: String,
+        type_: String,


😮‍💨

lightmark · 2023-03-26T04:32:15Z

aptos-move/framework/aptos-token-objects/sources/aptos_token.move

        let aptos_token = authorized_borrow(&token, signer::address_of(creator));
        assert!(
-            option::is_some(&aptos_token.property_mutator_ref),
+            are_properties_mutable(token),


What's the rationale behind this change? Basically you use a bool of a collection to decide whether tokens in this collection has properties mutable and store a mutable_ref in all tokens? Why do you have this ref. My guess is for potential upgrade to control each one individually but not sure how this can work later.

I figure it is okay to just make this a collection level configuration for our no-code solution... the code complexity here isn't great and it is mostly just a bunch of control flags.. so I became a little oppressive here and there.

lightmark · 2023-03-26T04:38:19Z

aptos-move/framework/aptos-token-objects/Move.toml

@@ -0,0 +1,13 @@
+[package]


A new package just because of shared names?

construct the container for aptos-token-objects migrate examples token-objects to framework/aptos-token-objects

if a user attempts to freeze, unfreeze a soul bound token for which the collection enables freeze, unfreeze, they would get a weird error.

The current abigen depends on TypeTags to represent abis. Unfortunately this excludes it from representing any generic entry function. That should NOT prevent us from writing these entry functions in Move or in the framework. The current code creates that unnecessary restriction, so instead of panicing, just return none as if it cannot understand it.

upgrade to more effective interfaces -- still need to improve both the abigen in move and then the rust sdk builder

* Named Objects * Named tokens * Named collections * Moved auth key generation back into the account_address module from ed25519 for multi-ed as that's where it was defined for other frameworks

* this validates much of the token objects code * demonstrates a means to read objects efficiently using python * how to perform property type operations * read from property types * how to tie your shoes

github-actions · 2023-03-26T08:32:18Z

✅ Forge suite `land_blocking` success on `8b74f89f142a6e7d587301b050edb5aa216e6bf6`

performance benchmark with full nodes : 5640 TPS, 7027 ms latency, 12300 ms p99 latency,(!) expired 20 out of 2408620 txns
Test Ok

github-actions · 2023-03-26T08:33:34Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `8b74f89f142a6e7d587301b050edb5aa216e6bf6`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7539 TPS, 5066 ms latency, 7500 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 8b74f89f142a6e7d587301b050edb5aa216e6bf6
compatibility::simple-validator-upgrade::single-validator-upgrade : 4732 TPS, 8735 ms latency, 12100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 8b74f89f142a6e7d587301b050edb5aa216e6bf6
compatibility::simple-validator-upgrade::half-validator-upgrade : 4600 TPS, 8905 ms latency, 11500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 8b74f89f142a6e7d587301b050edb5aa216e6bf6
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6404 TPS, 5928 ms latency, 10300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6 passed
Test Ok

github-actions · 2023-03-26T08:35:26Z

❌ Forge suite `framework_upgrade` failure on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `8b74f89f142a6e7d587301b050edb5aa216e6bf6`

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6 (PR)
Upgrade the nodes to version: 8b74f89f142a6e7d587301b050edb5aa216e6bf6
Test Failed: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
Trailing Log Lines:
INCLUDING DEPENDENCY MoveStdlib
BUILDING ExecuteProposal
::error::API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
test framework_upgrade::framework-upgrade ... FAILED
Error: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED
Test Statistics: 
Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6 (PR)
Upgrade the nodes to version: 8b74f89f142a6e7d587301b050edb5aa216e6bf6
Test Failed: API error: Unknown error Transaction committed on chain, but failed execution: Transaction Executed and Committed with Error CONSTRAINT_NOT_SATISFIED


Swarm logs can be found here: See fgi output for more information.
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:281"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-7277-1679818405-cb4ba0a57c998c60cbab","timestamp":"2023-03-26T08:35:19.849662Z","message":"Deleting namespace forge-framework-upgrade-pr-7277: Some(NamespaceStatus { phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:389"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-7277-1679818405-cb4ba0a57c998c60cbab","timestamp":"2023-03-26T08:35:19.849687Z","message":"aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-7277"}

failures:
    framework_upgrade::framework-upgrade

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

Failed to run tests:
Tests Failed
Error: Tests Failed
Debugging output:
NAME                                   READY   STATUS      RESTARTS   AGE
aptos-node-0-validator-0               1/1     Running     0          14m
aptos-node-1-validator-0               1/1     Running     0          12m
aptos-node-2-validator-0               1/1     Running     0          10m
aptos-node-3-validator-0               1/1     Running     0          8m43s
aptos-node-4-validator-0               1/1     Running     0          6m53s
genesis-aptos-genesis-eforge32-vw99k   0/1     Completed   0          19m

davidiw requested review from clay-aptos, saharct, movekevin, wrwg, banool, gregnazario and 0xmaayan as code owners March 18, 2023 22:31

davidiw force-pushed the davidiw_token_pt2 branch 2 times, most recently from 39c8ce2 to e009be7 Compare March 19, 2023 01:11

movekevin reviewed Mar 20, 2023

View reviewed changes

Base automatically changed from davidiw_token to main March 22, 2023 21:43

movekevin reviewed Mar 24, 2023

View reviewed changes

davidiw force-pushed the davidiw_token_pt2 branch from e009be7 to bc68f50 Compare March 24, 2023 00:52

movekevin reviewed Mar 24, 2023

View reviewed changes

davidiw enabled auto-merge (rebase) March 24, 2023 04:52

This comment has been minimized.

Sign in to view

movekevin reviewed Mar 24, 2023

View reviewed changes

movekevin approved these changes Mar 24, 2023

View reviewed changes

davidiw disabled auto-merge March 26, 2023 04:37

lightmark approved these changes Mar 26, 2023

View reviewed changes

davidiw force-pushed the davidiw_token_pt2 branch from 0375413 to 9bb3ecf Compare March 26, 2023 05:00

movekevin approved these changes Mar 26, 2023

View reviewed changes

davidiw enabled auto-merge (rebase) March 26, 2023 05:03

davidiw added 8 commits March 25, 2023 23:50

[framework] add aptos-token-objects

f91f343

construct the container for aptos-token-objects migrate examples token-objects to framework/aptos-token-objects

[framework] burn property map on token burn

ab1c884

[framework] cleanups for token after auditing and documenting

19ad7fb

[framework] Propertymap proper errors and renaming entry function param

5f6c58d

[framework] distinguish frozen and soulbound

893b321

if a user attempts to freeze, unfreeze a soul bound token for which the collection enables freeze, unfreeze, they would get a weird error.

[framework] kevin doesn't like transferrable collections

0ab5e63

[framework] remove all non-Object<T> entry functions from AptosToken

4f68f23

upgrade to more effective interfaces -- still need to improve both the abigen in move and then the rust sdk builder

davidiw force-pushed the davidiw_token_pt2 branch 2 times, most recently from 56f0c93 to b649d89 Compare March 26, 2023 06:54

davidiw enabled auto-merge (rebase) March 26, 2023 06:55

This comment has been minimized.

Sign in to view

davidiw added 5 commits March 26, 2023 00:35

[genesis] enable new input type parsing for devnet

c0bad5f

[framework] add view attribute to aptos_token readers

8fd440b

[python] add support for generating account addresses

8925e65

* Named Objects * Named tokens * Named collections * Moved auth key generation back into the account_address module from ed25519 for multi-ed as that's where it was defined for other frameworks

[python] add aptos token client

3fa430e

* this validates much of the token objects code * demonstrates a means to read objects efficiently using python * how to perform property type operations * read from property types * how to tie your shoes

docs fix

8b74f89

davidiw force-pushed the davidiw_token_pt2 branch 2 times, most recently from 56f0c93 to 8b74f89 Compare March 26, 2023 07:44

This comment has been minimized.

Sign in to view

davidiw merged commit 3a9984b into main Mar 26, 2023

davidiw deleted the davidiw_token_pt2 branch March 26, 2023 08:33

thepomeranian mentioned this pull request Apr 21, 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

Migrate token objects into the framework at 0x4 #7277

Migrate token objects into the framework at 0x4 #7277

davidiw commented Mar 18, 2023

movekevin Mar 20, 2023

davidiw Mar 20, 2023

davidiw Mar 20, 2023

movekevin Mar 20, 2023

lightmark Mar 26, 2023

davidiw Mar 26, 2023

movekevin Mar 24, 2023

movekevin Mar 24, 2023

movekevin Mar 24, 2023

movekevin Mar 24, 2023

davidiw Mar 24, 2023

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.

movekevin Mar 24, 2023

lightmark Mar 26, 2023

davidiw Mar 26, 2023

lightmark Mar 26, 2023

davidiw Mar 26, 2023

lightmark Mar 26, 2023

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 Mar 26, 2023

github-actions bot commented Mar 26, 2023

github-actions bot commented Mar 26, 2023

Migrate token objects into the framework at 0x4 #7277

Migrate token objects into the framework at 0x4 #7277

Conversation

davidiw commented Mar 18, 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

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.

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.

github-actions bot commented Mar 26, 2023

✅ Forge suite land_blocking success on 8b74f89f142a6e7d587301b050edb5aa216e6bf6

github-actions bot commented Mar 26, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6

github-actions bot commented Mar 26, 2023

❌ Forge suite framework_upgrade failure on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 8b74f89f142a6e7d587301b050edb5aa216e6bf6

✅ Forge suite `land_blocking` success on `8b74f89f142a6e7d587301b050edb5aa216e6bf6`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `8b74f89f142a6e7d587301b050edb5aa216e6bf6`

❌ Forge suite `framework_upgrade` failure on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `8b74f89f142a6e7d587301b050edb5aa216e6bf6`