AccountERC7579: document that isValidSignature has no native validation fallback - #6708
BhariGowda wants to merge 1 commit into
Conversation
…on fallback The `@dev` block said that ERC-1271 validation falls back to "native" validation by the abstract signer when module based validation fails. It does not. `isValidSignature` returns `0xffffffff` and never calls `super` or `_rawSignatureValidation`, which this contract overrides to return `false` by default. `_validateUserOp` is the function that does have that fallback, and its own NatSpec is accurate, so only the `isValidSignature` block changes. The replacement describes how the validator is selected, that the module's return value is forwarded as-is, and which paths yield `0xffffffff`. Comment only. Runtime bytecode of `$AccountERC7579Mock`, `$AccountERC7579HookedMock` and `$AccountMock` is byte-identical before and after once the trailing metadata hash is stripped.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe ERC-1271 documentation now states that signature validation uses only installed validator modules. It states that successful validator results are forwarded. It also documents Merge Risk: ⚪ Minimal · up to This PR clarifies existing signature-validation behavior without changing runtime code or user-facing functionality. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AccountERC7579.isValidSignatureis documented as falling back to "native" validation by the abstract signer. It does not, and it never has.draft-AccountERC7579.sol#L185-L191:The body (L192-L207) returns
bytes4(0xffffffff)on every path that does not reach an installed validator. It calls neithersupernor_rawSignatureValidation, and this same contract overrides_rawSignatureValidationat L423-L429 to returnfalse, under the comment "By default, only use the modules for validation of userOp and signature. Disable raw signatures."So the file states both that native validation is a fallback and that it is disabled. The code agrees with the second.
_validateUserOp(L209-L226) is the function that does have the fallback:super._validateUserOpresolves toAccount._validateUserOp, which is exactly_rawSignatureValidation(...) ? SIG_VALIDATION_SUCCESS : SIG_VALIDATION_FAILED. That makes the fallback meaningful for a derived contract that re-overrides_rawSignatureValidation(asAccountMockdoes), and its NatSpec ("Falls back to{Account-_validateUserOp}otherwise") is accurate.The asymmetry between the two functions looks deliberate: the
_rawSignatureValidationoverride states the same decision a second way. Only the description ofisValidSignatureis wrong, and it has been since #5657 introduced the function: that@devblock and the body have both been unchanged since, so this is a stale description rather than a behavior change that the docs missed. This PR rewrites that one@devblock to say how the validator is selected, that the module's return value is forwarded as-is, and which three paths produce0xffffffff._validateUserOpand the_rawSignatureValidationoverride are untouched.Verification
Comment-only, and checked rather than assumed: compiled
masterand this branch, stripped the trailing CBOR metadata fromdeployedBytecode, and the runtime code of$AccountERC7579Mock,$AccountERC7579HookedMockand$AccountMockis byte-identical across the two.npm run lint:solclean,npm testgreen (8417 passing: 361 solidity, 8056 mocha, 0 failing).Changeset
None. NatSpec only, no user-visible behavior change. Recent documentation-only PRs (#6681, #6648, #6636, #6633, #6623, #6607, #6422) all merged with no changeset and the
ignore-changesetlabel. Happy to add one instead if you prefer.PR Checklist
npx changeset add)