Make function calls atomic with ERC20 transfers in token supply source subnets

[cryptoAtwill]

Make the performCall atomic with respect to ERC20 call with value.

Close ENG-635

[linear]

ENG-635 Make `performCall` with ERC20 token atomic

[raulk]

This looks correct, but I'm starting to get uncomfortable with the complexity of performCall. Could we take this opportunity to add unit tests (not integration tests) for performCall?

[raulk]

Could you comment why the add opcode? There may be comments in the OZ library, but let's avoid reader indirection.

[cryptoAtwill]

Ok, I will add comments on the code.

[raulk]

This is the full function from OpenZeppelin. We need to handle the case where there is no return data. The add is needed to unpack the variable-size return value (first Ethereum word is the size).

function _revert(bytes memory returndata) private pure {
    // Look for revert reason and bubble it up if present
    if (returndata.length > 0) {
        // The easiest way to bubble the revert reason is using memory via assembly
        /// @solidity memory-safe-assembly
        assembly {
            let returndata_size := mload(returndata)
            revert(add(32, returndata), returndata_size)
        }
    } else {
        revert FailedInnerCall();
    }
}

[cryptoAtwill]

From my testing, if the ret is empty, calling revert(add(32, returndata), returndata_size) is the same as revert with no data. I think openzeppelin might be emphasising FailedInnerCall? But for us, if the contract returns empty, we can just return empty. Even if we dont, after some parsing of the error bytes, users will still get an empty ret.

[cryptoAtwill]

From my testing, if the ret is empty, calling revert(add(32, returndata), returndata_size) is the same as revert with no data. I think openzeppelin might be emphasising FailedInnerCall? But for us, if the contract returns empty, we can just return empty. Even if we dont, after some parsing of the error bytes, users will still get an empty ret.

[raulk]

@cryptoAtwill I suspect you're seeing this because mloading the return data pointer will return the size, which will be 0. You then revert providing an offset equal to the pointer+32, but with size 0. This is probably OK, but I'd rather play it safe and do the check anyway in high level Solidity.

[raulk]

This is the full function from OpenZeppelin. We need to handle the case where there is no return data. The add is needed to unpack the variable-size return value (first Ethereum word is the size).

function _revert(bytes memory returndata) private pure {
    // Look for revert reason and bubble it up if present
    if (returndata.length > 0) {
        // The easiest way to bubble the revert reason is using memory via assembly
        /// @solidity memory-safe-assembly
        assembly {
            let returndata_size := mload(returndata)
            revert(add(32, returndata), returndata_size)
        }
    } else {
        revert FailedInnerCall();
    }
}

[raulk]

Can we test performCall instead, please?

[cryptoAtwill]

added more tests

[raulk]

Let's remove the else and simply use revert(). If the target didn't provide an error, we should not add one into the mix; we're simply interested in passing through here, right?