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

SPE-118 Gas migration #4405

Merged
merged 12 commits into from
Sep 28, 2023
Merged

SPE-118 Gas migration #4405

merged 12 commits into from
Sep 28, 2023

Conversation

couralex6
Copy link
Contributor

Thank you for contributing to Spellbook!

Please refer to the top of the readme in the root of Spellbook to learn how to contribute to Spellbook on DuneSQL.

@couralex6 couralex6 changed the title [WIP] Gas migration [WIP] SPE-118 Gas migration Sep 26, 2023
@aalan3
Copy link
Contributor

aalan3 commented Sep 27, 2023

@hildobby @MSilb7 these tables are very heavy on the runs. And looking at our internal data the only one that's actually being queried right now is the optimism version.

How useful are these tables for you at the moment? And are there any plans to build on top of them? Otherwise we are considering not migrating these and removing them except for on Optimism.

EDIT: Disregard in this PR. I was referring to fees_traces which we can talk about here.

@hildobby
Copy link
Collaborator

@hildobby @MSilb7 these tables are very heavy on the runs. And looking at our internal data the only one that's actually being queried right now is the optimism version.

How useful are these tables for you at the moment? And are there any plans to build on top of them? Otherwise we are considering not migrating these and removing them except for on Optimism.

I was thinking of starting to use them soon. Do you think you could migrate them all but disable in prod all of them except optimism? That would make it much easier to re-add them when I want to start using them

Also, this spell came as a follow-up to 3 separate requests from devs for queries on some txs with the same method. I for one want to use it but I'm sure many devs would love to use them (very useful to compare gas optimisation for example). But like most of dune, one of the biggest issues is discoverability and ppl not being aware of the spells that are out there

@aalan3
Copy link
Contributor

aalan3 commented Sep 27, 2023

Sorry I made a mistake 🤦‍♂️ these tables should be migrated without exception. These actually do have some usage. My comment above was meant for fees_traces and not fees. I'll add a comment to the discussion and we can continue there. Sorry for the confusion!

@hildobby
Copy link
Collaborator

hildobby commented Sep 27, 2023

Sorry I made a mistake 🤦‍♂️ these tables should be migrated without exception. These actually do have some usage. My comment above was meant for fees_traces and not fees. I'll add a comment to the discussion and we can continue there. Sorry for the confusion!

Ah, consider my answer for fees_traces then actually haha, that's what I was thinking of actually

@aalan3
Copy link
Contributor

aalan3 commented Sep 27, 2023

Ah nice 😄 ok then we can exclude the non optimism ones for now and enable them once you start building on top of them again.

Copy link
Contributor

@aalan3 aalan3 left a comment

Choose a reason for hiding this comment

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

Could you also add remove to the legacy models?

txns.effective_gas_price/1e9 as gas_price_gwei,
txns.effective_gas_price/1e18 * p.price as gas_price_usd,
txns.gas_price/1e9 as gas_price_bid_gwei,
txns.gas_price/1e18 * p.price as gas_price_bid_usd,
txns.gas_used as gas_used,
txns.gas_limit as gas_limit,
txns.gas_used / txns.gas_limit * 100 as gas_usage_percent,
CASE
WHEN txns.gas_limit = 0 THEN NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do 0 on these instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're null on the spark version right now so I wanted to keep that. We're dividing by 0, so it's not technically 0

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that makes sense

@couralex6
Copy link
Contributor Author

Row counts match and the translation for num_nonzero_bytes in gas_optimism_fees returns the same results

@couralex6 couralex6 merged commit b057ee3 into main Sep 28, 2023
@couralex6 couralex6 deleted the gas_migration branch September 28, 2023 17:33
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2023
@couralex6 couralex6 changed the title [WIP] SPE-118 Gas migration SPE-118 Gas migration Sep 29, 2023
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