Implement LowLevelCall library #5094
Conversation
🦋 Changeset detectedLatest commit: 6a74179 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Some comments:
-
I don't think all the memory library is necessary here (for example the loadByte function). Agreeing on the low level call part should be our priority, and IMO we should not include additional things that are independant.
-
We should thing of the "low level call" functions as an extension of the Address library, in terms of features AND naming. Ideally, in 6.0 I would move them all in the same file (IMO they are different ways, which different security guarantees of doing the same thing)
-
Part of what makes low level call efficient is when you don't use abi.encode to prepare the call. This library doesn't support that. IMO its fine for now, but we should consider adding that later.
There was a problem hiding this comment.
Is there any case where the bubble is something other that the return data that we received from a call that was not successfull ?
I think that by definition of "bubble", it implies that its already an error that was reverted by an unsuccessfull call. Therefore, I'm would argue maybe the "low level call" function should include a boolean to select is yes/no we want to bubble any revert we get.
It could look like
function callEmpty(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success) {
assembly ("memory-safe") {
success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x00)
if and(iszero(success), bubble) {
let ptr := mload(0x40)
returndatacopy(ptr, 0, returndatasize())
revert(ptr, returndatasize())
}
}
}
function callScratch(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success, uint256 size, bytes32 x, bytes32 y) {
assembly ("memory-safe") {
success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x40)
if and(iszero(success), bubble) {
let ptr := mload(0x40)
returndatacopy(ptr, 0, returndatasize())
revert(ptr, returndatasize())
}
size := returndatasize()
x := mload(0x00)
y := mload(0x20)
}
}
function callFull(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success, bytes memory returndata) {
assembly ("memory-safe") {
success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x00)
returndata := mload(0x40)
returndatacopy(add(returndata, 0x20), 0, returndatasize())
if and(iszero(success), bubble) {
revert(add(returndata, 0x20), returndatasize())
}
mstore(returndata, returndatasize())
mstore(0x40, add(add(returndata, 0x20), returndatasize()))
}
}Note: The RETURNDATASIZE opcode is only 2 gas, and DUPX are 3 gas. So it turns out caching the returndatasize in the stack and dupping it is counterproductive !
EDIT: We decided to NOT do that bool bubble because the deicision to bubble or not might depend on the returndatasize we get.
|
I updated the PR following our discussion of the day. I'm curious what the gas comparaison script tells us. Note: the changes for SafeERC20 will be overwritten by the SafeERC20 PR. |
There was a problem hiding this comment.
I did a lot of changes following yesterday's discussion. Only thing I havent done yet is polishing the contracts/utils/LowLevelCall.sol natspecs.
@ernestognw Let me know what you think !
|
|
Current remarquable numbers:
|
| bool success = LowLevelCall.callNoReturn(target, value, data); | ||
| if (success && (LowLevelCall.returnDataSize() > 0 || target.code.length > 0)) { | ||
| return LowLevelCall.returnData(); | ||
| } else if (success) { | ||
| revert AddressEmptyCode(target); | ||
| } else if (LowLevelCall.returnDataSize() > 0) { | ||
| LowLevelCall.bubbleRevert(); | ||
| } else { | ||
| revert Errors.FailedCall(); | ||
| } |
There was a problem hiding this comment.
This saves a few more gas compared to your numbers here.
Same pattern in functionCallWithValue(..) & functionStaticCall(..) saves gas.
| bool success = LowLevelCall.callNoReturn(target, value, data); | |
| if (success && (LowLevelCall.returnDataSize() > 0 || target.code.length > 0)) { | |
| return LowLevelCall.returnData(); | |
| } else if (success) { | |
| revert AddressEmptyCode(target); | |
| } else if (LowLevelCall.returnDataSize() > 0) { | |
| LowLevelCall.bubbleRevert(); | |
| } else { | |
| revert Errors.FailedCall(); | |
| } | |
| if (LowLevelCall.callNoReturn(target, value, data)) { | |
| if (LowLevelCall.returnDataSize() > 0 || target.code.length > 0) { | |
| return LowLevelCall.returnData(); | |
| } | |
| revert AddressEmptyCode(target); | |
| } | |
| if (LowLevelCall.returnDataSize() > 0) { | |
| LowLevelCall.bubbleRevert(); | |
| } else { | |
| revert Errors.FailedCall(); | |
| } |
There was a problem hiding this comment.
I personally find it less readable. How must does it save ? I want cases do we save ?
Currently the "happy path" (the one that leads to return LowLevelCall.returnData();, and that we want to optimize), has a single if. Your proposal adds a second if to that path. I'm worried we might be optimising error costs at the expense of the happy path.
| @@ -76,26 +80,50 @@ library Address { | |||
| if (address(this).balance < value) { | |||
| revert Errors.InsufficientBalance(address(this).balance, value); | |||
| } | |||
| (bool success, bytes memory returndata) = target.call{value: value}(data); | |||
| return verifyCallResultFromTarget(target, success, returndata); | |||
There was a problem hiding this comment.
I think verifyCallResultFromTarget(..) is not used internally anymore (and not tested).
We can eventually in 5.x add unit tests for it (and deprecate usage until 6.0 removal if it is considered to be useless for developer, which am not sure about)
| @@ -76,26 +80,50 @@ library Address { | |||
| if (address(this).balance < value) { | |||
There was a problem hiding this comment.
How about using functionCallWithValue(..) instead of verifyCallResult(..) in Governor & Timelock?
There was a problem hiding this comment.
functionCallWithValue will revert with AddressEmptyCode if you try to do a call to an EOA. If we use it in Governor/Timelock, then they would not be able to send ETH to an EOA. We have a test for that.
Alternative to #5091
Fixes #5013
Requires #5189
PR Checklist
npx changeset add)