feat(libp2p): multi signature schemes & session signing #2599
Conversation
WalkthroughOhayo, sensei! The changes in this pull request introduce several enhancements across multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Relay
participant Provider
Client->>Relay: Send message with signature
Relay->>Relay: Call validate_signature
Relay->>Provider: Validate signature
Provider-->>Relay: Return validation result
Relay-->>Client: Return response based on validation
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
crates/torii/libp2p/src/types.rs (1)
12-17: Consider adding documentation for signature variants, sensei!While the enum structure is solid, adding documentation would help explain:
- The purpose and use case for each signature variant
- The expected format and constraints of each variant's data
- The relationship between TypedDataHash and Felt vectors in the Session variant
Example documentation:
/// Represents different signature schemes supported by the system #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Signature { /// WebAuthn signatures with variable length Webauthn(Vec<Felt>), /// Standard Starknet ECDSA signature (r, s) Starknet((Felt, Felt)), /// Session-based signature combining typed data hashes and signature components Session((Vec<TypedDataHash>, Vec<Felt>)), }crates/torii/libp2p/src/tests.rs (1)
686-686: Ohayo, sensei! Consider adding tests forSignaturehandling.To ensure robustness, it would be beneficial to include unit tests that specifically verify the handling of different
Signaturevariants in thepublishmethod.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/torii/libp2p/src/errors.rs(2 hunks)crates/torii/libp2p/src/server/mod.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)crates/torii/libp2p/src/types.rs(1 hunks)
🔇 Additional comments (8)
crates/torii/libp2p/src/types.rs (2)
6-10: Ohayo! Clean and well-structured TypedDataHash implementation.
The struct is properly designed with clear field names and appropriate derive macros.
22-22: Verify message handlers for signature type change.
The signature field type change from Vec<Felt> to Signature is a breaking change. Let's verify that all message handlers are updated accordingly.
✅ Verification successful
Let me gather more information about the signature type changes.
The results will help us verify if all message handlers are properly updated to handle both Starknet and Ethereum signature types.
Let me check one more aspect of the message handling to ensure complete verification.
Ohayo sensei! After analyzing the codebase thoroughly, I can now provide a conclusive response about the signature type change.
Signature type change is properly handled across the codebase
The change from Vec<Felt> to Signature enum is well-implemented with proper handling:
- All message handlers in
libp2p/src/server/mod.rscorrectly handle all three signature variants (Webauthn, Starknet, Session) - Test cases in
libp2p/src/tests.rsdemonstrate correct usage of the new signature type - No serialization issues found as the Message struct is only used within the libp2p crate
- The signature conversion logic is properly implemented for each variant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential message handling code that might need updates
echo "Searching for Message struct usage..."
rg -l "Message" --type rust
echo "Searching for signature field access patterns..."
rg "message.*signature" --type rust -A 2
Length of output: 2624
Script:
#!/bin/bash
# Check the old signature type definition
echo "Checking old signature type in git history..."
git log -p -S "Vec<Felt>" --type rust -- crates/torii/libp2p/src/types.rs
# Look for Signature enum definition and usage
echo "Looking for Signature enum definition..."
rg -p "enum Signature" --type rust -A 5
# Check for any other signature-related type handling
echo "Checking signature-related type handling..."
ast-grep --pattern 'fn $_($_: $$$) -> $_ {
$$$
signature
$$$
}'
Length of output: 679
Script:
#!/bin/bash
# Check message handling implementations
echo "Checking message handling implementations..."
rg -p "impl.*Message" --type rust -A 10
# Look for signature type conversions
echo "Looking for signature conversions..."
rg "Signature::(Webauthn|Starknet|Session)" --type rust -A 2
# Check for message serialization
echo "Checking message serialization code..."
rg "serialize.*Message" --type rust -A 5
Length of output: 9817
crates/torii/libp2p/src/errors.rs (2)
7-7: Ohayo! Clean import addition, sensei!
The import statement is well-placed and follows the existing import organization pattern.
49-51: LGTM! Error variant follows best practices!
The new ProviderError variant maintains consistency with existing error handling patterns by using #[error(transparent)] and deriving From via #[from].
crates/torii/libp2p/src/server/mod.rs (2)
41-41: Ohayo sensei! Importing Signature and TypedDataHash looks good.
The added import statements are necessary for the new signature handling logic. Well done!
413-475: Ohayo sensei! The validate_signature function enhances modularity.
The addition of the validate_signature function encapsulates the signature verification logic, improving code organization and maintainability. Great work!
crates/torii/libp2p/src/tests.rs (2)
548-548: Ohayo, sensei! The Signature import is properly added.
The import statement correctly includes Signature from crate::types, which is necessary for the updated Message struct.
686-686: Ohayo, sensei! The usage of Signature::Starknet in publish is correct.
The publish method now sends a Message with the Signature::Starknet variant, correctly passing the (r, s) tuple. This aligns with the changes to the Signature enum.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/libp2p/src/server/mod.rs (1)
413-477: Consider adding unit tests for signature validation.The new
validate_signaturefunction handles critical security functionality but lacks test coverage. Consider adding tests for:
- Valid/invalid signatures for each signature type
- Error cases (provider errors, encoding errors)
- Edge cases (zero values, empty signatures)
Would you like me to help generate comprehensive test cases for this function?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/libp2p/src/server/mod.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)crates/torii/libp2p/src/types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/libp2p/src/tests.rs
- crates/torii/libp2p/src/types.rs
🔇 Additional comments (2)
crates/torii/libp2p/src/server/mod.rs (2)
40-41: Ohayo sensei! The new imports look good.
The imports are well-organized and necessary for the new signature validation functionality.
305-313: Previous review comment is still applicable.
| if !match validate_signature( | ||
| &self.provider, | ||
| entity_identity, | ||
| &data.message, | ||
| &data.signature, | ||
| ) | ||
| .await |
There was a problem hiding this comment.
Since the function returns a bool, the construction of this and arms below may be much simpler:
if !validate_signature(...)? {
error!(..);
continue
}There was a problem hiding this comment.
this wouldn't work though since we don't want to propagate the error. we should just log it and not exit. since we're taking in user input
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/libp2p/src/server/mod.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)crates/torii/libp2p/src/types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/libp2p/src/tests.rs
- crates/torii/libp2p/src/types.rs
🔇 Additional comments (2)
crates/torii/libp2p/src/server/mod.rs (2)
40-41: Ohayo sensei! Clean import additions.
The new imports for TypedData and Signature types are well-organized and properly scoped.
451-452:
Ohayo sensei! Fix typo in function name.
The selector name contains a typo: "is_session_sigature_valid" should be "is_session_signature_valid".
-entry_point_selector: get_selector_from_name("is_session_sigature_valid")
+entry_point_selector: get_selector_from_name("is_session_signature_valid")Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2599 +/- ##
==========================================
- Coverage 55.13% 55.11% -0.03%
==========================================
Files 385 385
Lines 47748 47775 +27
==========================================
+ Hits 26328 26331 +3
- Misses 21420 21444 +24 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
ProviderErrorvariant for enhanced error handling.validate_signaturefunction for improved signature verification across different types.Signatureenum for better signature representation in messages.Bug Fixes
Tests