Skip to content

Conversation

@adamegyed
Copy link
Contributor

Motivation

In a few locations in code, we use catch expressions of the form catch (bytes memory revertReason). This is error-prone, as it does not catch cases where the revert data is a Panic or an error of type Error(string).

Context:

h/t @howydev for leaving a reminder for this in a todo comment.

Solution

Convert all catch (bytes memory ...) { ... } expressions to just catch { ... }.

Implement a helper function collectReturnData() that collects the contents of the return data buffer into a bytes memory type, using assembly to call returndatacopy.

Use collectReturnData() in all catch bodies that need access to the revert data.

Rationale

While we generally avoid inline assembly in the reference implementation, this is an unfortunate limitation of solidity itself, so it seems acceptable. Additionally, similar work is done in the ERC-4337 EntryPoint.

This approach is preferred over using multiple catch clauses due to that being more verbose, increasing codesize, and still being error-prone from abi-decoding issues.

@adamegyed adamegyed requested a review from a team August 22, 2024 00:22
@adamegyed adamegyed merged commit 0c18f92 into develop Aug 22, 2024
@adamegyed adamegyed deleted the adam/fix-catch branch August 22, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants