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.

Sign up for GitHub

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

Jump to bottom

Adding aave interests abstraction in Dune v2 #1434

Merged
merged 35 commits into from
Sep 7, 2022
Merged

Adding aave interests abstraction in Dune v2 #1434

merged 35 commits into from
Sep 7, 2022

Conversation

SohamP99
Copy link
Contributor

@SohamP99 SohamP99 commented Aug 25, 2022 •
edited
Loading

Brief comments on the purpose of your changes:
Add an abstraction in Dune v2 that stores hourly interest rates of all Aave reserves on Ethereum.

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

chuxinh
chuxinh suggested changes Aug 27, 2022
View reviewed changes
models/base_sources/aave_base_sources.yml Outdated Show resolved Hide resolved
models/base_sources/aave_base_sources.yml Outdated Show resolved Hide resolved
models/aave/aave_ethereum_schema.yml Outdated Show resolved Hide resolved
models/aave/aave_ethereum_schema.yml Outdated Show resolved Hide resolved
models/aave/Ethereum/aave_ethereum_interest_rates.sql Outdated Show resolved Hide resolved
models/aave/Ethereum/aave_ethereum_interest_rates.sql Outdated Show resolved Hide resolved
models/aave/Ethereum/aave_ethereum_interest_rates.sql Outdated Show resolved Hide resolved
models/aave/Ethereum/aave_ethereum_interest_rates.sql Outdated Show resolved Hide resolved
tests/aave/Ethereum/Aave_interests_test.sql Outdated Show resolved Hide resolved
tests/aave/Ethereum/Aave_interests_test.sql Outdated Show resolved Hide resolved
SohamP99 and others added 10 commits August 28, 2022 16:21
@jeff-dude
Copy link
Member

@SohamP99 @chuxinh can we edit all directory paths and files names to be lowercase for consistency?

@SohamP99
Merge branch 'duneanalytics:main' into master
a8b5be2
@SohamP99
Rename models/aave/Ethereum/aave_v2_ethereum_interest_rates.sql to mo…
96c25e4 
…dels/aave/ethereum/aave_v2_ethereum_interest_rates.sql

Renaming to smaller case for consistancy
@SohamP99
Rename tests/aave/Ethereum/Aave_interests_test.sql to tests/aave/ethe…
af7987b 
…reum/aave_interests_test.sql

Changing file names to lower case for consistency
@SohamP99
Copy link
Contributor Author

@SohamP99 @chuxinh can we edit all directory paths and files names to be lowercase for consistency?

Done.

chuxinh
chuxinh approved these changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@chuxinh chuxinh left a comment

Choose a reason for hiding this comment

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

LGTM after nits

@jeff-dude
Copy link
Member

@SohamP99 i'm unable to push changes to your forked repo branch (not sure if it's because it's master or just the fork in general), but i was looking to move the aave_ethereum_schema yaml file into the ethereum sub-directory, as it's specific to ethereum

@jeff-dude
Copy link
Member

based on how you populated the dbt project yaml file & the config block in the model, your output object will be named 'aave_ethereum.aave_interest' -- is this intended? do you want to remove 'aave' in alias since it's in schema? how about the 'v2' part, that is in the file name but not table name? that works, but just ensuring

@chuxinh
Copy link
Contributor

chuxinh commented Sep 1, 2022

based on how you populated the dbt project yaml file & the config block in the model, your output object will be named 'aave_ethereum.aave_interest' -- is this intended? do you want to remove 'aave' in alias since it's in schema? how about the 'v2' part, that is in the file name but not table name? that works, but just ensuring

That's a good point. Maybe we should follow the uniswap structure and this one should be aave_v2_ethereum_interests

@SohamP99
Merge branch 'duneanalytics:main' into master
c2fe824
@SohamP99
Update and rename models/aave/aave_ethereum_schema.yml to models/aave…
bff5737 
…/ethereum/aave_ethereum_schema.yml

Moving the schema file to source directory and changing the meta
@SohamP99
Rename models/base_sources/aave_base_sources.yml to models/aave/ether…
6d5101a 
…eum/aave_ethereum_sources.yml

Moving the source file to aave ethereum folder
@SohamP99
Update aave_interests_test.sql
8cc7173 
replaced evt_block_time with hour 
corrected ==
corrected unit_test2
@SohamP99
Merge branch 'main' into master
8203d7d
@SohamP99
Copy link
Contributor Author

SohamP99 commented Sep 2, 2022

based on how you populated the dbt project yaml file & the config block in the model, your output object will be named 'aave_ethereum.aave_interest' -- is this intended? do you want to remove 'aave' in alias since it's in schema? how about the 'v2' part, that is in the file name but not table name? that works, but just ensuring

That's a good point. Maybe we should follow the uniswap structure and this one should be aave_v2_ethereum_interests

Yes, I have made the changes based on the uniswap structure.

@SohamP99
Copy link
Contributor Author

SohamP99 commented Sep 2, 2022

@jeff-dude Thanks for the review, I have made the requested changes.

@dune-eng
Copy link

dune-eng commented Sep 3, 2022

Workflow run id 2983227983 approved.

@SohamP99
Copy link
Contributor Author

SohamP99 commented Sep 5, 2022

Hey @jeff-dude . Regarding the above check fail [b03fdf7] , Is it possible that the tokens_ethereum_erc20 sql file might have been deleted from dev environment? The code seems good to me and several other models are also using this abstraction.

@jeff-dude
Copy link
Member

Hey @jeff-dude . Regarding the above check fail [b03fdf7] , Is it possible that the tokens_ethereum_erc20 sql file might have been deleted from dev environment? The code seems good to me and several other models are also using this abstraction.

i'm seeing no issues with my testing for joining to ethereum erc20 tokens model. i think you're all set, unless it's failing for you?

@dune-eng
Copy link

dune-eng commented Sep 7, 2022

Workflow run id 3004955182 approved.

@dune-eng
Copy link

dune-eng commented Sep 7, 2022

Workflow run id 3004971859 approved.

@SohamP99
Copy link
Contributor Author

SohamP99 commented Sep 7, 2022

Hey @jeff-dude . Regarding the above check fail [b03fdf7] , Is it possible that the tokens_ethereum_erc20 sql file might have been deleted from dev environment? The code seems good to me and several other models are also using this abstraction.

i'm seeing no issues with my testing for joining to ethereum erc20 tokens model. i think you're all set, unless it's failing for you?

Yeah the checks are passing now, we can go ahead with the merge if no further changes are required

@dune-eng
Copy link

dune-eng commented Sep 7, 2022

Workflow run id 3008465400 approved.

@jeff-dude
Copy link
Member

thank you for the back & forth and first spell contribution!

@jeff-dude
Copy link
Member

fyi @soispoke

@jeff-dude jeff-dude merged commit 82a99af into duneanalytics:main Sep 7, 2022
@soispoke
Copy link
Contributor

soispoke commented Sep 8, 2022

Nice ! 🔥 @jeff-dude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chuxinh chuxinh chuxinh approved these changes

@jeff-dude jeff-dude jeff-dude approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SohamP99 @jeff-dude @chuxinh @dune-eng @soispoke