Tornado Cash: Fix sources, lowercase and improve materialization strategy #1460

soispoke · 2022-08-30T03:49:45Z

Brief comments on the purpose of your changes:

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.

height · 2022-08-30T03:49:50Z

This pull request has been linked to 1 task:

~~T-15760 fix tornado cash materialization strategy and lowercase~~ (unlink task)

💡Tip: Add "Close T-15760" 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.

This reverts commit f707c2e.

aalan3 · 2022-08-30T14:53:11Z

models/tornado_cash/tornado_cash_deposits.sql

@@ -35,13 +35,13 @@ FROM
                AND et.block_time >= (select min(evt_block_time) from {{ source('tornado_cash_ethereum','eth_evt_Deposit') }})
                {% endif %}
                {% if is_incremental() %}
-                AND et.block_time >= (select max(block_time) from {{ this }})
+                AND et.block_time >= date_trunc("day", now() - interval '1 week')


Shouldn't the LEFT JOIN above be an INNER JOIN instead? And then you could drop the where filter below.

added this common check to review doc, but many existing queries from v1 will have left joins to look out for

There are cases where you'd want to do a left join as I discovered here. But generally it seems like it should be inner.

aalan3 · 2022-08-30T14:54:40Z

models/tornado_cash/tornado_cash_deposits.sql

@@ -35,13 +35,13 @@ FROM
                AND et.block_time >= (select min(evt_block_time) from {{ source('tornado_cash_ethereum','eth_evt_Deposit') }})


Can't comment on the actual line but line 32 source('ethereum','transactions_0006') should be source('ethereum','transactions').

i believe that source was used specifically for performance reasons, but agreed to revert back if we're comfortable with new performance enhancement changes (could then also remove from source file)

That issue has been resolved now (since last week). We shouldn't refer to these ever though because they can (and will) change without warning.

@soispoke can you update this?

@jeff-dude Done

…cross dune

hildobby · 2022-08-30T21:23:42Z

Discussed this in DMs with @jeff-dude, imo naming should be consistent across the board and since avalanche_c is avalanche's default blockchain name, all abstractions should also stick with it for consistency, don't you guys think?

PS: Apologies for not changing the names myself when doing the first tornado cash PR, I will make sure to be careful on names being lowercase going forward!

…ategy-and-lowercase

soispoke

mades some small changes regarding lowercasing blockchains and moving from avalanche to avalanche_c for consistency

jeff-dude · 2022-08-31T19:02:22Z

macros/alter_table_properties.sql

