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

Replace trader_a values for 1inch in dex.trades #1368

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

fatayri
Copy link
Contributor

@fatayri fatayri commented Aug 11, 2022

Brief comments on the purpose of your changes:
Currently in dex.trades trader_a and tx_from are exactly the same for 1inch transactions. With this PR we replace the trader_a with the "from" value from the trace, not the transaction.

For Dune Engine V2
I've checked that:

  • [] I tested the query on dune.com after compiling the model with dbt compile (compiled queries are written to the target directory)
  • I used "refs" to reference other models in this repo and "sources" to reference raw or decoded tables
  • if adding a new model, I added a test
  • the filename is unique and ends with .sql
  • each sql file is a select statement and has only one view, table or function defined
  • column names are lowercase_snake_cased

When you are ready for a review, tag duneanalytics/data-experience. We will re-open your forked pull request as an internal pull request. Then your spells will run in dbt and the logs will be avaiable in Github Actions DBT Slim CI. This job will only run the models and tests changed by your PR compared to the production project.

@jeff-dude
Copy link
Member

thank you @fatayri for the PR. i assume this means you'd like to truncate dex trades for 1inch (attached script) and then reload fresh with the new field?

i can look to do that tomorrow, it'll take a good amount of time i'd imagine to reprocess, so the data will be gone for a bit.

@fatayri
Copy link
Contributor Author

fatayri commented Aug 12, 2022

Hey @jeff-dude! Thanks for the prompt reply.

Do we actually have any other options other than truncating and replacing? If not, it should be fine to do it so.

@jeff-dude
Copy link
Member

Hey @jeff-dude! Thanks for the prompt reply.

Do we actually have any other options other than truncating and replacing? If not, it should be fine to do it so.

the other option is that you can supply an update statement here in the comments, but that may end up a bit complex with all the different project names, versions and categories contained within. it also may take even more time than a delete/reload.

let me know your thoughts between the two and i'll look to perform on monday morning

@fatayri
Copy link
Contributor Author

fatayri commented Aug 19, 2022

Hey, sorry for the super long reply. We considered our options and decided that it might make more sense to truncate the values and fully rewrite it. But it's better to let it run over the weekend.

If it's not too late for today and you have some spare time - it would be perfect. If not, we can run it next Friday.

@jeff-dude
Copy link
Member

Hey, sorry for the super long reply. We considered our options and decided that it might make more sense to truncate the values and fully rewrite it. But it's better to let it run over the weekend.

If it's not too late for today and you have some spare time - it would be perfect. If not, we can run it next Friday.

no problem, i'll look to do it over the weekend at some point

@jeff-dude
Copy link
Member

this is still running and backfilling history, if anyone is noticing the data missing

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.

@fatayri this fix should be applied and backfilled again 👍

@jeff-dude jeff-dude merged commit 2b7c301 into duneanalytics:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants