duneanalytics / spellbook

SQL views for Dune
Other
1.15k stars 1.06k forks source link

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

Open Hosuke opened 10 months ago

Hosuke commented 10 months ago

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

Call for Contributions

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

jeff-dude commented 10 months ago

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

Hosuke commented 10 months ago

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 commented 9 months ago

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:

jeff-dude commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 #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 commented 9 months ago

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

Hosuke commented 9 months ago

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 commented 9 months ago

@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.

nyssarex commented 9 months ago

Hi guys, quick updated, i did base trade for quickswap (only in polygon ). Localy compiled with no error , in CI failed (https://github.com/duneanalytics/spellbook/pull/4895). May be to scale process fast and seemles. @Hosuke can make simple guide or me after i get everything clearly how we migrate to base trades in different cases. uniswap compitable, notation with v1,v2,v3 , etc.

Hosuke commented 9 months ago

@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.

I write a draft query to track migration counts: https://dune.com/queries/3239455/5419355

Will try to enhance more, but currently it serves as an MVP tracker.

jeff-dude commented 9 months ago

@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.

I write a draft query to track migration counts: https://dune.com/queries/3239455/5419355

Will try to enhance more, but currently it serves as an MVP tracker.

love it!

we should likely dig into the ones that aren't 100% match sooner rather than later, before it become too long of a list, to see why

tomfutago commented 9 months ago

I write a draft query to track migration counts: https://dune.com/queries/3239455/5419355

Will try to enhance more, but currently it serves as an MVP tracker.

I looked at the results of your query and was concerned about the ones not 100% matching, so I checked them. I forked your query and in addition to project I also added breakdown by blockchainand version - I think that (query) gives clearer picture, for example:

image

jeff-dude commented 9 months ago

Hi guys, quick updated, i did base trade for quickswap (only in polygon ). Localy compiled with no error , in CI failed (#4895). May be to scale process fast and seemles. @Hosuke can make simple guide or me after i get everything clearly how we migrate to base trades in different cases. uniswap compitable, notation with v1,v2,v3 , etc.

thanks again for helping!

i'm not sure how we can determine ahead of time what is uniswap compatible and what isn't (is there a resource out there?). we may just need to look at the code each time and determine, it's pretty obvious to know when seeing the code. as for standards on versions and such, let's mostly stick to what was in there previously. i believe dex lineage prefers 1 rather than v1, for example. there are some projects which use letters/words instead of numbers.

jeff-dude commented 9 months ago

I write a draft query to track migration counts: https://dune.com/queries/3239455/5419355 Will try to enhance more, but currently it serves as an MVP tracker.

I looked at the results of your query and was concerned about the ones not 100% matching, so I checked them. I forked your query and in addition to project I also added breakdown by blockchainand version - I think that (query) gives clearer picture, for example:

  • curve is actually matching 100% as it's only Celo trades that are migrated atm
  • ubeswap - prod version has an incorrect project start date set (my fault!), and beta trades are actually accurate
  • sushiswap - few reasons for discrepancy: 1) I added sushi v2 to beta trades so that adds more records to total counts, 2) with break down by version - v1 counts in prod are matching v2 counts in beta as per this note (tl;dr beta is correct)
  • there are still these left to check:

image

way ahead of me, two minutes after i commented to dig deeper 😆 thanks for helping here

agreed on adding blockchain and version, that gives us a clear picture of where to look. that also helps us only compare beta against what existed in old dex lineage, rather than net new projects in beta only.

tomfutago commented 9 months ago

way ahead of me, two minutes after i commented to dig deeper 😆 thanks for helping here

agreed on adding blockchain and version, that gives us a clear picture of where to look. that also helps us only compare beta against what existed in old dex lineage, rather than net new projects in beta only.

I added now amounts comparison to the query too, but it might be an overkill 😂

tomfutago commented 9 months ago

I'm looking at amount_usd column and I see a trend of blank records starting around block_time Feb 2021 and then decreasing over time (possibly something similar I noticed in my other PR

https://dune.com/queries/3263618/5462973

Polygon case seems to be quite clear:

image
jeff-dude commented 9 months ago

I'm looking at amount_usd column and I see a trend of blank records starting around block_time Feb 2021 and then decreasing over time (possibly something similar I noticed in my other PR

https://dune.com/queries/3263618/5462973

Polygon case seems to be quite clear:

image

this came up in the 1inch PR to rebuild lineage into dex_aggregator.trades. we merged this change, which should help cover some gaps in prices. it appears the pricing providor doesn't have as much history for wrapped tokens and such, so we flipped the id value to the native id for coverage.

this may take some time to propogate through prices.usd, then may require a refresh of sector-level spells to account for it.

jeff-dude commented 8 months ago

@Hosuke can you add a readme md file in models/_sector/dex/ that contains info on:

we should already have most of this info across gh issues/PRs/comments. it'd be good to summarize in a readme at the root level of the sector.

i'll be happy to review it with ya when ready

once that is merged, we can explore building a more generic sector readme up one directory in models/_sector/

Hosuke commented 8 months ago

@Hosuke can you add a readme md file in models/_sector/dex/ that contains info on:

  • how to contribute
  • example PRs
  • details on the design & why we've chosen that path
  • ...and so on

we should already have most of this info across gh issues/PRs/comments. it'd be good to summarize in a readme at the root level of the sector.

i'll be happy to review it with ya when ready

once that is merged, we can explore building a more generic sector readme up one directory in models/_sector/

I init a readme in this PR: #5027

hildobby commented 8 months ago

From all I can find Warden (wardenswap) is only a dex aggregator and doesn't have it's own liquidity (so not a direct DEX). If that's correct, shouldn't it only appear in the aggregator table (or as an aggregator if the two tables plan on being merged like they were on postgres 🤞)? Or am I missing something? I'm just confused about it showing up in dex.trades cc @chain-l @tomfutago

tomfutago commented 8 months ago

From all I can find Warden (wardenswap) is only a dex aggregator and doesn't have it's own liquidity (so not a direct DEX). If that's correct, shouldn't it only appear in the aggregator table (or as an aggregator if the two tables plan on being merged like they were on postgres 🤞)? Or am I missing something? I'm just confused about it showing up in dex.trades cc @chain-l @tomfutago

Looks like BSC is an exception - wardenswap got deployed as uniswap v2 fork. source: https://defillama.com/protocol/wardenswap

jeff-dude commented 8 months ago

@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:

whichever approach we take, should it also be a universal macro that can be used in other sector spells?

edit: reference in discord here

Hosuke commented 8 months ago

@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 commented 6 months ago

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

some thoughts:

Hosuke commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

i will work on how to finalize communication on this

Hosuke commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago
  • steps to migrate _beta to final table
  1. Rename the dex.trades and dex.trades_beta #5501
  2. Remove legacy code gradually
jeff-dude commented 6 months ago
  • 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