@@ -560,17 +560,17 @@ ALTER TABLE airdrop_optimism.addresses SET TBLPROPERTIES ('dune.public'='true',
 {% endset %}

 {% set tornado_cash_deposits %}
-ALTER TABLE tornado_cash.deposits SET TBLPROPERTIES ('dune.public'='true',
-                                                'dune.data_explorer.blockchains'='["ethereum", "bnb", "avalanche", "gnosis", "optimism", "arbitrum"]',
+ALTER VIEW tornado_cash.deposits SET TBLPROPERTIES ('dune.public'='true',


shouldn't these both be 'table'?

You're right they should, pushed a fix

jeff-dude · 2022-08-31T19:02:53Z

models/tornado_cash/tornado_cash_deposits.sql

@@ -35,13 +35,13 @@ FROM
                AND et.block_time >= (select min(evt_block_time) from {{ source('tornado_cash_ethereum','eth_evt_Deposit') }})


@soispoke can you update this?

jeff-dude · 2022-08-31T19:05:04Z

to @aalan3 point, should we use this PR to update all joins to base tables as inner join?

…ategy-and-lowercase

soispoke

@jeff-dude Done, updated all joins with base tables

Updated code

soispoke · 2022-09-01T06:58:21Z

Thanks !

jeff-dude · 2022-09-01T15:04:26Z

macros/alter_table_properties.sql

                                                'dune.data_explorer.category'='abstraction',
                                                'dune.data_explorer.abstraction.type'='project',
                                                'dune.data_explorer.abstraction.name'='tornado_cash',
                                                'dune.data_explorer.contributors'='["hildobby", "dot2dotseurat"]');
 {% endset %}

 {% set tornado_cash_withdrawals %}
-ALTER TABLE tornado_cash.withdrawals SET TBLPROPERTIES ('dune.public'='true',
-                                                'dune.data_explorer.blockchains'='["ethereum", "bnb", "avalanche", "gnosis", "optimism", "arbitrum"]',
+ALTER VIEW tornado_cash.withdrawals SET TBLPROPERTIES ('dune.public'='true',


@soispoke looks like this one didn't get updated back to 'table'

Was fixed here https://github.com/duneanalytics/spellbook/pull/1475/files#diff-89d8392c4db5b996c368bef69b027f7ccd442b9135924630007065e3a1d5569cR590

Fix sources, lowercase and improve materialization strat

bed9de8

soispoke requested review from aalan3, agaperste, dot2dotseurat, masquot and jeff-dude August 30, 2022 03:49

soispoke mentioned this pull request Aug 30, 2022

Tornado cash abstractions edits #1412

Merged

6 tasks

soispoke added 2 commits August 30, 2022 12:36

ALTER TABLE, not VIEW

f707c2e

Revert "ALTER TABLE, not VIEW"

6fbecf5

This reverts commit f707c2e.

jeff-dude added the v2_engine label Aug 30, 2022

aalan3 reviewed Aug 30, 2022

View reviewed changes

changed avalanche to avalanche_c in blockchain name for consistency a…

1e9f8ae

…cross dune

soispoke added 3 commits August 31, 2022 09:35

Lowercase for blockchains and versiosn on schema.yml

326dbcf

avalanche to avalanche_c in schema and macro files

bccaa5d

Merge branch 'main' into T-15760-fix-tornado-cash-materialization-str…

c2f497b

…ategy-and-lowercase

soispoke commented Aug 31, 2022

View reviewed changes

jeff-dude previously requested changes Aug 31, 2022

View reviewed changes

soispoke added 5 commits September 1, 2022 08:22

transactions_006 to transactions

861dba2

Merge branch 'main' into T-15760-fix-tornado-cash-materialization-str…

c449a7e

…ategy-and-lowercase

Update alter_table_properties.sql

5d81c41

fix transactions_006

edc1780

Replace LEFT BY INNER JOINS on base transactions tables

66f0bf3

soispoke requested review from aalan3 and jeff-dude September 1, 2022 00:29

soispoke commented Sep 1, 2022

View reviewed changes

aalan3 approved these changes Sep 1, 2022

View reviewed changes

soispoke merged commit b6adb5e into main Sep 1, 2022

jeff-dude deleted the T-15760-fix-tornado-cash-materialization-strategy-and-lowercase branch September 1, 2022 15:03

jeff-dude reviewed Sep 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tornado Cash: Fix sources, lowercase and improve materialization strategy #1460

Tornado Cash: Fix sources, lowercase and improve materialization strategy #1460

soispoke commented Aug 30, 2022 •

edited

Loading

height bot commented Aug 30, 2022 •

edited

Loading

aalan3 Aug 30, 2022

jeff-dude Aug 30, 2022

aalan3 Aug 30, 2022

aalan3 Aug 30, 2022

jeff-dude Aug 30, 2022

aalan3 Aug 30, 2022

jeff-dude Aug 31, 2022

soispoke Sep 1, 2022

hildobby commented Aug 30, 2022

soispoke left a comment

jeff-dude Aug 31, 2022

soispoke Sep 1, 2022

jeff-dude Aug 31, 2022

jeff-dude commented Aug 31, 2022

soispoke left a comment

soispoke commented Sep 1, 2022

jeff-dude Sep 1, 2022

aalan3 Sep 1, 2022

		@@ -35,13 +35,13 @@ FROM
		AND et.block_time >= (select min(evt_block_time) from {{ source('tornado_cash_ethereum','eth_evt_Deposit') }})

Tornado Cash: Fix sources, lowercase and improve materialization strategy #1460

Tornado Cash: Fix sources, lowercase and improve materialization strategy #1460

Conversation

soispoke commented Aug 30, 2022 • edited Loading

height bot commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildobby commented Aug 30, 2022

soispoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-dude commented Aug 31, 2022

soispoke left a comment

Choose a reason for hiding this comment

soispoke commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soispoke commented Aug 30, 2022 •

edited

Loading

height bot commented Aug 30, 2022 •

edited

Loading