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

Introduce dex_aggregator_base_trades #6653

Merged
merged 16 commits into from
Sep 16, 2024

Conversation

Hosuke
Copy link
Collaborator

@Hosuke Hosuke commented Sep 3, 2024

Background: #6215

@Hosuke Hosuke added the WIP work in progress label Sep 3, 2024
@Hosuke
Copy link
Collaborator Author

Hosuke commented Sep 3, 2024

I will try to refactor lifi.trades in this PR and test if the new workflow works.

@Hosuke Hosuke marked this pull request as ready for review September 3, 2024 15:06
@Hosuke
Copy link
Collaborator Author

Hosuke commented Sep 3, 2024

dex_aggregator.trades check
(Try to ensure only lifi shows in diff records with diff in amount_usd)

@Hosuke Hosuke added ready-for-review this PR development is complete, please review and removed WIP work in progress labels Sep 3, 2024
@Hosuke Hosuke added the dbt: dex covers the DEX dbt subproject label Sep 3, 2024
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.

left initial feedback below

Comment on lines 96 to 98
left join {{ source('tokens', 'erc20') }} erc20b
on erc20b.contract_address = dexs.token_sold_address
and erc20b.blockchain = 'optimism'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
left join {{ source('tokens', 'erc20') }} erc20b
on erc20b.contract_address = dexs.token_sold_address
and erc20b.blockchain = 'optimism'
left join {{ source('tokens', 'erc20') }} erc20b
on erc20b.contract_address = dexs.token_sold_address
and erc20b.blockchain = 'optimism'

while we're here, should we remove erc20 metadata too, and apply at the end with prices? similar to dex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I drafted a similar enrichment macro in this PR.

@jeff-dude jeff-dude self-assigned this Sep 3, 2024
@jeff-dude jeff-dude added WIP work in progress in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review labels Sep 3, 2024
Comment on lines 29 to 33
, token_bought_symbol
, token_sold_symbol
, token_pair
, token_bought_amount
, token_sold_amount
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try to remove these 5 columns from base_trades in order to remove erc20 source table from base_trades spells.

@Hosuke
Copy link
Collaborator Author

Hosuke commented Sep 4, 2024

checkpoint

, block_month
, block_date
, block_time
-- , block_number
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

block_number may be supported in future.

@Hosuke Hosuke removed the WIP work in progress label Sep 4, 2024
@jeff-dude jeff-dude added ready-for-review this PR development is complete, please review and removed in review Assignee is currently reviewing the PR labels Sep 5, 2024
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.

this is looking good, i think we're on the right track here 🙏

@jeff-dude jeff-dude added ready-for-merging and removed ready-for-review this PR development is complete, please review labels Sep 12, 2024
@jeff-dude jeff-dude merged commit 9bf7433 into duneanalytics:main Sep 16, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 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.

2 participants