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 Flashloans to NFT Wash Filter #3360

Merged
merged 31 commits into from
Jun 5, 2023

Conversation

hildobby
Copy link
Collaborator

Brief comments on the purpose of your changes:

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

@hildobby hildobby added WIP work in progress blocked NFT labels May 18, 2023
@hildobby hildobby removed the blocked label May 23, 2023
@hildobby hildobby added ready-for-review this PR development is complete, please review and removed WIP work in progress labels May 23, 2023
@hildobby hildobby requested a review from 0xRobin May 23, 2023 14:24
@0xRobin 0xRobin self-assigned this May 25, 2023
@0xRobin 0xRobin added in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review labels May 30, 2023
Copy link
Collaborator

@0xRobin 0xRobin left a comment

Choose a reason for hiding this comment

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

@hildobby ✔️ this looks good.

side note:
would you be interested in having these models implemented in a macro?
(1 macro that's then used for all the different chains, so you'd only need to change the logic in 1 place)
Or do you see the logic diverging a bit between different chains? In that case we'd need to keep the implementation chain specific.

@0xRobin 0xRobin added ready-for-merging and removed in review Assignee is currently reviewing the PR labels May 30, 2023
@hildobby
Copy link
Collaborator Author

you be interested in having these models implemented in a macro?
(1 macro that's then used for all the different chains, so you'd only need to change the logic in 1 place)
Or do you see the logic diverging a bit between different chains? In that case we'd need to keep the implementation chain specific.

I excluded Solana when making it crosschain since a lot of the fields were missing there for Solana trades and the filter thus didn't work (it was all is_wash_trade=true since buyer and seller were both NULLs and thus equal for example). I'm guessing that decoded Solana data changes things though, once Solana marketplaces have their own spell and necessary fileds are not NULL, the implementation should be the same afaik, but my Solana knowledge is limited. I would maybe keep it as is for now unless you view the change as necessary.

What do you think about keeping current structure and adding is_wash_trade in nft.trades? And have it be NULL for now for non-EVM chains. Imo many would find it practical, and we could also keep nft.wash_trades to keep the other per filter columns there for those who want to dig further (and also not overcrowd nft.trades with too many new columns)

@jeff-dude jeff-dude merged commit 50b4ae0 into duneanalytics:main Jun 5, 2023
2 checks passed
olgafetisova pushed a commit to olgafetisova/spellbook that referenced this pull request Nov 14, 2023
* add uni, bal, dydx

* fix

* fix

* fix

* fix versioning

* Update aave_v3_arb_flashloans to incremental view

* Update aave other flashloans spells

* Add missing schema and incremental filters

* Update config blocks to view

* Add missing schema and incremental filters for uniswap_flashloans

* Dune SQL modification trial

* make incremental

* fix and add link to ported DuneSQL query for easier migration when needed

* Change back to spark sql with incremental filter

* Add Flashloans to NFT Wash Filter

* add block_number

* undo

* fix

* fix

* fix

---------

Co-authored-by: Huang Geyang <Sukebeta@outlook.com>
Co-authored-by: 0xRob <83790096+0xRobin@users.noreply.github.com>
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.

None yet

4 participants