-
Notifications
You must be signed in to change notification settings - Fork 64
feat: make replaySafeHash wrapping optional with validation option #353
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
base: develop
Are you sure you want to change the base?
Conversation
6198497 to
f07232d
Compare
|
Contract sizes: | Contract | Runtime Size (B) | Initcode Size (B) | Runtime Margin (B) | Initcode Margin (B) |
|------------------------------|------------------|-------------------|--------------------|---------------------|
| AccountFactory | 6,604 | 7,161 | 17,972 | 41,991 |
| Address | 16 | 44 | 24,560 | 49,108 |
| AllowlistModule | 10,148 | 10,174 | 14,428 | 38,978 |
| Base64 | 16 | 44 | 24,560 | 49,108 |
| Create2 | 16 | 44 | 24,560 | 49,108 |
| ECDSA | 16 | 44 | 24,560 | 49,108 |
| ERC165Checker | 16 | 44 | 24,560 | 49,108 |
| ERC1967Utils | 16 | 44 | 24,560 | 49,108 |
| ExecutionInstallDelegate | 5,835 | 5,880 | 18,741 | 43,272 |
| FCL_Elliptic_ZZ | 16 | 44 | 24,560 | 49,108 |
| FCL_ecdsa | 16 | 44 | 24,560 | 49,108 |
| Math | 16 | 44 | 24,560 | 49,108 |
| MessageHashUtils | 16 | 44 | 24,560 | 49,108 |
-| ModularAccount | 22,935 | 23,323 | 1,641 | 25,829 |
+| ModularAccount | 23,310 | 23,698 | 1,266 | 25,454 |
| NativeTokenLimitModule | 4,917 | 4,943 | 19,659 | 44,209 |
| PaymasterGuardModule | 2,028 | 2,054 | 22,548 | 47,098 |
| SafeERC20 | 16 | 44 | 24,560 | 49,108 |
-| SemiModularAccount7702 | 23,336 | 23,717 | 1,240 | 25,435 |
-| SemiModularAccountBytecode | 23,812 | 24,200 | 764 | 24,952 |
+| SemiModularAccount7702 | 23,426 | 23,807 | 1,150 | 25,345 |
+| SemiModularAccountBytecode | 23,902 | 24,290 | 674 | 24,862 |
| SignatureChecker | 16 | 44 | 24,560 | 49,108 |
| SignedMath | 16 | 44 | 24,560 | 49,108 |
-| SingleSignerValidationModule | 3,853 | 3,879 | 20,723 | 45,273 |
+| SingleSignerValidationModule | 3,564 | 3,590 | 21,012 | 45,562 |
| StorageSlot | 16 | 44 | 24,560 | 49,108 |
| Strings | 16 | 44 | 24,560 | 49,108 |
| TimeRangeModule | 2,435 | 2,461 | 22,141 | 46,691 |
| WebAuthn | 16 | 44 | 24,560 | 49,108 |
-| WebAuthnValidationModule | 9,277 | 9,303 | 15,299 | 39,849 |
+| WebAuthnValidationModule | 8,979 | 9,005 | 15,597 | 40,147 |Code coverage:
|
6456bba to
9100d2b
Compare
9100d2b to
3feddad
Compare
adamegyed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, had one callout / design decision note
| /// enforcing the domain separator, which includes this contract's address, the chainId, and the validation | ||
| /// module & entity id. This is only relevant for 1271 validation because UserOp validation relies on the UO | ||
| /// hash and the Entrypoint has safeguards. | ||
| function replaySafeHash(bytes32 hash, ModuleEntity validationModuleEntity) public view returns (bytes32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this public will increase bytecode size, and isn't strictly necessary (plus, doing an eth_call during txn prep is usually slow and requires implementation knowledge anyways.
But, this would make it easier to debug.
Motivation
Solution