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.

Sign up for GitHub

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

Jump to bottom

[token] added functions to rotate collectiondata fields #5265

Merged
merged 1 commit into from
Oct 28, 2022
Merged

[token] added functions to rotate collectiondata fields #5265

merged 1 commit into from
Oct 28, 2022

Conversation

0xchloe
Copy link
Contributor

@0xchloe 0xchloe commented Oct 25, 2022 •
edited by geekflyer
Loading

Description

^

Test Plan

unit tests

This change is Reviewable

@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch from d94441a to 851e5a3 Compare October 25, 2022 07:41
@0xchloe 0xchloe requested a review from areshand October 25, 2022 07:42
@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch from 851e5a3 to d054fe0 Compare October 25, 2022 21:24
areshand
areshand requested changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@areshand areshand left a comment

Choose a reason for hiding this comment

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

need to check the tokendata maximum mutation to be bigger or equal to supply as well

@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch 4 times, most recently from 6ce323c to b5544fe Compare October 27, 2022 20:12
@0xchloe 0xchloe requested a review from areshand October 27, 2022 20:12
movekevin
movekevin reviewed Oct 27, 2022
View reviewed changes
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.

Are we not emitting events for these mutations?

aptos-move/framework/aptos-token/sources/token.move Outdated
assert_collection_exists(creator_address, collection_name);
let collection_data = table::borrow_mut(&mut borrow_global_mut<Collections>(creator_address).collection_data, collection_name);
// cannot change maximum from 0 and cannot change maximum to 0
assert!(collection_data.maximum != 0 && maximum !=0, error::invalid_argument(EINVALID_MAXIMUM));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after maximum !=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's really hard to add events for now given the current event framework, but we're going to add events in the future? cc @areshand

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we have a proposal. Now just need to add a converter at API side to deserialize propertyMap properly https://www.notion.so/aptoslabs/Add-General-Token-Events-9ee2eb3c95fe49fb8367879331b7e91f

clay-aptos
clay-aptos approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@clay-aptos clay-aptos left a comment

Choose a reason for hiding this comment

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

Thank you, Chloe! The Markdown LGTM.

@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch from b5544fe to 9798f00 Compare October 27, 2022 21:42
@0xchloe 0xchloe requested a review from movekevin October 27, 2022 21:49
@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch 3 times, most recently from 78843fa to 389273f Compare October 27, 2022 23:56
@0xchloe 0xchloe enabled auto-merge (squash) October 27, 2022 23:57
@0xchloe 0xchloe force-pushed the mutate-collection-metadata branch from 389273f to 63b93ee Compare October 28, 2022 00:39
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

✅ Forge suite compat success on 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 63b93ee7dc806ffe75a6c435976fc49428579c28

Compatibility test results for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 63b93ee7dc806ffe75a6c435976fc49428579c28 (PR)
1. Check liveness of validators at old version: 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7513 TPS, 5132 ms latency, 7100 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 63b93ee7dc806ffe75a6c435976fc49428579c28
compatibility::simple-validator-upgrade::single-validator-upgrade : 4906 TPS, 8485 ms latency, 11000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 63b93ee7dc806ffe75a6c435976fc49428579c28
compatibility::simple-validator-upgrade::half-validator-upgrade : 4570 TPS, 9296 ms latency, 13100 ms p99 latency,no expired txns
4. upgrading second batch to new version: 63b93ee7dc806ffe75a6c435976fc49428579c28
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6892 TPS, 5584 ms latency, 11400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 63b93ee7dc806ffe75a6c435976fc49428579c28 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 63b93ee7dc806ffe75a6c435976fc49428579c28

performance benchmark with full nodes : 6875 TPS, 5815 ms latency, 10000 ms p99 latency,(!) expired 40 out of 2935880 txns
Test Ok

@0xchloe 0xchloe merged commit ef6d3f4 into main Oct 28, 2022
@0xchloe 0xchloe deleted the mutate-collection-metadata branch October 28, 2022 01:45
This was referenced Nov 3, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clay-aptos clay-aptos clay-aptos approved these changes

@areshand areshand areshand approved these changes

@davidiw davidiw Awaiting requested review from davidiw

@wrwg wrwg Awaiting requested review from wrwg

@rajkaramchedu rajkaramchedu Awaiting requested review from rajkaramchedu

@movekevin movekevin Awaiting requested review from movekevin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xchloe @areshand @movekevin @clay-aptos