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

migrate all dex models to the new structure / design #4759

Open
Hosuke opened this issue Nov 10, 2023 · 35 comments
Open

migrate all dex models to the new structure / design #4759

Hosuke opened this issue Nov 10, 2023 · 35 comments
Assignees
Labels
dbt: dex covers the DEX dbt subproject dune team created by dune team enhancement New feature or request

Comments

@Hosuke
Copy link
Collaborator

Hosuke commented Nov 10, 2023

Updated at Apr 23, 2024:
Since all dexes have been migrated to the new dex.trades lineage.
We continue to remove/modify the legacy code project by project.


Hello Team,

I'm opening this issue to keep track of all the Pull Requests (PRs) that implement the new macro approach in dex.trades, as introduced in PR #4533.

Purpose

  • To monitor and discuss the integration of new dexs into the _beta dex lineage.
  • Share insights and updates on how these integrations are progressing.

Call for Contributions

  • If you are working on a dex PR that incorporates this new macro approach, please link it here.
  • We can submit 1 PR <--> 1 project to keep it clean.
  • Share any thoughts or feedback on the macro implementation.

Looking forward to seeing the innovative ways this new approach is being utilized and enhanced!

Migration tracking table

Dune Query by @tomfutago: https://dune.com/queries/3240358

Project dex.trades dex.base_trades dex.trades_beta Contributors
oneinch #5303
aerodrome #4837 @tomfutago
airswap #4864 @tomfutago
apeswap #4866 @Hosuke
arbswap #4869 @tomfutago
babyswap #4923 @tomfutago
balancer #4940 @tomfutago
bancor #4972 @tomfutago
beethoven_x #5047 @tomfutago
biswap #4924 @tomfutago
camelot #4867 @tomfutago
carbon_defi #4868 @tomfutago
carbonhood #4842 @tomfutago
clipper #5043 @tomfutago
curvefi #5139 @tomfutago
defiswap @Hosuke
dfx #5044 @tomfutago
dodo #5049 @tomfutago
ellipsis_finance #5125 @tomfutago
equalizer #5052 @tomfutago
firebird_finance #4907 #4907 @ARDev097
fraxswap #4922 @tomfutago
glacier #5053 @tomfutago
gmx #5056 @tomfutago
hashflow #4999 @ARDev097
honeyswap #4928 @tomfutago
integral #5057 @tomfutago
iziswap #5058 @tomfutago
kyberswap #5062 @tomfutago
mauve #5073 @tomfutago
maverick #5074 @tomfutago
mdex #4929 @tomfutago
mento #4413 @tomfutago
mstable #5075 @tomfutago
nomiswap #5077 @tomfutago
onepunchswap #5126 @tomfutago
openocean #5127 @tomfutago
openxswap #5120 @tomfutago
opx_finance #5106 @tomfutago
pancakeswap #4921 @tomfutago
platypus_finance #5118 @tomfutago
quickswap #4895 @nyssarex
rubicon #5133 @tomfutago
shibaswap #4925 @tomfutago
spartacus_exchange #5081 @tomfutago
spiritswap #4931 @tomfutago
spookyswap #4930 @tomfutago
sushiswap #4832 @tomfutago
swapr #5086 @tomfutago
synthetix #5117 @tomfutago
thena #5113 @tomfutago
trader_joe #4859 @chef-seaweed @Hosuke
ubeswap #4349 @tomfutago
uniswap @jeff-dude @Hosuke @lequangphu
velodrome #5112 @tomfutago
verse_dex #5011 @tomfutago
wardenswap #5012 @tomfutago
wigoswap #5013 @tomfutago
wombat #5015 @tomfutago
woofi #4769 @ARDev097
xchange #5111 @tomfutago
zigzag #5279 @Hosuke
zipswap #4932 @tomfutago

Updated records in dex.trades_beta compared to dex.trades

Related PR

Enhancement PR

@Hosuke Hosuke added the dbt: dex covers the DEX dbt subproject label Nov 10, 2023
@Hosuke Hosuke self-assigned this Nov 10, 2023
@jeff-dude
Copy link
Member

jeff-dude commented Nov 10, 2023

thanks for the intiative @Hosuke 🙌

should we repurpose this issue to be dex.trades lineage specific? so rather than just general macro usage, this issue will track all new dex's that get added to the new _beta dex lineage

we could have separate issues for separate sectors

edit: based on the title and label on issue, maybe you intended dex only? reading the description made me doubt that a bit, just want to clarify

@jeff-dude jeff-dude added WIP work in progress dune team created by dune team labels Nov 10, 2023
@jeff-dude jeff-dude self-assigned this Nov 10, 2023
@jeff-dude jeff-dude added the enhancement New feature or request label Nov 10, 2023
@Hosuke
Copy link
Collaborator Author

Hosuke commented Nov 12, 2023

thanks for the intiative @Hosuke 🙌

should we repurpose this issue to be dex.trades lineage specific? so rather than just general macro usage, this issue will track all new dex's that get added to the new _beta dex lineage

we could have separate issues for separate sectors

edit: based on the title and label on issue, maybe you intended dex only? reading the description made me doubt that a bit, just want to clarify

Sorry my phrase may not clear enough. I intended in the dex sector only.

@jeff-dude jeff-dude changed the title [ENHANCEMENT] Tracking Implementations of the New Macro Approach in dex.trades Tracking Implementations of the New Macro Approach in dex.trades Nov 13, 2023
@jeff-dude jeff-dude changed the title Tracking Implementations of the New Macro Approach in dex.trades migrate all dex models to the new structure / design Nov 13, 2023
@jeff-dude
Copy link
Member

next steps in the dex.trades redesign workstream

following the format for uniswap in liked PR from issue description above, add all dex models in dex.trades lineage to the new beta design.

notes from the original redesign gh issue:

  • this foundational PR focused on uniswap dex across all chains, as well as one uniswap fork, which allowed us to leverage a macro approach to the logic to use universally in all spells. this allows for a structure in place to do the same for other protocols which have many forks
  • naming standards
    • there was some back and forth & slight differences in opinion, but ultimately, the majority of these spells are meant for the backend data engineers and not user-facing on the dune app, so we landed on base_ prefix for table aliases to indicate it's a stepping stone to a final trades table
      • this also sets us up to be able to create views on top of dex.trades for project-specific spells, which would ultimately use the same naming standard as old pipeline, therefore replacing the old schema/alias name, and leaving base_ for the pipelines only (i.e. no app usage)
    • similar for the macro name, we landed on using uniswap_ prefix and all that forked can use it
  • directory structure
    • macros/models/_sector/dex/
    • models/_sector/dex/trades//platforms/
  • materialization strategy
    • platform/project level: incremental
    • blockchain level: view
    • sector level: incremental
  • model config settings
    • always include both schema and alias -- let's avoid adding schema to project file, that'll be cleaned up over time
    • no changes to table configs -- incremental/merge/delta
    • add incremental predicates -- this allows the target to also be filtered on incremental loads to the same as the source
  • macro vs. code in model
    • if standalone dex, more than fine to have code directly in model
    • if forked dex and/or repeatable logic, use or create a macro

@jeff-dude
Copy link
Member

@Hosuke are you comfortable leading this? we can also reach out to users in the community to help migrate to new design in this issue, and coordinate who is helping migrate which dex.

@jeff-dude
Copy link
Member

it is recommended to open one PR per dex to keep review cycle clean & quick as possible. it's okay to do all blockchains of a dex in one PR, but at minimum keep the PR to one dex.

@Hosuke
Copy link
Collaborator Author

Hosuke commented Nov 14, 2023

@Hosuke are you comfortable leading this? we can also reach out to users in the community to help migrate to new design in this issue, and coordinate who is helping migrate which dex.

I am very enthusiastic about leading this initiative. However, there may be times when my language or technical descriptions might not be as precise as needed. Any additional directions or specific content that you can provide to ensure the success of this project are welcomed.

