Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refer to create rather than create_from_account for token v2 #9430

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

banool
Copy link
Contributor

@banool banool commented Aug 2, 2023

Description

The docs refer to create_from_account but this function is deprecated, so I make them refer to create instead.

Additionally, create_from_account is marked as deprecated but nothing says what to use instead. I add a comment explaining what to do.

As you can see my editor also obliterates trailing spaces, hopefully that's alright. We should probably make the linter do that anyway.

Test Plan

👀

@banool banool marked this pull request as ready for review August 2, 2023 12:42
@banool banool requested review from gregnazario and xbtmatt August 2, 2023 12:42
Copy link
Contributor

@movekevin movekevin left a comment

Choose a reason for hiding this comment

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

Lets hold off on this a bit as this is not enabled on testnet and mainnet yet

@banool
Copy link
Contributor Author

banool commented Aug 2, 2023

Got it, I'll add a label.

@xbtmatt
Copy link
Contributor

xbtmatt commented Aug 2, 2023

I brought this up with Kevin already but yeah we may need to think more about how the new create_object is going to be inserted into contracts bc 1. it makes objects even harder to track, especially in terms of when it's invoked by a contract that's built around using GUIDs, but also importantly 2. because it will break a few tests in some of the token examples that assume a GUID is being used to create an object.

@davidiw
Copy link
Contributor

davidiw commented Aug 5, 2023

  1. because it will break a few tests in some of the token examples that assume a GUID is being used to create an object.

@xbtmatt can you enumerate those? I updated the Python example to use events, it wasn't too bad.

@xbtmatt
Copy link
Contributor

xbtmatt commented Aug 5, 2023

  1. because it will break a few tests in some of the token examples that assume a GUID is being used to create an object.

@xbtmatt can you enumerate those? I updated the Python example to use events, it wasn't too bad.

I thought there were more but maybe not :p

These are the only two I could find, in @gregnazario's test_utils module for the marketplace contract

aptos_token::mint(
seller,
collection_name,
string::utf8(b"description"),
string::utf8(b"token_name"),
string::utf8(b"uri"),
vector::empty(),
vector::empty(),
vector::empty(),
);
let obj_addr = object::create_guid_object_address(seller_addr, token_creation_num);

aptos_token::mint(
seller,
collection_name,
string::utf8(b"description"),
string::utf8(b"token_name_2"),
string::utf8(b"uri"),
vector::empty(),
vector::empty(),
vector::empty(),
);
let obj_addr = object::create_guid_object_address(seller_addr, token_creation_num);

Although they will obviously not fail in certain environments, like for the CLI they won't until it uses the release that enables the auid feature flag

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

now is the time!

@davidiw
Copy link
Contributor

davidiw commented Aug 26, 2023

Gah there are other conflicts...

@davidiw
Copy link
Contributor

davidiw commented Sep 25, 2023

@banool please update and we'll land...

@banool banool force-pushed the banool/token-docs-create branch from f4c822b to 1a39fa2 Compare October 24, 2023 18:06
@banool banool enabled auto-merge (squash) October 24, 2023 18:07
@banool banool disabled auto-merge October 24, 2023 18:11
@banool banool merged commit cf2e8a3 into main Oct 24, 2023
57 of 76 checks passed
@banool banool deleted the banool/token-docs-create branch October 24, 2023 18:11
@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 compat success on aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb

Compatibility test results for aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb (PR)
1. Check liveness of validators at old version: aptos-node-v1.7.2
compatibility::simple-validator-upgrade::liveness-check : committed: 3476 txn/s, latency: 6022 ms, (p50: 5100 ms, p90: 9300 ms, p99: 20600 ms), latency samples: 208580
2. Upgrading first Validator to new version: 1a39fa2efe16c551279d70229184ed5a5e7a6afb
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1780 txn/s, latency: 16203 ms, (p50: 19200 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92600
3. Upgrading rest of first batch to new version: 1a39fa2efe16c551279d70229184ed5a5e7a6afb
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1696 txn/s, latency: 15712 ms, (p50: 19100 ms, p90: 21900 ms, p99: 22600 ms), latency samples: 89900
4. upgrading second batch to new version: 1a39fa2efe16c551279d70229184ed5a5e7a6afb
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3600 txn/s, latency: 8760 ms, (p50: 9600 ms, p90: 12600 ms, p99: 12800 ms), latency samples: 144000
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 1a39fa2efe16c551279d70229184ed5a5e7a6afb

two traffics test: inner traffic : committed: 8040 txn/s, latency: 4882 ms, (p50: 4800 ms, p90: 5700 ms, p99: 9900 ms), latency samples: 3473500
two traffics test : committed: 100 txn/s, latency: 2181 ms, (p50: 2100 ms, p90: 2700 ms, p99: 6100 ms), latency samples: 1760
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.214, avg: 0.199", "QsPosToProposal: max: 0.169, avg: 0.151", "ConsensusProposalToOrdered: max: 0.616, avg: 0.597", "ConsensusOrderedToCommit: max: 0.514, avg: 0.492", "ConsensusProposalToCommit: max: 1.124, avg: 1.089"]
Max round gap was 1 [limit 4] at version 1612428. Max no progress secs was 4.242273 [limit 10] at version 1612428.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb

Compatibility test results for aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb (PR)
Upgrade the nodes to version: 1a39fa2efe16c551279d70229184ed5a5e7a6afb
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4033 txn/s, latency: 4950 ms, (p50: 4800 ms, p90: 8000 ms, p99: 8500 ms), latency samples: 242000
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> 1a39fa2efe16c551279d70229184ed5a5e7a6afb passed
Test Ok

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