Open hildobby opened 6 months ago
Thanks for the feedback @hildobby! My quick thoughts:
- how should it be used if unaggregated by hour/day/week?
Think of the current balances as the most raw form we can get the data.
You get the result of the read-only balanceOf(..)
function for every time the balance was updated. I imagine (just as with evt_Transfer
) that the most useful tables will have to built on top. As an example. I'm reworking balances_daily
here to have it perform a forward fill so it can be properly used as time-series data. Unfortunately, as we're dealing with a ton of data, performance issues can be a limiting factor for spells built on top..
- I would much rather have
nft.balances
andfungible.balances
I agree that splitting them up will remove some confusions and current schema issues.
- rename
type
totoken_standard
Good suggestion.
wallet_address
is also a weird name, not all addresses are wallets, I would just stick toaddress
Also a good suggestion, although address
on it's own can be a bit ambiguous. Don't have a better suggestion though.
- There's also a lot of inconsistencies with some spells using
symbol
and some usingtoken_symbol
, it would be cool for you guys to define a default one of the two and enforce it across spellbook, same withdecimals
andtoken_decimals
If we have token_address
and token_standard
, using token_symbol
makes sense. I try to be consistent within a table (either all columns referring to the entity have a prefix (token_
), or none.)
This is great feedback @hildobby thanks for writing this out. I'll address them below. We are redoing the balances spells atm so your timing is perfect, will try to incorporate as much as possible.
Also, native token contract (ETH) should be 0x0000000000000000000000000000000000000000 and not 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee to match other tables imo. Rn we need to do weird stuff like this: https://github.com/duneanalytics/spellbook/pull/5469/files#diff-c01f4362f7d33bda62723a0a403a479138265718f51899ef6f3113b46b0c9fd6R177
Also, native token contract (ETH) should be 0x0000000000000000000000000000000000000000 and not 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee to match other tables imo. Rn we need to do weird stuff like this: https://github.com/duneanalytics/spellbook/pull/5469/files#diff-c01f4362f7d33bda62723a0a403a479138265718f51899ef6f3113b46b0c9fd6R177
we should be leveraging this global variable (guilty of not always validating myself): https://github.com/duneanalytics/spellbook/blob/main/dbt_project.yml#L19
@hildobby on 0x000.. vs 0xeeeee... Relevant EIP: https://eips.ethereum.org/EIPS/eip-7528 and the discussion: https://ethereum-magicians.org/t/eip-7528-eth-native-asset-address-convention/15989
@hildobby on 0x000.. vs 0xeeeee...
Relevant EIP: https://eips.ethereum.org/EIPS/eip-7528
and the discussion: https://ethereum-magicians.org/t/eip-7528-eth-native-asset-address-convention/15989
yeah but this is only applicable to eth mainnet and its L2s, 0x000 seems to me like an easy neutral ground for all evm chains' native asset. if its 0xeee for ETH, what would the standard for bnb, avalanche, gnosis, celo, etc be?
either way, i just want consistency across tables, regardless of the specific address.
I'm confused by the tokens_{chain}.balances tables, so here is some feedback:
nft.balances
andfungible.balances
, especially since some columns are only needed for NFTs (token_id, collection_name). I would also much rather have crosschain (views or ideally materialized tables but that's another topic) not inefficiently plagued by having both token types.type
is a really bad column name since it's interpreted as a function,token_standard
would make more sense as it would align it with other spellswallet_address
is also a weird name, not all addresses are wallets, I would just stick toaddress
symbol
and some usingtoken_symbol
, it would be cool for you guys to define a default one of the two and enforce it across spellbook, same withdecimals
andtoken_decimals
I'm curious on your thoughts @aalan3 @jeff-dude @0xRobin @MSilb7