Adding BLS aggregation for exits #123
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luke Hackett <luke@obol.tech>
…rk/obol-sdk into luke/exit-sdk-commands
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luke Hackett <luke@obol.tech>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luke Hackett <luke@obol.tech>
|
""" WalkthroughA new asynchronous method, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExitClass
participant BLSLib
Caller->>ExitClass: recombineExitBlobs(exitBlob)
ExitClass->>BLSLib: initBLS()
ExitClass->>ExitClass: Extract and sort partial signatures
ExitClass->>BLSLib: aggregateSignatures(sortedSignatures)
BLSLib-->>ExitClass: aggregatedSignature
ExitClass-->>Caller: FullExitBlob (with aggregated signature)
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Caution No docstrings were generated. |
![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 (3)
test/exit/exit.spec.ts (1)
663-674: Consider adding a test for undefined shares_exit_data.While the test handles empty arrays, consider adding a test case where
shares_exit_datais undefined to ensure the implementation handles this edge case gracefully.Add this test case:
it('should handle empty shares_exit_data', async () => { const mockExistingBlob: ExistingExitValidationBlobData = { public_key: '0x' + '1234'.repeat(24), epoch: '100', validator_index: '200', shares_exit_data: [], }; await expect(exit.recombineExitBlobs(mockExistingBlob)).rejects.toThrow( 'No valid signatures found for aggregation', ); }); + + it('should handle undefined shares_exit_data gracefully', async () => { + const mockExistingBlob: ExistingExitValidationBlobData = { + public_key: '0x' + '1234'.repeat(24), + epoch: '100', + validator_index: '200', + shares_exit_data: undefined as any, + }; + await expect(exit.recombineExitBlobs(mockExistingBlob)).rejects.toThrow(); + }); });src/exits/exit.ts (2)
620-622: Document the index adjustment for clarity.The conversion from zero-based to one-based indexing (adding 1) should be more clearly documented to prevent confusion.
Enhance the comment to explain the indexing conversion:
- // Convert string index to number and add 1 (matching Go's sigIdx+1) + // Convert string index to number and add 1 to match Go's one-based indexing + // JavaScript uses zero-based indexing, but the Go implementation expects one-based const sigIdx = parseInt(sigIdxStr, 10); signaturesByIndex.set(sigIdx + 1, sigBytes);
644-646: Consider adding a comment about BLS threshold aggregation.The comment mentions that @chainsafe/bls doesn't have explicit threshold aggregation, but it would be helpful to clarify that standard aggregation is sufficient when signatures are ordered correctly.
Add clarification:
// Aggregate signatures (equivalent to tbls.ThresholdAggregate in Go) - // Note: @chainsafe/bls doesn't have explicit threshold aggregation, but ordering should be preserved + // Note: @chainsafe/bls doesn't have explicit threshold aggregation, but standard aggregation + // works correctly when signatures are properly ordered by their share indices const fullSig = aggregateSignatures(rawSignatures);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/exits/exit.ts(3 hunks)src/types.ts(1 hunks)test/exit/exit.spec.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
test/exit/exit.spec.ts (2)
585-628: LGTM! Well-structured test suite for the new aggregation functionality.The test cases comprehensively cover the happy path, including proper verification that signatures are sorted by operator index before aggregation, and that the BLS library is initialized correctly.
630-661: LGTM! Good coverage of error scenarios.The error handling tests appropriately validate edge cases including empty signatures, invalid signature lengths, and missing data.
src/types.ts (2)
451-468: Well-documented interface enhancement!The detailed JSDoc comments provide excellent clarity on the purpose and structure of each field in the
ExistingExitValidationBlobDatainterface.
470-497: Clear and comprehensive type definition for the aggregated exit blob.The
FullExitBlobinterface is well-structured with detailed documentation that clearly explains the purpose of each field and its relationship to the BLS aggregation functionality.src/exits/exit.ts (1)
566-588: Comprehensive method documentation.The JSDoc comment clearly explains the purpose, parameters, return value, and potential errors of the
recombineExitBlobsmethod.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Luke Hackett <luke@obol.tech>
|
#109
Adding bls aggregation for a set of exit blobs to be recombined into a full validator exit message
Summary by CodeRabbit