Add v1.11.0 cluster definition hashing with List[Bytes65,32] signatures.#220
Conversation
Support Charon's variable-length Safe multisig signatures by hashing up to 32 concatenated 65-byte ECDSA chunks while keeping v1.10.0 on fixed Bytes65. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds v1.11.0 cluster verification support. It updates the SDK version constant and docs example, adds new SSZ schemas and hashing helpers for cluster definitions and locks, wires v1.11.0 into the verifier dispatch paths, and adds fixtures and tests for the new version. Changesv1.11.0 Cluster Verification Support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/verification/v1.11.0.ts (1)
34-46: ⚡ Quick winReject oversized signature lists before SSZ hashing.
This helper enforces 65-byte chunking but not the
List[Bytes65,32]upper bound. A 33-chunk Safe signature will currently fail later inside SSZ hashing instead of at the parse boundary, which makes the error harder to diagnose.Proposed change
const splitK1SignatureList = (signature: string): Uint8Array[] => { const bytes = fromHexString(signature); if (bytes.length === 0) return []; if (bytes.length % 65 !== 0) { throw new Error('Signature length must be a multiple of 65 bytes'); } + const chunkCount = bytes.length / 65; + if (chunkCount > 32) { + throw new Error('Signature list cannot exceed 32 chunks'); + } const chunks: Uint8Array[] = []; for (let i = 0; i < bytes.length; i += 65) { chunks.push(bytes.slice(i, i + 65)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/verification/v1.11.0.ts` around lines 34 - 46, splitK1SignatureList currently validates 65-byte chunking but doesn't reject lists larger than the SSZ List[Bytes65,32] max; update splitK1SignatureList to check bytes.length (or computed chunk count) against the 32-item upper bound and throw a clear Error (e.g., "Signature list exceeds maximum of 32 signatures") before returning chunks so oversized (33+) signature lists are rejected at parse time; reference the splitK1SignatureList function and ensure the check occurs prior to slicing/returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/client/methods.spec.ts`:
- Around line 298-311: The test needs a real v1.11 fixture instead of only
changing the version string: create v1x11Definition by cloning
clusterLockV1X10.cluster_definition, then call
clusterConfigOrDefinitionHash(clonedDef, true) to recompute and set
clonedDef.config_hash before computing the definition hash via
clusterConfigOrDefinitionHash(clonedDef, false); also modify at least one
creator/operator signature field in the clonedDef to be a multi-chunk signature
(split into a List[Bytes65,32] style multi-chunk value) so the new v1.11
splitting logic is exercised when computing definitionHash (refer to
clusterConfigOrDefinitionHash, v1x11Definition and
clusterLockV1X10.cluster_definition).
---
Nitpick comments:
In `@src/verification/v1.11.0.ts`:
- Around line 34-46: splitK1SignatureList currently validates 65-byte chunking
but doesn't reject lists larger than the SSZ List[Bytes65,32] max; update
splitK1SignatureList to check bytes.length (or computed chunk count) against the
32-item upper bound and throw a clear Error (e.g., "Signature list exceeds
maximum of 32 signatures") before returning chunks so oversized (33+) signature
lists are rejected at parse time; reference the splitK1SignatureList function
and ensure the check occurs prior to slicing/returning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4e925ea-c45e-4b29-a7a4-76ff52250fab
📒 Files selected for processing (6)
src/constants.tssrc/index.tssrc/verification/common.tssrc/verification/sszTypes.tssrc/verification/v1.11.0.tstest/client/methods.spec.ts
… order
- Add clusterLockV1X11 fixture: real cluster with a 2-of-N Safe multisig
creator signature (two 65-byte chunks) and empty operator addresses/sigs,
exercising the List[Bytes65,32] signature-list hashing.
- Rework the v1.11 unit test to assert golden config_hash/definition_hash
(produced by charon + the dev API) instead of only version-flip inequality,
addressing the CodeRabbit review comment.
- Add test/verification/v1x11-parity.spec.ts golden-vector parity check
(config/definition/lock hash) under the CI test glob.
- sdk-package e2e: load .env via jest setupFiles ("dotenv/config") so the RPC
is set before the SDK is imported, so PROVIDER_MAP resolves and Safe /
ERC-1271 lock verification works locally (previously fell back to EOA).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/verification/v1x11-parity.spec.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing
as anycasts if fixture types align with function parameters.The
as anycasts on lines 15, 21, and 27 bypass TypeScript's type checking. IfclusterLockV1X11and itscluster_definitionproperty already conform to the expected parameter types ofclusterConfigOrDefinitionHashandclusterLockHash, removing the casts would provide compile-time assurance that the fixture shape matches the hashing functions' contracts. If the types don't align, consider tightening the fixture type instead.Also applies to: 21-21, 27-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/verification/v1x11-parity.spec.ts` at line 15, The parity test is bypassing TypeScript safety by casting the fixture to any before calling clusterConfigOrDefinitionHash and clusterLockHash. Remove the as any casts in v1x11-parity.spec.ts at the calls using def and clusterLockV1X11.cluster_definition, and if the fixture types do not match, tighten the fixture or function parameter types so clusterLockV1X11 and its cluster_definition satisfy the expected inputs without casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/verification/v1x11-parity.spec.ts`:
- Line 15: The parity test is bypassing TypeScript safety by casting the fixture
to any before calling clusterConfigOrDefinitionHash and clusterLockHash. Remove
the as any casts in v1x11-parity.spec.ts at the calls using def and
clusterLockV1X11.cluster_definition, and if the fixture types do not match,
tighten the fixture or function parameter types so clusterLockV1X11 and its
cluster_definition satisfy the expected inputs without casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0887bdfb-a3bd-4eb1-8559-9522f022a569
📒 Files selected for processing (4)
test/client/methods.spec.tstest/fixtures.tstest/sdk-package/jest-e2e.jsontest/verification/v1x11-parity.spec.ts
Fixes the CI prettier-ci failure on src/verification/v1.11.0.ts (never formatted since it was added) and applies prettier to the new v1.11 fixture and test files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
2 participants
Support Charon's variable-length Safe multisig signatures by hashing up to 32 concatenated 65-byte ECDSA chunks while keeping v1.10.0 on fixed Bytes65.
Summary by CodeRabbit
acceptClusterDefinitionJSDoc example to reference v1.11.0.config_hash,definition_hash, andlock_hashvalues.