Skip to content

refactor: module suffix to execution and validation interfaces#134

Merged
jaypaik merged 3 commits into
developfrom
08-09-refactor_module_suffix_to_execution_and_validation_interfaces
Aug 14, 2024
Merged

refactor: module suffix to execution and validation interfaces#134
jaypaik merged 3 commits into
developfrom
08-09-refactor_module_suffix_to_execution_and_validation_interfaces

Conversation

@jaypaik

@jaypaik jaypaik commented Aug 9, 2024

Copy link
Copy Markdown
Collaborator

Open to pushback on this. I personally think this makes things clearer in understanding that all of these things are modules, and unifies file/contract naming across our module types.

@jaypaik jaypaik requested a review from a team August 9, 2024 17:04
@jaypaik jaypaik force-pushed the 08-09-refactor_module_suffix_to_execution_and_validation_interfaces branch 2 times, most recently from 088d4f3 to 7fb76b7 Compare August 9, 2024 17:59

@adamegyed adamegyed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure this is a net improvement. It helps in the concrete type naming (i.e. SingleSignerValidationModule) where it lines up with some other naming schemes (like AllowlistModule), but in the cases of the interfaces, it makes it really wordy.

Also flagged some instances where I think the added "address" wording is unnecessary.

I'm not sure it helps comprehension, but obviously this is coming from a very familiar perspective.

Comment thread deployments/arb-sepolia.md
Comment thread src/account/AccountFactory.sol Outdated
Comment thread src/account/UpgradeableModularAccount.sol

@jaypaik jaypaik left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pass this by the rest of the working group - I think it's a helpful change but if I'm the minority, happy to find alternatives or keep things as is!

Comment thread deployments/arb-sepolia.md
Comment thread src/account/AccountFactory.sol Outdated
Comment thread src/account/UpgradeableModularAccount.sol
@jaypaik jaypaik force-pushed the 08-09-refactor_module_suffix_to_execution_and_validation_interfaces branch from 7fb76b7 to 3bcaa2d Compare August 14, 2024 19:43
@jaypaik jaypaik requested a review from adamegyed August 14, 2024 19:45
@jaypaik

jaypaik commented Aug 14, 2024

Copy link
Copy Markdown
Collaborator Author

Some things to also look at:

The events ExecutionInstalled, ValidationInstalled, etc.

event ExecutionInstalled(address indexed module, ExecutionManifest manifest);
event ExecutionUninstalled(address indexed module, bool onUninstallSucceeded, ExecutionManifest manifest);
event ValidationInstalled(address indexed module, uint32 indexed entityId);
event ValidationUninstalled(address indexed module, uint32 indexed entityId, bool onUninstallSucceeded);

And other places where we reference "validation functions" as Validation:

error ValidationAlreadySet(bytes4 selector, ModuleEntity validationFunction);

In this PR, we separate the concept of a "validation module" and a "validation function" from both being under the umbrella term of "validation", which I think is a good thing.

@jaypaik jaypaik merged commit 47b912a into develop Aug 14, 2024
@jaypaik jaypaik deleted the 08-09-refactor_module_suffix_to_execution_and_validation_interfaces branch August 14, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants