Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: catch data in all cases #151

Merged
merged 1 commit into from
Aug 22, 2024
Merged

fix: catch data in all cases #151

merged 1 commit into from
Aug 22, 2024

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
3 checks passed
@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.

2 participants