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

zereox - v2 macro replacements 2 #7168

Merged
merged 40 commits into from
Dec 3, 2024
Merged

Conversation

RantumBits
Copy link
Contributor

Thank you for contributing to Spellbook 🪄

Please open the PR in draft and mark as ready when you want to request a review.

Description:

[...]


quick links for more information:

@RantumBits RantumBits changed the title V2 macros2 zereox - v2 macro replacements 2 Nov 20, 2024
@jeff-dude jeff-dude added WIP work in progress dbt: dex covers the DEX dbt subproject labels Nov 20, 2024
@RantumBits
Copy link
Contributor Author

RantumBits commented Nov 27, 2024

@jeff-dude i got this updated v2 macro to run successfully. there was an issue with a invalid taker value internally in some cases that i didn't catch for a while. i've implemented a few enhancements - lmk if you see room for anything further.

@RantumBits RantumBits marked this pull request as ready for review November 27, 2024 17:55
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.

(assuming i didn't break anything, CI is running as i type)

this looks good to me. i added a few comments, more for my understanding as i looked closer at the macros. i also attempted to simplify the metadata reads in final details macro, to simply pull the data incrementally per chain and then later downstream the join from transactional data to metadata has our required timesetamp / address conditions.

one piece i looked closely at but wasn't sure how to improve performance (or if it can be) is around this logic in the settler_txs macro:

WHERE
        (a.settler_address IS NOT NULL OR tr.to in (0x0000000000001fF3684f28c67538d4D072C22734,0x0000000000005E88410CcDFaDe4a5EfaE4b49562,0x000000000000175a8b9bC6d539B3708EEd92EA6c))

i've noticed dunesql engine can struggle on those OR statements including varbinary addresses. also just joining on address in general back to larger raw tables, but that is seemingly the only way to cut down the number of transactions tied to settler addresses

@jeff-dude jeff-dude added ready-for-merging and removed WIP work in progress labels Dec 2, 2024
@jeff-dude jeff-dude assigned jeff-dude and unassigned Hosuke Dec 2, 2024
@jeff-dude
Copy link
Member

interesting, looks like we had performance enhancements with simplifying the metadata CTEs. it's possible it's noise due to CI env being less busy than last run, but still a good sign to consider before merging. i will likely merge this tomorrow

@jeff-dude jeff-dude merged commit e025533 into duneanalytics:main Dec 3, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dbt: dex covers the DEX dbt subproject ready-for-merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants