Peer Review - 250126

[jimmychu0807]

Questions/comments from @JohnGuilding:

1. Why did you opt to build an executor and a validator? As opposed to keeping all logic in the validator?

2. When running SIMULATE=true forge test --ffi, I get two failures:

   Failing tests:
   Encountered 2 failing tests in test/integration/IntegrationTest.t.sol:IntegrationTest
   [FAIL: revert: UserOperation simulation failed] test_BalanceTransferSingleMemberInitiateTx() (gas: 1789088023)
   [FAIL: revert: UserOperation simulation failed] test_TxCallMultiMembersInitiateTxSignTxExecuteTx() (gas: 1789747216)
   

   SIMULATE=true tests against 4337 validation rules (not all of them last I checked but this may have changed) https://github.com/rhinestonewtf/modulekit?tab=readme-ov-file#testing-modules

   For completion of the project, we should make sure that the validator is indeed 4337 compatible and do a full e2e test

3. For the following comment:

   //TODO: what is a good way to delete entries associated with `acctTxCount[account]`,
   //   The following line will make the compiler fail.
   // delete acctTxCount[account];
   

   You could use a mechanism similar to an enumerable map, where you can loop over all the txHashs associated with an account https://docs.openzeppelin.com/contracts/5.x/api/utils#EnumerableMap. Since Solidity doesn't have generics and you are using a custom struct, you could explore a custom enumerable map too https://github.com/zkemail/email-recovery/blob/main/src/libraries/EnumerableGuardianMap.sol

[JohnGuilding]

Comment from @jimmychu0807 taken from discord:

For #1, I originally have everything in a validator, until the point when I work on executeTx(). Only an IERC7579Executor can call the following without a revert, a validator will fail.

returnData = IERC7579Account(account).executeFromExecutor(
 ModeLib.encodeSimpleSingle(), abi.encodePacked(ecc.targetAddr, ecc.value, ecc.callData)
)[0];

And using call directly without executeFromExecutor will make the msg.sender in the called contract to be the Executor address instead of the smart account address.

Understood regarding revert. Why do we need executeTx in the first place?

[jimmychu0807]

Understood regarding revert. Why do we need executeTx in the first place?

The validator currently is designed to block all other calls other than Executor contract and the initiateTx(), signTx(), and executeTx().

So the validator enforce all transactions from the smart acct has to go through the initiateTx() -> signTx() -> executeTx() path, with executeTx() being the last step to execute the tx on behalf of the smart acct.

[JohnGuilding]

@jimmychu0807 I'm still a little confused on why the executor is concretely needed. Why do we need to enforce that txs should pass through the "initiateTx() -> signTx() -> executeTx()" flow

Why not only build a validator? E.g. If I was building a new passkey validator or zkemail validator, I would not build an executor with it. Building things this was would encapsulate the validation logic in one place and would allow you to remove LOC in the form of the executor

[jimmychu0807]

@JohnGuilding I see. I have in mind the smart acct installing these two modules want a M-of-N multi-sig behavior. So I restrict the smart acct to go thru the "initiateTx() -> signTx() -> executeTx()" flow. Otherwise, the smart acct (owner) can bypass the signature phase and issue any transactions directly.