Update ERC-7751 revert bubbling #889
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Forge code coverage:
|
marktoda
reviewed
Oct 18, 2024
src/libraries/CustomRevert.sol
Outdated
| /// @dev this function should only be used with custom errors of the type `CustomError(address target, bytes revertReason)` | ||
| function bubbleUpAndRevertWith(bytes4 selector, address addr) internal pure { | ||
| /// @notice bubble up the revert message returned by a call and revert with a wrapped ERC-7751 error | ||
| function bubbleUpAndRevertWith(address target, bytes4 functionSelector, bytes4 additionalContext) internal pure { |
There was a problem hiding this comment.
consider renaming these to be a bit more clear, maybe:
address revertingContract
address revertingFunctionSelector
bytes4 additionalContext
There was a problem hiding this comment.
also -- why is additionalContext only bytes4 here but bytes in the custom error? seems context could be a custom error w/ params so should we support that here too?
There was a problem hiding this comment.
I guess keeping it bytes4 makes the encoding more simple if we specifically dont plan on having params here
There was a problem hiding this comment.
any additional context we had in the reverts before was the target address (hook, erc20 token, recipient) based on the action, so we only needed 4bytes for the selector
marktoda
approved these changes
Oct 18, 2024
marktoda
previously approved these changes
Oct 18, 2024
hensha256
reviewed
Oct 28, 2024
0age
previously approved these changes
Oct 28, 2024
0age
approved these changes
Oct 29, 2024
hensha256
approved these changes
Oct 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.