-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
[index] current unified balances #366
Conversation
08c906e
to
6b8504e
Compare
6b8504e
to
676bc28
Compare
rust/processor/Cargo.toml
Outdated
@@ -59,6 +59,8 @@ url = { workspace = true } | |||
native-tls = { workspace = true } | |||
postgres-native-tls = { workspace = true } | |||
tokio-postgres = { workspace = true } | |||
tiny-keccak = { version = "2.0.2", features = ["sha3"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: workspace = true
owner_address: &str, | ||
metadata_address: &str, | ||
) -> anyhow::Result<String> { | ||
let mut preimage = hex_to_raw_bytes(owner_address)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the address padded with 0 if not full 64 long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow the existing logic.
rust/processor/src/models/fungible_asset_models/v2_fungible_asset_balances.rs
Outdated
Show resolved
Hide resolved
9486104
to
a03a8c5
Compare
@@ -0,0 +1,21 @@ | |||
-- current fungible asset balances | |||
CREATE TABLE IF NOT EXISTS current_unified_fungible_asset_balances ( | |||
storage_id VARCHAR(66) UNIQUE PRIMARY KEY NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you don't really need UNIQUE here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew, let's remove it.
last_transaction_timestamp_v1 TIMESTAMP, | ||
last_transaction_timestamp_v2 TIMESTAMP, | ||
last_transaction_timestamp TIMESTAMP GENERATED ALWAYS AS (GREATEST(last_transaction_timestamp_v1, last_transaction_timestamp_v2)) STORED, | ||
inserted_at TIMESTAMP NOT NULL DEFAULT NOW() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CapCap Do we still need this column (and the corresponding index)? This will likely cause scalability issue later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we move everything to parquet writes, we can remove these columns- until then these are used just for ingest :-(
VERY SOON THOUGH!
last_transaction_timestamp TIMESTAMP GENERATED ALWAYS AS (GREATEST(last_transaction_timestamp_v1, last_transaction_timestamp_v2)) STORED, | ||
inserted_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
); | ||
CREATE INDEX IF NOT EXISTS cufab_owner_at_index ON current_unified_fungible_asset_balances (owner_address, asset_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waht's the relationship among owner_address, asset_type, and storage_id? Any chance we can remove this index in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdym?
.do_update() | ||
.set( | ||
( | ||
owner_address.eq(excluded(owner_address)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a conflict between v1 and v2, are these (owner_address, asset_type, is_frozen) fields the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdym?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_frozen is not. most of the time it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The the order of conflict upserts matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we should ignore this field for v1.
Fix lint, you need to import |
ca71635
to
080b6ec
Compare
6c8f121
to
d1a230b
Compare
Since this is deployed to devnet now, can you add these to the test plan:
|
rust/processor/src/models/fungible_asset_models/v2_fungible_asset_balances.rs
Outdated
Show resolved
Hide resolved
rust/processor/src/models/fungible_asset_models/v2_fungible_asset_balances.rs
Show resolved
Hide resolved
rust/processor/migrations/2024-05-04-025823_current_unified_fungible_asset_balance/up.sql
Show resolved
Hide resolved
} | ||
} | ||
|
||
fn get_primary_fungible_store_address( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same as the existing code that you deleted? do we need to test that nothing changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your fabulous is_primary_test
cover this function.
f0fd19e
to
5fe8abe
Compare
// Process the unified balance | ||
let current_unified_fungible_asset_balances = current_fungible_asset_balances | ||
.iter() | ||
.map(CurrentUnifiedFungibleAssetBalance::from) | ||
.collect::<Vec<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note is that we can't have partial fill for current_fungible_asset_balances b/c the partials would override current_unified_fungible_asset_balances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by partial fill and override?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say we have wallet A and we transfer some $$ there and the it gets deleted. We'd have partial write for the delete correct? But this logic would miss the delete I think. I guess it won't be overridden automatically since the deletes would be a different map.
Add a new table
current_unified_fungible_asset_balances
to reflect the total balance of a coin.tested locally with devnet data:
Indexer Testing
Just make sure for every step in [Correctness Testing the
current_unifed_fungible_asset_balances
table shows the correct number at the correct row&col, which matchescurrent_coin_balances
andcurrent_fungible_asset_balances
.This is the final state on devnet with running fungible asset processor locally.
Row #1 (Shows as column here for better layout) is the default profile APT balance
Row #2 (Shows as column here for better layout) is the test profile APT balance
Row #3 (Shows as column here for better layout) is the default profile MoonCoin balance
Row#4 (Shows as column here for better layout) is the default profile a Pure FA balance
sent another txn to send 20000 octa to 0x1 who already has 1+ apt, then we get: