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

feat: consistency in hook UDVTs for ValidationDataView #188

Merged

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Sep 30, 2024

For consistency, updating

struct ValidationDataView {
    ...
    // The pre validation hooks for this validation function.
    ModuleEntity[] preValidationHooks;
    // Execution hooks to run with this validation function.
    HookConfig[] executionHooks;
    ...
}

to:

struct ValidationDataView {
    ...
    // The validation hooks for this validation function.
    HookConfig[] validationHooks;
    // Execution hooks to run with this validation function.
    HookConfig[] executionHooks;
    ...
}

Validation hooks are defined in the spec as always running before a validation function, so we can make this simplification here. However, I see a slight edge in keeping preUserOpValidationHook, preRuntimeValidationHook, and preSignatureValidationHook prefixed with "pre" in the rare event that postUserOpValidationHook etc. are introduced in the future. This would allow for backward compatibility for v0.8 validation hook modules and the above proposed change also allows us to keep the ValidationDataView interface the same.

I've opted to also update the storage in the reference account to store HookConfig[] instead of ModuleEntity[] for simplicity.

@jaypaik jaypaik requested a review from a team September 30, 2024 23:12
@jaypaik jaypaik force-pushed the 09-30-feat_consistency_in_hook_UDVTs_for_ValidationDataView branch from 3cfbc7e to afd714e Compare October 1, 2024 04:40
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.

Looks good, 1 small comment

test/utils/AccountTestBase.sol Outdated Show resolved Hide resolved
@jaypaik jaypaik force-pushed the 09-30-fix_add_missing_isUserOpValidation_to_ValidationDataView_in_spec branch from 42c537d to d2e8540 Compare October 1, 2024 20:19
Base automatically changed from 09-30-fix_add_missing_isUserOpValidation_to_ValidationDataView_in_spec to develop October 1, 2024 20:42
@jaypaik jaypaik force-pushed the 09-30-feat_consistency_in_hook_UDVTs_for_ValidationDataView branch from afd714e to d06cfbc Compare October 1, 2024 20:43
Copy link

octane-security-app bot commented Oct 1, 2024

Summary by Octane

New Contracts

No new contracts were added in this PR.

Updated Contracts

  • AccountStorage.sol: Replaced preValidationHooks with validationHooks in the validation function configuration.
  • ModularAccountView.sol: Changed preValidationHooks to validationHooks to generalize and consolidate hook configurations.
  • ModuleManagerInternals.sol: Consolidated pre-validation hooks into generalized validation hooks for better modularity and maintenance.
  • ReferenceModularAccount.sol: Updated the smart contract to utilize HookConfig for validation hooks, replacing ModuleEntity references with moduleEntity() method calls.
  • IModularAccountView.sol: Pre-validation hooks merged into a single validationHooks structure.

🔗 Commit Hash: fd241dc

This comment was marked as resolved.

@jaypaik jaypaik merged commit 8c09ff9 into develop Oct 1, 2024
4 checks passed
@jaypaik jaypaik deleted the 09-30-feat_consistency_in_hook_UDVTs_for_ValidationDataView branch October 1, 2024 20:56
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