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

Making nft tables' trade_category more consistent #1841

Merged
merged 5 commits into from
Nov 10, 2022
Merged

Making nft tables' trade_category more consistent #1841

merged 5 commits into from
Nov 10, 2022

Conversation

hildobby
Copy link
Collaborator

Brief comments on the purpose of your changes:

Before this PR, we have: Buy, Offer Accepted, Collection Offer Accepted, Sell, Auction Settled, Private Sale, Private Sales, Private Buy, Auction, Collection Offer, Collection/Trait & Trait Offer.

This PR narrows it down to: Buy, Offer Accepted, Collection Offer Accepted, Sell (now only for sudoswap, other platforms were switched to Offer Accepted), Auction Settled, Private Sale, Collection Offer Accepted, Trait Offer Accepted & Collection/Trait Offer Accepted (couldn't tell how to split that one into Collection Offer Accepted and Trait Offer Accepted).

Those were overlaps with other names so I switched them to the more used name: Private Sales, Private Buy, Auction.

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 contributors)

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')

@augustog augustog added the WIP work in progress label Oct 26, 2022
@augustog
Copy link
Contributor

Hmm... "pre" commit hooks are failing and I can't figure the cause. Maybe @dot2dotseurat / @jeff-dude do you know what could be the cause of this error?

Error: The head commit for this pull_request event is not ahead of the base commit. Please submit an issue on this action's GitHub repo.

@hildobby
Copy link
Collaborator Author

Just merged main into the branch in case that was the issue surfaced by the pre hooks.

@dot2dotseurat
Copy link
Collaborator

@hildobby looking into this error. so far I've just found a github issue with approx 1M people saying "same here" jitterbit/get-changed-files#11

@dot2dotseurat
Copy link
Collaborator

@hildobby ah this error is due to probably squashing commits. which is a great practice! if you want to avoid this error in the future, you can squash at the end of the PR. But I know you are using the hooks locally, so just make a note that you already tested locally and they passed.

@jeff-dude jeff-dude self-assigned this Nov 10, 2022
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR and removed WIP work in progress labels Nov 10, 2022
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.

after getting branch up-to-date, passes test here. lgtm 🤝

@jeff-dude jeff-dude merged commit d85f319 into duneanalytics:main Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Assignee is currently reviewing the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants