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: [v0.8-develop] invert validation mapping #85

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

adamegyed
Copy link
Contributor

Motivation

As noted in a comment here, it may be useful to have the loupe functions expose "selectors per validation", rather than "validations per selectors".

Solution

Instead of storing an enumerable mapping validations in SelectorData, we can store an enumerable mapping of selectors in ValidationData.

This only changes the external loupe function getValidations(bytes4 selector) external view returns (FunctionReference[] memory) to getSelectors(FunctionReference validationFunction) external view returns (bytes4[] memory). No other account functions or behavior is changed.

@adamegyed adamegyed requested review from fangting-alchemy and a team June 24, 2024 19:12
Copy link
Contributor

@huaweigu huaweigu left a comment

Choose a reason for hiding this comment

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

LGTM. This aligns better with most of the use cases, getting selectors through validation.
Another useful quick check could be isSelectorProtectedByValidation(FunctionReference validationFunction).

src/interfaces/IAccountLoupe.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@fangting-alchemy fangting-alchemy 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

Copy link
Collaborator

@howydev howydev left a comment

Choose a reason for hiding this comment

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

LGTM

@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch 2 times, most recently from 3866b0c to 364e06b Compare June 26, 2024 19:48
@adamegyed adamegyed changed the base branch from v0.8-develop to adam/self-call-restrictions June 26, 2024 19:49
@adamegyed
Copy link
Contributor Author

Changing base to point to #78, so this can be used as a base to build off of while waiting on blocking PRS.

@adamegyed adamegyed force-pushed the adam/self-call-restrictions branch from 1e415b1 to 7e76786 Compare June 27, 2024 19:29
@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from 364e06b to b78c45b Compare June 27, 2024 19:29
@adamegyed adamegyed force-pushed the adam/self-call-restrictions branch from 7e76786 to 2569ebd Compare June 27, 2024 20:11
@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from b78c45b to 05cea78 Compare June 27, 2024 20:11
@adamegyed adamegyed force-pushed the adam/self-call-restrictions branch from 2569ebd to 1d34450 Compare June 28, 2024 15:31
@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from 05cea78 to 4cfa5bf Compare June 28, 2024 15:31
@adamegyed adamegyed force-pushed the adam/self-call-restrictions branch from 1d34450 to 5fda9a2 Compare June 28, 2024 18:39
@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from 4cfa5bf to 509ba03 Compare June 28, 2024 18:39
@adamegyed adamegyed force-pushed the adam/self-call-restrictions branch from 5fda9a2 to ce90f55 Compare July 10, 2024 15:23
Base automatically changed from adam/self-call-restrictions to v0.8-develop July 10, 2024 15:41
@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from 509ba03 to 1bfd893 Compare July 10, 2024 15:46
@adamegyed adamegyed merged commit 072fa15 into v0.8-develop Jul 10, 2024
3 checks passed
@adamegyed adamegyed deleted the adam/refactor-validation-mapping branch July 10, 2024 15:55
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.

4 participants