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

Adding Odos Ethereum and Base trades #5718

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

amalashkevich
Copy link
Contributor

Thank you for contributing to Spellbook!

Thank you for taking the time to submit code in Spellbook. A few things to consider:

  • If you are a first-time contributor, please sign the CLA by copy & pasting exactly what the bot mentions in PR comment
  • Refer to docs section below to answer questions
  • Dune team will review submitted PRs as soon as possible

Best practices

To speed up your development process in PRs, keep these tips in mind:

  • Each commit to your feature branch will rerun CI tests (see example)
    • This includes all modified models on your branch
    • This includes all history of the data
  • Two tips for faster development iteration:
    • Ensure dbt is installed locally (refer to main readme) and run dbt compile
      • This will output raw SQL in target/ directory to copy/paste and run on Dune directly for initial query testing
    • Hardcode a WHERE filter for only ~7 days of history on large source tables, i.e. ethereum.transactions
      • This will speed up the CI tests and output results quicker -- whether that's an error or fully successful run
      • Once comfortable with small timeframe, remove filter and let full history run

Incremental model setup

  • Make sure your unique key columns are exactly the same in the model config block, schema yml file, and seed match columns (where applicable)
  • There cannot be nulls in the unique key columns
    • Be sure to double check key columns are correct or COALESCE() as needed on key column(s), otherwise the tests may fail on duplicates

🪄 Use the built CI tables for testing 🪄

Once CI completes, you can query the CI tables and errors in dune when it finishes running.

  • For example:
    • In the run initial models and test initial models, there will be a schema that looks like this: test_schema.git_dunesql_4da8bae_sudoswap_v2_base_pool_creations
    • This can be temporarily queried in Dune for ~24 hours

Leverage these tables to perform QA testing on Dune query editor -- or even full test dashboards!

Spellbook contribution docs

The docs directory has been implemented to answer as many questions as possible. Please take the time to reference each .md file within this directory to understand how to efficiently contribute & why the repo is designed as it is 🪄

Example questions to be answered:

Please navigate through the docs directory to find as much info as you can.

Note: happy to take PRs to improve the docs, let us know 🤝

@dune-eng
Copy link

dune-eng commented Apr 2, 2024

Workflow run id 8526776877 approved.

@dune-eng
Copy link

dune-eng commented Apr 2, 2024

Workflow run id 8526776761 approved.

@dune-eng
Copy link

dune-eng commented Apr 4, 2024

Workflow run id 8555660997 approved.

@dune-eng
Copy link

dune-eng commented Apr 4, 2024

Workflow run id 8555661196 approved.

@jeff-dude jeff-dude self-assigned this Apr 5, 2024
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR dbt: dex covers the DEX dbt subproject labels Apr 5, 2024
@jeff-dude jeff-dude linked an issue Apr 5, 2024 that may be closed by this pull request
Copy link
Member

Choose a reason for hiding this comment

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

block_month isn't being pulled in this view, so downstream is failing on the column missing:
https://github.com/duneanalytics/spellbook/blob/main/models/odos/odos_trades.sql#L24-L47

make sure all columns in dex_aggregator_trades.sql are being selected in odos_trades.sql

Copy link
Member

Choose a reason for hiding this comment

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

while in this spell, you can remove comment at the top about not migrated

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8569727093 approved.

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8569727341 approved.

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8569785309 approved.

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8569785493 approved.

@@ -33,7 +33,7 @@ with event_data AS (
contract_address AS project_contract_address,
evt_tx_hash AS tx_hash,
evt_index,
array[-1] as trace_address
CAST(ARRAY[-1] as array<bigint>) 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.

accidental trailing comma on these changes :)

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8570411862 approved.

@dune-eng
Copy link

dune-eng commented Apr 5, 2024

Workflow run id 8570411985 approved.

@amalashkevich
Copy link
Contributor Author

@jeff-dude checks passed!

@jeff-dude jeff-dude added ready-for-merging and removed in review Assignee is currently reviewing the PR labels Apr 5, 2024
@jeff-dude jeff-dude merged commit 8037373 into duneanalytics:main Apr 5, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 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.

No records in dex_aggregator.trades
3 participants