refactor: remove unneeded signature from Mostro replies #433
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant DM as send_dm Function
participant Rec as Recipient
Client->>DM: call send_dm(receiver_pubkey, sender_keys, payload, expiration)
DM->>DM: Prepare content tuple (payload, None)
DM->>Rec: Send DM with content (without signature)
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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 (2)
src/util.rs (2)
788-804: Enhance test coverage for message content verification.The current test only verifies that
send_dmcompletes successfully. Consider adding tests to verify:
- The correct message content structure
- The behavior with and without signatures
- Message deserialization on the receiver's side
#[tokio::test] async fn test_send_dm() { initialize(); - // Mock the send_dm function let receiver_pubkey = Keys::generate().public_key(); let uuid = uuid!("308e1272-d5f4-47e6-bd97-3504baea9c23"); let message = Message::Order(MessageKind::new( Some(uuid), None, None, Action::FiatSent, None, )); let payload = message.as_json().unwrap(); let sender_keys = Keys::generate(); let result = send_dm(&receiver_pubkey, sender_keys, payload, None).await; - assert!(result.is_ok()); + // Verify successful send + assert!(result.is_ok()); + + // Verify message content structure + let content = serde_json::from_str::<(Message, Option<String>)>(&payload).unwrap(); + assert!(content.1.is_none(), "Signature should be None"); + assert_eq!(content.0.action(), Action::FiatSent); }
296-300: Enhance logging with structured fields and context.Consider using structured logging to improve debuggability and add more context for troubleshooting.
- info!( - "sender key {} - receiver key {}", - sender_keys.public_key().to_hex(), - receiver_pubkey.to_hex() - ); + info!( + sender_key = %sender_keys.public_key().to_hex(), + receiver_key = %receiver_pubkey.to_hex(), + "Preparing to send DM" + ); - info!( - "Sending DM, Event ID: {} with payload: {:#?}", - event.id, payload - ); + info!( + event_id = %event.id, + payload_size = payload.len(), + "Sending DM" + ); + debug!(payload = ?payload, "DM payload");Also applies to: 314-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (1)
src/util.rs (1)
302-302: Verify the impact of removing message signatures.The removal of message signatures could affect message integrity verification on the receiver's side. This change might be breaking if receivers expect signed messages for validation.
Let's check if there are any message signature validations in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for message signature validation code # Test: Look for signature validation patterns. Expect: No critical dependencies on message signatures. # Search for signature validation patterns rg -i "signature|verify|validate" -A 5 -B 5 # Search for message handling code ast-grep --pattern 'Message::from_json($_)'Length of output: 16584
DM Signature Removal: Receivers Validate Sender Identity
Our investigation shows that while several parts of the code (e.g., in
src/app.rs) do perform signature validations, they fallback to checking whether the event sender matches the message’s pubkey if no signature is provided. In our DM sending code atsrc/util.rs, the signature is now set toNone, which is acceptable provided the sender’s identity is reliably verified.
- Affected file:
src/util.rs(DM message creation)- Signature validation behavior:
- When a signature is provided, the system verifies it.
- If the signature is missing, the receiver only accepts messages when the sender matches the expected pubkey.
- Implication:
- For DM messages (which should have consistent sender identity), this change should not break integrity verification.
- However, if any DM is received where the sender does not match the rumor pubkey, the absence of a signature would cause a verification failure.
Overall, the current design handles missing signatures appropriately, so this change appears safe under the assumption that DM messages originate from the claimed sender.
| let message = Message::from_json(&payload).unwrap(); | ||
| // We sign the message | ||
| let sig = message.get_inner_message_kind().sign(&sender_keys); | ||
| // We compose the content | ||
| let content = (message, sig); | ||
| let content = (message, Option::<String>::None); | ||
| let content = serde_json::to_string(&content).unwrap(); |
There was a problem hiding this comment.
Replace unwrap() with proper error handling.
Using unwrap() can lead to runtime panics if JSON operations fail. Consider using proper error handling with ? operator or explicit error handling.
- let message = Message::from_json(&payload).unwrap();
- let content = (message, Option::<String>::None);
- let content = serde_json::to_string(&content).unwrap();
+ let message = Message::from_json(&payload)
+ .map_err(|e| anyhow::anyhow!("Failed to parse message: {}", e))?;
+ let content = (message, Option::<String>::None);
+ let content = serde_json::to_string(&content)
+ .map_err(|e| anyhow::anyhow!("Failed to serialize content: {}", e))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let message = Message::from_json(&payload).unwrap(); | |
| // We sign the message | |
| let sig = message.get_inner_message_kind().sign(&sender_keys); | |
| // We compose the content | |
| let content = (message, sig); | |
| let content = (message, Option::<String>::None); | |
| let content = serde_json::to_string(&content).unwrap(); | |
| let message = Message::from_json(&payload) | |
| .map_err(|e| anyhow::anyhow!("Failed to parse message: {}", e))?; | |
| let content = (message, Option::<String>::None); | |
| let content = serde_json::to_string(&content) | |
| .map_err(|e| anyhow::anyhow!("Failed to serialize content: {}", e))?; |
There was a problem hiding this comment.
@bilthon please apply rabbit suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
I will fix in safer way also this point in my refactor. |
Summary by CodeRabbit