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

OP Refactor Tokens & Housekeeping #2207

Merged
merged 116 commits into from
Jan 10, 2023

Conversation

MSilb7
Copy link
Contributor

@MSilb7 MSilb7 commented Dec 4, 2022

Brief comments on the purpose of your changes:

  • Add functionality to automatically pull tokens from deterministic creators (i.e. Aave, L2StandardTokenFactory)
  • Add flags to erc20 tokens to determine if/how we should handle the token price (i.e. if it's a deposit-receipt token - i.e. LP token - we don't want to count it in Optimism on-chain value or TVL, even if it's actively traded on DEXs)
  • Other housekeeping things (moving ovm1 folder under optimism)
  • Small one-off static updates

My local dbt is erroring out,
no dbt_project.yml found at expected path [peronsal path] /spellbook/dbt_packages/dbt_utils 2/dbt_project.yml
so draft PRing here to do checks
Fixed this

For Dune Engine V2

I've checked that:

General checks:

  • 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
  • if adding a new model, I edited the dbt project YAML file with new directory path for both models and seeds (if applicable)
  • if wanting to expose a model in the UI (Dune data explorer), I added a post-hook in the JINJA config to add metadata (blockchains, sector/project, name and contributor Dune usernames)

Pricing checks:

  • coin_id represents the ID of the coin on coinpaprika.com
  • all the coins are active on coinpaprika.com (please remove inactive ones)

Join logic:

  • if joining to base table (i.e. ethereum transactions or traces), I looked to make it an inner join if possible

Incremental logic:

  • I used is_incremental & not is_incremental jinja block filters on both base tables and decoded tables
    • where block_time >= date_trunc("day", now() - interval '1 week')
  • if joining to base table (i.e. ethereum transactions or traces), I applied join condition where block_time >= date_trunc("day", now() - interval '1 week')
  • if joining to prices view, I applied join condition where minute >= date_trunc("day", now() - interval '1 week')

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614069689 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614069687 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614089630 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614089631 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614121651 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614121653 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614135931 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614135932 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614147613 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614147617 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614170196 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614170194 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614187274 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614187269 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614191324 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614191313 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614196774 approved.

@dune-eng
Copy link

dune-eng commented Dec 4, 2022

Workflow run id 3614196778 approved.

GarvMaggu and others added 16 commits January 9, 2023 16:34
* cast to varchar in concat in view

* define varchar length'
* use batch api calls for price checker
…regator and pools (duneanalytics#2169)

* update dodo_ethereum_trades && add dodo_bnb_trades

* split dodo_trades to pools & aggregator on Ethereum and BNBChain

* label different dodoV2 pools

* fix schema & sql

* add dodo_proxies on Ethereum

* Modify dodo_bnb_trades to incremental view

* Apply check_dex_seed and modify dodo_eth_trades to incremental view

* Add DODO bnb trade seeds

* Apply check_dex_seed to DODO bnb trades model

* update dex_trade_seeds

* fix

* Add DODO bnb in project.yml

* remove gitkeep in dodo

* Update models/dodo/bnb/dodo_pools_bnb_trades.sql

* Update check_dex_seed in dodo_ethereum_schema.yml

* fix dodo_ethereum_schema

* Remove schema in the config blocks

* Add dodo_trades files by scoffie

* add dppOracle & dppAdvanced to dodo_bnb

* fix

* update dbt_project on dodo

* fix dodo_bnb_sources.yml

* move cast to base model

* update dex.trades seeds on DODO_BNBChain

* final formatting & cleanup expose spell macro

Co-authored-by: Huang Geyang <Sukebeta@outlook.com>
Co-authored-by: jeff-dude <jeff@dune.xyz>
* use most natural form of fee computation when possible

* add comment about imprecision
* trigger test

* enable test in CI

* defer in test

* fix aragon

* fix native token test

* fix native mints unique id

* fix nft trader labels

* union all in nft labels

* union all for performance

* trigger wash_trades full refresh

* fix cow protocol distinct labels

* revert null wallet_address fix

Co-authored-by: soispoke <66172107+soispoke@users.noreply.github.com>
* add eth transfers and small type fixes in op eth transfers

* comment update

* update schema

* update transfer id

* add tx to and from

* add tx_to and tx_from

* nit remove spaces

Co-authored-by: soispoke <66172107+soispoke@users.noreply.github.com>
* allow for case insensitive token symbol matching

* adapt token checker error/warns

* check for warning in test instead of error
* base is_incremental

* add force-incremental variable
@jeff-dude
Copy link
Member

apologies for all the commits, must have messed up the merge with main.

here is summary of final tweaks, hopefully good with you:

  • added the_granary schema default, in case other blockchains are added in future, removed default to view as the root of the file does so for all objects now to keep it clean(ish)
  • added aave_v3 schema name in model, to keep consistent with dune explorer and file name values
  • renamed directory models/optimism/optimism/ to models/optimism/ovm -- made an assumption here based on contents within, but duplicating optimism didn't seem to make sense (feel free to change back if needed)
  • added optimism in the_granary schema/source files within the optimism directory, again prepping for future blockchains
  • renamed optimism erc20 view back, i don't think detailed is necessary in the name even with added info/fields -- again, if you feel strongly, push the change back

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.

looks good 🤝

cool jinja usage in the aave model. you've outsmarted the CI test attached, it doesn't know how to interpret the loop. we can safely ignore the failure. more ideas for us to ideally enhance the test moving forward.

we can ignore the (beta) test failure on prices too. many times, the API will return values from another blockchain, but the id being valid is the biggest key. then we ask to prove contract address manually if needed, which @Hosuke helped with. the symbol on 'fBOMB' error can be ignored too, as that is different between the blockchains it appears.

@jeff-dude
Copy link
Member

apologies for all the commits, must have messed up the merge with main.

here is summary of final tweaks, hopefully good with you:

  • added the_granary schema default, in case other blockchains are added in future, removed default to view as the root of the file does so for all objects now to keep it clean(ish)
  • added aave_v3 schema name in model, to keep consistent with dune explorer and file name values
  • renamed directory models/optimism/optimism/ to models/optimism/ovm -- made an assumption here based on contents within, but duplicating optimism didn't seem to make sense (feel free to change back if needed)
  • added optimism in the_granary schema/source files within the optimism directory, again prepping for future blockchains
  • renamed optimism erc20 view back, i don't think detailed is necessary in the name even with added info/fields -- again, if you feel strongly, push the change back

if you agree with all of this, we can merge

@jeff-dude jeff-dude added question Further information is requested ready-for-merging and removed in review Assignee is currently reviewing the PR labels Jan 9, 2023
@MSilb7
Copy link
Contributor Author

MSilb7 commented Jan 10, 2023

Hey, yeah all makes sense! I don't remember what I was doing with the folder naming haha. ovm is kind of an outdated term, which is probably why I edited it, but we still use it everywhere in these tables. It works as a placeholder for "Optimism system things."

@MSilb7
Copy link
Contributor Author

MSilb7 commented Jan 10, 2023

looks good 🤝

cool jinja usage in the aave model. you've outsmarted the CI test attached, it doesn't know how to interpret the loop. we can safely ignore the failure. more ideas for us to ideally enhance the test moving forward.

we can ignore the (beta) test failure on prices too. many times, the API will return values from another blockchain, but the id being valid is the biggest key. then we ask to prove contract address manually if needed, which @Hosuke helped with. the symbol on 'fBOMB' error can be ignored too, as that is different between the blockchains it appears.

Yeah trial and error on the jinja in Aave haha. I think chatgpt helped me there too.

@jeff-dude
Copy link
Member

looks good 🤝
cool jinja usage in the aave model. you've outsmarted the CI test attached, it doesn't know how to interpret the loop. we can safely ignore the failure. more ideas for us to ideally enhance the test moving forward.
we can ignore the (beta) test failure on prices too. many times, the API will return values from another blockchain, but the id being valid is the biggest key. then we ask to prove contract address manually if needed, which @Hosuke helped with. the symbol on 'fBOMB' error can be ignored too, as that is different between the blockchains it appears.

Yeah trial and error on the jinja in Aave haha. I think chatgpt helped me there too.

oh man, are we going to have to edit our readme and put:
"if you're stuck on a spell, use chatgpt. that's it."

1 similar comment
@jeff-dude
Copy link
Member

looks good 🤝
cool jinja usage in the aave model. you've outsmarted the CI test attached, it doesn't know how to interpret the loop. we can safely ignore the failure. more ideas for us to ideally enhance the test moving forward.
we can ignore the (beta) test failure on prices too. many times, the API will return values from another blockchain, but the id being valid is the biggest key. then we ask to prove contract address manually if needed, which @Hosuke helped with. the symbol on 'fBOMB' error can be ignored too, as that is different between the blockchains it appears.

Yeah trial and error on the jinja in Aave haha. I think chatgpt helped me there too.

oh man, are we going to have to edit our readme and put:
"if you're stuck on a spell, use chatgpt. that's it."

@jeff-dude jeff-dude removed the question Further information is requested label Jan 10, 2023
@jeff-dude jeff-dude merged commit 9250098 into duneanalytics:main Jan 10, 2023
@MSilb7 MSilb7 deleted the op-contract-updates-dec22 branch January 10, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.