Skip to content

Conversation

@jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Nov 22, 2024

Copy link
Collaborator Author

jaypaik commented Nov 22, 2024

@jaypaik jaypaik marked this pull request as ready for review November 22, 2024 22:29
@octane-security-app
Copy link

octane-security-app bot commented Nov 22, 2024

Summary by Octane

New Contracts

No new contracts were added in this PR.

Updated Contracts

  • AccountStorage.sol: Updated validation uses ValidationFlags bitmask for compact flag representation.
  • ModularAccountView.sol: Streamlined validation data by consolidating validation flags into a single field.
  • ModuleManagerInternals.sol: The smart contract update consolidates validation flags and configures them using a single ValidationFlags parameter instead of individual boolean flags.
  • ReferenceModularAccount.sol: The smart contract now uses ValidationFlags for validation checks and functionality improvements with modular account operations.
  • IModularAccount.sol: Added ValidationFlags type and renamed validation flags to ValidationFlags for clarity.
  • IModularAccountView.sol: Added ValidationFlags to streamline validation settings and improve function flag handling.
  • ValidationConfigLib.sol: The smart contract merged ValidationFlags import, improving flag configuration and address-packing efficiency.

🔗 Commit Hash: cf43884

@jaypaik jaypaik force-pushed the 11-22-feat_update_validationdataview_with_validationflags branch 2 times, most recently from 9e224f1 to e3b058b Compare November 22, 2024 22:31
@jaypaik jaypaik force-pushed the 11-22-feat_update_validationdataview_with_validationflags branch from e3b058b to cf43884 Compare November 22, 2024 22:33
@jaypaik jaypaik requested a review from a team November 22, 2024 22:33
//
// Validation flags layout:

type ValidationFlags is uint8;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved this type from ValidationConfigLib into IModularAccount. I think it's slightly more consistent for all our UDVTs (like HookConfig) to be in the interfaces and not the libraries.

@octane-security-app

This comment was marked as resolved.

@nikita-quantstamp
Copy link
Contributor

This makes sense; it simplifies things, given that we already encoded those flags. No strong opinions either way. This does move some decoding efforts to the client side right?

@jaypaik
Copy link
Collaborator Author

jaypaik commented Nov 26, 2024

This makes sense; it simplifies things, given that we already encoded those flags. No strong opinions either way. This does move some decoding efforts to the client side right?

@nikita-quantstamp Correct. It does make the interface a bit more forward compatible and accommodating to additional flags in the future (or custom flags if an account wants to keep track of additional attributes).

Also, HookConfig needs to be decoded anyway (in the same struct), so I imagine it won't be much additional effort client side.

Copy link
Contributor

@nikita-quantstamp nikita-quantstamp left a comment

Choose a reason for hiding this comment

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

LGTM, I like the forward compatability approach for future flags

Copy link
Collaborator Author

jaypaik commented Nov 27, 2024

Merge activity

  • Nov 27, 10:35 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 27, 10:35 AM EST: A user merged this pull request with Graphite.

@jaypaik jaypaik merged commit fc91fa5 into develop Nov 27, 2024
@jaypaik jaypaik deleted the 11-22-feat_update_validationdataview_with_validationflags branch November 27, 2024 15:35
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.

[Improvement] A more forward-compatible ValidationDataView struct

3 participants