feat: revert to previous 1271 implementation #55
Conversation
| /// @dev bytes4(keccak256("isValidSignature(bytes32,bytes)")) | ||
| bytes4 internal constant _1271_MAGIC_VALUE_SUCCESS = 0x1626ba7e; | ||
| bytes4 internal constant _1271_MAGIC_VALUE_FAILURE = 0xffffffff; | ||
| bytes32 internal constant _MESSAGE_TYPEHASH = keccak256("LightAccountMessage(bytes message)"); |
There was a problem hiding this comment.
(No change needed) Do you know what the rationale was for the field being a bytes instead of a bytes32? I might have something a little hazy here, but I believe this field is always containing the hash that's being asked to be signed, right?
There was a problem hiding this comment.
Thanks for the review!
I was thinking I might want to change that too, but left it as is. This is also what MultiOwnerPlugin has.
I believe the original reason was because that's what Safe does. I'm happy to change it if it won't cause SDK headaches.
Cc @howydev
There was a problem hiding this comment.
Thanks for the explanation. I think it's fine to stick to what Safe does, I was just curious.
There was a problem hiding this comment.
Yep, we inherited that conversion of bytes32 digest -> encoded into bytes -> hashed back to a bytes32 from Safe. It added a little overhead that's probably unnecessary, but I think the SDK already accounts for this. 100% should address this in the next iteration though
Given that there are some issues to address for the 1271 flow (context: https://x.com/howydev/status/1780353754333634738), reverting back to the previous version's implementation of this until we reach consensus on a good approach.
Choosing to keep the
SignatureTypeprefix which improves gas estimation.