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

feat(L2toL2CDM): revert on target call failure in relayMessage #12526

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

tremarkley
Copy link
Contributor

@tremarkley tremarkley commented Oct 18, 2024

Closes Issue #143

Summary:

This PR updates the L2ToL2CrossDomainMessenger#relayMessage to revert when the target contract call fails.

Problem:

Previously, when the target contract call within relayMessage failed, it would only emit a FailedRelayedMessage. This caused gas estimation tools to produce unreliable estimates. Specifically, gas estimators would return the minimum amount of gas required for relayMessage itself to succeed, without accounting for the actual gas needed by the target contract call.

Solution:

By introducing a revert when the target call fails, we ensure that gas estimation tools are forced to include the gas required for the target call to succeed. This leads to more accurate and consistent gas estimates across different tools and scenarios, particularly when interacting with complex contracts where gas usage can vary significantly.

Impact:

  • More accurate gas estimation: Gas tools will now estimate based on the real cost of the full transaction, including the target call.
  • Improved developer experience: Reduces the likelihood of transactions failing due to underestimated gas limits.

@tremarkley
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tremarkley and the rest of your teammates on Graphite Graphite

@tremarkley tremarkley force-pushed the harry/improve_l2tol2cdm_gas_estimation branch from 19855aa to 64e261a Compare October 18, 2024 19:36
Copy link
Contributor

semgrep-app bot commented Oct 18, 2024

Semgrep found 1 golang_fmt_errorf_no_params finding:

  • op-deployer/pkg/deployer/bootstrap/bootstrap.go

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

@tremarkley tremarkley force-pushed the harry/improve_l2tol2cdm_gas_estimation branch from 64e261a to 6baaa61 Compare October 18, 2024 19:47
@tremarkley tremarkley marked this pull request as ready for review October 18, 2024 19:57
@tremarkley tremarkley changed the title feat(L2toL2CDM): improve gas estimation feat(L2toL2CDM): Fix Gas Estimation Accuracy by Reverting on Target Call Failure in L2ToL2CrossDomainMessenger#relayMessage Oct 18, 2024
@tremarkley tremarkley changed the title feat(L2toL2CDM): Fix Gas Estimation Accuracy by Reverting on Target Call Failure in L2ToL2CrossDomainMessenger#relayMessage feat(L2toL2CDM): Fix gas estimation accuracy by reverting on target call failure in L2ToL2CrossDomainMessenger#relayMessage Oct 18, 2024
@tremarkley tremarkley changed the title feat(L2toL2CDM): Fix gas estimation accuracy by reverting on target call failure in L2ToL2CrossDomainMessenger#relayMessage feat(L2toL2CDM): improve gas estimation by reverting on target call failure in relayMessage Oct 18, 2024
@tremarkley tremarkley requested a review from mds1 October 18, 2024 21:52
@tremarkley tremarkley force-pushed the harry/improve_l2tol2cdm_gas_estimation branch 4 times, most recently from fe96cba to 24aa0e4 Compare October 18, 2024 22:13
@tremarkley tremarkley changed the title feat(L2toL2CDM): improve gas estimation by reverting on target call failure in relayMessage feat(L2toL2CDM): revert on target call failure in relayMessage Oct 18, 2024
@tremarkley tremarkley force-pushed the harry/improve_l2tol2cdm_gas_estimation branch from 24aa0e4 to 87b9e70 Compare October 19, 2024 20:27
@tremarkley tremarkley requested a review from tynes October 19, 2024 20:27
@tremarkley tremarkley enabled auto-merge October 19, 2024 20:35
@tremarkley tremarkley added this pull request to the merge queue Oct 21, 2024
@tynes
Copy link
Contributor

tynes commented Oct 21, 2024

This may impact the rollback feature, if its problematic then we can figure out how to make the rollback feature work well on top of this given the gas estimation issues using the old scheme

Merged via the queue into develop with commit 76c1e1d Oct 21, 2024
49 checks passed
@tremarkley tremarkley deleted the harry/improve_l2tol2cdm_gas_estimation branch October 21, 2024 04:39
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
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.

2 participants