Token transfers for ethereum #4633
Conversation
belen-pruvost
force-pushed
the
token_transfers
branch
2 times, most recently
from
15c8fe1 to
511ea96
Compare
aalan3
force-pushed
the
token_transfers
branch
from
511ea96 to
93c5b9e
Compare
| {% if is_incremental() %} | ||
| AND {{incremental_predicate('block_time')}} | ||
| {% endif %} | ||
|
|
There was a problem hiding this comment.
Are you planning to add gas fees further in spells? It's also sort of transfer which affects balances
There was a problem hiding this comment.
Do you have any more insight on this? gas fees makes balances a bit tricky but aren't a lot of the fees in the traces?
There was a problem hiding this comment.
you can do just:
select "from" as spender, gas_price * gas_used as amount from {blockchain}.transactions
but it's better to doublecheck if this is applicable for L2 as well
There was a problem hiding this comment.
Thank you. I will look into this in a follow up PR.
aalan3
force-pushed
the
token_transfers
branch
from
3f2ce50 to
748e7db
Compare
|
@aalan3 genesis balances should be included as well! |
Good point. We have this issue for optimism as well . The question is, should we consider these transfers or just include them in the balances later? We would have to put dummy values for block_number, trace call etc. |
I'd say both options are ok. Let's see what others think? |
aalan3
requested review from
jeff-dude and
grkhr
and removed request for
grkhr
| -- We have to create this because evt_index and trace_address can be null | ||
| , {{dbt_utils.generate_surrogate_key(['t.block_number', 'tx.index', 't.evt_index', "array_join(trace_address, ',')"])}} as unique_key |
There was a problem hiding this comment.
do we prefer this approach of adding surrogate key, rather than coalesce() each unique key to a default value?
this could be a decision that has an impact across spellbook. of course, we don't have to standardize it all right away, but will be nice to be consistent moving forward at minimum (and on larger sector spells)
There was a problem hiding this comment.
TBH I don't like having unique_key in most cases but here I think it works because not sure what we should coalesce to. I think it can be confusing for users to have a value in evt_index/trace_address that doesn't lead anywhere.
I think if we do have a unique key column we should always try to use generate_surrogate_key because 1) it hashes the output so that it takes less space and 2) it works the same everywhere.
There was a problem hiding this comment.
i'm okay with the rationale behind the first part. i will look to consider the approach in new dex.trades design, if applicable.
agree fully with the second part.
There was a problem hiding this comment.
I still believe we should adapt the incremental logic to use IS NOT DISTINCT FROM as equality operation so that the merge works as we would want with respect to null values.
There was a problem hiding this comment.
i'm comfortable with this current state. thank you for taking in all the input from the community and helping get this moving 🚀
for those following along, keep in mind that this is in somewhat of a "beta" state and we welcome all feedback once live on how to improve. in the meantime, other blockchain versions will look to follow suit of ethereum here.
Related to #4521
This PR implements a transfers_base macro which materializes ERC20 transfers and native transfers into a single table and a transfers_enrichment macro. The design is based on the
fungibletransfers.They are meant to be used together where the base is for materialization and the enrichment is a view on top of it. I'm not yet sure the enrichment step will work as a view performance-wise. I think
prices_usd_forward_fillis a bit inefficient but we'll see.