-
Notifications
You must be signed in to change notification settings - Fork 21
fix(1271): separate policyData and signature #177
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Separates concatenated policy data from the validator signature in ERC-1271 flow by introducing an explicit length-delimited policyData segment inside the aggregated signature format. Updates test construction of composite signatures and parsing logic in SmartSession to extract policy data and remaining validator signature.
- Introduces new signature layout: permissionId | policyDataLength | policyData | validatorSignature
- Adjusts isValidSignatureWithSender to parse and dispatch policyData and validator signature separately
- Refactors tests to build signatures following the new structured layout
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test/unit/base/ERC1271Base.t.sol | Updates test helper to construct new composite signature with embedded policyData length and blob. |
| contracts/SmartSession.sol | Parses new signature structure: extracts policyData length, slices policyData for ERC1271 policy checks, and separates remaining validator signature. |
Comments suppressed due to low confidence (1)
test/unit/base/ERC1271Base.t.sol:1
- [nitpick] Hard-coded length literal (uint256(32)) and an opaque bytes32 constant reduce clarity of the new signature structure; consider introducing named constants or helper builders (e.g., mockPolicyData(), POLICY_DATA_LENGTH) to document intent and avoid duplication across tests.
import "../../Base.t.sol";
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
contracts/SmartSession.sol
Outdated
| requestSender: sender, | ||
| hash: hash, | ||
| signature: signature, | ||
| signature: signature[64:remainingSignatureOffset], // extract policy data from the signature |
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.
👍
|
added some bounds checks & consts @zeroknots |
There's a bug in 1271 where we pass the whole signature to both the policy and validator, meaning that both the validator and the policy would need to know how to destructure the same data, which obviously doesn't work with OwnableValidator etc.
This PR fixes this by extracting policyData from the signature before passing it to the policy and then passes the signature data to the stateless validator.