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

[Aptos Framework][Coin] Switch to an explicit opt-out model for coin transfers #5871

Merged
merged 4 commits into from
Dec 18, 2022

Conversation

movekevin
Copy link
Contributor

@movekevin movekevin commented Dec 14, 2022

Description

Context:
Currently, in many cases, coins (ERC-20 tokens, including APT) cannot be sent directly to an account if:

  • The account has not been created. aptos_account::transfer or account::create_account needs to be called by another account that can pay for gas. This is generally a slight annoyance but not too big of a pain.
  • The account has not registered to receive that coin type. This has to be done for every custom coin (APT is registered by default when creating an account). This is the main cause of complaints/pain.

Proposed solution:
Switch over to an explicit opt-out model. By default, an account can receive transfers of coins they have not registered via aptos_account::transfer_coins. They can explicitly opt-out any time via aptos_account:: set_allow_direct_coin_transfers

Test Plan

Unit tests

@movekevin movekevin requested a review from areshand December 15, 2022 00:12
@movekevin movekevin force-pushed the coin-register branch 3 times, most recently from e4886b9 to 2103c60 Compare December 15, 2022 20:42
@@ -22,13 +42,37 @@ module aptos_framework::aptos_account {
coin::register<AptosCoin>(&signer);
}

/// Convenient function to transfer APT to a recipient account that might not exist.
/// This would create the recipient account first, which also registers it to receive APT, before transferring.
public entry fun transfer(source: &signer, to: address, amount: u64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we call this function by a more explicit name? smth like transfer_to_existing_or_nonexisting_account - this does sound way more verbose than transfer, but think transfer is a simple name that people might mis-use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really verbose. I'm not sure how people would misuse transfer here as it's meant to implicitly create an account (and may be register the coin type) if one doesn't exist yet. As proposed (I'll create an AIP soon), this new flow/function should be the default to use so users wouldn't need to worry about the nuances of accounts and coin registration.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I don't feel strong about this, just thought that some devs might use the transfer function here as the default transfer function without realizing that they could be transferring coins to an account that doesn't exist yet. as long as we make it clear, sgtm

@movekevin movekevin enabled auto-merge (rebase) December 16, 2022 07:55
@movekevin movekevin disabled auto-merge December 16, 2022 08:35
@movekevin movekevin enabled auto-merge (rebase) December 16, 2022 08:35
@movekevin movekevin disabled auto-merge December 18, 2022 01:51
@movekevin movekevin enabled auto-merge (squash) December 18, 2022 01:51
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 7089c7be63f5f7043a941356438c6213fa00b94b

performance benchmark with full nodes : 6046 TPS, 6554 ms latency, 12700 ms p99 latency,(!) expired 20 out of 2581900 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7089c7be63f5f7043a941356438c6213fa00b94b

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7089c7be63f5f7043a941356438c6213fa00b94b (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7186 TPS, 5402 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 7089c7be63f5f7043a941356438c6213fa00b94b
compatibility::simple-validator-upgrade::single-validator-upgrade : 4266 TPS, 9542 ms latency, 12600 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 7089c7be63f5f7043a941356438c6213fa00b94b
compatibility::simple-validator-upgrade::half-validator-upgrade : 4455 TPS, 9141 ms latency, 11500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 7089c7be63f5f7043a941356438c6213fa00b94b
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6366 TPS, 6190 ms latency, 10900 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7089c7be63f5f7043a941356438c6213fa00b94b 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.

3 participants