feat: hash orderinfo separately #135
Conversation
Avoids stack too deep without bricking EIP712 and cleaner
src/lib/DutchLimitOrderLib.sol
Outdated
| "uint256 startTime,", | ||
| "uint256 endTime,", | ||
| "address inputToken,", | ||
| "uint256 inputStartAmount,", | ||
| "uint256 inputEndAmount,", | ||
| "DutchOutput[] outputs)", | ||
| DUTCH_OUTPUT_TYPE | ||
| DUTCH_OUTPUT_TYPE, | ||
| OrderInfoLib.ORDER_INFO_TYPE |
There was a problem hiding this comment.
Is Dutch before OrderInfo because of alphabetical?
|
|
||
| bytes internal constant ORDER_TYPE = abi.encodePacked( | ||
| bytes internal constant EXCLUSIVE_DUTCH_LIMIT_ORDER_TYPE = abi.encodePacked( |
There was a problem hiding this comment.
Nit: inconsistency that sometimes you defined a TYPE constant just using a singular string, and sometimes you use encodePacked - even with this one that doesnt use any variables.
For readability you could use a multi-line encodePacked everywhere? Just a nit though, feel free to ignore
There was a problem hiding this comment.
yea my general approach has been: if small, single line else multiline
There was a problem hiding this comment.
but will switch around for consistency
There was a problem hiding this comment.
actually ehhh it looks pretty ugly for single liners
bytes internal constant TOKEN_PERMISSIONS_TYPE =
abi.encodePacked("TokenPermissions(", "address token", "uint256 amount)");
There was a problem hiding this comment.
mm yeah i think single line fine to just keep w/o abi.encodePacked
src/lib/DutchLimitOrderLib.sol
Outdated
| @@ -42,25 +43,23 @@ struct DutchLimitOrder { | |||
|
|
|||
| /// @notice helpers for handling dutch limit order objects | |||
| library DutchLimitOrderLib { | |||
| using OrderInfoLib for OrderInfo; | |||
|
|
|||
| bytes internal constant DUTCH_OUTPUT_TYPE = | |||
| "DutchOutput(address token,uint256 startAmount,uint256 endAmount,address recipient,bool isFeeOutput)"; | |||
| bytes32 internal constant DUTCH_OUTPUT_TYPE_HASH = keccak256(DUTCH_OUTPUT_TYPE); | |||
|
|
|||
| bytes internal constant ORDER_TYPE = abi.encodePacked( | |||
There was a problem hiding this comment.
Nit: Could be neat to separate this out like you do with EXCLUSIVE_DUTCH_LIMIT_ORDER_TYPE?
There was a problem hiding this comment.
strangely this changes gas even though its a constant?
| ); | ||
|
|
||
| bytes internal constant ORDER_TYPE = | ||
| abi.encodePacked(DUTCH_LIMIT_ORDER_TYPE, DUTCH_OUTPUT_TYPE, OrderInfoLib.ORDER_INFO_TYPE); |
There was a problem hiding this comment.
Can we leave a comment about it being important that we encode the types in alphabetical order.. feel like thats a nuanced bit in the 712 standard for nested types.
|
|
||
| bytes internal constant ORDER_TYPE = abi.encodePacked( | ||
| bytes internal constant EXCLUSIVE_DUTCH_LIMIT_ORDER_TYPE = abi.encodePacked( |
There was a problem hiding this comment.
mm yeah i think single line fine to just keep w/o abi.encodePacked
how do you do that in foundry? without foundry-rs/foundry#4818 |
Yeah I think can just make it a future todo, but we have some static/hardcoded tests in permit2 comparing signatures generated from mm. |
* feat: hash orderinfo separately Avoids stack too deep without bricking EIP712 and cleaner * fix: struct ordering * fix: alice comments * feat: add comment
Avoids stack too deep without bricking EIP712 and cleaner