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] fix collection supply underflow #5096

Merged
merged 1 commit into from
Oct 31, 2022
Merged

[token] fix collection supply underflow #5096

merged 1 commit into from
Oct 31, 2022

Conversation

areshand
Copy link
Contributor

@areshand areshand commented Oct 17, 2022 •
edited by geekflyer
Loading

Description

Test Plan

This change is Reviewable

@areshand areshand requested review from movekevin and davidiw October 17, 2022 13:56
movekevin
movekevin reviewed Oct 26, 2022
View reviewed changes
aptos-move/framework/aptos-token/sources/token.move
destroy_collection_data(table::remove(&mut collections.collection_data, collection_data.name));
if (collection_data.maximum > 0) {
collection_data.supply = collection_data.supply - 1;
// delete the collection data if the collection supply equals 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to delete the collection? This would force the creator to have to re-create it later if they want to mint the tokens, which costs a lot more gas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create collection only create a collectData, which is about 0.002 APT as well. we probably want to be more proactive in cleaning unused data now since there is no rent

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msmouse will love @areshand

@areshand areshand requested review from 0xchloe and movekevin October 29, 2022 03:29
@areshand areshand mentioned this pull request Oct 31, 2022
Closed
@areshand areshand enabled auto-merge (rebase) October 31, 2022 02:38
@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 land_blocking success on 3ca25c0b79a68a2e292357423b8d99bc3fd84f54

performance benchmark with full nodes : 6743 TPS, 5909 ms latency, 8100 ms p99 latency,(!) expired 580 out of 2880180 txns
Test Ok

@areshand areshand merged commit c9ff6b6 into aptos-labs:main Oct 31, 2022
@github-actions
Copy link
Contributor

✅ Forge suite compat success on 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3ca25c0b79a68a2e292357423b8d99bc3fd84f54

Compatibility test results for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3ca25c0b79a68a2e292357423b8d99bc3fd84f54 (PR)
1. Check liveness of validators at old version: 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7484 TPS, 5176 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 3ca25c0b79a68a2e292357423b8d99bc3fd84f54
compatibility::simple-validator-upgrade::single-validator-upgrade : 4464 TPS, 9124 ms latency, 12100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 3ca25c0b79a68a2e292357423b8d99bc3fd84f54
compatibility::simple-validator-upgrade::half-validator-upgrade : 4587 TPS, 9049 ms latency, 12100 ms p99 latency,no expired txns
4. upgrading second batch to new version: 3ca25c0b79a68a2e292357423b8d99bc3fd84f54
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7036 TPS, 5524 ms latency, 9300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 3ca25c0b79a68a2e292357423b8d99bc3fd84f54 passed
Test Ok

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

@lightmark lightmark lightmark left review comments

@movekevin movekevin movekevin approved these changes

@0xchloe 0xchloe 0xchloe approved these changes

@davidiw davidiw Awaiting requested review from davidiw

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
@areshand @lightmark @0xchloe @movekevin