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

[1/n] Introduce EntityId in account and plugins #93

Merged
merged 11 commits into from
Jul 17, 2024

Conversation

fangting-alchemy
Copy link
Collaborator

@fangting-alchemy fangting-alchemy commented Jul 11, 2024

[1/n]: Introduce entityId with type uint32. Remove functionId. This PR only updates the types and names and any code logic depends on the type. Storage and other code logic of entityId will be introduced in the follow up PRs.

Context:

@fangting-alchemy fangting-alchemy requested a review from a team July 11, 2024 21:49
@fangting-alchemy fangting-alchemy changed the title [1/n] Introduce ValidationId in account and plugins to store validation data [1/n] Introduce EntityId in account and plugins Jul 11, 2024
@adamegyed
Copy link
Contributor

I'm on board for renaming functionId to entityId and increasing the size to uint32, but I think FunctionReference makes more sense as a name than PackedPluginEntity. An ID provides the plugin function with the information needed to identify an entity, but there's still a function that's being called on the plugin itself. It doesn't encode which type of function to call, because that is implicitly known from which storage field the FunctionReference is held in. I also think "Packed" is redundant, but PluginEntity is maybe too generic.

Curious if you have any other naming ideas.

@fangting-alchemy
Copy link
Collaborator Author

but I think FunctionReference makes more sense as a name than PackedPluginEntity. An ID provides the plugin function with the information needed to identify an entity, but there's still a function that's being called on the plugin itself. It doesn't encode which type of function to call, because that is implicitly known from which storage field the FunctionReference is held in. I also think "Packed" is redundant, but PluginEntity is maybe too generic.

The goal of the naming change for FunctionReference is to update the name to reflect what it does, holds plugin address + entity id, instead of how it can used and/or what it leads to.

@fangting-alchemy
Copy link
Collaborator Author

Packed wording can remind users that this type is a custom packed type in the codebase.
Following that, ValidationConfig => PackedValidationConfig make the complex codebase more readable.

@adamegyed
Copy link
Contributor

The goal of the naming change for FunctionReference is to update the name to reflect what it does, holds plugin address + entity id, instead of how it can used and/or what it leads to.

Yeah this is starting to make sense to me, I like the PluginEntity naming.

I still think adding "Packed" to the name is unnecessary. In general, conciseness leads to more readable code. And in this case specifically, because it's not distinguishing it from a different type, I think it's unnecessary. There is no "Unpacked" version, like struct ValidationConfig or struct PluginEntity, that has the same fields but in a different format.

@fangting-alchemy
Copy link
Collaborator Author

fangting-alchemy commented Jul 11, 2024

I still think adding "Packed" to the name is unnecessary.

Maybe Packed is not the best choice. The intention is to distinguish those (PluginEntity & ValidationConfig) from the struct datatypes (ValidationData etc.) as those require more careful handling.
Custom in the beginning? Reference in the end? Any suggestions?

@adamegyed
Copy link
Contributor

They're already unique by not having location specifiers (memory, storage, or calldata) from being a user-defined value type, and dealing with those different data locations is generally what the careful handling is for. Do they need to be more distinguished?

@fangting-alchemy
Copy link
Collaborator Author

They're already unique by not having location specifiers (memory, storage, or calldata) from being a user-defined value type, and dealing with those different data locations is generally what the careful handling is for. Do they need to be more distinguished?

Good point. Updated.

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, have a few small nits

src/helpers/PluginEntityLib.sol Outdated Show resolved Hide resolved
src/helpers/PluginEntityLib.sol Show resolved Hide resolved
src/helpers/PackedPluginEntityLib.sol Outdated Show resolved Hide resolved
src/interfaces/IValidation.sol Outdated Show resolved Hide resolved
src/interfaces/IExecutionHook.sol Outdated Show resolved Hide resolved
@@ -12,81 +12,81 @@ import {FunctionReference, ValidationConfig} from "../interfaces/IPluginManager.
// 0x______________________________________________000000000000000000 // unused

library ValidationConfigLib {
Copy link
Collaborator

Choose a reason for hiding this comment

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

small nit: could we update the comments in L8-L12 on layout also?

@@ -75,18 +75,18 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference)
// 0x____________________________________________BB__________________ is post hook
Copy link
Collaborator

Choose a reason for hiding this comment

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

small nit: prefer to update these

@fangting-alchemy fangting-alchemy merged commit d2e3c3a into v0.8-develop Jul 17, 2024
3 checks passed
@fangting-alchemy fangting-alchemy deleted the validation branch July 17, 2024 00:49
adamegyed pushed a commit that referenced this pull request Jul 26, 2024
…w multiple validation installation on account (#93)
adamegyed pushed a commit that referenced this pull request Aug 5, 2024
…w multiple validation installation on account (#93)
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