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.

Sign up for GitHub

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

Jump to bottom

Celo Mento Protocol trades #4413

Merged
merged 33 commits into from
Nov 14, 2023
Merged

Celo Mento Protocol trades #4413

merged 33 commits into from
Nov 14, 2023

Conversation

tomfutago
Copy link
Contributor

Aim: add Mento Protocol to dex.trades

@dune-eng
Copy link

Workflow run id 6290937343 approved.

@dune-eng
Copy link

Workflow run id 6290937357 approved.

@dune-eng
Copy link

Workflow run id 6301154159 approved.

@dune-eng
Copy link

Workflow run id 6301154240 approved.

@dune-eng
Copy link

Workflow run id 6301201432 approved.

@dune-eng
Copy link

Workflow run id 6301201218 approved.

@dune-eng
Copy link

Workflow run id 6301302269 approved.

@dune-eng
Copy link

Workflow run id 6301302341 approved.

@dune-eng
Copy link

Workflow run id 6301582904 approved.

@dune-eng
Copy link

Workflow run id 6301583123 approved.

@dune-eng
Copy link

Workflow run id 6302060948 approved.

@dune-eng
Copy link

Workflow run id 6302061021 approved.

@dune-eng
Copy link

Workflow run id 6302152615 approved.

@dune-eng
Copy link

Workflow run id 6302152743 approved.

@dune-eng
Copy link

Workflow run id 6302186809 approved.

@dune-eng
Copy link

Workflow run id 6302186885 approved.

@dune-eng
Copy link

Workflow run id 6310528458 approved.

@dune-eng
Copy link

Workflow run id 6310528635 approved.

@dune-eng
Copy link

Workflow run id 6311087439 approved.

@jeff-dude jeff-dude added the in review Assignee is currently reviewing the PR label Nov 13, 2023
@tomfutago
Copy link
Contributor Author

  • the current state here only feeds into the new dex.trades_beta, is that okay for the short term until it replaces dex.trades completely? or do you need to add here to include in existing dex.trades structure?

yes, adding to dex.trades_beta is fine for now

  • what's the end goal for the mento_celo_pools spells?

    • one main reason i ask is due to usage of tokens erc20 metadata within the spells. you are materializing them at the most upstream level with this metadata, which is a pattern we're moving away from in new sector designs, especially since token metadata will soon flip to a source rather than ref (more to come on this!)
    • the goal will be to have all spells which read from them to be a view and/or the end of the dbt lineage, to avoid long lineages and full refresh dependencies all over in orchestration.

agreed! will remove tokens erc20 refs and bring pool models to follow dex_pools model

@dune-eng
Copy link

Workflow run id 6864368405 approved.

@dune-eng
Copy link

Workflow run id 6864368552 approved.

@dune-eng
Copy link

Workflow run id 6865783322 approved.

@dune-eng
Copy link

Workflow run id 6865783518 approved.

@dune-eng
Copy link

Workflow run id 6865808669 approved.

@dune-eng
Copy link

Workflow run id 6865808959 approved.

@tomfutago
Copy link
Contributor Author

pools updated now

  • one main reason i ask is due to usage of tokens erc20 metadata within the spells. you are materializing them at the most upstream level with this metadata, which is a pattern we're moving away from in new sector designs, especially since token metadata will soon flip to a source rather than ref (more to come on this!)

on that last bit - exciting stuff! but does it mean my #4762 doesn't have a chance to get in before new magic starts flowing?

jeff-dude
jeff-dude reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

few comments below

also, this shouldn't happen in this PR, but seems like dex.pools is a nice candidate for a redesign like dex.trades -- should be a bit easier to do, on number of models alone

models/mento/celo/mento_celo_pools.sql Outdated
Comment on lines 10 to 11
ref('mento_celo_pools_v1'),
ref('mento_celo_pools_v2')
Copy link
Member

Choose a reason for hiding this comment

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

similar to dex.trades lineage, should these be named as below?
mento_v1_celo_pools
mento_v2_celo_pools

Copy link
Member

Choose a reason for hiding this comment

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

would mean changes to file names, config settings, refs, etc etc

models/mento/celo/mento_celo_pools_v1.sql Outdated
select
'celo' as blockchain,
'mento' as project,
'v1' as version,
Copy link
Member

Choose a reason for hiding this comment

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

can you do me a favor and double check the usage of version in other dex pool models? do they preface with v in front of the version number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, looks like most pool models don't use 'v' prefix - only uniswap models do (fix in a separate pr?).

I'll remove 'v' for Mento.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, we can fix in later PR for others, no worries

models/mento/celo/mento_celo_pools_v1.sql Outdated
config(
schema = 'mento_celo',
alias = 'pools_v1',
materialized = 'table'
Copy link
Member

Choose a reason for hiding this comment

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

any reason v1 is full table refresh each time, while v2 is incremental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v1 is static, new pools won't get added, while v2 get updated from time to time (not too often tho).

I wasn't aware that table materialization gets refreshed every time - any better option to use here?

Copy link
Member

Choose a reason for hiding this comment

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

the combo of materialized = 'table' and tags = ['static'] will do the trick

we set up our orchestration to exclude anything static, so they only run when modified on main branch

@jeff-dude
Copy link
Member

on that last bit - exciting stuff! but does it mean my #4762 doesn't have a chance to get in before new magic starts flowing?

nope, these will continue until that work is final. it's in a PoC phase at the moment. i'll get it merged soon 🤝

@dune-eng
Copy link

Workflow run id 6866742695 approved.

@dune-eng
Copy link

Workflow run id 6866742670 approved.

@tomfutago
Copy link
Contributor Author

few comments below

also, this shouldn't happen in this PR, but seems like dex.pools is a nice candidate for a redesign like dex.trades -- should be a bit easier to do, on number of models alone

@hildobby do you want to take it over as you were the one who created dex.pools originally?

@jeff-dude
Copy link
Member

@hildobby do you want to take it over as you were the one who created dex.pools originally?

no rush here. if not prioritized, dune team will get to it eventually. just focusing on most commonly used ones first

@dune-eng
Copy link

Workflow run id 6867014031 approved.

@dune-eng
Copy link

Workflow run id 6867014199 approved.

jeff-dude
jeff-dude approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

looks good -- thank you!

@jeff-dude jeff-dude added ready-for-merging and removed question Further information is requested in review Assignee is currently reviewing the PR labels Nov 14, 2023
@jeff-dude
Copy link
Member

fyi @Hosuke
after this is merged, you may want to pull in from main branch into your open PR for cleaning up seed tests on dex trades new lineage for the mento project here

@jeff-dude jeff-dude merged commit 4b35fca into duneanalytics:main Nov 14, 2023
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeff-dude jeff-dude jeff-dude approved these changes

@Hosuke Hosuke Hosuke approved these changes

Assignees

@Hosuke Hosuke

@jeff-dude jeff-dude

Labels
dbt: dex covers the DEX dbt subproject ready-for-merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomfutago @dune-eng @jeff-dude @Hosuke