Code refactoring and bug fixes #96
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant modifications across multiple files, primarily focusing on the transition from using Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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
🧹 Outside diff range and nitpick comments (8)
src/cli/send_dm.rs (1)
Line range hint
11-17: LGTM! Consider documenting boolean parameters.The refactoring to use
send_message_synclooks correct. However, the purpose of the last two boolean parameters (true, true) is not immediately clear.Consider adding a comment explaining what these boolean flags control:
send_message_sync( client, None, trade_keys, receiver, message.to_string(), + // Controls message encryption true, + // Controls message persistence true, )src/cli/take_dispute.rs (1)
Boolean flags in dispute message need adjustment
The current implementation uses
true, falsefor dispute messages, but this appears inconsistent with the pattern in the codebase:
- All administrative actions (take_buy, take_sell, rate_user, add_invoice) use
true, true- Only regular messages in send_msg.rs use
true, false- The dispute handling is an administrative action similar to other "take_" operations
The flags should be changed to
true, trueto maintain consistency with other administrative actions.
src/cli/take_dispute.rs: Change the boolean flags insend_message_synccall fromtrue, falsetotrue, true🔗 Analysis chain
Line range hint
31-39: LGTM! Verify boolean flags for dispute messages.The refactoring to
send_message_synclooks correct. Note that this function uses different boolean flags (true, false) compared tosend_dm.rs(true, true).Let's verify if these are the intended flags for dispute messages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dispute-related message sending to verify consistency rg -A 5 "Message::new_dispute.*send_message_sync"Length of output: 51
Script:
#!/bin/bash # Let's try a broader search for dispute-related messages rg -A 5 "new_dispute" # Also search for the current usage pattern rg -A 5 "send_message_sync.*true.*false" # And look for the implementation of send_message_sync to understand the flags ast-grep --pattern 'fn send_message_sync($$$) { $$$ }'Length of output: 421
Script:
#!/bin/bash # Let's search for the send_message_sync function definition to understand the parameters rg -A 10 "pub fn send_message_sync" # Also search for other usages of send_message_sync to compare flags rg "send_message_sync\(" -A 2Length of output: 1282
Script:
#!/bin/bash # Let's look at the full function signature and its parameters rg -A 15 "pub async fn send_message_sync" # And get more context for each usage to see the boolean flags rg "send_message_sync\(" -B 2 -A 4Length of output: 3325
Script:
#!/bin/bash # Let's get the full context of each call to see the boolean flags rg "send_message_sync\(" -B 2 -A 6 # And specifically look at dispute-related code for more context rg -B 5 -A 10 "Action::AdminTakeDispute"Length of output: 4213
src/cli/rate_user.rs (1)
Line range hint
38-46: LGTM! Consider improving error handling.The refactoring to
send_message_synclooks correct. However, usingstd::process::exit(0)is not idiomatic Rust error handling.Consider returning a Result instead of calling exit:
- std::process::exit(0); + Ok(())Also, earlier in the function:
- println!("Rating must be in the range 1 - 5"); - std::process::exit(0); + return Err(anyhow::anyhow!("Rating must be in the range 1 - 5"));This allows the caller to handle errors appropriately and makes the function more testable.
src/cli/add_invoice.rs (1)
Line range hint
33-39: Improve error handling in invoice validationThe error from
is_valid_invoiceis only printed and not propagated. Consider proper error handling to prevent silent failures.- Err(e) => println!("{}", e), + Err(e) => return Err(anyhow::anyhow!("Invalid invoice: {}", e)),src/cli/take_sell.rs (3)
Line range hint
13-21: Consider using a builder pattern for parametersThe function has many arguments (as noted by the clippy allow). Consider using a builder pattern or a configuration struct to improve readability and maintainability.
Example structure:
struct TakeSellConfig { order_id: Uuid, invoice: Option<String>, amount: Option<u32>, identity_keys: Keys, trade_keys: Keys, trade_index: i64, mostro_key: PublicKey, client: Client, }
Line range hint
31-37: Consistent error handling neededSimilar to add_invoice.rs, the invoice validation error is only printed. Consider using consistent error handling across all invoice validation code.
- Err(e) => println!("{}", e), + Err(e) => return Err(anyhow::anyhow!("Invalid invoice: {}", e)),
Line range hint
41-52: Simplify payload construction logicThe nested match expressions for payload construction are complex and could be simplified.
Consider extracting the payload construction logic into a separate function:
fn construct_payload(invoice_payload: Option<Payload>, amount: Option<u32>) -> Option<Payload> { match (invoice_payload, amount) { (Some(Payload::PaymentRequest(a, b, _)), Some(amt)) => Some(Payload::PaymentRequest(a, b, Some(amt as i64))), (None, Some(amt)) => Some(Payload::Amount(amt.into())), (payload, None) => payload, _ => None, } }src/util.rs (1)
104-114: Consider enhancing error handling for payment request processingThe payment request handling could benefit from proper error handling instead of unwrapping. Consider using the
?operator or match expressions to handle potential None values gracefully.- if let Some(Payload::PaymentRequest(ord, inv, _)) = - &el.0.get_inner_message_kind().payload - { - println!("NEW MESSAGE:"); - println!( - "Mostro sent you this hold invoice for order id: {}", - ord.as_ref().unwrap().id.unwrap() - ); + if let Some(Payload::PaymentRequest(Some(ord), inv, _)) = + &el.0.get_inner_message_kind().payload + { + if let Some(order_id) = &ord.id { + println!("NEW MESSAGE:"); + println!( + "Mostro sent you this hold invoice for order id: {}", + order_id + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cli/add_invoice.rs(2 hunks)src/cli/get_dm.rs(3 hunks)src/cli/new_order.rs(3 hunks)src/cli/rate_user.rs(2 hunks)src/cli/send_dm.rs(2 hunks)src/cli/send_msg.rs(2 hunks)src/cli/take_buy.rs(2 hunks)src/cli/take_dispute.rs(2 hunks)src/cli/take_sell.rs(2 hunks)src/db.rs(5 hunks)src/util.rs(8 hunks)
🔇 Additional comments (13)
src/cli/take_buy.rs (1)
Line range hint 37-44: Document the boolean parameters in send_message_sync
The boolean parameters true and false lack context. Consider adding named parameters or documentation to clarify their purpose.
Let's verify the consistency of parameter usage across all refactored calls:
✅ Verification successful
The search results show the function signature in src/util.rs with named parameters wait_for_dm_ans and to_user for the boolean arguments. Looking at all usages:
- Most CLI commands use
true, falseconsistently - Only
send_dm.rsusestrue, true - Only
send_msg.rsusesfalse, false
Consider using named arguments for better readability
The boolean parameters in send_message_sync are documented in the function signature as wait_for_dm_ans and to_user. Consider using named arguments in the function calls for better readability:
send_message_sync(
client,
Some(identity_keys),
trade_keys,
mostro_key,
take_buy_message,
wait_for_dm_ans: true,
to_user: false,
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of send_message_sync to verify parameter consistency
rg "send_message_sync\(" -A 7
Length of output: 3080
src/cli/add_invoice.rs (1)
Line range hint 46-53: Verify error handling consistency
The function uses the same parameter pattern as other refactored calls to send_message_sync.
Let's verify error handling consistency across similar functions:
✅ Verification successful
Let me check the error handling in send_message_sync calls to ensure consistency.
Let me check how the result of send_message_sync is handled in these calls.
Let me check the implementation of send_message_sync and how its result is used.
Error handling is consistent across all usages
The verification shows that:
send_message_syncreturns aResult<()>type- The invoice validation pattern is consistent in both
take_sell.rsandadd_invoice.rs - The parameter pattern in the
add_invoice.rscall matches other calls tosend_message_syncacross the codebase - All calls use the same error handling approach with
.unwrap()on the result
The error handling approach is consistent with the codebase's patterns, though it's worth noting that using .unwrap() is a deliberate choice across all CLI commands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for invoice validation patterns
ast-grep --pattern 'match is_valid_invoice($_) {
Ok($_) => $_,
Err($_) => $_
}'
Length of output: 665
Script:
#!/bin/bash
# Search for send_message_sync calls and their error handling
ast-grep --pattern 'send_message_sync($$$)'
Length of output: 3219
Script:
#!/bin/bash
# Search for send_message_sync function definition and its return type
ast-grep --pattern 'fn send_message_sync($$$)'
# Also check how the result is handled in the calling functions
rg -A 2 "send_message_sync\(" src/cli/
Length of output: 1176
Script:
#!/bin/bash
# Search for send_message_sync implementation
rg -A 10 "fn send_message_sync" --type rust
# Search for Result handling after send_message_sync calls
rg -B 2 -A 4 "send_message_sync\(" --type rust
Length of output: 3177
src/cli/send_msg.rs (2)
2-2: LGTM: Import statement updated correctly
The import statement has been updated to reflect the renamed function, maintaining consistency with the broader refactoring effort.
Line range hint 58-63: Verify message synchronization behavior
The function call has been updated from send_order_id_cmd to send_message_sync. While the parameters remain consistent, we should verify that the synchronization behavior matches the previous implementation.
✅ Verification successful
Let me analyze the function usage and generate another verification script to check the implementation details.
Let me try a different approach to check the implementation.
Let me check one more aspect to see the complete implementation.
Synchronization behavior verified and consistent
The send_message_sync function maintains the same synchronization behavior as it:
- Sends encrypted direct messages using the same key parameters
- Waits for and processes notifications synchronously
- Handles payment request responses in the same way
- Uses the same encryption and conversation key derivation mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of send_message_sync across the codebase
# Look for any remaining instances of send_order_id_cmd and verify parameter consistency
rg -A 3 "send_message_sync|send_order_id_cmd"
Length of output: 3135
Script:
#!/bin/bash
# Check the implementation of send_message_sync and any previous send_order_id_cmd
ast-grep --pattern 'fn send_message_sync($$$) { $$$ }'
# Also check git history for the old implementation
git log -p --no-merges -S "send_order_id_cmd"
Length of output: 56
Script:
#!/bin/bash
# Check the implementation details of send_message_sync
rg -A 20 "^pub async fn send_message_sync" src/util.rs
# Also check if there are any tests that might show the behavior
rg -l "send_message_sync.*test"
Length of output: 555
Script:
#!/bin/bash
# Get more context of the send_message_sync implementation
rg -A 40 "^pub async fn send_message_sync" src/util.rs
# Also check the send_dm function it's using
rg -A 15 "^pub async fn send_dm" src/util.rs
Length of output: 2058
src/cli/get_dm.rs (2)
4-4: LGTM: Improved type safety with Message struct
The change from using raw strings to the Message type improves type safety and reduces potential runtime errors from message parsing.
Also applies to: 19-19
45-67: Verify message payload handling completeness
The message processing logic has been updated to handle different payload types. However, we should verify that all possible payload types are handled appropriately.
✅ Verification successful
Based on the search results, I can see the usage patterns of the Payload enum, which includes:
- PaymentRequest
- TextMessage
- Order
- Amount
- RatingUser
Let's verify if this is the complete enum definition:
Based on the code analysis results, I can generate the final response:
The message payload handling is complete and correctly implemented
The code handles all the known Payload variants that are used throughout the codebase:
- PaymentRequest: Handled explicitly with invoice display
- TextMessage: Handled explicitly with text display
- All other variants (Order, Amount, RatingUser): Handled by the catch-all case which prints the action and payload in debug format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all possible payload types in the Message struct
# to ensure complete coverage in the processing logic
ast-grep --pattern 'enum Payload {
$$$
}'
Length of output: 45
Script:
#!/bin/bash
# Let's try to find the Payload enum definition using ripgrep
rg "enum Payload" -A 20
Length of output: 25
Script:
#!/bin/bash
# Let's try to find files that might contain the Payload enum definition
fd -e rs | xargs rg "Payload::"
# Also search for any struct or usage patterns of Payload
rg "Payload" -B 2 -A 2
Length of output: 7737
Script:
#!/bin/bash
# Search for the actual Payload enum definition in mostro_core
rg -p "enum Payload" mostro_core --type rust -A 10
Length of output: 104
src/cli/new_order.rs (1)
137-144: Verify the boolean parameter changes in send_message_sync
The send_message_sync call now uses true for what appears to be a synchronization flag. This change from the previous false value needs verification.
src/db.rs (4)
43-43: LGTM: Schema change for request_id field
The addition of the nullable INTEGER field for request_id is well-placed and correctly typed.
217-219: LGTM: Order struct field addition
The request_id field is correctly defined as Option, matching the database schema.
227-227: LGTM: Method signature update
The request_id parameter is correctly added as Option, maintaining consistency with the struct field.
254-258: LGTM: SQL query updates
The request_id field is correctly added to both the column list and values list in the INSERT query, with proper binding.
Also applies to: 277-279
src/util.rs (2)
244-244: LGTM: Improved type safety with Message struct
The change from raw strings to the Message struct improves type safety and adds proper message verification.
Also applies to: 276-276, 286-286
309-317: LGTM: Robust message signature verification
The addition of signature verification for unwrapped messages is a crucial security enhancement that properly validates message authenticity.
Summary by CodeRabbit
New Features
request_idfor each order to enhance tracking.Bug Fixes
Documentation
Refactor
send_order_id_cmdwithsend_message_syncacross various functions for improved message communication.Chores