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

Add pancakeswap to dex.trades_beta #4921

Merged
merged 26 commits into from
Dec 13, 2023
Merged

Add pancakeswap to dex.trades_beta #4921

merged 26 commits into from
Dec 13, 2023

Conversation

tomfutago
Copy link
Contributor

@tomfutago tomfutago commented Dec 2, 2023 •
edited
Loading

linked to #4759

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7069836699 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7069836729 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7069895467 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7069895468 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070611278 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070611257 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070699250 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070699224 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070789364 approved.

@dune-eng
Copy link

dune-eng commented Dec 2, 2023

Workflow run id 7070789354 approved.

@tomfutago tomfutago marked this pull request as ready for review December 2, 2023 14:40
@Hosuke Hosuke self-assigned this Dec 3, 2023
Hosuke
Hosuke approved these changes Dec 3, 2023
View reviewed changes
Copy link
Collaborator

@Hosuke Hosuke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.✅
Thank you @tomfutago

pancakeswap base_trades check

models/_sector/dex/trades/ethereum/platforms/pancakeswap_v2_ethereum_base_trades.sql Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice hybrid spell.

@Hosuke Hosuke added dbt: dex covers the DEX dbt subproject ready-for-final-review labels Dec 3, 2023
@Hosuke Hosuke mentioned this pull request Dec 3, 2023
Open
@Hosuke Hosuke assigned jeff-dude and unassigned Hosuke Dec 4, 2023
@jeff-dude
Copy link
Member

looks like the relationship test is failing? can we add values to the info table and/or fix the gaps?

@jeff-dude jeff-dude added WIP work in progress and removed ready-for-final-review labels Dec 4, 2023
@dune-eng
Copy link

dune-eng commented Dec 7, 2023

Workflow run id 7127117766 approved.

@dune-eng
Copy link

dune-eng commented Dec 7, 2023

Workflow run id 7127117778 approved.

@tomfutago
Copy link
Contributor Author

tomfutago commented Dec 7, 2023 •
edited
Loading

Phew, and I thought this dex will be quick & easy addition..

Few notes on my changes today:

  • I removed quotes around each version entry in _schema for BNB & ETH as check_seed_macro is adding quotes for strings (and all other schema entries are without quotes, so added consistency too). @Hosuke once again thanks for quick fix to this macro!
  • there's this model: nexusmutual_ethereum.capital_pool_eth_daily_transaction_summary which seems to be broken. I removed seed check on it - to be revisited in a separate PR (maybe original contributor can help @guyhow ? 🙏)

@tomfutago tomfutago requested a review from Hosuke December 7, 2023 11:30
@jeff-dude
Copy link
Member

Phew, and I thought this dex will be quick & easy addition..

Few notes on my changes today:

  • I removed quotes around each version entry in _schema for BNB & ETH as check_seed_macro is adding quotes for strings (and all other schema entries are without quotes, so added consistency too). @Hosuke once again thanks for quick fix to this macro!
  • there's this model: nexusmutual_ethereum.capital_pool_eth_daily_transaction_summary which seems to be broken. I removed seed check on it - to be revisited in a separate PR (maybe original contributor can help @guyhow ? 🙏)

note for future:
if we need to change a macro, especially one that touches on many spells / out of scope of this PR, we should move to new PR and then merge / refresh in original PR. we can see the headaches it can cause

with that said, thanks for continuing to help finalize!

@jeff-dude jeff-dude added ready-for-review this PR development is complete, please review and removed WIP work in progress labels Dec 8, 2023
@Hosuke
Copy link
Collaborator

Hosuke commented Dec 9, 2023

  • I removed quotes around each version entry in _schema for BNB & ETH as check_seed_macro is adding quotes for strings (and all other schema entries are without quotes, so added consistency too). @Hosuke once again thanks for quick fix to this macro!

Seems the macro will auto wrap values in the filters into string.

jeff-dude
jeff-dude reviewed Dec 11, 2023
View reviewed changes
seeds/zeroex/polygon/zeroex_polygon_nft_fills_sample.csv Outdated
Copy link
Member

Choose a reason for hiding this comment

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

fyi, i fixed this seed here #4975
we can merge in main and restore this file

@jeff-dude
Copy link
Member

i'm a bit confused on where we stand on this one. here is my vote:

  • open new PR to fix macro for seeds
  • move any seeds that are included in this PR solely due to change from seed macro, not anything to do with dex / pancakeswap
  • get that merged
  • rebase with main here and proceed with clean setup

plz note that #4975 was merged recently to fix one of the 0x seeds, as it was blocking another PR too

@jeff-dude
Copy link
Member

plz note that #4975 was merged recently to fix one of the 0x seeds, as it was blocking another PR too

while looking into this, i'm piecing together that the @state:modified in the dbt seed step for CI test is pulling in any seed in the entire dbt lineage of a modified spell, not just those spells modified directly in PR. i believe that is what @ prefix does. while the use case in this PR is different, we modified the macro directly which causes all to run, it's still good to know for other CI test failure use cases.

this PR is modifying some static metadata spells, but still kicked off the 0x seed (plus mnay others), since one of the modified spells is in the lineage in which that seeds runs on.

the intention of the @ is to help get around issues of missing seeds when modifying existing spells that we've hit in CI often (i.e. metadata does not exist failure). working through the kinks!

@jeff-dude jeff-dude added WIP work in progress and removed ready-for-review this PR development is complete, please review labels Dec 11, 2023
@Hosuke
Copy link
Collaborator

Hosuke commented Dec 12, 2023 •
edited
Loading

I will open a new PR #4978 for updating the check_seed macro first.

Hosuke added a commit to Hosuke/spellbook that referenced this pull request Dec 12, 2023
@Hosuke
Update fixed seed from duneanalytics#4921
4f81143
Hosuke added a commit to Hosuke/spellbook that referenced this pull request Dec 12, 2023
@Hosuke
Update fixed seed from duneanalytics#4921
2f03f50
Hosuke
Hosuke reviewed Dec 12, 2023
View reviewed changes
seeds/dex/offers/dex_offers_seed.csv Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @tomfutago for fixing so many seed records.
I have moved those changes/fixes into this PR #4978 .
You can have a look to see if I miss anything.

jeff-dude pushed a commit that referenced this pull request Dec 12, 2023
@Hosuke
Upgrade check_seed_macro to support multiple values filters. (#4978)
8c4ebf2 
* Update check_seed_macro

* Update fixed seed from #4921

* Update fixed seed from #4921

* Comment test in nexusmutual_ethereum_schema
@jeff-dude
Copy link
Member

this one is ready to rebase with main now 👍

be sure to take the main version of zeroex_polygon_nft_fills_sample plz

@Hosuke Hosuke added ready-for-review this PR development is complete, please review and removed WIP work in progress labels Dec 13, 2023
@jeff-dude jeff-dude removed the ready-for-review this PR development is complete, please review label Dec 13, 2023
@jeff-dude jeff-dude merged commit 6eea03b into duneanalytics:main Dec 13, 2023
1 of 2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 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 requested changes

@Hosuke Hosuke Hosuke approved these changes

Assignees

@Hosuke Hosuke

@jeff-dude jeff-dude

Labels
dbt: dex covers the DEX dbt subproject
Projects
None yet
Milestone
No milestone
Development

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