Make MinimalForwarder production-ready
#4065
Conversation
|
| keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); | ||
| bytes32 private constant _EXPIRING_FORWARD_REQUEST_TYPEHASH = | ||
| keccak256( | ||
| "ExpiringForwardRequest(uint48 deadline,address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)" |
There was a problem hiding this comment.
why put the deadline as the first argument? I would personnaly put it after the nonce.
There was a problem hiding this comment.
Made sense to me to pack them this way. It's not a big deal since it's calldata, but I thought it was better to put the uint48 next to an address
There was a problem hiding this comment.
I believe ABI encoding does not pack values. Every value takes up 32 bytes.
| constructor() EIP712("MinimalForwarder", "0.0.1") {} | ||
| constructor() EIP712(name(), version()) {} | ||
|
|
||
| function version() public view virtual returns (string memory) { |
There was a problem hiding this comment.
Why do we have this function ?
There was a problem hiding this comment.
Customizing EIP712 version, similar to Governor:
I can't see immediate reasons why someone would override it, but I feel it makes sense to leave the option open? What do you think?
There was a problem hiding this comment.
I can't neither think about a reason why someone would need it, but I'd say the same logic of why we set virtual in public functions should apply here.
"Why would someone override version and name?" is kinda the same question as "Why would someone override X function?" from my perspective.
No strong opinion if we remove it.
|
|
||
| function getNonce(address from) public view returns (uint256) { | ||
| function name() public view virtual returns (string memory) { |
There was a problem hiding this comment.
Why do we have this function ?
There was a problem hiding this comment.
I believe the reason was to customize the EIP712 name, but here I don't agree that it makes sense to make it customizable. It should be fine to make it always "MinimalForwarder".
There was a problem hiding this comment.
Same as #4065 (comment)
|
|
||
| mapping(address => uint256) private _nonces; | ||
|
|
||
| constructor() EIP712("MinimalForwarder", "0.0.1") {} | ||
| constructor() EIP712(name(), version()) {} |
There was a problem hiding this comment.
why change from constructor() EIP712("MinimalForwarder", "0.0.1") {} ?
There was a problem hiding this comment.
Replied in your comments about version() and name().
There was a problem hiding this comment.
Same as #4065 (comment)
| bool expired = block.timestamp >= req.deadline; | ||
| return | ||
| getNonce(req.from) == req.nonce && // Signed nonce corresponds with current nonce | ||
| signer == req.from && // From is signer | ||
| !expired; |
There was a problem hiding this comment.
| bool expired = block.timestamp >= req.deadline; | |
| return | |
| getNonce(req.from) == req.nonce && // Signed nonce corresponds with current nonce | |
| signer == req.from && // From is signer | |
| !expired; | |
| return | |
| getNonce(req.from) == req.nonce && // Signed nonce corresponds with current nonce | |
| signer == req.from && // From is signer | |
| block.timestamp < req.deadline; // Expired |
| @@ -69,4 +99,51 @@ contract MinimalForwarder is EIP712 { | |||
|
|
|||
| return (success, returndata); | |||
| } | |||
|
|
|||
| function _verify(ExpiringForwardRequest memory req, address signer) private view returns (bool) { | |||
There was a problem hiding this comment.
Note that we're missing require(msg.value == req.value).
ernestognw
added
the
on hold
| return _execute(req); | ||
| } | ||
|
|
||
| function _execute(ExpiringForwardRequest memory req) private returns (bool, bytes memory) { |
There was a problem hiding this comment.
Is the fact that _execute is not reverting when the call fails intended and left to the contract inheriting from MinimalForwarder?
If yes and given that this version will be considered as production ready, it would be nice to add a warning about this to avoid silent failure on the implementation side. Else, I think we should revert while bubbling up the error.
There was a problem hiding this comment.
Yes, its an intended behavior.
This PR is on hold. The code it contains should not be seen as a production-ready version of the forwarder. For sure we will want to challenge that design choice (IMO there are good reason for this behavior), and document it.
There was a problem hiding this comment.
➕ to @Amxx explanation.
I'll leave this open and I added a note at the top of the PR indicating this is currently on hold.
ernestognw
force-pushed
the
lib-643-production-ready-minimal-forwarder
branch
from
ebd901e to
5bee13c
Compare
ernestognw
force-pushed
the
lib-643-production-ready-minimal-forwarder
branch
from
5bee13c to
4c1cd22
Compare
Fixes #3884
Fixes #3664
Currently,
MinimalForwarderis not considered production-ready since it was designed mainly for testing purposes in the library. However, it's not that far from a MinimalForwarder that's something we may recommend.The new MinimalForwarder should guarantee:
PR Checklist
npx changeset add)