[module events] update token events and switch v1 with v2 #14432

lightmark · 2024-08-27T09:41:13Z

Description

omitted some fields. add them.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

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 · 2024-08-27T09:41:17Z

⏱️ 1h 20m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟥
forge-framework-upgrade-test / forge	12m	🟥
rust-move-unit-coverage	10m	🟩
rust-move-tests	9m	🟩
rust-move-tests	8m	🟩
rust-move-unit-coverage	7m	🟩
execution-performance / single-node-performance	6m	🟥
check	3m	🟩
general-lints	3m	🟩 🟩
rust-cargo-deny	3m	🟩 🟩
check-dynamic-deps	2m	🟩 🟩
semgrep/ci	1m	🟩 🟩 🟩
file_change_determinator	22s	🟩 🟩
file_change_determinator	21s	🟩 🟩
permission-check	8s	🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-08-27T09:52:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.8%. Comparing base (63de0c5) to head (00fca21).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #14432     +/-   ##
=========================================
- Coverage    59.8%    59.8%   -0.1%     
=========================================
  Files         852      852             
  Lines      207730   207730             
=========================================
- Hits       124346   124336     -10     
- Misses      83384    83394     +10

Flag	Coverage Δ
	`59.8% <ø> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

junkil-park

It seems that there is no corresponding v2 event for the 0x1::account::CoinRegisterEvent. Can we add one for that?

junkil-park · 2024-10-03T19:34:34Z

aptos-move/framework/aptos-framework/sources/account.move

+                    type_info: type_info::type_of<CoinType>(),
+                },
+            );
+        } else {


This is a good example of avoiding double-emission. I believe the migration of the other core V2 events should follow a similar approach.

junkil-park

There are still several places where double-emitting will happen. Could you update those, too?

junkil-park · 2024-10-18T02:59:43Z

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

@@ -669,7 +733,7 @@ module aptos_token::token {
        let Token { id: _, amount: burned_amount, token_properties: _ } = withdraw_token(owner, token_id, amount);
        let token_store = borrow_global_mut<TokenStore>(signer::address_of(owner));
        if (std::features::module_event_migration_enabled()) {
-            event::emit(BurnToken { id: token_id, amount: burned_amount });
+            event::emit(Burn { account: signer::address_of(owner), id: token_id, amount: burned_amount });
        };
        event::emit_event<BurnTokenEvent>(


This will double emit.

junkil-park · 2024-10-18T03:00:07Z

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

@@ -600,7 +664,7 @@ module aptos_token::token {
        let Token { id: _, amount: burned_amount, token_properties: _ } = withdraw_with_event_internal(owner, token_id, amount);
        let token_store = borrow_global_mut<TokenStore>(owner);
        if (std::features::module_event_migration_enabled()) {
-            event::emit(BurnToken { id: token_id, amount: burned_amount });
+            event::emit(Burn { account: owner, id: token_id, amount: burned_amount });
        };
        event::emit_event<BurnTokenEvent>(


This will double emit.

junkil-park · 2024-10-18T03:00:42Z

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

@@ -892,7 +956,8 @@ module aptos_token::token {
            direct_deposit(token_owner, new_token);
            update_token_property_internal(token_owner, new_token_id, keys, values, types);
            if (std::features::module_event_migration_enabled()) {
-                event::emit(MutateTokenPropertyMap {
+                event::emit(MutatePropertyMap {


This will double emit.

junkil-park · 2024-10-18T03:01:36Z

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

@@ -126,7 +129,8 @@ module aptos_token::token_transfers {

        if (std::features::module_event_migration_enabled()) {
            event::emit(


this will double emit.

junkil-park · 2024-10-19T05:46:26Z

aptos-move/aptos-debugger/src/aptos_debugger.rs

@@ -416,7 +416,7 @@ fn is_reconfiguration(vm_output: &TransactionOutput) -> bool {
    vm_output
        .events()
        .iter()
-        .any(|event| event.event_key() == Some(&new_epoch_event_key))
+        .any(|event| event.type_tag() == || event.event_key() == Some(&new_epoch_event_key))


Is this intended or by mistake? Anyway, this would give a syntax error.

junkil-park

The changes look good to me.

However, this minor issue (https://github.com/aptos-labs/aptos-core/pull/14432/files#r1807194788) and the CI test error need to be addressed.

github-actions · 2024-10-26T07:20:47Z

✅ Forge suite `realistic_env_max_load` success on `63f1d2a68b2b5c48625bae2380a44902eb5204e2`

two traffics test: inner traffic : committed: 14289.29 txn/s, latency: 2781.33 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3600 ms), latency samples: 5433080
two traffics test : committed: 99.94 txn/s, latency: 1578.89 ms, (p50: 1400 ms, p70: 1500, p90: 1600 ms, p99: 8600 ms), latency samples: 1700
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.020, avg: 1.610", "ConsensusProposalToOrdered: max: 0.320, avg: 0.295", "ConsensusOrderedToCommit: max: 0.370, avg: 0.358", "ConsensusProposalToCommit: max: 0.662, avg: 0.653"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.86s no progress at version 2459241 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.41s no progress at version 2459239 (avg 8.41s) [limit 15].
Test Ok

github-actions · 2024-10-26T07:22:35Z

✅ Forge suite `framework_upgrade` success on `f38ec72e975a4dff4e9919e7a1d8118a75858bab` ==> `63f1d2a68b2b5c48625bae2380a44902eb5204e2`

Compatibility test results for f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2 (PR)
Upgrade the nodes to version: 63f1d2a68b2b5c48625bae2380a44902eb5204e2
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1207.35 txn/s, submitted: 1210.02 txn/s, failed submission: 2.67 txn/s, expired: 2.67 txn/s, latency: 2670.63 ms, (p50: 2400 ms, p70: 2700, p90: 4200 ms, p99: 6900 ms), latency samples: 99480
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1108.49 txn/s, submitted: 1110.28 txn/s, failed submission: 1.79 txn/s, expired: 1.79 txn/s, latency: 2657.81 ms, (p50: 2000 ms, p70: 2500, p90: 5400 ms, p99: 7100 ms), latency samples: 98940
5. check swarm health
Compatibility test for f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2 passed
Upgrade the remaining nodes to version: 63f1d2a68b2b5c48625bae2380a44902eb5204e2
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1241.44 txn/s, submitted: 1243.19 txn/s, failed submission: 1.76 txn/s, expired: 1.76 txn/s, latency: 2402.29 ms, (p50: 2100 ms, p70: 2400, p90: 4300 ms, p99: 6300 ms), latency samples: 113100
Test Ok

github-actions · 2024-10-26T07:24:30Z

✅ Forge suite `compat` success on `f38ec72e975a4dff4e9919e7a1d8118a75858bab` ==> `63f1d2a68b2b5c48625bae2380a44902eb5204e2`

Compatibility test results for f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2 (PR)
1. Check liveness of validators at old version: f38ec72e975a4dff4e9919e7a1d8118a75858bab
compatibility::simple-validator-upgrade::liveness-check : committed: 14745.43 txn/s, latency: 2204.24 ms, (p50: 2100 ms, p70: 2100, p90: 2400 ms, p99: 7900 ms), latency samples: 556600
2. Upgrading first Validator to new version: 63f1d2a68b2b5c48625bae2380a44902eb5204e2
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6013.87 txn/s, latency: 4762.26 ms, (p50: 5500 ms, p70: 5800, p90: 5900 ms, p99: 5900 ms), latency samples: 112220
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 5644.94 txn/s, latency: 5374.74 ms, (p50: 5900 ms, p70: 6000, p90: 6100 ms, p99: 6300 ms), latency samples: 210840
3. Upgrading rest of first batch to new version: 63f1d2a68b2b5c48625bae2380a44902eb5204e2
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6737.03 txn/s, latency: 4195.30 ms, (p50: 4800 ms, p70: 5100, p90: 5200 ms, p99: 5400 ms), latency samples: 123460
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6396.30 txn/s, latency: 5082.08 ms, (p50: 5100 ms, p70: 5200, p90: 6900 ms, p99: 7100 ms), latency samples: 224860
4. upgrading second batch to new version: 63f1d2a68b2b5c48625bae2380a44902eb5204e2
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8569.95 txn/s, latency: 3230.05 ms, (p50: 3100 ms, p70: 3300, p90: 5000 ms, p99: 5200 ms), latency samples: 160620
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8705.62 txn/s, latency: 3624.25 ms, (p50: 3200 ms, p70: 4600, p90: 5100 ms, p99: 6400 ms), latency samples: 286060
5. check swarm health
Compatibility test for f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2 passed
Test Ok

msmouse · 2024-10-31T17:03:58Z

aptos-move/framework/aptos-framework/sources/aptos_governance.move

+        } else {
+            let events = borrow_global_mut<GovernanceEvents>(@aptos_framework);
+            event::emit_event<CreateProposalEvent>(
+                &mut events.create_proposal_events,
+                CreateProposalEvent {
+                    proposal_id,
+                    proposer: proposer_address,
+                    stake_pool,
+                    execution_hash,
+                    proposal_metadata,
+                },
+            );
        };
-        let events = borrow_global_mut<GovernanceEvents>(@aptos_framework);
-        event::emit_event<CreateProposalEvent>(
-            &mut events.create_proposal_events,
-            CreateProposalEvent {
-                proposal_id,
-                proposer: proposer_address,
-                stake_pool,
-                execution_hash,
-                proposal_metadata,
-            },
-        );
        proposal_id


GovernanceEvents nv1 not emitted any more?

lightmark requested a review from junkil-park August 27, 2024 09:41

lightmark requested a review from areshand as a code owner August 27, 2024 09:41

lightmark requested a review from bowenyang007 August 27, 2024 09:41

lightmark force-pushed the lightmark/update_token_events_v2 branch from a465e7b to ff30778 Compare August 27, 2024 17:22

junkil-park reviewed Sep 27, 2024

View reviewed changes

lightmark force-pushed the lightmark/update_token_events_v2 branch from ff30778 to 00fca21 Compare September 27, 2024 18:10

lightmark requested review from alinush, davidiw, movekevin and wrwg as code owners September 27, 2024 18:10

junkil-park reviewed Oct 3, 2024

View reviewed changes

lightmark force-pushed the lightmark/update_token_events_v2 branch from 00fca21 to 50054c5 Compare October 17, 2024 23:47

lightmark changed the title ~~[module events] update token events~~ [module events] update token events and switch v1 with v2 Oct 17, 2024

junkil-park reviewed Oct 18, 2024

View reviewed changes

lightmark force-pushed the lightmark/update_token_events_v2 branch from 50054c5 to d527334 Compare October 18, 2024 23:07

lightmark requested review from msmouse and grao1991 as code owners October 19, 2024 02:12

lightmark force-pushed the lightmark/update_token_events_v2 branch from 020dac5 to bf89257 Compare October 19, 2024 02:17

junkil-park reviewed Oct 19, 2024

View reviewed changes

junkil-park approved these changes Oct 19, 2024

View reviewed changes

lightmark force-pushed the lightmark/update_token_events_v2 branch from bf89257 to c54794e Compare October 20, 2024 06:09

lightmark requested review from zekun000, vgao1996 and georgemitenkov as code owners October 20, 2024 06:09

lightmark force-pushed the lightmark/update_token_events_v2 branch 3 times, most recently from 7721e7d to fc7fcdc Compare October 22, 2024 17:23

rahxephon89 self-requested a review October 22, 2024 17:29

lightmark added 3 commits October 25, 2024 15:47

deal with NewEpochEvent

c14fcc8

fix rosetta

1c681ce

perf impr

346202c

lightmark force-pushed the lightmark/update_token_events_v2 branch from 8de9c3f to a8fa2da Compare October 25, 2024 23:55

This comment has been minimized.

Sign in to view

lightmark force-pushed the lightmark/update_token_events_v2 branch 2 times, most recently from 7e9214e to f0bf11e Compare October 26, 2024 03:02

This comment has been minimized.

Sign in to view

fix upgrade

63f1d2a

lightmark force-pushed the lightmark/update_token_events_v2 branch from f0bf11e to 63f1d2a Compare October 26, 2024 06:54

This comment has been minimized.

Sign in to view

lightmark merged commit 5010494 into main Oct 26, 2024
48 checks passed

lightmark deleted the lightmark/update_token_events_v2 branch October 26, 2024 07:24

msmouse mentioned this pull request Oct 28, 2024

Calibrate single node performance #15100

Merged

2 tasks

junkil-park mentioned this pull request Oct 28, 2024

Event V2 Translation #14615

Open

21 tasks

msmouse reviewed Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[module events] update token events and switch v1 with v2 #14432

[module events] update token events and switch v1 with v2 #14432

lightmark commented Aug 27, 2024

trunk-io bot commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

junkil-park left a comment

junkil-park Oct 3, 2024

junkil-park left a comment

junkil-park Oct 18, 2024

junkil-park Oct 18, 2024

junkil-park Oct 18, 2024

junkil-park Oct 18, 2024

junkil-park Oct 19, 2024

junkil-park left a comment •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 26, 2024

github-actions bot commented Oct 26, 2024

github-actions bot commented Oct 26, 2024

msmouse Oct 31, 2024

		@@ -126,7 +129,8 @@ module aptos_token::token_transfers {

		if (std::features::module_event_migration_enabled()) {
		event::emit(

[module events] update token events and switch v1 with v2 #14432

[module events] update token events and switch v1 with v2 #14432

Conversation

lightmark commented Aug 27, 2024

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Aug 27, 2024 • edited Loading

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

junkil-park left a comment

Choose a reason for hiding this comment

junkil-park Oct 3, 2024

Choose a reason for hiding this comment

junkil-park left a comment

Choose a reason for hiding this comment

junkil-park Oct 18, 2024

Choose a reason for hiding this comment

junkil-park Oct 18, 2024

Choose a reason for hiding this comment

junkil-park Oct 18, 2024

Choose a reason for hiding this comment

junkil-park Oct 18, 2024

Choose a reason for hiding this comment

junkil-park Oct 19, 2024

Choose a reason for hiding this comment

junkil-park left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 26, 2024

✅ Forge suite realistic_env_max_load success on 63f1d2a68b2b5c48625bae2380a44902eb5204e2

github-actions bot commented Oct 26, 2024

✅ Forge suite framework_upgrade success on f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2

github-actions bot commented Oct 26, 2024

✅ Forge suite compat success on f38ec72e975a4dff4e9919e7a1d8118a75858bab ==> 63f1d2a68b2b5c48625bae2380a44902eb5204e2

msmouse Oct 31, 2024

Choose a reason for hiding this comment

trunk-io bot commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

junkil-park left a comment •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `63f1d2a68b2b5c48625bae2380a44902eb5204e2`

✅ Forge suite `framework_upgrade` success on `f38ec72e975a4dff4e9919e7a1d8118a75858bab` ==> `63f1d2a68b2b5c48625bae2380a44902eb5204e2`

✅ Forge suite `compat` success on `f38ec72e975a4dff4e9919e7a1d8118a75858bab` ==> `63f1d2a68b2b5c48625bae2380a44902eb5204e2`