@jeff-dude
Copy link
Member

I am very enthusiastic about leading this initiative. However, there may be times when my language or technical descriptions might not be as precise as needed. Any additional directions or specific content that you can provide to ensure the success of this project are welcomed.

no problem -- i'll be closely engaged all along 🤝 thanks for leading this!

@jeff-dude
Copy link
Member

jeff-dude commented Nov 14, 2023

specific next steps:

  1. ensure block_number is pulled through all base_trades spells in first PR (uniswap, defiswap) — new PR, if needed, to add. if they already exist, move on but ensure future ones include too
  2. revisit seed PR Add check_seed tests and seed files with new schema to base_trades models #4774, break out into project-specific spells and follow seed directory format for nft seeds, only include columns in seed which are used downstream (join conditions, compare columns in model schema yml file tests) — ensure block_number is part of join condition checks. use the generic check seed test, follow format in nft projects.
  3. start to migrate other projects like uniswap. 1 PR <--> 1 project to keep it clean. comment in the issue which projects are being worked on to avoid overlap work and tag PR in issue for tracking

once we reach step 3, we can look to see if anyone in the community is interested in helping migrate any dex projects to the new structure 🙏

@nyssarex
Copy link
Contributor

Once you reach step 3 and finalized instructions created, i would like to help to migrate spells to the new structure

@Hosuke
Copy link
Collaborator Author

Hosuke commented Nov 20, 2023

Thank you @nyssarex , as I think we are at step 3, you can take any project to migrate and list them here if you want.

@jeff-dude
Copy link
Member

@Hosuke can we monitor the beta spell over time, as more are added, in terms of matching existing dex.trades?

this comment had a query to compare during the initial build and results matched. sadly i didn't save the query & it's a screenshot, but should be quick to rewrite (and enhance as needed). if results differ at all as more dexes are added, we can group by project / blockchain / version and find specific spells which may be the issue.

we could explore adding as a test in the repo too, so it runs in CI automatically, if that's easier.

@Hosuke
Copy link
Collaborator Author

Hosuke commented Jan 4, 2024

@Hosuke @tomfutago another enhancement i don't want us to forget about is how we assign amount_usd in the final stages. as of now, we've continued the pattern from early days of dex.trades and default to token bought, then fallback to token sold if bought isn't available in prices.usd

we should look for a better if this/then that scenario to assign a value. can you guys brainstorm some ideas? random thoughts on my side:

  • have a static list of "trust tokens" -- think USDC/WETH/etc -- then always use those when present. else, do the bought/sold coalesce

I have done some experiments to rank token stability in this query:
https://dune.com/queries/3327086

If this rank is useful to solve this problem, I can extend the query into a spell.

edit: which is better? static or dynamic stable coin rank?

@jeff-dude
Copy link
Member

@Hosuke where do we stand on the status? what steps are left to finalize the migration?

some thoughts:

  • coverage of dexes included
  • test query showing data matches (or v close, as some calculations can differ slightly / had some bug fixes in new lineage)
  • steps to migrate _beta to final table
  • communication plan (i can help here)

@Hosuke Hosuke removed the WIP work in progress label Feb 20, 2024
@Hosuke
Copy link
Collaborator Author

Hosuke commented Feb 20, 2024

@Hosuke where do we stand on the status? what steps are left to finalize the migration?

some thoughts:

  • coverage of dexes included
  • test query showing data matches (or v close, as some calculations can differ slightly / had some bug fixes in new lineage)
  • steps to migrate _beta to final table
  • communication plan (i can help here)

The current dex.trades_beta covers all existing dex.trades:
https://dune.com/queries/3446010
and sushiswap_base.trades discrepancy is tested in #5376

@jeff-dude
Copy link
Member

@Hosuke where do we stand on the status? what steps are left to finalize the migration?
some thoughts:

  • coverage of dexes included
  • test query showing data matches (or v close, as some calculations can differ slightly / had some bug fixes in new lineage)
  • steps to migrate _beta to final table
  • communication plan (i can help here)

