[Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 #3367

lightmark · 2022-08-23T23:42:14Z

…n from storage

Description

we need to delete a token entry from the table in TokenStore when its amount drops to 0.
Same applies to TokenData in a collection if supply drops to 0.

Move the sufficient balance checker inside withdraw_with_event_internal function to achieve better encapsulation.

Also, I don't think (left + right) / 2 is a good practice for binary search (theoretically potential overflow).

Test Plan

ut

This change is

areshand

Nice work, Bro.

lightmark · 2022-08-24T00:05:43Z

@areshand boger memeda

jjleng · 2022-08-24T04:56:05Z

aptos-move/framework/aptos-stdlib/sources/simple_map.move

-            let mid = (left + right) / 2;
+            let mid = left + (right - left) / 2;


left + ((right - left) / 2) = left + right/2 - left /2 = left/2 + right/2 = (left + right) / 2

Why bother?

see kevin's comment

please help me fix the ts tests tmr...

thoughtful lol. then how about left/2 + right/2?

this cannot ensure the correctness because of division rounding towards 0
if left = 1 and right = 3, left/2 + right/2 = 1.... but you want 2...

aptos-move/framework/aptos-stdlib/sources/simple_map.move

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

movekevin · 2022-08-24T05:02:58Z

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

@@ -520,11 +523,13 @@ module aptos_token::token {
            error::not_found(EBALANCE_NOT_PUBLISHED),
        );
        let balance = &mut table::borrow_mut(tokens, id).amount;
-        if (id.property_version == 0) {
+        if (id.property_version == 0 && *balance > amount) {


Why add && *balance > amount?

if balance == amount, I will delete the entry from the map. otherwise it is still there since the final balance is not 0,

Can we remove the id.property_version == 0 check ? If there's a single version and it gets deleted, balance and amount should both be 1?

Good point. @areshand

another question is: is it possible that amount = 0, and return Token {amount: 0, ..}?

@lightmark this is a good catch. we should assert amount > 0. It doesn't make sense to withdraw 0 token.

movekevin

Looks fine to me. I'll leave it to Bo to have a final say though

movekevin · 2022-08-24T06:05:49Z

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

@@ -520,11 +523,13 @@ module aptos_token::token {
            error::not_found(EBALANCE_NOT_PUBLISHED),
        );
        let balance = &mut table::borrow_mut(tokens, id).amount;
-        if (id.property_version == 0) {
+        if (id.property_version == 0 && *balance > amount) {


Can we remove the id.property_version == 0 check ? If there's a single version and it gets deleted, balance and amount should both be 1?

lightmark · 2022-08-24T22:40:29Z

@jjleng plz take a look for .ts files

…n from storage

github-actions · 2022-08-25T01:44:24Z

Forge is running with `153974bf53aaf8d40b6300e07ecafa45bdc6ce65`

❌ Forge test failure on `153974bf53aaf8d40b6300e07ecafa45bdc6ce65`

Forge test runner terminated

Grafana dashboard
Validator 0 logs
Humio Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `4f4e79b8733679feaea165ac235e5448e79292e0`

✅ Forge test success on `4f4e79b8733679feaea165ac235e5448e79292e0`

performance benchmark with full nodes : 6761 TPS, 4427 ms latency, 7000 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `97a2f3af343b919ceeac8f7948d928bbf2a5e8d1`

✅ Forge test success on `97a2f3af343b919ceeac8f7948d928bbf2a5e8d1`

performance benchmark with full nodes : 6533 TPS, 4570 ms latency, 6000 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `885c44f303c4bafc481416c8e9aeb5a68ccd2f4b`

✅ Forge test success on `885c44f303c4bafc481416c8e9aeb5a68ccd2f4b`

performance benchmark with full nodes : 6289 TPS, 4777 ms latency, 6000 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `0ec428c0ecccbbe5b6ba180ac8c73ba06de07547`

✅ Forge test success on `0ec428c0ecccbbe5b6ba180ac8c73ba06de07547`

performance benchmark with full nodes : 6297 TPS, 4751 ms latency, 6050 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

…n from storage (#3367)

…n from storage (aptos-labs#3367)

lightmark requested a review from areshand August 23, 2022 23:42

lightmark requested review from davidiw, movekevin and wrwg as code owners August 23, 2022 23:42

areshand approved these changes Aug 24, 2022

View reviewed changes

lightmark enabled auto-merge (squash) August 24, 2022 00:12

lightmark force-pushed the token_deletion branch 2 times, most recently from 4f4e79b to 97a2f3a Compare August 24, 2022 04:14

lightmark requested review from jjleng, gregnazario and banool as code owners August 24, 2022 04:14

lightmark changed the title ~~[Aptos Framework][Token] fully support deletion of TokenData and Toke…~~ [Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 Aug 24, 2022

jjleng reviewed Aug 24, 2022

View reviewed changes

movekevin reviewed Aug 24, 2022

View reviewed changes

lightmark requested a review from 0xchloe August 24, 2022 19:41

lightmark force-pushed the token_deletion branch 2 times, most recently from 19b64d9 to 885c44f Compare August 24, 2022 22:39

[Aptos Framework][Token] fully support deletion of TokenData and Toke…

0ec428c

…n from storage

lightmark force-pushed the token_deletion branch from 885c44f to 0ec428c Compare August 25, 2022 01:05

lightmark merged commit 0e05976 into main Aug 25, 2022

lightmark deleted the token_deletion branch August 25, 2022 01:44

banool mentioned this pull request Aug 25, 2022

Add TS component of the First NFT with SDK tutorial #3428

Merged

0xchloe pushed a commit that referenced this pull request Aug 28, 2022

[Aptos Framework][Token] fully support deletion of TokenData and Toke…

3236434

…n from storage (#3367)

Markuze pushed a commit to Markuze/aptos-core that referenced this pull request Sep 2, 2022

[Aptos Framework][Token] fully support deletion of TokenData and Toke…

d905e32

…n from storage (aptos-labs#3367)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 #3367

[Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 #3367

lightmark commented Aug 23, 2022 •

edited by geekflyer

Loading

areshand left a comment

lightmark commented Aug 24, 2022

jjleng Aug 24, 2022

lightmark Aug 24, 2022

lightmark Aug 24, 2022

jjleng Aug 24, 2022

lightmark Aug 24, 2022 •

edited

Loading

movekevin Aug 24, 2022

lightmark Aug 24, 2022

movekevin Aug 24, 2022

lightmark Aug 24, 2022

lightmark Aug 24, 2022

areshand Aug 24, 2022

movekevin left a comment

movekevin Aug 24, 2022

lightmark commented Aug 24, 2022

github-actions bot commented Aug 25, 2022

		let mid = (left + right) / 2;
		let mid = left + (right - left) / 2;

[Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 #3367

[Aptos Framework][Token] fully support deletion of TokenData and Token if reach 0 #3367

Conversation

lightmark commented Aug 23, 2022 • edited by geekflyer Loading

Description

Test Plan

areshand left a comment

Choose a reason for hiding this comment

lightmark commented Aug 24, 2022

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 Aug 24, 2022 • edited Loading

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

movekevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Aug 24, 2022

github-actions bot commented Aug 25, 2022

Forge is running with 153974bf53aaf8d40b6300e07ecafa45bdc6ce65

❌ Forge test failure on 153974bf53aaf8d40b6300e07ecafa45bdc6ce65

Forge is running with 4f4e79b8733679feaea165ac235e5448e79292e0

✅ Forge test success on 4f4e79b8733679feaea165ac235e5448e79292e0

Forge is running with 97a2f3af343b919ceeac8f7948d928bbf2a5e8d1

✅ Forge test success on 97a2f3af343b919ceeac8f7948d928bbf2a5e8d1

Forge is running with 885c44f303c4bafc481416c8e9aeb5a68ccd2f4b

✅ Forge test success on 885c44f303c4bafc481416c8e9aeb5a68ccd2f4b

Forge is running with 0ec428c0ecccbbe5b6ba180ac8c73ba06de07547

✅ Forge test success on 0ec428c0ecccbbe5b6ba180ac8c73ba06de07547

lightmark commented Aug 23, 2022 •

edited by geekflyer

Loading

lightmark Aug 24, 2022 •

edited

Loading

Forge is running with `153974bf53aaf8d40b6300e07ecafa45bdc6ce65`

❌ Forge test failure on `153974bf53aaf8d40b6300e07ecafa45bdc6ce65`

Forge is running with `4f4e79b8733679feaea165ac235e5448e79292e0`

✅ Forge test success on `4f4e79b8733679feaea165ac235e5448e79292e0`

Forge is running with `97a2f3af343b919ceeac8f7948d928bbf2a5e8d1`

✅ Forge test success on `97a2f3af343b919ceeac8f7948d928bbf2a5e8d1`

Forge is running with `885c44f303c4bafc481416c8e9aeb5a68ccd2f4b`

✅ Forge test success on `885c44f303c4bafc481416c8e9aeb5a68ccd2f4b`

Forge is running with `0ec428c0ecccbbe5b6ba180ac8c73ba06de07547`

✅ Forge test success on `0ec428c0ecccbbe5b6ba180ac8c73ba06de07547`