[FA migration] Dynamically process FA mapping #698

bowenyang007 · 2025-01-27T21:39:26Z

Summary

We were getting the FA mapping via a hardcoded list of top 100 coins. Now we want to generate this dynamically. The idea is to:

Maintain a mapping of v1 coin type to fungible asset address (derived from coin type)
Persist that mapping in a new table called coin_to_fungible_asset_mapping
When starting processor, bootstrap the mapping from postgres table.
Update and use mapping locally
Store new entries into postgres for next time.

This strategy works because the mapping is guaranteed to be idempotent.

Also to be absolutely correct we're still using the constants file. Only if it can't find it in the constants file will we look in the dynamic mapping.

[WARNING] This only applies to processors built on top of SDK because we can't easily persist a mapping with the old code.

Backfill

Genesis because we need to create the mapping table
Testnet: 0
Mainnet: 0

Testing

codecov · 2025-01-27T21:51:09Z

Codecov Report

Attention: Patch coverage is 76.56766% with 71 lines in your changes missing coverage. Please review.

Project coverage is 50.2%. Comparing base (279ad33) to head (4928c52).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ocessor/src/processors/fungible_asset_processor.rs	71.4%	32 Missing ⚠️
...t_fungible_asset_processor/parquet_fa_extractor.rs	0.0%	17 Missing ⚠️
...t_models/raw_v2_fungible_asset_to_coin_mappings.rs	83.7%	7 Missing ⚠️
...ngible_asset_processor/fungible_asset_extractor.rs	79.3%	6 Missing ⚠️
...uet_processors/parquet_fungible_asset_processor.rs	0.0%	4 Missing ⚠️
.../fungible_asset_processor/fungible_asset_storer.rs	85.1%	4 Missing ⚠️
rust/integration-tests/src/models/fa_v2_models.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #698     +/-   ##
=======================================
+ Coverage   50.0%   50.2%   +0.1%     
=======================================
  Files        251     253      +2     
  Lines      28335   28575    +240     
=======================================
+ Hits       14181   14352    +171     
- Misses     14154   14223     +69

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

just-in-chang

few comments but overall looks good

rust/processor/src/db/postgres/migrations/2025-01-23-164631_coin_infos_for_migration/up.sql

just-in-chang · 2025-01-30T23:59:57Z

rust/sdk-processor/src/processors/fungible_asset_processor.rs

+        let mut fa_extractor = FungibleAssetExtractor::new();
+        fa_extractor
+            .bootstrap_coin_to_fa_mapping(self.db_pool.clone())
+            .await?;


Maybe instead just call the bootstrapping method in the new method/merge the new and bootstrap methods? And just pass the db pool into the new method

i thought about it but it's the norm to have new not async. In this case it's probably a nonissue but I can imagine some cases (e.g. singleton) where that delineation avoids bugs.

just-in-chang · 2025-01-31T00:03:00Z

rust/integration-tests/src/models/fa_v2_models.rs

+#[derive(Clone, Debug, Deserialize, FieldCount, Identifiable, Insertable, Serialize)]
+#[diesel(primary_key(coin_type))]
+#[diesel(table_name = coin_to_fungible_asset_mappings)]
+pub struct CoinToFungibleAssetMapping {


why do we need to duplicate this struct for tests lol

I guess they include whatever here for testing. If it's not here then it won't be used in tests.

the current paradigm is for those models with inserted at column, seems like this table doesn't, then we can probably reuse the model struct defined in the db.

just-in-chang · 2025-01-31T00:07:51Z

...ocessor/src/db/common/models/fungible_asset_models/raw_v2_coin_to_fungible_asset_mappings.rs

+pub trait CoinToFungibleAssetMappingConvertible {
+    fn from_raw(raw: RawCoinToFungibleAssetMapping) -> Self;
+}


Don't think we need an explicit trait just for this

Maybe. I'm just following the existing paradigm. I agree that it feels a bit redundant vs just using From. Let's keep discussing in slack.

good point @just-in-chang yeah since we don't do any batch conversion,and will have distinct structs for parquet and postgres models I think we can simply use From here instead. I can do as a part of refactoring I am doing

just-in-chang · 2025-01-31T00:08:49Z

...processor/src/db/postgres/models/fungible_asset_models/v2_coin_to_fungible_asset_mappings.rs

+impl CoinToFungibleAssetMappingConvertible for CoinToFungibleAssetMapping {
+    fn from_raw(raw: RawCoinToFungibleAssetMapping) -> Self {
+        Self {
+            coin_type: raw.coin_type,
+            fungible_asset_metadata_address: raw.fungible_asset_metadata_address,
+            last_transaction_version: raw.last_transaction_version,
+        }
+    }
+}


Instead of having a custom trait, u could just impl From<RawCoinToFungibleAssetMapping> for CoinToFungibleAssetMapping and call .into() when needed

nvm seems like all the other models have a trait like this lol... dunno why but ok

...ocessor/src/db/common/models/fungible_asset_models/raw_v2_coin_to_fungible_asset_mappings.rs

rust/sdk-processor/src/steps/parquet_fungible_asset_processor/parquet_fa_extractor.rs

rust/sdk-processor/src/steps/fungible_asset_processor/fungible_asset_extractor.rs

just-in-chang · 2025-01-31T00:23:52Z

rust/processor/src/processors/fungible_asset_processor.rs

+    let mut kv_mapping: CoinToFungibleAssetMappings = AHashMap::new();
+    for mapping in data {
+        kv_mapping.extend(mapping);
+    }


Can try .into_par_iter().fold() https://docs.rs/rayon/latest/rayon/iter/struct.Fold.html

hmm this is also the existing paradigm let's fix it together in a different PR lol.

yuunlimm · 2025-01-31T05:37:20Z

rust/integration-tests/src/models/events_models.rs

-        // TODO: make this more generic to load all fields, then we should be able to run tests for all processor in one test case.
-
+* this is created b/c there is inserted_at field which isn't defined in the Event struct, we can't just load the events directly without specifying the fields.
+* TODO: make this more generic to load all fields, then we should be able to run tests for all processor in one test case.


nit: TODO comment can be removed

yuunlimm · 2025-01-31T05:40:05Z

rust/integration-tests/src/models/fa_v2_models.rs

+#[derive(Clone, Debug, Deserialize, FieldCount, Identifiable, Insertable, Serialize)]
+#[diesel(primary_key(coin_type))]
+#[diesel(table_name = coin_to_fungible_asset_mappings)]
+pub struct CoinToFungibleAssetMapping {


the current paradigm is for those models with inserted at column, seems like this table doesn't, then we can probably reuse the model struct defined in the db.

yuunlimm · 2025-01-31T05:42:18Z

...ocessor/src/db/common/models/fungible_asset_models/raw_v2_coin_to_fungible_asset_mappings.rs

+        }
+    }
+
+    /// Get the asset_type_v1 (coin type) from either the mapping or the constant mapping


nit: comment was a bit confusing to me. either the dynamic mapping table or the static ?

yuunlimm · 2025-01-31T05:48:26Z

...ocessor/src/db/common/models/fungible_asset_models/raw_v2_coin_to_fungible_asset_mappings.rs

+pub trait CoinToFungibleAssetMappingConvertible {
+    fn from_raw(raw: RawCoinToFungibleAssetMapping) -> Self;
+}


good point @just-in-chang yeah since we don't do any batch conversion,and will have distinct structs for parquet and postgres models I think we can simply use From here instead. I can do as a part of refactoring I am doing

just-in-chang

Left few nits but could also just address these in a separate PR cuz they aren't specific to the changes here

just-in-chang · 2025-02-02T05:11:16Z

...ocessor/src/db/common/models/fungible_asset_models/raw_v2_fungible_asset_to_coin_mappings.rs

+pub trait FungibleAssetToCoinMappingConvertible {
+    fn from_raw(raw: RawFungibleAssetToCoinMapping) -> Self;
+}


Lets just get rid of this cuz we're stopping with this pattern

just-in-chang · 2025-02-02T05:11:40Z

...processor/src/db/postgres/models/fungible_asset_models/v2_fungible_asset_to_coin_mappings.rs

+impl FungibleAssetToCoinMappingConvertible for FungibleAssetToCoinMapping {
+    fn from_raw(raw: RawFungibleAssetToCoinMapping) -> Self {
+        Self {
+            fungible_asset_metadata_address: raw.fungible_asset_metadata_address,
+            coin_type: raw.coin_type,
+            last_transaction_version: raw.last_transaction_version,
+        }
+    }
+}


Instead we impl From like here https://github.com/aptos-labs/aptos-indexer-processors/pull/699/files#diff-e456b28db47d7037f3aa140daa9bccffa059cdbcc3435b1283aa2176b0afe54fR46-R47

just-in-chang · 2025-02-02T05:13:49Z

rust/processor/src/processors/fungible_asset_processor.rs

@@ -355,6 +395,11 @@ impl ProcessorTrait for FungibleAssetProcessor {
                .map(CurrentUnifiedFungibleAssetBalance::from_raw)
                .collect();

+        let postgres_fa_to_coin_mappings: Vec<FungibleAssetToCoinMapping> = fa_to_coin_mappings
+            .into_iter()
+            .map(FungibleAssetToCoinMapping::from_raw)


Instead of the trait we can just Into::into

bowenyang007 requested review from rtso and yuunlimm January 27, 2025 21:39

bowenyang007 changed the title [] [FA migration] Dynamically process FA mapping Jan 27, 2025

rtso approved these changes Jan 27, 2025

View reviewed changes

bowenyang007 requested a review from rtso January 30, 2025 07:30

rtso approved these changes Jan 30, 2025

View reviewed changes

bowenyang007 requested a review from just-in-chang January 30, 2025 22:02

bowenyang007 marked this pull request as ready for review January 30, 2025 23:33

just-in-chang reviewed Jan 31, 2025

View reviewed changes

yuunlimm reviewed Jan 31, 2025

View reviewed changes

bowenyang007 force-pushed the fix_balance branch 2 times, most recently from ee8e9b5 to 6b65601 Compare February 1, 2025 22:20

just-in-chang approved these changes Feb 2, 2025

View reviewed changes

yuunlimm mentioned this pull request Feb 3, 2025

support multi txn test #703

Merged

bowenyang007 added 8 commits February 3, 2025 10:39

create schema for new mapping table

9a8cf0d

add handling for the new mapping table

e215ab6

added integration test

a4bd5a8

lint

3f97791

add additional flags for skipping tables

b605b32

fix bug and clean up

35b12f9

add logging for bootstrap function

2e8d45b

lint

4928c52

bowenyang007 force-pushed the fix_balance branch from b2b4775 to 4928c52 Compare February 3, 2025 18:39

bowenyang007 merged commit 9bcb77e into main Feb 4, 2025
11 checks passed

bowenyang007 deleted the fix_balance branch February 4, 2025 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FA migration] Dynamically process FA mapping #698

[FA migration] Dynamically process FA mapping #698

bowenyang007 commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

just-in-chang left a comment

just-in-chang Jan 30, 2025

bowenyang007 Jan 31, 2025

just-in-chang Jan 31, 2025

bowenyang007 Jan 31, 2025

yuunlimm Jan 31, 2025

just-in-chang Jan 31, 2025

bowenyang007 Jan 31, 2025

yuunlimm Jan 31, 2025

just-in-chang Jan 31, 2025

just-in-chang Jan 31, 2025

bowenyang007 Jan 31, 2025

just-in-chang Jan 31, 2025

bowenyang007 Jan 31, 2025

yuunlimm Jan 31, 2025

yuunlimm Jan 31, 2025

yuunlimm Jan 31, 2025

yuunlimm Jan 31, 2025

just-in-chang left a comment

just-in-chang Feb 2, 2025

just-in-chang Feb 2, 2025

just-in-chang Feb 2, 2025

[FA migration] Dynamically process FA mapping #698

[FA migration] Dynamically process FA mapping #698

Conversation

bowenyang007 commented Jan 27, 2025 • edited Loading

Summary

Backfill

Testing

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

just-in-chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

just-in-chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenyang007 commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading