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

erc721 category causing dupes #1231

Merged
merged 3 commits into from
Jul 1, 2022
Merged

erc721 category causing dupes #1231

merged 3 commits into from
Jul 1, 2022

Conversation

dot2dotseurat
Copy link
Collaborator

Brief comments on the purpose of your changes:
Multiple categories per collection is causing duplicate rows which our unit tests alerted us to.

I'm dropping this column until we have a fix that will convert the category column into categories.

For Dune Engine V2
I've checked that:

  • 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

When you are ready for a review, tag duneanalytics/data-experience. We will re-open your forked pull request as an internal pull request. Then your spells will run in dbt and the logs will be avaiable in Github Actions DBT Slim CI. This job will only run the models and tests changed by your PR compared to the production project.

@dot2dotseurat
Copy link
Collaborator Author

@hildobby for transparency

@@ -54,7 +54,7 @@ models:
- name: standard
tests:
- accepted_values:
values: ['erc721', 'erc1155' , 'erc20', 'cryptopunks','superrare']
values: ['erc721', 'erc1155' , 'erc20', 'cryptopunks','superrare', 'unknown']
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what 'unknown' tokens are / where they come from ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screen Shot 2022-07-01 at 10 19 10 AM
dencentaland wearables?

Copy link
Contributor

Choose a reason for hiding this comment

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

wow ok, i guess that is unknown

'ethereum' as blockchain,
d.hour,
b.wallet_address,
b.token_address,
b.tokenId,
nft_tokens.name as collection,
nft_tokens.category as category
Copy link
Contributor

Choose a reason for hiding this comment

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

Is deleting the whole category column the only/best way ? It seems like it's an easy/temporary fix but it would still be cool to include categories in erc721 balances. Wouldn't distinct fix it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, just a temp fix. we should try to do a group by list aggregation but I need to figure out how to do it in sparksql.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if you think it'll take a long time to find the right fix we can push this but it made me a bit hesitant at first because it kind of feels like it wouldn't be too much work to get it fixed without excluding categories

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll still approve I don't want it to block you or anything

@dot2dotseurat dot2dotseurat merged commit 2c73590 into master Jul 1, 2022
@dot2dotseurat dot2dotseurat deleted the erc721-patch branch July 1, 2022 14:54
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.

2 participants