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

[FA] get rid of migration flag #15269

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[FA] get rid of migration flag #15269

wants to merge 2 commits into from

Conversation

lightmark
Copy link
Contributor

Description

  1. Will not create PFS when register account. Deposit will create that automatically.
  2. Get rid of unnecessary MigrationFlag.

How Has This Been Tested?

ut

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Nov 13, 2024 •
edited
Loading

⏱️ 1h 16m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-cargo-deny 11m 🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps 11m 🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests 9m 🟥 
rust-move-tests 9m 🟩 
rust-move-tests 8m 🟥 
rust-move-tests 7m 🟥 
rust-move-tests 7m 🟥 
rust-move-tests 7m 🟥 
general-lints 3m 🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci 3m 🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator 1m 🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check 26s 🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check 26s 🟩 🟩 🟩 🟩 🟩 (+2 more)

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
check-dynamic-deps 3m 1m +102%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@lightmark lightmark requested review from igor-aptos, WGB5445, davidiw and movekevin and removed request for davidiw, alinush, wrwg and movekevin November 13, 2024 22:43
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch from 9c614c4 to 77a70c2 Compare November 13, 2024 23:54
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch from 77a70c2 to e9d3ee5 Compare November 15, 2024 05:02
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch 5 times, most recently from daea913 to 0e82d73 Compare November 21, 2024 00:34
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch from 0e82d73 to 23a5985 Compare November 21, 2024 01:59
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch 2 times, most recently from fbf3e00 to 85e784a Compare November 21, 2024 03:50
igor-aptos
igor-aptos reviewed Nov 21, 2024
View reviewed changes
api/src/tests/events_test.rs
@@ -208,7 +208,7 @@ async fn test_module_events() {
== "0x0"
})
.collect::<Vec<_>>();
assert_eq!(events.len(), 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate these obscure tests.

can you improve the test and change it to have a list of 9 events we expect, and then assert those?
that way, a next time things change - it's obvious why.

i.e. same thing as I did here - #15109

aptos-move/aptos-vm/tests/sharded_block_executor.rs Show resolved Hide resolved
aptos-move/e2e-move-tests/src/tests/module_event.rs
@@ -45,7 +45,7 @@ fn test_module_event_enabled() {
vec![bcs::to_bytes(&10u64).unwrap()],
);
let events = h.get_events();
assert_eq!(events.len(), 13);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, let's have a list of event types we expect (not the values)

aptos-move/e2e-tests/src/executor.rs Outdated
@@ -554,6 +556,14 @@ impl FakeExecutor {
self.read_apt_coin_store_resource_at_address(account.address())
}

/// Reads the CoinStore resource value for an account from this executor's data store.
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong comment

@manudhundi manudhundi dismissed their stale review November 21, 2024 21:22

We decided to ignore sharding tests in this PR and have a subsequent PR to address them

@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch 3 times, most recently from 31c576a to f983046 Compare November 22, 2024 02:27
igor-aptos
igor-aptos approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

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

cc @davidiw on deprecating MigrationFlag. Basically the only change is making it such that if someone has APT PFS, and someone sends them APT coins:

  • previously it would abort
  • now it would be deposited to PFS

If they have both CoinStore and PFS, it will go to CoinStore.

@lightmark - address comments inside

aptos-move/framework/aptos-framework/sources/coin.move
@@ -826,11 +829,13 @@ module aptos_framework::coin {
account_address: address,
metadata: Object<Metadata>
): bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this function from migrated_primary_fungible_store_exists to can_receive_paired_fungible_asset

also then new_accounts_default_to_fa_apt_store_enabled is not really gating just apt, but also other paired coins, which seems a bit odd. maybe it should be

(features::new_accounts_default_to_fa_apt_store_enabled() && object:address(metadata) == 0xa) || ...

but I don't know what should be the default for other coins.

Copy link
Contributor Author

@lightmark lightmark Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

inconsistent behavior will only make things more complicated. If we do this only for apt. The other coin has to register, that's so complicated.
When we enable this, most Dapp and CEX should support APT coin + FA. So they should as well do that for other coins they supported. It is just adding an entry to a mapping. But yeah, let's do another AIP and feature to migrate other coins.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good for the second part.

for the first - let's rename this function from migrated_primary_fungible_store_exists to can_receive_paired_fungible_asset

@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch from edf8645 to 9e63f8d Compare November 22, 2024 19:17
@lightmark lightmark force-pushed the lightmark/deprecate_migration_flag branch from 9e63f8d to e181a8d Compare November 22, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-aptos igor-aptos igor-aptos approved these changes

@manudhundi manudhundi manudhundi left review comments

@WGB5445 WGB5445 Awaiting requested review from WGB5445

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin is a code owner

@junkil-park junkil-park Awaiting requested review from junkil-park junkil-park is a code owner

@msmouse msmouse Awaiting requested review from msmouse msmouse is a code owner

@grao1991 grao1991 Awaiting requested review from grao1991 grao1991 is a code owner

@banool banool Awaiting requested review from banool banool is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario is a code owner

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 is a code owner

@vgao1996 vgao1996 Awaiting requested review from vgao1996 vgao1996 is a code owner

@georgemitenkov georgemitenkov Awaiting requested review from georgemitenkov georgemitenkov is a code owner

At least 2 approving reviews are required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@lightmark @manudhundi @igor-aptos