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

Event V2 Translation #14615

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Event V2 Translation #14615

wants to merge 1 commit into from
+1,985 −183

Conversation

junkil-park
Copy link
Contributor

@junkil-park junkil-park commented Sep 12, 2024 •
edited
Loading

Description

  • This PR introduces the Event V2 Translation Engine, a framework for translating well-known V2 events into V1 format. So, the Node APIs can output those events in the V1 format.

  • Added the new flag enable_event_v2_translation to the node config for feature-gating.

  • Added the event translators in the account and coin modules, translating:

    • coin::CoinDeposit -> coin::DepositEvent
    • coin::CoinWithdraw -> coin::WithdrawEvent
    • account::CoinRegister -> account::CoinRegisterEvent
    • account::KeyRotation -> account::KeyRotationEvent

  • Added a new feature flag ACCOUNT_AND_COIN_MODULE_EVENT_MIGRATION, and used it to guard the event migration for the account and coin modules. So, we can ship this first.

  • Implemented the e2e API test environment where we can simulate and test the module event migration. And, added the e2e tests for the event transaction for the account and coin modules.

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
  • Other (specify)

How Has This Been Tested?

Tested on the localnet.
Added various e2e API tests.

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 Trunk.io
Copy link

trunk-io bot commented Sep 12, 2024 •
edited
Loading

⏱️ 2h 42m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
general-lints 20m 🟩 🟩 🟩 🟩 🟩 (+7 more)
rust-cargo-deny 18m 🟩 🟩 🟩 🟩 🟩 (+7 more)
check-dynamic-deps 13m 🟩 🟩 🟩 🟩 🟩 (+7 more)
rust-move-tests 10m 🟩 
rust-move-tests 10m 🟩 
rust-move-tests 10m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 9m 🟩 
rust-move-tests 6m ⬜ 
semgrep/ci 5m 🟩 🟩 🟩 🟩 🟩 (+7 more)

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

@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 9 times, most recently from 50b9c0f to 832b754 Compare September 16, 2024 16:22
grao1991
grao1991 previously requested changes Sep 16, 2024
View reviewed changes
storage/aptosdb/src/db/include/aptosdb_reader.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
storage/indexer_schemas/src/schema/translated_v1_event/mod.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 4 times, most recently from 80c5021 to bfbe123 Compare September 24, 2024 03:46
@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 10 times, most recently from 123fbfe to 27650b3 Compare October 3, 2024 16:15
@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 2 times, most recently from ccd6843 to c38fc96 Compare October 16, 2024 11:27
@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 2 times, most recently from 32088d7 to 523e22f Compare October 23, 2024 07:11
@junkil-park junkil-park dismissed grao1991’s stale review October 28, 2024 17:59

Addressed the change request.

@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 7 times, most recently from 58a7d26 to 7fd09ac Compare November 7, 2024 21:02
lightmark
lightmark reviewed Nov 8, 2024
View reviewed changes
api/src/context.rs Outdated Show resolved Hide resolved
api/src/transactions.rs Show resolved Hide resolved
api/test-context/src/test_context.rs Show resolved Hide resolved
aptos-move/aptos-release-builder/src/components/feature_flags.rs
@@ -132,6 +132,7 @@ pub enum FeatureFlag {
TransactionSimulationEnhancement,
CollectionOwner,
EnableLoaderV2,
AccountAndCoinModuleEventMigration,
Copy link
Contributor

Choose a reason for hiding this comment

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

could we call it Transaction instead of migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean "translation" instead of migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but why you want a separate migration flag for coin and account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to toggle only the switches for account and coin events in this PR (or release) so the other event translation (e.g., for tokens) can come later.

aptos-move/framework/aptos-framework/sources/account.move
@@ -741,7 +741,7 @@ module aptos_framework::account {
);
table::add(address_map, new_auth_key, originating_addr);

if (std::features::module_event_migration_enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same reason as: https://github.com/aptos-labs/aptos-core/pull/14615/files#r1841101915

storage/indexer/src/db_indexer.rs Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
types/src/account_config/events/key_rotation_event.rs Outdated Show resolved Hide resolved
areshand
areshand reviewed Nov 11, 2024
View reviewed changes
storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
storage/indexer/src/db_indexer.rs
@@ -413,6 +482,22 @@ impl DBIndexer {

assert_eq!(num_transactions, version - start_version);

if self.indexer_db.event_v2_translation_enabled() {
batch.put::<InternalIndexerMetadataSchema>(
Copy link
Contributor

Choose a reason for hiding this comment

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

how do you plan to handle seq number backup and restore? since all data are in one db now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will sync with you about this offline soon.

storage/indexer/src/db_indexer.rs Outdated Show resolved Hide resolved
@junkil-park junkil-park force-pushed the jpark/event-v2-translation branch 2 times, most recently from 788fc94 to 7bae9ad Compare November 13, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightmark lightmark lightmark left review comments

@areshand areshand areshand left review comments

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

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

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

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

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

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

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

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

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

@bowenyang007 bowenyang007 Awaiting requested review from bowenyang007

@igor-aptos igor-aptos Awaiting requested review from igor-aptos

@larry-aptos larry-aptos Awaiting requested review from larry-aptos

@grao1991 grao1991 Awaiting requested review from grao1991 grao1991 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.
4 participants
@junkil-park @areshand @lightmark @grao1991