Conversation
willmeister
left a comment
There was a problem hiding this comment.
Left a few comments, but LGTM 👍
Might also suggest @karlfloersch to take a look, as this is right at the core of our stuff.
packages/contracts/contracts/optimistic-ethereum/ovm/ExecutionManager.sol
Show resolved
Hide resolved
|
|
||
| // Internal Logic | ||
| function addToOVMRefund(uint _refund) internal { | ||
| OVMRefund += _refund; |
There was a problem hiding this comment.
Do we need to require(MAX_UINT - OVMRefund >= _refund, "overflow");? Based on context, we may not care.
There was a problem hiding this comment.
I don't think it's possible for any reasonable value for the constants at the top of the file, simply due to the EVM's gas limit.
packages/contracts/contracts/optimistic-ethereum/ovm/StateManagerGasSanitizer.sol
Show resolved
Hide resolved
packages/contracts/contracts/optimistic-ethereum/ovm/StateManagerGasSanitizer.sol
Show resolved
Hide resolved
| // todo safemath negatives | ||
| GasConsumer gasConsumer = GasConsumer(resolveGasConsumer()); | ||
| uint gasAlreadyConsumed = initialGas - gasleft(); | ||
| uint gasLeftToConsume = _sanitizedGasCost - gasAlreadyConsumed; |
There was a problem hiding this comment.
Just want to make sure it's not possible for this to underflow.
There was a problem hiding this comment.
We already have to make an assumption about the max gas an SM implementation will consume, which is why the constants above are called *_GAS_COST_UPPER_BOUND. If that breaks, then we will have bigger issues than the underflow here, so I figured it's fine.
| uint _sanitizedGasCost, | ||
| uint _virtualGasCost | ||
| ) internal { | ||
| uint initialGas = gasleft(); |
There was a problem hiding this comment.
Should there be a require(_sanitizedGasCost >= _virtualGasCost, "Logic will underflow if sanitized gas cost is lower than virtual gas cost"); ?
There was a problem hiding this comment.
Since those are parameters/constants we must set ourselves, I think okay to omit such a check.
|
|
||
| // Overhead for checking methodId etc in this function before the actual call() | ||
| // This was figured out empirically during testing--adding methods or changing compiler settings will require recalibration. | ||
| uint constant constantOverheadEOA = 947; |
There was a problem hiding this comment.
Will this ever change through hardforks, and if so, how can we update it?
There was a problem hiding this comment.
It could, but it's a case where other critical components will break first. In this particular instance I believe the impact would be a constant size change in OVM gas consumed, but still deterministic.
| uint gasToAlloc = _amount - constantOverhead; | ||
| // Overhead for checking methodId, etc. in this function before the actual call() | ||
| // This was figured out empirically during testing--adding methods or changing compiler settings will require recalibration. | ||
| uint constant constantOverheadInternal = 2514; |
packages/contracts/contracts/optimistic-ethereum/utils/libraries/GasConsumer.sol
Show resolved
Hide resolved
packages/contracts/contracts/optimistic-ethereum/utils/libraries/GasConsumer.sol
Show resolved
Hide resolved
|
This PR didn't get in until some big upstream changes, closing in favor of #217 which built on top of this PR and then merged. |
### Description Documents the `op-alloy-consensus` crate in the mdbook.
Closes #212 --------- Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
Description
This PR implements a proxy contract which sits between the EM and SM. The proxy serves two purposes:
GASopcode in code contracts.Questions
Metadata
Fixes
Contributing Agreement