Fix signature issue for ecdsa plugin #103
Conversation
| @@ -60,31 +64,32 @@ contract SafeECDSAPlugin is BaseAccount { | |||
| ); | |||
| } | |||
|
|
|||
| function enableMyself() public { | |||
| function enableMyself(address ownerKey) public { | |||
There was a problem hiding this comment.
Since the address storage is now a mapping, it's no longer ideal to set the signing address in the constructor. So I'm setting the key here instead. This should also enable multiple Safe's to use this module as a validator for userops.
|
|
||
| function exposed_validateNonce(uint256 nonce) external view { | ||
| function exposedValidateNonce(uint256 nonce) external pure { |
There was a problem hiding this comment.
My linter was complaining about this function name not being camelcase. I can change if back if we don't want to keep that rule.
There was a problem hiding this comment.
I'm not too opinionated on which naming convention we use for tests. I Originally started following the naming convention from the foundry docs for the exposed functions.
Think it would be good to align on the same naming convention throughout, but again, not bothered what we chose.
I also wonder if it's worth configuring linting rules globally, as opposed to just doing it at the top of the file for each like /* solhint-disable func-name-mixedcase */.
There was a problem hiding this comment.
Nice, looks good to me! Just wanted to check one thing before approving :)
I tried running the integration tests locally and got this error: Error: nonce has already been used.
That was without the --unsafe flag, I also tried with the --unsafe flag and they passed. Just wanted to check that was the expected behaviour from your end too? And if fixing the nonce error was a separate issue?
|
|
||
| function exposed_validateNonce(uint256 nonce) external view { | ||
| function exposedValidateNonce(uint256 nonce) external pure { |
There was a problem hiding this comment.
I'm not too opinionated on which naming convention we use for tests. I Originally started following the naming convention from the foundry docs for the exposed functions.
Think it would be good to align on the same naming convention throughout, but again, not bothered what we chose.
I also wonder if it's worth configuring linting rules globally, as opposed to just doing it at the top of the file for each like /* solhint-disable func-name-mixedcase */.
| @@ -19,21 +19,25 @@ interface ISafe { | |||
| ) external returns (bool success); | |||
| } | |||
|
|
|||
| struct ECDSAOwnerStorage { | |||
There was a problem hiding this comment.
I understand using the mapping means we don't break the 4337 storage rule. Out of interest, is the struct needed as well to stop us breaking the rules? Or is that just in case we want to add to the struct later?
There was a problem hiding this comment.
I also copied this pattern from Kernel since they support recovery as well. I don't think it's needed to be a struct. I do think it's a little more readable than just making the mapping mapping(address => address)
There was a problem hiding this comment.
| emit OWNER_UPDATED(_owner, newOwner); | ||
| _owner = newOwner; | ||
| function enable(bytes calldata _data) external payable { | ||
| address newOwner = address(bytes20(_data[0:20])); |
There was a problem hiding this comment.
Out of interest, what's the purpose of logic like address(bytes20(_data[0:20]));, as opposed to how the previous updateOwner function worked
There was a problem hiding this comment.
I think it is more gas-efficient to use calldata here. That way the parameter isn't stored in memory. I copied this pattern from the Kernel wallet
@jzaki are you able to confirm if this is true?
I ran again and still getting the same issue. Since your tests are always passing locally and mine aren't I'm going to assume it is an issue with my local setup and approve. Will look into my setup next week and try figure it out 🚀 |
Changing the address storage on the SafeECDSAPlugin to be a mapping. This fixes all of our current tests. I need to do further testing but I think this should make a single SafeECDSAPlugin compatible with multiple safes.
#104