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

Adding registrar4 to ENS registration and renewal views #3468

Closed
wants to merge 7 commits into from

Conversation

web3lyt
Copy link
Contributor

@web3lyt web3lyt commented Jun 3, 2023

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

@dune-eng
Copy link

dune-eng commented Jun 3, 2023

Workflow run id 5165499718 approved.

@dune-eng
Copy link

dune-eng commented Jun 3, 2023

Workflow run id 5165499717 approved.

@dune-eng
Copy link

dune-eng commented Jun 5, 2023

Workflow run id 5179666916 approved.

@dune-eng
Copy link

dune-eng commented Jun 5, 2023

Workflow run id 5179666917 approved.

@web3lyt web3lyt changed the title Adding registrar4 to ENS registration and renewal tables Adding registrar4 to ENS registration and renewal views Jun 5, 2023
Copy link
Collaborator

@Hosuke Hosuke left a comment

Choose a reason for hiding this comment

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

Hi @web3lyt,
as this PR is currently out-of-date, please run git merge main or use other ways to sync with the latest main branch.

Thank you.

Comment on lines +6 to +12
SELECT *, NULL AS premium
FROM {{source('ethereumnameservice_ethereum', 'ETHRegistrarController_1_evt_NameRegistered')}}
UNION
SELECT *
SELECT *, NULL AS premium
FROM {{source('ethereumnameservice_ethereum', 'ETHRegistrarController_2_evt_NameRegistered')}}
UNION
SELECT *
SELECT *, NULL AS premium
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @web3lyt, we can cast NULL to its expected type for premium field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't seem necessary. In sparksql, the datatype requirements are lax, and in dunesql, this still makes the field end up as a UINT256. Further, since this view needs to work in both sparksql and dunesql, I can't CAST the datatype to a common one unless I change it to INT in the raw table as well in both versions. It's much messier if I cast, this works better.

@dune-eng
Copy link

dune-eng commented Jun 6, 2023

Workflow run id 5189720908 approved.

@dune-eng
Copy link

dune-eng commented Jun 6, 2023

Workflow run id 5189720911 approved.

@dune-eng
Copy link

dune-eng commented Jun 6, 2023

Workflow run id 5189726973 approved.

@dune-eng
Copy link

dune-eng commented Jun 6, 2023

Workflow run id 5189726975 approved.

@web3lyt
Copy link
Contributor Author

web3lyt commented Jun 6, 2023

I merged but this PR isn't ready anyway. Was fixing some issues. Reverted back some changes to see why they were causing issue, as it actually should have been working back then.

@Hosuke Hosuke added the WIP work in progress label Jun 6, 2023
@jeff-dude
Copy link
Member

thinking out loud here.

my best guess is since the view is built on spark engine (all of spellbook is, as of now), the fields are still string and not unit256, which is only available on dunesql. when using the dune app, an override occurs which shows the field(s) as uint256 as expected, but that happens due to a backend process. since this dunesql test doesn't run on the dune app, i wonder if the conversion of data type to unit256 isn't happening, hence the failure on adding varchar/string fields.

@jmsgu can you help confirm this theory or help us see why this is failing?

@jmsgu
Copy link
Contributor

jmsgu commented Jun 7, 2023

thinking out loud here.

my best guess is since the view is built on spark engine (all of spellbook is, as of now), the fields are still string and not unit256, which is only available on dunesql. when using the dune app, an override occurs which shows the field(s) as uint256 as expected, but that happens due to a backend process. since this dunesql test doesn't run on the dune app, i wonder if the conversion of data type to unit256 isn't happening, hence the failure on adding varchar/string fields.

@jmsgu can you help confirm this theory or help us see why this is failing?

@jeff-dude i'm looking into this, currently suspect this is an issue with the catalog used in the test query as directly querying test_schema.git_cd5e4773_ens_view_registrations with these changes using dunesql works fine. i'll touch base with the team on the recent dev catalog changes and circle back here.

@jmsgu
Copy link
Contributor

jmsgu commented Jun 7, 2023

Catalog setup appears to be working as intended. In the meantime, can we do an explicit type conversion from varchar to uint256 for those columns being added together? Did a quick test on this, and I had to add a cast on the cost column for each of the other queries in the chain of unions.

@jeff-dude
Copy link
Member

Catalog setup appears to be working as intended. In the meantime, can we do an explicit type conversion from varchar to uint256 for those columns being added together? Did a quick test on this, and I had to add a cast on the cost column for each of the other queries in the chain of unions.

i don't think we can add a cast to unit256 in the spell, as the view is built on spark first, which should fail on that. if our only option is to cast, we will need to either materialize as table or wait until spellbook migration to trino

@jeff-dude jeff-dude self-assigned this Jun 8, 2023
@jeff-dude jeff-dude added the in review Assignee is currently reviewing the PR label Jun 8, 2023
@jeff-dude
Copy link
Member

closing PRs marked as wip state, prior to the freeze announced and tracked here, we can reopen and reformat post-freeze

@jeff-dude jeff-dude closed this Jun 20, 2023
@web3lyt
Copy link
Contributor Author

web3lyt commented Jun 20, 2023

closing PRs marked as wip state, prior to the freeze announced and tracked here, we can reopen and reformat post-freeze

seems like it wasn't very possible when view spells were supposed to work on both engines at the same time anyway. I look forward to having to only write it in dunesql

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 WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants