-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Token balances tables feedback #5467
Comments
Thanks for the feedback @hildobby!
Think of the current balances as the most raw form we can get the data.
I agree that splitting them up will remove some confusions and current schema issues.
Good suggestion.
Also a good suggestion, although
If we have |
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 |
we should be leveraging this global variable (guilty of not always validating myself): |
@hildobby on 0x000.. vs 0xeeeee... |
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
The text was updated successfully, but these errors were encountered: