Kernel code review #1

[derekchiang]

• Reference to "threshold" is outdated

https://github.com/zerodevapp/zerodev-wallet-kernel/blob/acbc843ffbafa2eaf11bc6e93ed5e5d0438a24bd/src/Kernel.sol#L17

• I know the new entrypoint is not released yet, but would we want to validate & increment nonce here if it was sent directly by the owner?

https://github.com/zerodevapp/zerodev-wallet-kernel/blob/acbc843ffbafa2eaf11bc6e93ed5e5d0438a24bd/src/Kernel.sol#L66

As I typed out this comment, I realize that that would require us to pass in a nonce in this function, which is annoying.

• I wonder if it makes sense to check two signatures, once with toEthSignedMessageHash and one without. The reason is that some signers use the \x19Ethereum Signed Message:\n prefix and some don't, and we want to work with both kinds of signers. Does that make sense?

https://github.com/zerodevapp/zerodev-wallet-kernel/blob/acbc843ffbafa2eaf11bc6e93ed5e5d0438a24bd/src/Kernel.sol#L137

https://github.com/zerodevapp/zerodev-wallet-kernel/blob/acbc843ffbafa2eaf11bc6e93ed5e5d0438a24bd/src/Kernel.sol#L180

• Right now the AccountFactory is tied to MinimalAccount. Would it make sense to create an abstract contract with the initialize method, and have the AccountFactory tie to that instead? That way we can use the AccountFactory with the PluginAccount too.

[leekt]

I know the new entrypoint is not released yet, but would we want to validate & increment nonce here if it was sent directly by the owner?

I don't think we need to do that here since it does not really need nonce for execution and nonce is for validation logic. I wonder how it's going to work out but let's just stick with what we have now

I wonder if it makes sense to check two signatures, once with toEthSignedMessageHash and one without. The reason is that some signers use the \x19Ethereum Signed Message:\n prefix and some don't, and we want to work with both kinds of signers. Does that make sense?

makes sense, i'll check both hash for isValidSignature

Right now the AccountFactory is tied to MinimalAccount. Would it make sense to create an abstract contract with the initialize method, and have the AccountFactory tie to that instead? That way we can use the AccountFactory with the PluginAccount too.

If "abstract contract" means literally abstract contract on solidity, no it can't be done. we need to deploy any kind of non-abstract contract and that's why i'm using minimal account for that.