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

Daily Arbitrum Stablecoin Balances #6757

Merged
merged 12 commits into from
Sep 24, 2024
Merged

Conversation

Synthquest
Copy link
Contributor

Thank you for contributing to Spellbook 🪄

Update!

Please build spells in the proper subproject directory. For more information, please see the main readme, which also links to a GH discussion with the option to ask questions.

Contribution type

Please check the type of contribution this pull request is for:

  • New spell(s)
  • Adding to existing spell lineage
  • Bug fix

Note: You can safely discard any section below which doesn't apply based on selection above


For new spell(s)

If you are building new spell(s), please provide the following information:

  • Spell name(s): stablecoins_arbitrum.balances
  • Description: materialized table of daily balances of stablecoins
  • Who are the new spell(s) for? Internal team
  • How will the new spell(s) be used downstream? Using solely the daily_balances table hits runtime errors & recreating the logic for daily views of stablecoin balances among wallets hits query complexity/runtime errors.

This query is for anyone interested in network views of stablecoin balances

These tokens will utilize the balances_incremental_subset_daily macro to generate materialized daily tables of tokens listed ^

Following the same approach as here:
#6608

  • Test instructions: unique address/balance/day
  • Related issue(s): [Link to related issues, if any]

For adding to existing spell lineage

If you are adding to an existing spell lineage, please provide the following information:

  • Description: [Description of the changes made]

For bug fixes

If you are fixing a bug, please provide the following information:

  • Description: [Description of the bug fix]
  • Steps to reproduce: [How to reproduce the bug]
  • Implementation details: [Information on how the bug was fixed]
  • Test instructions: [How to test the fix]
  • Related issue(s): [Link to related issues, if any]

Additional information

Please provide any additional information that might help us review your pull request:

  • [Any additional information]

Thank you for your contribution!

@dune-eng
Copy link

Workflow run id 10929899636 approved.

@dune-eng
Copy link

Workflow run id 10929899630 approved.

@Synthquest Synthquest marked this pull request as draft September 18, 2024 20:49
@Synthquest Synthquest marked this pull request as ready for review September 18, 2024 22:01
@Synthquest
Copy link
Contributor Author

@jeff-dude

I'm planning to test one chain and run through the rest of them in a separate PR if it looks good

@jeff-dude jeff-dude self-assigned this Sep 19, 2024
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR question Further information is requested labels Sep 19, 2024
@jeff-dude
Copy link
Member

jeff-dude commented Sep 19, 2024

gm @Synthquest
thank you for using the macro here 🙏
do you think we can move this code to the daily subproject, as the data is at daily level? it is also heavy code to compute, especially as our tokens prod env is rather small compared to daily.

edit:
you can see just arbitrum took ~an hour, then we'd have all other EVMs to consider

@dune-eng
Copy link

Workflow run id 10944669542 approved.

@dune-eng
Copy link

Workflow run id 10944670007 approved.

@dune-eng
Copy link

Workflow run id 10946158848 approved.

@dune-eng
Copy link

Workflow run id 10946158999 approved.

@Synthquest
Copy link
Contributor Author

Synthquest commented Sep 19, 2024

@jeff-dude
I moved to the daily folder and seem to have a longer runtime + a "remove" error popping up.

Chatgpt seems to think it timed out or hit some resource constraint 😅. On the previous run it was able to initialize the model but not able to complete the incremental run

@jeff-dude
Copy link
Member

jeff-dude commented Sep 19, 2024

@jeff-dude I moved to the daily folder and seem to have a longer runtime + a "remove" error popping up.

Chatgpt seems to think it timed out or hit some resource constraint 😅. On the previous run it was able to initialize the model but not able to complete the incremental run

it could've been bad timing with other PRs running code at the same time, on the same cluster. you can always try to monitor that via the actions tab. let's see how this latest run goes.

edit:
we are aware this macro can hit the timeout limit for CI. if it does again, you can supply a hardcoded date filter on the source data read, to see smaller chunks go through to prove it works as intended, then open it back up again.

@dune-eng
Copy link

Workflow run id 10950558230 approved.

@dune-eng
Copy link

Workflow run id 10950558467 approved.

@dune-eng
Copy link

Workflow run id 10952278872 approved.

@dune-eng
Copy link

Workflow run id 10952278792 approved.

@Synthquest
Copy link
Contributor Author

@jeff-dude
I limited it down to just USDC for testing - we may just have to limit the amount of stables we try and track here

@jeff-dude
Copy link
Member

@jeff-dude I limited it down to just USDC for testing - we may just have to limit the amount of stables we try and track here

i don't think we need to limit ourselves on the stablecoins, it's a pretty small list. i haven't seen a commit here yet where you push a hardcoded date into the macro -- can you try that?
i.e. block_time >= date '2024-08-01' outside of the incrmental logic, so history is much shorter

@dune-eng
Copy link

Workflow run id 10961562717 approved.

@dune-eng
Copy link

Workflow run id 10961562733 approved.

@Synthquest
Copy link
Contributor Author

@jeff-dude

I added in a more recent start date. For context this spell was able to fully compile in the token folder and was able to initialize in the daily folder on the first run but failed at the incremental test.

shortening the time period seems to have worked as well. Not 100% sure this is what you meant though

@jeff-dude
Copy link
Member

shortening the time period seems to have worked as well. Not 100% sure this is what you meant though

yeah, that commit looks like what i'm picturing. how do the results look? have you queried the CI output table to verify? if so, we can remove the short timeframe and prep for merge

@Synthquest
Copy link
Contributor Author

Awesome, I quickly did a check on:

  1. if top wallet balances lined up
  2. a few random balances for some of the other stables
  3. outlier on max() values on balances
  4. negative balances

For future guidance when we add more chains should we follow a similar approach (1 PR per chain with shortened timeframe for review)?

@dune-eng
Copy link

Workflow run id 10965416955 approved.

@dune-eng
Copy link

Workflow run id 10965417046 approved.

@Synthquest
Copy link
Contributor Author

@jeff-dude
All checks passed

re-

For future guidance when we add more chains should we follow a similar approach (1 PR per chain with shortened timeframe for review)?

@jeff-dude
Copy link
Member

For future guidance when we add more chains should we follow a similar approach (1 PR per chain with shortened timeframe for review)?

you can submit multiple chains per PR, if using same macro. you would more than likely have to restrict timeframe to see it run, yes

@jeff-dude jeff-dude removed the question Further information is requested label Sep 23, 2024
@dune-eng
Copy link

Workflow run id 11001525630 approved.

@dune-eng
Copy link

Workflow run id 11001525861 approved.

@dune-eng
Copy link

Workflow run id 11001959864 approved.

@dune-eng
Copy link

Workflow run id 11001960009 approved.

@Synthquest
Copy link
Contributor Author

@jeff-dude

  1. removed local sources

  2. updated existing sources: 2159408

  3. Rechecked from shorter time frame

Query is currently running tests but is set for the full time frame
lmk if there's anything else 🙏

Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

think we're all good now 🙏

@jeff-dude jeff-dude removed the in review Assignee is currently reviewing the PR label Sep 24, 2024
@jeff-dude jeff-dude merged commit 1b46dc4 into duneanalytics:main Sep 24, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants