Skip to content
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 transfers more chains #4698

Merged
merged 23 commits into from
Dec 1, 2023
Merged

Token transfers more chains #4698

merged 23 commits into from
Dec 1, 2023

Conversation

aalan3
Copy link
Contributor

@aalan3 aalan3 commented Oct 27, 2023

I generated these using a python script. I'm not sure if this is a good idea or not yet but consider it experimental.

There's a lot of things missing here btw.

  • YAML schema updates ✅
  • I think for some blockchains such as Polygon and Optimism, their base token already emits erc20 transfer events?
  • All the wrapped tokens have not been mapped
  • All native token address have not been mapped
  • For Polygon, gas fees are not taken into account like here

Update: I've added schema updates and a some more metadata on native symbol and contracts where i could find them. Let me know if there are more that we could add otherwise this should be ready.

Related to: #4521
Depends on: #4633

@aalan3 aalan3 requested review from 0xRobin and jeff-dude October 27, 2023 13:12
@aalan3 aalan3 force-pushed the token_transfer_more_chains branch from 5ee9991 to bf81593 Compare October 27, 2023 13:24
@aalan3 aalan3 marked this pull request as ready for review November 8, 2023 09:51
@aalan3 aalan3 requested a review from Hosuke November 8, 2023 09:51
Copy link
Collaborator

@0xRobin 0xRobin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking a bit more into depth in the different chain scenarios:
We have a lot of edge cases, that are often very different by chain.

  • Wrapped tokens emitting transfer events on deposit/withdraw
  • Some native tokens emitting transfer evts
  • Polygon just moved to a new native token..
  • OP did a re-genesis of the chain..
  • StarkNet did a chain upgrade which removed non-migrated token balances.
    There's an older WETH deployment address (0x083d49bb326143166683618566c1df8486875acf9b44355e0ebea95d78219840) that doesn't even emit events on deposit/withdraw so we should get those transfers from the call tables..
    And this list will probably only grow longer as more chains perform upgrades/migrations and/or are added to dune.

I believe we should:

  • split the current transfer macro which try to do it all in multiple different minimal macro. Then the chain specific models can use and join these macro's if they are applicable to their chain. This way we can also simply add support for multiple wrapped token deployments by utilizing the macro twice.
    From a quick scan, we could have:
  • macro to get erc20 transfers (simple)
  • macro to get native transfers (which don't emit erc20 evts)
  • macro to get wrapped deposit/withdraw transfers
  • macro to get gas fee transfers/burns
  • macro to get block rewards (for ETH, these are not in traces..)

Splitting the macro logic up will keep the macro's lean and move the edge case logic to the chain models (which macro's apply to this chain?) instead of trying to fit all edge cases into 1 macro.
For simplicity we could/should limit the scope of the PR to only ERC20 and native?

Given all these chain differences, the python script is good for creating the initial setup (and the schemas) but maybe we should not keep it in version control.

@aalan3
Copy link
Contributor Author

aalan3 commented Nov 8, 2023

I agree that there's a bit of rabbit hole of edge cases here. The scope of this PR is currently ERC20, native and (limited) wrapped tokens. Gas fees and block rewards will have to be done in follow up PRs. I need to understand native balances a bit better.

For genesis we need to decide if those should be transfers at all (because technically they aren't) or if we should just include those in the balances later.

I am a bit wary to create 5-6 different macros because I think it makes the code much less readable. Consider reading one macro with some if statements in one window, to jumping between several try to patch them up in your head. It also sounds like for Polygon we might just need a separate macro all together. But I do like the idea of having a base macro and then do "add-ons" and I think there can be a balance.

My suggestion is that we keep this PR mostly as is, unless there's something super obvious to break out in your opinion. These tables are not going to be exposed in the data explorer yet either way. And then I can follow up with another PR to do native transfers with gas fees for Ethereum and some other chain so that we can get a better idea of how to split things up.

@0xRobin
Copy link
Collaborator

0xRobin commented Nov 8, 2023

My suggestion is that we keep this PR mostly as is, unless there's something super obvious to break out in your opinion

Sounds good, I think I would still break out the WETH deposit/withdraw transfers.

@aalan3 aalan3 merged commit 024099c into main Dec 1, 2023
1 of 2 checks passed
@aalan3 aalan3 deleted the token_transfer_more_chains branch December 1, 2023 09:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants