Fix tests account#30
Closed
Dargon789 wants to merge 9 commits into
Closed
Conversation
Add testERC20TransferViaPortoOrchestratorWithPasskey() benchmark to isolate passkey authentication costs from spend limit enforcement costs. - Uses secp256k1 passkey for transaction signing - Sets execution permissions for ERC20 transfers - Requires spend limits (set to max) for passkey operations - Gas cost: 116,094 (vs 97,030 without passkey, 117,083 with restrictive limits) - Provides clean measurement of passkey overhead (~19k gas) Resolves ithacaxyz#272 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Tanishk Goyal <legion2002@users.noreply.github.com>
…ated: IthacaAccount
…' into fix-tests-account
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideAdds Merkle tree–backed wrapped signature support to IthacaAccount, extends unwrapAndValidateSignature with a Merkle flag, fixes EIP712 domain handling in tests, and updates tests/benchmarks (including a new orchestrator benchmark) to use the extended wrapped signature format and cover Merkle verification paths. Sequence diagram for Merkle-backed wrapped signature verification in IthacaAccountsequenceDiagram
actor User
participant Orchestrator
participant IthacaAccount
participant MerkleProofLib
User->>Orchestrator: submit_authorized_action(wrappedSignature)
Orchestrator->>IthacaAccount: unwrapAndValidateSignature(digest, wrappedSignature)
IthacaAccount->>IthacaAccount: detect_wrapped_signature()
IthacaAccount->>IthacaAccount: parse_keyHash_prehash_flag_merkle_flag()
alt merkle_flag_is_set
IthacaAccount->>IthacaAccount: _verifyMerkleSig(digest, innerMerkleSignature)
IthacaAccount->>MerkleProofLib: verifyCalldata(proof, root, digest)
alt merkle_proof_valid
MerkleProofLib-->>IthacaAccount: true
IthacaAccount->>IthacaAccount: unwrapAndValidateSignature(root, rootSig)
IthacaAccount-->>Orchestrator: isValid,keyHash
else merkle_proof_invalid
MerkleProofLib-->>IthacaAccount: false
IthacaAccount-->>Orchestrator: false,0x0
end
else merkle_flag_not_set
IthacaAccount->>IthacaAccount: optional_prehash(digest)
IthacaAccount->>IthacaAccount: load_key_by_keyHash()
IthacaAccount-->>Orchestrator: isValid,keyHash
end
Orchestrator-->>User: action_result
Updated class diagram for IthacaAccount Merkle signature supportclassDiagram
class IthacaAccount {
+unwrapAndValidateSignature(digest_bytes32, signature_bytes_calldata) bool,bytes32
-_verifyMerkleSig(digest_bytes32, signature_bytes_calldata) bool,bytes32
+_eip712Version() string,string
}
class MerkleProofLib {
+verifyCalldata(proof_bytes32_array_calldata, root_bytes32, leaf_bytes32) bool
}
class EfficientHashLib {
+sha2(input_bytes32) bytes32
}
class LibBytes {
+loadCalldata(data_bytes_calldata, offset_uint256) bytes32
+truncatedCalldata(data_bytes_calldata, newLength_uint256) bytes_calldata
}
class IIthacaAccount
class EIP712
class GuardedExecutor
IthacaAccount --|> IIthacaAccount
IthacaAccount --|> EIP712
IthacaAccount --|> GuardedExecutor
IthacaAccount ..> MerkleProofLib : uses
IthacaAccount ..> EfficientHashLib : uses
IthacaAccount ..> LibBytes : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @Dargon789, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces Merkle signature verification capabilities to the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
unwrapAndValidateSignature, the wrapped signature layout now relies on hard‑coded offsets (e.g.0x22,n + 1,n + 2); consider introducing named constants or a small helper to document and centralize this layout to reduce the risk of off‑by‑one bugs in future changes. - The inline assembly in
_verifyMerkleSigassumes the exact ABI layout ofabi.encode(bytes32[] proof, bytes32 root, bytes rootSig); adding brief comments explaining the head/tail offsets (e.g. what lives atsignature.offset,+0x20,+0x40) would make this easier to maintain and safer to modify.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `unwrapAndValidateSignature`, the wrapped signature layout now relies on hard‑coded offsets (e.g. `0x22`, `n + 1`, `n + 2`); consider introducing named constants or a small helper to document and centralize this layout to reduce the risk of off‑by‑one bugs in future changes.
- The inline assembly in `_verifyMerkleSig` assumes the exact ABI layout of `abi.encode(bytes32[] proof, bytes32 root, bytes rootSig)`; adding brief comments explaining the head/tail offsets (e.g. what lives at `signature.offset`, `+0x20`, `+0x40`) would make this easier to maintain and safer to modify.
## Individual Comments
### Comment 1
<location> `src/IthacaAccount.sol:498-507` </location>
<code_context>
+ /// - Note: Each leaf of the merkle tree should be a standard digest.
+ /// - The signature for using merkle verification is encoded as:
+ /// - bytes signature = abi.encode(bytes32[] proof, bytes32 root, bytes rootSig)
+ function _verifyMerkleSig(bytes32 digest, bytes calldata signature)
+ internal
+ view
+ returns (bool isValid, bytes32 keyHash)
+ {
+ bytes32[] calldata proof;
+ bytes32 root;
+ bytes calldata rootSig;
+
+ assembly ("memory-safe") {
+ let proofOffset := add(signature.offset, calldataload(signature.offset))
+ proof.length := calldataload(proofOffset)
+ proof.offset := add(proofOffset, 0x20)
+
+ root := calldataload(add(signature.offset, 0x20))
+
+ let rootSigOffset := add(signature.offset, calldataload(add(signature.offset, 0x40)))
+ rootSig.length := calldataload(rootSigOffset)
+ rootSig.offset := add(rootSigOffset, 0x20)
</code_context>
<issue_to_address>
**issue (bug_risk):** Calldata decoding for `signature` in `_verifyMerkleSig` is misaligned due to the `bytes` length word.
Because `signature` is `bytes calldata`, `signature.offset` points to the bytes length word, and the ABI-encoded `(bytes32[] proof, bytes32 root, bytes rootSig)` head actually starts at `signature.offset + 0x20`.
The current assembly treats `signature.offset` as the start of the ABI head, so:
- `calldataload(signature.offset)` reads the length of `signature`, not the `proof` offset.
- `root := calldataload(add(signature.offset, 0x20))` reads the word after the length, which is not guaranteed to be `root`.
This misalignment will produce wrong offsets and can cause garbage or out-of-bounds reads. You need to first skip the bytes length and then treat the remaining region as the ABI head, e.g.:
```solidity
assembly ("memory-safe") {
let head := add(signature.offset, 0x20) // skip bytes length
let proofRel := calldataload(head)
let rootWord := calldataload(add(head, 0x20))
let rootSigRel:= calldataload(add(head, 0x40))
let proofOffset := add(head, proofRel)
proof.length := calldataload(proofOffset)
proof.offset := add(proofOffset, 0x20)
root := rootWord
let rootSigOffset := add(head, rootSigRel)
rootSig.length := calldataload(rootSigOffset)
rootSig.offset := add(rootSigOffset, 0x20)
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The wrapped signature layout is now fairly intricate (inner vs root vs outer, plus the keyHash/prehash/merkle trailer) and hard-coded offsets like
0x22,n + 1, andn + 2makeunwrapAndValidateSignaturebrittle; consider centralizing the encoding/decoding logic (or at least the trailer size and flag positions) behind named constants/helpers so future changes can’t silently desynchronize tests and production code. - In
_verifyMerkleSig, the inline assembly assumes a well‑formedabi.encode(bytes32[] proof, bytes32 root, bytes rootSig)blob; it would be safer to add minimal length/offset sanity checks (or arequireon expectedsignature.length) to avoid undefined behavior when given malformed or adversarial data.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The wrapped signature layout is now fairly intricate (inner vs root vs outer, plus the keyHash/prehash/merkle trailer) and hard-coded offsets like `0x22`, `n + 1`, and `n + 2` make `unwrapAndValidateSignature` brittle; consider centralizing the encoding/decoding logic (or at least the trailer size and flag positions) behind named constants/helpers so future changes can’t silently desynchronize tests and production code.
- In `_verifyMerkleSig`, the inline assembly assumes a well‑formed `abi.encode(bytes32[] proof, bytes32 root, bytes rootSig)` blob; it would be safer to add minimal length/offset sanity checks (or a `require` on expected `signature.length`) to avoid undefined behavior when given malformed or adversarial data.
## Individual Comments
### Comment 1
<location> `src/IthacaAccount.sol:528-533` </location>
<code_context>
+ return (false, bytes32(0));
+ }
+
/// @dev Returns if the signature is valid, along with its `keyHash`.
/// The `signature` is a wrapped signature, given by
- /// `abi.encodePacked(bytes(innerSignature), bytes32(keyHash), bool(prehash))`.
</code_context>
<issue_to_address>
**issue (bug_risk):** Signature wrapper encoding comment now describes `abi.encode`, but the decoding logic still assumes a custom packed layout.
The docstring now describes `abi.encode(bytes(innerSignature), bytes32(keyHash), bool(prehash), bool(merkle))`, but the implementation still assumes a custom packed layout: `bytes(innerSignature) || bytes32(keyHash) || byte(prehash) || byte(merkle)`.
- `n = signature.length - 0x22` assumes a 34‑byte trailer `[keyHash (32)] [prehash (1)] [merkle (1)]`.
- `LibBytes.truncatedCalldata(signature, n)` just strips those 34 bytes, not an ABI head.
- `prehash`/`merkle` are read from `n + 1` / `n + 2`, i.e., tail-relative offsets, not ABI slots.
With canonical `abi.encode(bytes, bytes32, bool, bool)`, these fields sit in 32‑byte head slots near the start, so any caller following the docstring will get incorrect decoding. Either:
- keep the packed layout and update the docstring/externally documented encoding, or
- switch to real `abi.encode` and update decoding to read the ABI head/tail instead of using `length - 0x22` and tail-relative offsets.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Merkle tree-backed signature support to IthacaAccount, a valuable enhancement. The implementation is accompanied by thorough fuzz tests that cover various scenarios, ensuring the robustness of the new feature. The updates across the test suite to align with the new wrapped signature format are also well-executed. My review includes a couple of suggestions for IthacaAccount.sol to enhance documentation accuracy and code clarity, which should improve maintainability.
Dargon789
commented
Feb 10, 2026
Dargon789
added
bug
Dargon789
commented
Feb 15, 2026
auto-merge was automatically disabled
February 17, 2026 03:46
Pull request was closed
Merged
Merged
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary by Sourcery
Add Merkle tree–based wrapped signature verification to IthacaAccount and update existing tests and benchmarks to support the extended signature encoding.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: