refactor: update license + docs + var renaming #244
Merged
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
Summary by OctaneNew ContractsNo new contracts were added in this PR. Updated Contracts
🔗 Commit Hash: 2c744c2 |
|
Contract sizes: | Contract | Size (B) | Margin (B) |
|-------------------------------|----------|------------|
| AccountFactory | 4,814 | 19,762 |
| AllowlistModule | 9,230 | 15,346 |
| ECDSAValidationModule | 3,646 | 20,930 |
| ModularAccount | 26,520 | -1,944 |
| NativeTokenLimitModule | 4,714 | 19,862 |
| PaymasterGuardModule | 1,797 | 22,779 |
| SemiModularAccountBytecode | 28,371 | -3,795 |
| SemiModularAccountStorageOnly | 28,906 | -4,330 |
| TimeRangeModule | 2,000 | 22,576 |
| WebauthnValidationModule | 7,854 | 16,722 |Code coverage:
|
Overview
Detailed findings
|
jaypaik
reviewed
Oct 14, 2024
jaypaik
approved these changes
Oct 14, 2024
fangting-alchemy
changed the title
howydev
reviewed
Oct 14, 2024
howydev
reviewed
Oct 14, 2024
| @@ -132,7 +153,7 @@ contract AllowlistModule is IExecutionHookModule, IValidationHookModule, BaseMod | |||
| external | |||
| view | |||
| override | |||
| noValidationData(userOp.signature) | |||
| assertNoData(userOp.signature) | |||
There was a problem hiding this comment.
Ok I'm probably dumb - could u explain this line? The UO signature has to be empty to not revert, but wouldn't that cause the UO validation to fail?
There was a problem hiding this comment.
I believe the ExecutionLib.invokeUserOpCallBuffer manipulates the signature field to not include the actual signature but only extra data after. And those hooks with this modifier wants to make sure there isnt any extra data sent (can be a form of inflation attack through attaching large unused data).
howydev
approved these changes
Oct 14, 2024
jaypaik
approved these changes
Oct 15, 2024
Comment on lines
+33
to
+37
| error UnexpectedDataPassed(); | ||
|
|
||
| modifier noValidationData(bytes calldata validationData) { | ||
| if (validationData.length > 0) { | ||
| revert UnexpectedValidationData(); | ||
| modifier assertNoData(bytes calldata data) { | ||
| if (data.length > 0) { | ||
| revert UnexpectedDataPassed(); |
There was a problem hiding this comment.
cc @adamegyed on this naming update
Comment on lines
33
to
34
| /// @notice This module allows for the setting and enforcement of allowlists with ERC20 spend limit for avalidation | ||
| /// entity. |
There was a problem hiding this comment.
Have we defined what a "validation entity" is anywhere? Maybe clearer to say this:
Suggested change
| /// @notice This module allows for the setting and enforcement of allowlists with ERC20 spend limit for avalidation | |
| /// entity. | |
| /// @notice This module allows for the setting and enforcement of allowlists with ERC20 spend limit for a validation | |
| /// function and a given entity ID. |
There was a problem hiding this comment.
Or maybe module entity? Since ModuleEntity is defined as the address + entity ID
There was a problem hiding this comment.
Updated working to avoid confusing terms.
|
No sweat on the bullet spacing stuff - let's do a pass on all the files for consistency later! |
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
The license and docs need to be updated.
Solution
Updated license to what agreed upon.
Updated docs to be more consistent and clear.
Update some var names.