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] add partial withdraw with capability #5652

Merged
merged 1 commit into from
Dec 1, 2022
Merged

[token] add partial withdraw with capability #5652

merged 1 commit into from
Dec 1, 2022

Conversation

areshand
Copy link
Contributor

Description

address the request on enabling partial withdraw token with capability.

This allows people list x fungible tokens and buyer can buy a smaller than x amount. The rest of the tokens can still be listed on sale until all the tokens are bought

Test Plan

added unit test

CapCap
CapCap reviewed Nov 30, 2022
View reviewed changes
aptos-move/framework/aptos-token/sources/token.move Outdated
@@ -272,6 +275,14 @@ module aptos_token::token {
expiration_sec: u64,
}

/// capability to do partial withdraw with signer. The capability owner can withdraw up to amount token
struct PartialWithdrawCapability has drop, store {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if I create both a WithdrawCapability and a PartialWithdrawCapability for the same item?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks weird. It seems the WithdrawCapability can include PartialWithdrawCapability and add a withdraw_with_capability_v2 with an extra amount param. So why not just reuse WithdrawCapability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. withdraw capability is just a capability that allows the capability owner to withdraw from NFT holder's token store. There is no guarantee that token exists.
    This is designed this way to avoid escrowing the token. If there is no sufficient balance, the withdraw txn fails.

  2. WithdrawCapability and PartialWithdrawCapability have very different meaning. The first is one time capability. You use it once and it is gone no matter if you have withdrawn the full amount. The 2nd allows you to use multiple times untils you withdraw entire amount.
    So the NFT holder should explicitly create this PartialWithdrawCapability to give permission on multiple partial withdraws.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the API and the fields they have. There is no reason to distinguish them IMO. allowing partial withdraw should be a subset of allowing full withdraw. I mean, if you allow ppl to withdraw in full, you should allow them to withdraw partially by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

They is real a CapCap PR. hahahahahahah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, remove the redundant struct and only keep the new function

@areshand areshand requested a review from CapCap December 1, 2022 04:43
@areshand areshand force-pushed the add_partial_withdraw_capability branch from 18e86bb to facd18c Compare December 1, 2022 05:40
@areshand areshand force-pushed the add_partial_withdraw_capability branch from facd18c to 9fbaa55 Compare December 1, 2022 05:45
@areshand areshand force-pushed the add_partial_withdraw_capability branch from 9fbaa55 to 08f49b7 Compare December 1, 2022 19:49
@areshand areshand enabled auto-merge (rebase) December 1, 2022 19:49
@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

github-actions bot commented Dec 1, 2022

✅ Forge suite land_blocking success on 08f49b779a97e648718c3efdc3d4b70fdd1de234

performance benchmark with full nodes : 6673 TPS, 5888 ms latency, 13200 ms p99 latency,(!) expired 2160 out of 2851840 txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 08f49b779a97e648718c3efdc3d4b70fdd1de234

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 08f49b779a97e648718c3efdc3d4b70fdd1de234 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7380 TPS, 5224 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 08f49b779a97e648718c3efdc3d4b70fdd1de234
compatibility::simple-validator-upgrade::single-validator-upgrade : 4884 TPS, 8185 ms latency, 10300 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 08f49b779a97e648718c3efdc3d4b70fdd1de234
compatibility::simple-validator-upgrade::half-validator-upgrade : 4556 TPS, 8877 ms latency, 11400 ms p99 latency,no expired txns
4. upgrading second batch to new version: 08f49b779a97e648718c3efdc3d4b70fdd1de234
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6887 TPS, 5623 ms latency, 9000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 08f49b779a97e648718c3efdc3d4b70fdd1de234 passed
Test Ok

@lightmark lightmark merged commit c4f5fb6 into aptos-labs:main Dec 1, 2022
@Markuze Markuze mentioned this pull request Dec 5, 2022
Closed
areshand added a commit to areshand/aptos-core-1 that referenced this pull request Dec 16, 2022
@areshand
[token] add partial withdraw with capability (aptos-labs#5652)
a7c2733
areshand added a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
@areshand
[token] add partial withdraw with capability (aptos-labs#5652)
e899179
movekevin pushed a commit that referenced this pull request Dec 19, 2022
@areshand @movekevin
[token] add partial withdraw with capability (#5652)
5a28a40
@Markuze Markuze mentioned this pull request Dec 26, 2022
Closed
@Markuze Markuze mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightmark lightmark lightmark approved these changes

@0xchloe 0xchloe 0xchloe approved these changes

@movekevin movekevin Awaiting requested review from movekevin

@davidiw davidiw Awaiting requested review from davidiw

@CapCap CapCap Awaiting requested review from CapCap

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