Low level call and memory management

[Amxx]

Fixes #5013

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9ff7d4d

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

[Amxx]

@ernestognw Can you remember why a try catch was not good enough here ? Is it related to truncating the result (and ignoring high bits from the returned value?)

[ernestognw]

Isn't it because the return value may revert at decoding? A try IERC20Metadata(address(asset_)).decimals() returns (uint8 value) would've failed if the token returned an uint256. Nothing in the ERC20 restricts the return value to uint8

[Amxx]

I tried using a library like this one https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/utils/Exec.sol. The result did not really look good to me. I found the use of assembly actually cleaner ... because we automatically copy X bytes of the returndata, without having to call an additional returndatacopy.

I found that the most valuable thing from the 4337 draft PR was the free memory management, to dealocate memory.

[Amxx]

Should this revert if delay is too large ?

Suggested change

	delay := and(mload(0x20), 0xFFFFFFFF)
	delay := mload(0x20)
	if gt(delay, 0xFFFFFFFF) { revert(0,0) }

[ernestognw]

abi.decode reverts, so I guess we should revert too to keep backwards compatibility

[ernestognw]

Left some comments, though I've been thinking about the set of "low level" functions we can offer, and I feel there are essentially 3 tricks people would do with this library:

• Calling and ignoring the return data
• Caling and saving the returndata in scratch space
• Calling and saving the returndata on top of another another memory pointer

So, I feel it'd make sense to offer a library with the following functions:

EDIT: Made a PoC at #5094

library LowLevelCall {
  function callRaw(address target, uint256 value, bytes memory data) internal returns (bool success) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0, 0)
        }
    }

    function callReturnBytes32(
        address target,
        uint256 value,
        bytes memory data
    ) internal returns (bool success, bytes32 result) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0, 0x20)
            result := mload(0)
        }
    }

    function callReturnBytes32Tuple(
        address target,
        uint256 value,
        bytes memory data
    ) internal returns (bool success, bytes32 result1, bytes32 result2) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0, 0x40)
            result1 := mload(0)
            result2 := mload(0x20)
        }
    }

    function callReturnOverride(
        address target,
        uint256 value,
        bytes memory data,
        bytes memory resultPtr
    ) internal returns (bool success) {
        // WARNING: Memory safetiness depends on the caller to ensure that writing to `resultPtr` is safe
        assembly ("memory-safe") {
            let maxSize := mload(resultPtr)
            success := call(gas(), target, value, add(data, 0x20), mload(data), resultPtr, maxSize)
        }
    }
}

The changes for this PR should be doable with these variants so far, I'll iterate this.

[ernestognw]

I think this should be just Pointer, it doesn't have anything to do with the free memory pointer strictly talking (e.g. the library may add functions for dealing with pointers)

Suggested change

	type FreePtr is bytes32;
	type Pointer is bytes32;

[ernestognw]

Similarly, I think this should be very explicit, otherwise reading Memory.save() doesn't tell much.

Suggested change

	function save() internal pure returns (FreePtr ptr) {
	function saveFreePointer() internal pure returns (Pointer ptr) {

[ernestognw]

Accordingly

Suggested change

	function load(FreePtr ptr) internal pure {
	function loadFreePointer(Pointer ptr) internal pure {