Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: module suffix to execution and validation interfaces #134

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Aug 9, 2024

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
Copy link
Contributor

@adamegyed adamegyed left a comment

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.

Copy link
Collaborator Author

@jaypaik jaypaik left a comment

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!

@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
Copy link
Collaborator Author

jaypaik commented Aug 14, 2024

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
3 checks passed
@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