[FA migration] Make storage id consistent for transaction tables #688
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #688 +/- ##
=======================================
- Coverage 49.6% 49.5% -0.2%
=======================================
Files 252 252
Lines 28526 28535 +9
=======================================
- Hits 14166 14137 -29
- Misses 14360 14398 +38 ☔ View full report in Codecov by Sentry. |
|
I think integration test might fail, |
bowenyang007
changed the title
|
|
||
| // Storage id should be derived (for the FA migration) | ||
| let metadata_addr = get_paired_metadata_address(&coin_type); | ||
| let storage_id = get_primary_fungible_store_address(&owner_address, &metadata_addr) |
There was a problem hiding this comment.
what if the coin hasn't migrated yet? does it matter what this value is?
There was a problem hiding this comment.
yea cuz eventually you will migrate and we'll have a hard time debugging.
There was a problem hiding this comment.
makes sense. and storage_id can be arbitrary for coin balances since it's only used for the PK of the table.
| // Storage id should be derived (for the FA migration) | ||
| let metadata_addr = get_paired_metadata_address(&coin_type); | ||
| let storage_id = get_primary_fungible_store_address(&owner_address, &metadata_addr) | ||
| .expect("calculate primary fungible store failed"); |
There was a problem hiding this comment.
this replaces all coin balances with a new primary key. how do we delete the old rows so the balances don't show up twice in the table?
There was a problem hiding this comment.
this only affects parquet so we'll need to first wipe everythign.
There was a problem hiding this comment.
@yuunlimm correct me if i'm wrong but i think this is also used in the postgres processors
There was a problem hiding this comment.
it's the legacy table, right? then it was deprecated so the code can be removed
There was a problem hiding this comment.
oh i see, the RawCurrentFungibleAssetBalance is already using get_primary_fungible_store_address
|
@yuunlimm we also should enable the cfab tests (non-legacy) so this PR has enough testing |
|
yuunlimm
force-pushed
the
fix_storage_id
branch
from
848e722 to
c07eeaf
Compare
yuunlimm
force-pushed
the
fix_storage_id
branch
from
c07eeaf to
5dd1d06
Compare
Even those are the same assets storage id is different.
Backfill
Testnet: 0
Mainnet: 0
This affects fungible_asset_activities and fungible_asset_balances (for analytics)
Test
Storage id is now the same!