The current dex.trades_beta covers all existing dex.trades: https://dune.com/queries/3446010 and sushiswap_base.trades discrepancy is tested in #5376

fantastic!

want to open a PR which renames the _beta lineage to normal naming standards (i.e. remove beta)? should just be that final spell?
then we can also delete the entire legacy dex.trades lineage within the same PR

@jeff-dude
Copy link
Member

i will work on how to finalize communication on this

@Hosuke
Copy link
Collaborator Author

Hosuke commented Feb 21, 2024

@Hosuke where do we stand on the status? what steps are left to finalize the migration?
some thoughts:

  • coverage of dexes included
  • test query showing data matches (or v close, as some calculations can differ slightly / had some bug fixes in new lineage)
  • steps to migrate _beta to final table
  • communication plan (i can help here)

The current dex.trades_beta covers all existing dex.trades: https://dune.com/queries/3446010 and sushiswap_base.trades discrepancy is tested in #5376

fantastic!

want to open a PR which renames the _beta lineage to normal naming standards (i.e. remove beta)? should just be that final spell? then we can also delete the entire legacy dex.trades lineage within the same PR

We may keep the original dex.trades maybe as dex.trades_legacy, and we can try to test/compare the some more detailed trades:
https://dune.com/queries/3467096

Here is a simple test we may use for testing data consistency.

@jeff-dude
Copy link
Member

We may keep the original dex.trades maybe as dex.trades_legacy, and we can try to test/compare the some more detailed trades: https://dune.com/queries/3454010

Here is a simple test we may use for testing data consistency.

okay, we can keep it live under _legacy for a bit, but after a little time, we'll remove completely

the linked query here will need to evolve a bit as analysis continues, to account for some intentional changes we made of switching bought/sold during migration (as tom found bugs, for example). so we could add case statements for certain projets/versions/blockchains to ignore in results, if we know we flipped intentionally.

@Hosuke
Copy link
Collaborator Author

Hosuke commented Feb 27, 2024

We may keep the original dex.trades maybe as dex.trades_legacy, and we can try to test/compare the some more detailed trades: https://dune.com/queries/3454010
Here is a simple test we may use for testing data consistency.

okay, we can keep it live under _legacy for a bit, but after a little time, we'll remove completely

the linked query here will need to evolve a bit as analysis continues, to account for some intentional changes we made of switching bought/sold during migration (as tom found bugs, for example). so we could add case statements for certain projets/versions/blockchains to ignore in results, if we know we flipped intentionally.

Since we have fixed flipped tokens in both dex.trades and dex.trades_beta, the diff is caused by token_bought_address = token_sold_address in dex.trades:
https://dune.com/queries/3467096

@Hosuke
Copy link
Collaborator Author

Hosuke commented Mar 4, 2024

  • steps to migrate _beta to final table
  1. Rename the dex.trades and dex.trades_beta SPE-345 finalize and migrate dex.trades to new structure #5501
  2. Remove legacy code gradually

@jeff-dude
Copy link
Member

jeff-dude commented Mar 4, 2024

  • steps to migrate _beta to final table
  1. Rename the dex.trades and dex.trades_beta Rename dex.trades #5501
  2. Remove legacy code gradually

i wonder if we quickly add one final test / validation, we fork this dashboard:
https://dune.com/hagaetc/dex-metrics

and update a few queries to read from dex.trades_beta and see how the dashboard looks relative to original, since post-merge on attached PR will flip the main dashboard to this new dex spell.

we don't need to do all the queries and visuals, but maybe 2-3, such as:
1 - DEX 24 hours volume
2 - DEX 7 days value
3 - Ranked DEX by volume

if you think you can do this in ~an hour or two, let's see how that looks 🙏

edit:
i think we may end up with quite different numbers, as we have more projects and fixed some bugs, but could be nice to see anyway. or at least on the 3rd item above, where it's project specific, so easier to compare
i know the query covers most of this already, so this isn't a must, just a nice quick visual

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dbt: dex covers the DEX dbt subproject dune team created by dune team enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants