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

DuneSQL Migration: balancer_trades #4097

Merged
merged 134 commits into from
Aug 23, 2023

Conversation

thetroyharris
Copy link
Contributor

Thank you for contributing to Spellbook!

Please refer to the top of the readme in the root of Spellbook to learn how to contribute to Spellbook on DuneSQL.

@dune-eng
Copy link

Workflow run id 5939536845 approved.

@dune-eng
Copy link

Workflow run id 5939536837 approved.

@dune-eng
Copy link

Workflow run id 5940499252 approved.

@dune-eng
Copy link

Workflow run id 5940499245 approved.

@dune-eng
Copy link

Workflow run id 5940849200 approved.

@dune-eng
Copy link

Workflow run id 5940849176 approved.

@jeff-dude jeff-dude self-assigned this Aug 22, 2023
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR DuneSQL migration labels Aug 22, 2023
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.

i'll submit quick feedback on arbitrum first, which most will apply across all blockchains

@dune-eng
Copy link

Workflow run id 5944453683 approved.

@dune-eng
Copy link

Workflow run id 5944453841 approved.

swap.poolId AS pool_id,
swap_fees.swap_fee_percentage / POWER(10, 18) AS swap_fee,
swap.evt_tx_hash AS tx_hash,
'' AS trace_address,
Copy link
Member

Choose a reason for hiding this comment

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

we are deprecating this field from downstream dex.trades, so it can be removed from all balancer spells

if you'd prefer to take note and remove when you revisit these to update the varbinary types, that works too, then we can just ignore the field downstream. if you want to do now, that is good too

Copy link
Member

Choose a reason for hiding this comment

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

it can be removed from unique key, unique combo test in schema files, and all throughout spells in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So trace_address can be removed from everywhere and we will be fine to merge? We won't have to wait additional time to merge?

Copy link
Member

Choose a reason for hiding this comment

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

i will plan to merge tomorrow morning. if you have time to remove those fields before, please do. if not, please plan to do on your follow up PR to clean out data types and such

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do it on the follow up PR. Thank you.

@dune-eng
Copy link

Workflow run id 5944723708 approved.

@dune-eng
Copy link

Workflow run id 5944723914 approved.

@jeff-dude jeff-dude added ready-for-merging and removed in review Assignee is currently reviewing the PR labels Aug 22, 2023
@dune-eng
Copy link

Workflow run id 5945022170 approved.

@dune-eng
Copy link

Workflow run id 5945022167 approved.

@jeff-dude jeff-dude merged commit 74eb4da into duneanalytics:main Aug 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2023
@thetroyharris thetroyharris deleted the migration-balancer-trades branch August 23, 2023 14:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants