-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add ERC7739 compatibility #241
Conversation
Summary by OctaneNew Contracts
Updated Contracts
🔗 Commit Hash: a205db3 |
Contract sizes: | Contract | Size (B) | Margin (B) |
|-------------------------------|----------|------------|
| AccountFactory | 4,814 | 19,762 |
| AllowlistModule | 9,230 | 15,346 |
| ModularAccount | 25,167 | -591 |
| NativeTokenLimitModule | 4,714 | 19,862 |
| PaymasterGuardModule | 1,797 | 22,779 |
-| SemiModularAccountBytecode | 27,124 | -2,548 |
-| SemiModularAccountStorageOnly | 27,642 | -3,066 |
-| SingleSignerValidationModule | 3,646 | 20,930 |
+| SemiModularAccountBytecode | 27,775 | -3,199 |
+| SemiModularAccountStorageOnly | 28,293 | -3,717 |
+| SingleSignerValidationModule | 4,300 | 20,276 |
| TimeRangeModule | 2,000 | 22,576 |
| WebAuthnValidationModule | 7,854 | 16,722 | Code coverage:
|
OverviewOctane AI analysis has finished. No vulnerabilities were found. Cheers! 🎉🎉🎉 🔗 Commit Hash: a205db3 |
5d62bcf
to
aa4f20f
Compare
@@ -47,6 +47,7 @@ contract ModularAccountTest is AccountTestBase { | |||
event ReceivedCall(bytes msgData, uint256 msgValue); | |||
|
|||
function setUp() public override { | |||
_revertSnapshot = vm.snapshot(); |
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.
more context on why this is here - in the repo, to do tests on MA + SMA we take a snapshot in the test constructor in AccountTestBase
and this saves a copy of the chain state. The withSMATest
modifier causes tests to run once on MAs, then does a revert to the snapshotted state and runs tests on SMAs
However, because it happens during the constructor, when the state is resetted, the test contract ends up having 0 bytecode. For some reason this causes external calls to revert which we rely on here as an easy way to convert bytes memory
into bytes calldata
This PR moves the snapshot into setUp()
, so when reverting to the snapshot, the test contract address will contain the bytecode with the required external helper functions
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.
Mostly looks good, but we should investigate the fields
left shift issue.
7a697f3
to
7e14d10
Compare
chore: improve comments and dedupe constants Co-authored-by: Adam Egyed <[email protected]> Update src/helpers/ERC7739ReplaySafeWrapper.sol Update src/helpers/ERC7739ReplaySafeWrapper.sol Co-authored-by: Zer0dot <[email protected]> Update src/helpers/ERC7739ReplaySafeWrapper.sol Co-authored-by: Zer0dot <[email protected]> Co-authored-by: Adam Egyed <[email protected]> chore: gas bad chore: fix test name Co-authored-by: Adam Egyed <[email protected]>
7e14d10
to
1148b11
Compare
signingKey, | ||
bytes memory deferredValidationSig; | ||
bytes memory deferredValidationDatas; | ||
{ |
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.
stack too deep
Co-authored-by: Adam Egyed <[email protected]>
Co-authored-by: Adam Egyed <[email protected]>
Switch the current
ReplaySafeHash
scheme to use an ERC7739 compatible signature scheme insteadImplementation uses pieces from, and/or is inspired by Solady and ERC7579's ERC7739 validator implementation