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

WIP T 16269 broken erc721 balances tables #1525

Merged
merged 6 commits into from
Sep 8, 2022

Conversation

soispoke
Copy link
Contributor

@soispoke soispoke commented Sep 7, 2022

Brief comments on the purpose of your changes:
This PR aims to improve/fix erc721 balances on ethereum.
Changes have been added (mainly fix the sign for amounts, and the send/receive fields + some improved incremental filters), but there are still issues with duplicate owners that need to be addressed, see https://dune.com/queries/1246260

The test was also changed (and is currently failing), so that wallet address is not part of it (therefore, it is now actually testing for unique owners)

@hildobby @0xRobin @aalan3

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

@height
Copy link

height bot commented Sep 7, 2022

This pull request has been linked to 1 task:

💡Tip: Add "Close T-16269" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

@0xRobin 0xRobin mentioned this pull request Sep 8, 2022
@0xRobin
Copy link
Collaborator

0xRobin commented Sep 8, 2022

@soispoke, I fixed some grouping and partitioning issues I found. (see PR mentioned above)
I believe query should now return correct results but I was not able to fully test it.

Copy link
Collaborator

@0xBoxer 0xBoxer left a comment

Choose a reason for hiding this comment

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

Seems good to me, this should make sure that every NFT (combination of address and id) only exists once in the balances table for any given day or hour.

@soispoke
Copy link
Contributor Author

soispoke commented Sep 8, 2022

thanks @0xBoxer

@soispoke soispoke merged commit 3e08848 into main Sep 8, 2022
@jeff-dude jeff-dude deleted the T-16269-broken-erc721-balances-tables branch September 8, 2022 16:14
@0xBoxer
Copy link
Collaborator

0xBoxer commented Sep 8, 2022

@soispoke @0xRobin

I found a edge case where the script goes wrong:

https://dune.com/queries/1079379

https://etherscan.io/token/0xc7457f480416e145093edb355f626faa0fc7f90e?a=2116

This is a weird contract that minted a token_id twice.
The currently running script assumes that the first owner is the correct one and actually changes the balance of the first owner on a transfer 20 days later, despite that owner not even having an outgoing transfer for this token.

The second minter is the correct one and sells his NFT a few minutes after minting, this tx is completely missing from our results. Therefore, in the period between 01/05 to 20/05 this token has wrong owner data.

This finding makes me question what usually happens in the script if a token gets traded multiple times within a day, because that seems to be what's going wrong here.

@0xRobin
Copy link
Collaborator

0xRobin commented Sep 8, 2022

@0xBoxer Good catch!
This is actually expected behaviour from the current query.
When 2 addresses have positive delta for that day (meaning more incoming then outgoing transfers for that ID) the query itself produces 2 records.
The deduplication happens later on the table merge cause those entries will have identical unique keys. So I guess it's currently quite random which owner will be chosen as we don't have strong ordering guarantees within the source table partitions.

We could update the query to include the deduplication and favour the entry which had the last incoming transfer. Another option might be to provide ordered output if that guarantees us the deduplication by the merge will choose the correct owner. (Will need to look into spark/databricks docs to confirm)

@0xRobin 0xRobin mentioned this pull request Sep 9, 2022
14 tasks
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.

4 participants