fix(ctb): finalizeWithdrawalTransaction gas limit dos (v2)

[clabby]

Overview

V2 of #4954, see that PR for the full explanation of the changes. This one implements @smartcontracts' proposed method of breaking out the callWithMinGas function into SafeCall and testing it independently, which is a good bit cleaner.

Will be followed up with a PR to use the new callWithMinGas function in the CrossDomainMessenger.

Metadata
Fixes CLI-3387
Fixes CLI-3386

[changeset-bot]

⚠️ No Changeset found

Latest commit: aabf6fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[codecov]

Codecov Report

Merging #5017 (aabf6fc) into develop (83b6312) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5017      +/-   ##
===========================================
+ Coverage    40.98%   41.00%   +0.01%     
===========================================
  Files          339      339              
  Lines        20787    20790       +3     
  Branches       771      771              
===========================================
+ Hits          8520     8524       +4     
- Misses       11609    11611       +2     
+ Partials       658      655       -3     

Flag	Coverage Δ
bedrock-go-tests	36.66% <ø> (+0.02%)	⬆️
contracts-bedrock-tests	49.74% <100.00%> (-0.20%)	⬇️
contracts-tests	98.86% <ø> (ø)
core-utils-tests	60.41% <ø> (ø)
dtl-tests	47.15% <ø> (ø)
fault-detector-tests	33.88% <ø> (ø)
sdk-tests	38.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../contracts-bedrock/contracts/L1/OptimismPortal.sol	86.95% <100.00%> (-0.28%)	⬇️
...contracts-bedrock/contracts/libraries/SafeCall.sol	57.14% <100.00%> (-42.86%)	⬇️
op-node/heartbeat/service.go	57.89% <0.00%> (+2.63%)	⬆️
op-node/sources/batching.go	85.71% <0.00%> (+3.06%)	⬆️

[clabby]

Moving back into draft- docs could use some rewording + tests should preferably account for fluctuating dynamic gas in the external call.

[mergify]

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

[clabby]

Current dependencies on/for this PR:

• develop
  • PR fix(ctb): finalizeWithdrawalTransaction gas limit dos (v2) #5017 👈

This comment was auto-generated by Graphite.

[maurelian]

Applied the do-not-merge label because I'd like this to get 2 code owner reviews.

[clabby]

Applied the do-not-merge label because I'd like this to get 2 code owner reviews.

Agree- Will poke Kelvin or Mark about this one if I come across them today 😄

[tynes]

This is definitely the best solution yet

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.