feat: modify spec to leave arbitrary data params open-ended #170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamegyed
reviewed
Aug 30, 2024
standard/ERCs/erc-6900.md
Outdated
Comment on lines
103
to
110
| /// Flags: 1 byte | ||
| type ValidationConfig is bytes25; | ||
|
|
||
| type HookConfig is bytes26; | ||
| /// A packed representation of a hook function and its associated flags. | ||
| /// Consists of the following, left-aligned: | ||
| /// Module address: 20 bytes | ||
| /// Entity ID: 4 bytes | ||
| /// Flags: 1 byte |
There was a problem hiding this comment.
I think it's worth putting the flags encoding here, too. We can probably just copy/paste from the corresponding library files.
There was a problem hiding this comment.
Sounds good! I didn't want to bloat it but in hindsight, it doesn't really make sense to just say "flags" without specifying which flags?
standard/ERCs/erc-6900.md
Outdated
| /// @param moduleInstallData Optional data to be decoded and used by the module to setup initial module data | ||
| /// for the modular account. | ||
| /// @param manifest the manifest describing functions to install. | ||
| /// @param moduleInstallData Optional data to be arbitrarily used by the account to handle the initial execution setup. |
There was a problem hiding this comment.
What if we phrase it as "Optional data to be used by the account to handle the initial execution setup. Data encoding is implementation-specific." ? I think "arbitrarily use" has really a connotation of being really lax, and we still want to encourage similar-ish use.
There was a problem hiding this comment.
Makes sense! Addressing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, the spec dictates the encoding of certain arbitrary
bytesandbytes[]parameters. However, it may be beneficial to leave the encoding schemes up to individual accounts.This clarifies that accounts have the option to require that these parameters be encoded as they see fit, as long as the behaviour specified in the spec is upheld.
Solution
Mostly, this change just removes a few lines of comments from the spec. I also took the liberty to remove some TODOs which are already complete, and add comments explaining the
HookConfigandValidationConfigdata types.