feat(ctb): Account for worst-case CALL gas in SafeCall
#5470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
✅ Deploy Preview for opstack-docs canceled.
|
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
b5c2a44
to
2201cee
Compare
maurelian
reviewed
Apr 17, 2023
packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
tynes
reviewed
Apr 17, 2023
tynes
reviewed
Apr 17, 2023
tynes
added a commit
that referenced
this pull request
Apr 17, 2023
Clean up the script that is used to migrate legacy withdrawals. This script is very important for testing the legacy withdrawal migration. A bug was introduced with #4911 which was a sherlock audit fix. This was a bug because the change resulted in the withdrawal hashes changing, meaning that the storage slots computed client side also changed. This resulted in breaking the script. This commit reverts this change. The changes landing in #5470 will cause the buffer in the gas limit for the migrated withdrawals to be too small, so we can reintroduce this code behind a switch with the network. Its not ideal that the migration code isn't going to match 1:1 with goerli but thats ok because we will be able to thoroughly test it.
|
Fixes CLI-3832 |
|
Hey @clabby! This PR has merge conflicts. Please fix them before continuing review. |
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
2201cee
to
ec0adb7
Compare
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
ec0adb7
to
0dfefff
Compare
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
0dfefff
to
bc124c4
Compare
|
Semgrep found 2
Unchecked type assertion. Created by unchecked-type-assertion. |
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
bc124c4
to
5b8ab37
Compare
maurelian
reviewed
Apr 19, 2023
packages/contracts-bedrock/contracts/L2/L2CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
38bc389
to
cfb60fb
Compare
maurelian
reviewed
Apr 19, 2023
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
cfb60fb
to
f7e83ad
Compare
refcell
requested changes
Apr 19, 2023
packages/contracts-bedrock/contracts/L2/L2CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/contracts/L1/L1CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
refcell
approved these changes
Apr 20, 2023
maurelian
approved these changes
Apr 21, 2023
|
Waiting for @tynes' approval to remove the |
|
Do you mind squashing this PR in a commit or a few commits? |
tynes
reviewed
Apr 24, 2023
tynes
reviewed
Apr 24, 2023
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
2 times, most recently
from
c136b42
to
48ef885
Compare
tynes
approved these changes
Apr 24, 2023
clabby
force-pushed
the
clabby/ctb/call-with-min-gas-fix
branch
from
48ef885
to
1c61224
Compare
|
This PR has been added to the merge queue, and will be merged soon. |
|
This PR is next in line to be merged, and will be merged as soon as checks pass. |
seolaoh
added a commit
to kroma-network/kroma
that referenced
this pull request
Aug 9, 2023
…Gas` Adjusts `SafeCall.callWithMinGas` to account for the worst-case cost of 3/5 of the `CALL` opcode's dynamic gas factors: - `address_access_cost` - `positive_value_cost` - `value_to_empty_account_cost` As for `memory_expansion_cost` & `code_execution_cost`, these are expected to be covered by the `minGasLimit` itself. Also removes the division from `hasMinGas` for extra precision. See: - ethereum-optimism/optimism#5470 - ethereum-optimism/optimism#5540
nitaliano
pushed a commit
that referenced
this pull request
May 20, 2024
Clean up the script that is used to migrate legacy withdrawals. This script is very important for testing the legacy withdrawal migration. A bug was introduced with #4911 which was a sherlock audit fix. This was a bug because the change resulted in the withdrawal hashes changing, meaning that the storage slots computed client side also changed. This resulted in breaking the script. This commit reverts this change. The changes landing in #5470 will cause the buffer in the gas limit for the migrated withdrawals to be too small, so we can reintroduce this code behind a switch with the network. Its not ideal that the migration code isn't going to match 1:1 with goerli but thats ok because we will be able to thoroughly test it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Adjusts
SafeCall.callWithMinGasto account for the worst-case cost of 3/5 of theCALLopcode's dynamic gas factors:address_access_costpositive_value_costvalue_to_empty_account_costAs for
memory_expansion_cost&code_execution_cost, these are expected to be covered by theminGasLimititself.