feat: [v0.8-develop, experimental] split up interfaces by type [1/N]

[adamegyed]

Motivation

Currently, IPlugin implements both functions necessary for all plugins like pluginManifest(), and all functions a plugin could possibly define across the different plugin types. This means that a plugin implementing one type would need to provide stubs of functions to conform to the solidity interface, even if those functions go unused. It also makes it more expensive to introduce any new plugin functions, since all plugins would need to implement it.

Solution

Introduce new interfaces IValidation, IValidationHook, IExecutionHook to organize possible plugin functions.

Update BasePlugin, UpgradeableModularAccount, and tests for these new interfaces.

We also implicitly required plugins to support ERC-165, but did not actually include that in the IPlugin interface. This makes that requirement more obvious to readers.

[huaweigu]

Would naming the functions runtimeValidation and userOpValidation be more succinct and consistent?

[adamegyed]

Yeah, it would be. We could switch them to be in the verb form of validateUserOp and validateRuntime, but it would be an overload of the function name validateUserOp due to the different parameters compared to 4337's IAccount with validateUserOp. What do you think of validateUserOp and validateRuntime?

[huaweigu]

validateUserOp and validateRuntime look good to me. If we switch to verb forms, should we also update other functions for consistency?

[adamegyed]

Updated to validateUserOp and validateRuntime. Which other functions were you thinking of renaming?

[huaweigu]

For example, preUserOpValidationHook, but I can't think of a better name at the moment, haha.

[jaypaik]

On function overloading, I'd be against it if it were on the same contract, since that would cause some ABI parsing issues (at least with viem/hardhat when I tried last time). But these are on plugins so it could be okay. Still might be confusing in communication if they're the same name.

Just riffing, and don't know if we want to address all of these here, but thoughts on:

onInstall
onUninstall
onValidateUserOp
onValidateRuntime or onValidateRuntimeTx
onBeforeValidateUserOp
onBeforeValidateRuntime or onBeforeValidateRuntimeTx
onBeforeExecute
onAfterExecute

Convention follows HTML event handlers (e.g., onBeforeUnload, onUnload).

[adamegyed]

I feel like there's a difference in connotation between "callback" functions and "handler" functions - e.g. onInstall is something that fires on installation, but it doesn't check any return value other than success/failure. While, functions like validateUserOp do have a return value, and the return value has some contextual meaning. I know technically validateRuntime doesn't have a return value, but it still feels like it has a different use case than a callback.

Separately, I think we should move refactors like this to another PR - don't want to overload a single change with too much.

[huaweigu]

Initially, I thought appending function after a function name was a bit redundant. However, it might be more effective to apply different naming conventions consistently according to their types. Agreed we should probably handle the renaming in a follow up PR if needed.

[huaweigu]

(maybe on*** is suitable for passive functions, while handle*** works well for proactive functions)

[jaypaik]

Fine with revisiting renaming later!