Bubble up returndata from reverted Create2 deployments
#5052
Conversation
|
Thank you @dimitriospapathanasiou for this PR. I like the idea ... but I'm not sure I like the current implementation. |
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
ernestognw
changed the title
returndata from reverted Create2 deployments
There was a problem hiding this comment.
LGTM. The CI should be fine after we merge #5055.
I shared some thoughts in the original issue. Although I'm approving, it may make sense to reconsider.
| returndatacopy(0, 0, returndatasize()) | ||
| revert(0, returndatasize()) |
There was a problem hiding this comment.
This snippet is explicitly mentioned in Solidity documentation as something that is not memory-safe.
https://docs.soliditylang.org/en/latest/assembly.html#memory-safety
cc @Amxx @ernestognw
There was a problem hiding this comment.
Been thinking about the consequences of this, but does it even matter given it's reverting? It's "memory unsafe" because it may overwrite memory but there's no way to do something unsafe afterwards.
There was a problem hiding this comment.
Yeah the docs say it matters if the assembly block reverts and this exact snippet is used to illustrate the point.
There was a problem hiding this comment.
Yeah, it's kinda unclear what's the relationship between being "mainly about the optimizer" and still needing to ensure memory safetiness
There was a problem hiding this comment.
Yeah I should look into this for my next blog post to give you a reason to read it. 😁
Fixes #5046
I am uploading this PR to propose a solution for issue #5046. The current implementation simply reverts with a generic FailedDeployment error, which obscures potentially useful debugging information contained in the returndata.
So in the changes, deploy function was enhanced in the following way:
The changes were tested locally, and the expected output was given.
npx changeset add)