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

Add Ringers Metadata Spell #1493

Merged
merged 4 commits into from
Sep 5, 2022
Merged

Add Ringers Metadata Spell #1493

merged 4 commits into from
Sep 5, 2022

Conversation

catherine-2
Copy link
Contributor

@catherine-2 catherine-2 commented Sep 2, 2022

Brief comments on the purpose of your changes:

Hi! This is my first spell PR so using a basic query first that is all static data :)

Please let me know if I missed any files or information needed

One thing to note: I created a new subfolder under nft_ethereum called "metadata" to store metadata tables for NFT collections. I think these would be useful to have as spells (I have a number of user generated metadata tables I can upload so others can use them too). Let me know if there is a different way you'd like to organize these tables.

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 adding a new model, I edited the alter table macro to display new database object (table or view) in UI explorer
  • if adding a new materialized table, I edited the optimize table macro

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 Sep 2, 2022

Workflow run id 2981822738 approved.

@soispoke soispoke self-requested a review September 5, 2022 10:40
@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

Hi @catherine-2 thanks for your submission !

Can you please adapt the nft_ethereum_metadata_ringers.sql model so it a table with static values (by following the model here for example (https://github.com/duneanalytics/spellbook/blob/main/models/tokens/ethereum/tokens_ethereum_erc20.sql)
Where yours work, we would like to have static tables structured consistently across spells

@dune-eng
Copy link

dune-eng commented Sep 5, 2022

Workflow run id 2994365976 approved.

@catherine-2
Copy link
Contributor Author

Thank you @soispoke ! I just updated the format to match that example

@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

@catherine-2 thanks for applying changes, would you like to expose this view on Dune.com as well ?
If you do, there is an extra step needed, where you add metadata to the view/table you want to expose:

Here is the macro where you need to add your new model:
https://github.com/duneanalytics/spellbook/blob/main/macros/alter_table_properties.sql

After this lgtm!

@catherine-2
Copy link
Contributor Author

Thank you @soispoke - yeah I can add that. To make sure I understand - when you say 'expose this view on dune.com' does that mean in the online documentation, in the table list in the query editor, or both?

@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

@catherine-2 Good question, "exposing a table" means showing it the query editor, or data explorer. You don't need to add it in the macro to have it show up in the online documentation, this is done automatically

@catherine-2
Copy link
Contributor Author

@soispoke Got it! In that case if it will work could we merge this and I will expose it in the query editor in a future pull request?

This first PR was mostly to make sure I could figure out how to get dbt working and make the pull request :)

I will submit another PR this week or next that has several other metadata tables in it. Probably makes sense to expose the "metadata" folder in the query editor once there are more tables populated? Or we can keep this open until I have those ready and I can edit the name and add them all to this - whatever you prefer!

@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

@catherine-2 It's really your call, if you rather want to wait a bit before showing this on Dune.com in the explorer it's completely fine and we can merge now 👍

@catherine-2
Copy link
Contributor Author

@soispoke awesome let's merge now then to remove from the queue and I will make a note to expose this table alongside the others once those are added

thank you so much for your help!

@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

sounds good, thanks for submitting this first spell 🔥 @catherine-2 !

@soispoke soispoke merged commit 089f0bd into duneanalytics:main Sep 5, 2022
@catherine-2
Copy link
Contributor Author

@soispoke is there a delay for the table to show up on the documentation page? I had set it up to have a folder called 'metadata' under the nft model category but am not seeing it. Want to make sure I didn't mess it up and if I did make sure I know how to fix it in the next PR!

https://spellbook-docs.dune.com/#!/model/model.spellbook.nft_ethereum_aggregators

@soispoke
Copy link
Contributor

soispoke commented Sep 5, 2022

@catherine-2 yes there is, the best way to make sure everything worked is just to query your table on Dune :)

https://dune.com/queries/1227549?d=1&category=abstraction

@catherine-2
Copy link
Contributor Author

@soispoke ah got it - was typing the name in wrong trying to check it :) thank you for opening the other PR to fix!

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.

3 participants