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

fix: optimism gas refunds #989

Merged
merged 4 commits into from
Jan 20, 2024
Merged

Conversation

Wollac
Copy link
Contributor

@Wollac Wollac commented Jan 18, 2024

Prior to the Regolith update, deposit transactions should not receive gas refunds as ETH. For all other cases, non-deposit or post-regolith gas refunds are handled as in Mainnet.

@Wollac
Copy link
Contributor Author

Wollac commented Jan 18, 2024

I am a bit confused as to why test_consume_gas_with_refund fails. This should now match exactly the version before the API changes:

let is_gas_refund_disabled = env.cfg.optimism && is_deposit && !SPEC::enabled(REGOLITH);
if is_gas_refund_disabled {
0
} else {
mainnet::calculate_gas_refund::<SPEC>(env, gas)
}

And my full OP block execution tests with this PR pass while I was getting receipt mismatches before.

@gakonst
Copy link
Collaborator

gakonst commented Jan 18, 2024

cc @clabby

@Wollac
Copy link
Contributor Author

Wollac commented Jan 19, 2024

Actually, the test was wrong.
With #888, the optimism tests were simply copied over, but before that, handle_call_return did not include the refund calculation, which happened in calculate_gas_refund. Now both happen in first_frame_return and the tests need to be adapted.

To make this a bit clearer, I also renamed handle_call_return to first_frame_return, which is the actual name in the handler.

@rakita
Copy link
Member

rakita commented Jan 19, 2024

Makes sense! would want to check it up as I messed it up, but it will be merged today. Thank you!

@rakita rakita merged commit 6cd0bfc into bluealloy:main Jan 20, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Jan 20, 2024
@Wollac Wollac deleted the fix/optimism-gas-refund branch January 20, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants