implement request_id on mostro messages #376
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in this pull request involve the addition of a Changes
Poem
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: 21
🧹 Outside diff range and nitpick comments (35)
src/app/admin_add_solver.rs (3)
Line range hint
19-32: Consider enhancing error handling with custom error types.The current error handling uses generic logging. Consider creating domain-specific error types to provide more context and better error handling.
#[derive(Debug, thiserror::Error)] pub enum AdminError { #[error("Invalid message content: {0}")] InvalidContent(String), #[error("Missing public key")] MissingPubKey, // ... other error variants } // Then replace the current error handling: let content = inner_message.content.ok_or(AdminError::MissingPubKey)?; let npubkey = match content { Content::TextMessage(p) => p, _ => return Err(AdminError::InvalidContent("Expected text message".into())), };
Line range hint
39-45: Document magic numbers and consider making them configurable.The
User::new(public_key, 0, 1, 0, 0)call contains magic numbers that should be documented or made configurable.+ // Initial user configuration: (reputation: 0, role: 1 (solver), trades: 0, disputes: 0) let user = User::new(public_key, 0, 1, 0, 0);Consider using constants or configuration:
const INITIAL_REPUTATION: i32 = 0; const SOLVER_ROLE: i32 = 1; const INITIAL_TRADES: i32 = 0; const INITIAL_DISPUTES: i32 = 0;
Line range hint
11-51: Consider splitting the function into smaller, focused components.The function currently handles multiple responsibilities: validation, database operations, and messaging. Consider splitting it into smaller, more focused components following the Single Responsibility Principle.
Suggested structure:
- Validation layer: Check admin privileges and message content
- Service layer: Handle solver creation business logic
- Repository layer: Handle database operations
- Messaging layer: Handle response messages
Example pseudo-structure:
async fn validate_admin_request(event: &UnwrappedGift, my_keys: &Keys) -> Result<()>; async fn create_solver(pubkey: String, pool: &Pool<Sqlite>) -> Result<User>; async fn notify_admin(sender: &PublicKey, request_id: u64) -> Result<()>; pub async fn admin_add_solver_action(...) -> Result<()> { validate_admin_request(event, my_keys).await?; let solver = create_solver(public_key, pool).await?; notify_admin(&event.sender, request_id).await?; Ok(()) }src/app/fiat_sent.rs (2)
18-18: Consider usingArc<AtomicU64>for thread-safe request tracking.Given the concurrent nature of message handling, using a plain
u64forrequest_idmight lead to race conditions. Consider usingArc<AtomicU64>to ensure thread-safe access and modification of request IDs.use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; // In function signature request_id: Arc<AtomicU64> // When using the request_id let id = request_id.load(Ordering::SeqCst);
Line range hint
22-24: Enhance error handling and add success tracing.The error handling could be improved with:
- More descriptive error messages
- Tracing for successful operations
- Structured error types instead of generic errors
- return Err(Error::msg("No order id")); + return Err(Error::msg("Order ID missing in message payload")); + tracing::error!("Failed to process fiat sent action: missing order ID");Consider adding success tracing after successful status updates and notifications:
tracing::info!("Fiat sent confirmation processed successfully for order {}", order_id);src/app/take_buy.rs (2)
Line range hint
71-81: Consider using the function parameter instead of extracting request_id againThe code extracts
request_idfrom the message when we already have it as a function parameter. This could lead to inconsistencies if they differ.- msg.get_inner_message_kind().request_id, + request_id,
Line range hint
1-124: Consider implementing proper synchronization as mentioned in PR descriptionWhile the request_id changes improve request tracking, the PR description mentions that proper synchronization using Rust's
Atomictypes would be more appropriate. Consider implementing this to address potential race conditions and improve the overall architecture.Example approach:
use std::sync::atomic::{AtomicU64, Ordering}; static REQUEST_COUNTER: AtomicU64 = AtomicU64::new(0); // Use this to generate unique request IDs let request_id = REQUEST_COUNTER.fetch_add(1, Ordering::SeqCst);src/app/order.rs (1)
Line range hint
1-124: Consider implementing structured request trackingAs mentioned in the PR description, the current implementation could benefit from better synchronization. Consider:
- Implementing a proper request tracking structure using
std::sync::atomic::AtomicU64for request IDs- Creating a dedicated RequestContext struct to encapsulate request-related data
- Using a builder pattern for message construction to ensure consistent request ID handling
Example structure:
use std::sync::atomic::{AtomicU64, Ordering}; struct RequestTracker { next_id: AtomicU64, } struct RequestContext { id: u64, sender: XOnlyPublicKey, // Add other relevant fields } impl RequestTracker { fn new_request(&self, sender: XOnlyPublicKey) -> RequestContext { RequestContext { id: self.next_id.fetch_add(1, Ordering::SeqCst), sender, } } }src/flow.rs (2)
10-10: Consider using AtomicU64 for request_id generation.While using
Option<u64>is a good choice for the parameter, defaulting to1when None could lead to request ID collisions. As mentioned in the PR description, usingstd::sync::atomic::AtomicU64would provide better synchronization and unique request IDs.Consider implementing a request ID generator like this:
use std::sync::atomic::{AtomicU64, Ordering}; static REQUEST_COUNTER: AtomicU64 = AtomicU64::new(1); fn generate_request_id() -> u64 { REQUEST_COUNTER.fetch_add(1, Ordering::SeqCst) }
Line range hint
121-143: Consider adding request_id to all invoice-related functions.For consistency in request tracking,
hold_invoice_settlementandhold_invoice_canceledshould also accept and utilize therequest_idparameter. This would ensure comprehensive request monitoring across all invoice operations.Consider updating these functions similarly:
-pub async fn hold_invoice_settlement(hash: &str) -> Result<()> { +pub async fn hold_invoice_settlement(hash: &str, request_id: Option<u64>) -> Result<()> { -pub async fn hold_invoice_canceled(hash: &str) -> Result<()> { +pub async fn hold_invoice_canceled(hash: &str, request_id: Option<u64>) -> Result<()> {src/app/admin_settle.rs (2)
87-95: Consider improving function call formatting.While the request_id addition is correct, the function call formatting could be more concise.
Consider this formatting:
- settle_seller_hold_invoice( - event, - ln_client, - Action::AdminSettled, - true, - &order, - request_id, - ) + settle_seller_hold_invoice(event, ln_client, Action::AdminSettled, true, &order, request_id)
Line range hint
129-145: Consider consolidating message sending logic.While the request_id handling is correct, the message sending logic could be more robust and DRY.
Consider creating a helper function:
async fn send_settlement_message(message: &str, recipients: &[&PublicKey]) -> Result<()> { for recipient in recipients { if let Err(e) = send_dm(recipient, message).await { error!("Failed to send settlement message to {}: {}", recipient, e); } } Ok(()) }Then use it like:
- send_dm(&event.sender, message.clone()).await?; - if let Some(ref seller_pubkey) = order_updated.seller_pubkey { - send_dm(&PublicKey::from_str(seller_pubkey)?, message.clone()).await?; - } - if let Some(ref buyer_pubkey) = order_updated.buyer_pubkey { - send_dm(&PublicKey::from_str(buyer_pubkey)?, message.clone()).await?; - } + let mut recipients = vec![&event.sender]; + if let Some(ref seller_pubkey) = order_updated.seller_pubkey { + if let Ok(key) = PublicKey::from_str(seller_pubkey) { + recipients.push(&key); + } + } + if let Some(ref buyer_pubkey) = order_updated.buyer_pubkey { + if let Ok(key) = PublicKey::from_str(buyer_pubkey) { + recipients.push(&key); + } + } + send_settlement_message(&message, &recipients).await?;src/main.rs (2)
111-113: Enhance test coverage for request_id handlingWhile the tests have been updated to include
request_id, consider adding test cases that verify:
- Messages with missing request_id
- Messages with invalid request_id values
- Request_id overflow scenarios
Here's a suggested test case to add:
#[test] fn test_message_request_id_variants() { // Test missing request_id let msg_no_id = r#"{"order":{"version":1,"id":"test-id","action":"fiat-sent","content":null}}"#; // Test invalid request_id let msg_invalid_id = r#"{"order":{"version":1,"request_id":-1,"id":"test-id","action":"fiat-sent","content":null}}"#; // Test request_id overflow let msg_overflow_id = r#"{"order":{"version":1,"request_id":4294967296,"id":"test-id","action":"fiat-sent","content":null}}"#; }Also applies to: 120-122
Line range hint
1-156: Consider architectural improvements for request trackingThe current implementation of request tracking could benefit from the following architectural improvements:
- Implement a centralized request ID management system using
Arc<AtomicU32>to ensure thread-safe ID generation and tracking- Create a dedicated type for request tracking (e.g.,
RequestTracker) that encapsulates the synchronization logic- Consider adding a request context that can be propagated through the async calls
Here's a suggested approach:
use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::Arc; struct RequestTracker { next_id: Arc<AtomicU32>, } impl RequestTracker { pub fn new() -> Self { Self { next_id: Arc::new(AtomicU32::new(1)), } } pub fn next_request_id(&self) -> u32 { self.next_id.fetch_add(1, Ordering::SeqCst) } }This would help address the "spaghetti" concerns mentioned in the PR description while providing a more maintainable solution for request tracking.
src/app/admin_cancel.rs (4)
Line range hint
36-42: Consider consistent error handling across all paths.While the non-assigned solver case uses
send_new_order_msg, the database error case at line 42 only logs the error. Consider sending an error message to the user in the database error case as well for consistent user experience.Err(e) => { error!("Error checking if solver is assigned to order: {:?}", e); + send_new_order_msg( + msg.get_inner_message_kind().request_id, + Some(order_id), + Action::DatabaseError, + None, + &event.sender, + ) + .await; return Ok(()); }
Line range hint
62-71: Add error logging for message sending failures.The current implementation silently ignores message sending errors. Consider logging these failures for better observability.
if let Ok(message) = message.as_json() { - let _ = send_dm(&event.sender, message).await; + if let Err(e) = send_dm(&event.sender, message).await { + error!("Failed to send cooperative cancel message: {:?}", e); + } }
Line range hint
128-144: Consider refactoring message sending logic.The current implementation repeats the message sending logic for each party. Consider extracting this into a helper function for better maintainability and consistent error handling.
+ async fn send_cancel_message( + message: String, + recipients: &[&PublicKey], + ) -> Result<()> { + for recipient in recipients { + if let Err(e) = send_dm(recipient, message.clone()).await { + error!("Failed to send cancel message to {}: {:?}", recipient, e); + } + } + Ok(()) + } + let message = Message::new_order(request_id, Some(order.id), Action::AdminCanceled, None); let message = message.as_json()?; - // Message to admin - send_dm(&event.sender, message.clone()).await?; - + let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { (Some(seller), Some(buyer)) => ( PublicKey::from_str(seller.as_str())?, @@ - (_, None) => return Err(Error::msg("Missing buyer pubkey")), }; - send_dm(&seller_pubkey, message.clone()).await?; - send_dm(&buyer_pubkey, message).await?; + send_cancel_message( + message, + &[&event.sender, &seller_pubkey, &buyer_pubkey], + ).await?;
Line range hint
1-144: Consider implementing proper request synchronization.As mentioned in the PR description, the current implementation could benefit from proper synchronization. Consider:
- Using
std::sync::atomic::AtomicU64for thread-safe request ID handling- Implementing a request tracking mechanism to prevent duplicate processing
- Adding request correlation in logs for better traceability
This would improve the robustness of the request handling system.
src/app/take_sell.rs (3)
70-76: Consider more idiomatic error handling formatWhile the error handling is correct, the formatting could be more concise using Rust's builder pattern.
- send_cant_do_msg( - request_id, - Some(order.id), - Some(e.to_string()), - &event.sender, - ) - .await; + send_cant_do_msg(request_id, Some(order.id), Some(e.to_string()), &event.sender).await;
159-167: Consider more concise function call formattingWhile the functionality is correct, the multi-line formatting could be more concise.
- show_hold_invoice( - my_keys, - pr, - &buyer_pubkey, - &seller_pubkey, - order, - request_id, - ) - .await?; + show_hold_invoice(my_keys, pr, &buyer_pubkey, &seller_pubkey, order, request_id).await?;
Line range hint
1-169: Consider implementing proper synchronizationAs mentioned in the PR description, the current implementation might benefit from proper synchronization. Consider using Rust's
Arc<AtomicU64>for thread-safe request ID handling, especially if this code runs in a concurrent context.Example synchronization pattern:
use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; struct RequestTracker { current_id: Arc<AtomicU64>, } impl RequestTracker { fn new() -> Self { Self { current_id: Arc::new(AtomicU64::new(0)), } } fn get_next_id(&self) -> u64 { self.current_id.fetch_add(1, Ordering::SeqCst) } }src/app/admin_take_dispute.rs (1)
44-46: Consider implementing a consistent request_id handling strategy.The current implementation shows inconsistent patterns in request_id usage, sometimes using the function parameter and sometimes retrieving it from the message. Consider:
- Always using the function parameter for consistency and reliability
- Adding validation to ensure message.request_id matches the parameter
- Documenting the expected behavior when IDs differ
src/app/rate_user.rs (2)
79-79: Enhance error messages for better user experience.While the error handling is correct, consider providing more specific error messages to help users understand and resolve issues:
- send_cant_do_msg(request_id, Some(order.id), None, &event.sender).await; + send_cant_do_msg( + request_id, + Some(order.id), + Some(Content::String(format!("Order status '{}' is not valid for rating", order.status))), + &event.sender + ).await;- send_cant_do_msg(request_id, Some(order.id), None, &event.sender).await; + send_cant_do_msg( + request_id, + Some(order.id), + Some(Content::String("User not found in order participants".to_string())), + &event.sender + ).await;Also applies to: 100-100
Line range hint
168-173: LGTM! Consider future architectural improvements.The message sending logic correctly uses the request_id from the original message. However, given the author's note about the code being "a bit spaghetti", consider these future improvements:
- Use a dedicated ReputationManager struct to encapsulate the rating logic
- Implement proper error types instead of using string messages
- Consider using a state machine pattern for order status transitions
src/app/dispute.rs (3)
Line range hint
44-52: Consider enhancing error logging for status validationWhile the status validation works correctly, consider adding error logging when the status parsing fails to help with debugging.
if let Ok(st) = Status::from_str(&order.status) { if matches!(st, Status::Active | Status::FiatSent) { order } else { + error!("Invalid order status for dispute: {}", order.status); send_new_order_msg( msg.get_inner_message_kind().request_id, Some(order.id), Action::NotAllowedByStatus,
Line range hint
116-120: Consider using a larger token range for better securityThe current token generation uses a range of 100-999 (900 possible values), which might be insufficient for a high-volume system. Consider using a larger range or a more secure token generation method to reduce collision probability.
-dispute.buyer_token = Some(rng.gen_range(100..=999)); -dispute.seller_token = Some(rng.gen_range(100..=999)); +// Use a 6-digit token for better security (1M possible values) +dispute.buyer_token = Some(rng.gen_range(100_000..=999_999)); +dispute.seller_token = Some(rng.gen_range(100_000..=999_999));
Line range hint
146-150: Enhance error message for pubkey parsingConsider including the actual pubkey string in the error message to aid debugging.
-error!("Error parsing counterpart pubkey: {:#?}", e); +error!("Error parsing counterpart pubkey '{}': {:#?}", counterpart, e);src/app/add_invoice.rs (2)
Line range hint
113-119: Use the function parameterrequest_idinstead of extracting from message.Same inconsistency with request_id source.
Apply this change:
send_new_order_msg( - msg.get_inner_message_kind().request_id, + request_id, Some(order.id), Action::NotAllowedByStatus, None, &event.sender, )
Line range hint
158-172: Use the function parameterrequest_idinstead of extracting from message.Two more instances of the same inconsistency with request_id source.
Apply these changes:
send_new_order_msg( - msg.get_inner_message_kind().request_id, + request_id, Some(order.id), Action::BuyerTookOrder, Some(Content::Order(order_data.clone())), &seller_pubkey, ) .await; send_new_order_msg( - msg.get_inner_message_kind().request_id, + request_id, Some(order.id), Action::HoldInvoicePaymentAccepted, Some(Content::Order(order_data)), &buyer_pubkey, )src/app.rs (3)
Line range hint
150-150: Implement PayInvoice action handlerThe
PayInvoiceaction is currently marked astodo!(). This should be implemented to maintain consistency with other actions.Would you like me to help create a template for the
PayInvoiceaction handler implementation?
Line range hint
87-204: Consider reducing action handler boilerplateThe current implementation repeats similar error handling patterns across all action handlers. Consider introducing a helper function to reduce duplication and improve maintainability.
async fn handle_action<F, Fut>(action: &Action, f: F) where F: FnOnce() -> Fut, Fut: std::future::Future<Output = Result<()>>, { if let Err(e) = f().await { warning_msg(action, e); } } // Usage example: handle_action(&action, || async { order_action(msg, &event, &my_keys, &pool, request_id).await }).await;
Line range hint
82-204: Consider implementing proper request synchronizationAs mentioned in the PR description, using an
Atomicfor request synchronization would be more appropriate. This could help track request states more reliably and reduce the "spaghetti code" nature of the current implementation.Consider:
- Implementing a
RequestTrackerstruct usingAtomicU64for request IDs- Adding request state management (pending/completed/failed)
- Using a proper synchronization mechanism for request lifecycle management
Example structure:
use std::sync::atomic::{AtomicU64, Ordering}; struct RequestTracker { current_id: AtomicU64, // Add other tracking fields as needed } impl RequestTracker { fn track_request(&self, request_id: u64) -> bool { // Implement request tracking logic } fn complete_request(&self, request_id: u64) { // Implement request completion logic } }src/app/cancel.rs (1)
Line range hint
223-240: Add transaction support for database operationsMultiple database operations (order updates, state transitions) are performed without transaction support. This could lead to inconsistent states if any operation fails.
Consider wrapping related database operations in a transaction:
let mut tx = pool.begin().await?; if order.price_from_api { order.amount = 0; order.fee = 0; } edit_buyer_pubkey_order(&mut tx, order.id, None).await?; update_order_to_initial_state(&mut tx, order.id, order.amount, order.fee).await?; // Commit the transaction only if all operations succeed tx.commit().await?;Also applies to: 292-309
src/scheduler.rs (1)
107-107: LGTM! Consider enhancing the retry mechanism.The addition of
Noneas therequest_idparameter is correct for this context since these are automated retries of failed payments rather than direct user requests.However, there are some opportunities to improve the retry mechanism:
Consider implementing the following enhancements:
- Add exponential backoff between retries to prevent overwhelming the system
- Track and log the
request_id(when present) through the retry attempts for better traceability- Consider using a proper job queue system for more reliable retry handling
Example implementation of exponential backoff:
let backoff = std::time::Duration::from_secs( 2_u64.pow(payment_failed.payment_attempts as u32) * interval ); tokio::time::sleep(backoff).await;src/app/release.rs (1)
Line range hint
283-353: Consider refactoring duplicated range order handling logicThere's significant code duplication in the range order handling logic, particularly around event creation and publishing. Consider extracting this into a separate function.
async fn create_and_publish_range_order( new_order: &Order, my_keys: &Keys, pool: &Pool<Sqlite> ) -> Result<()> { // Transform order fields to tags let tags = crate::nip33::order_to_tags(new_order, None); // Create and publish event let event = crate::nip33::new_event( my_keys, "", new_order.id.to_string(), tags, )?; // Update and create order let mut order_to_create = new_order.clone(); order_to_create.event_id = event.id.to_string(); order_to_create.create(pool).await?; // Publish event NOSTR_CLIENT .get() .unwrap() .send_event(event) .await .map(|_s| ()) .map_err(|err| err.to_string())?; Ok(()) }This would simplify the main function and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.toml(1 hunks)src/app.rs(5 hunks)src/app/add_invoice.rs(6 hunks)src/app/admin_add_solver.rs(3 hunks)src/app/admin_cancel.rs(5 hunks)src/app/admin_settle.rs(7 hunks)src/app/admin_take_dispute.rs(5 hunks)src/app/cancel.rs(14 hunks)src/app/dispute.rs(5 hunks)src/app/fiat_sent.rs(5 hunks)src/app/order.rs(5 hunks)src/app/rate_user.rs(4 hunks)src/app/release.rs(12 hunks)src/app/take_buy.rs(5 hunks)src/app/take_sell.rs(7 hunks)src/flow.rs(5 hunks)src/main.rs(3 hunks)src/scheduler.rs(1 hunks)src/util.rs(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (53)
src/app/admin_add_solver.rs (2)
Line range hint 11-17: LGTM: Function signature updated correctly.
The addition of request_id parameter aligns with the PR objectives to improve request tracking.
36-36: Verify request_id usage consistency.
The request_id is now used in both error messages and success notifications. Let's verify this pattern is consistent across the codebase.
Also applies to: 47-47
✅ Verification successful
Based on the shell script results, I can now generate the final response:
request_id usage is consistent across error and success notifications
The codebase shows consistent usage of request_id in both error messages via send_cant_do_msg() and success notifications via Message::new_dispute(). All error paths properly propagate the request_id parameter, maintaining traceability of requests throughout the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent request_id usage in message creation and error handling
# Test 1: Check for send_cant_do_msg usage patterns
echo "Checking send_cant_do_msg usage patterns..."
rg "send_cant_do_msg\(" -A 2
# Test 2: Check for Message::new_dispute usage patterns
echo "Checking Message::new_dispute usage patterns..."
rg "Message::new_dispute\(" -A 2
Length of output: 4397
src/app/take_buy.rs (4)
21-21: LGTM: Request ID parameter addition
The addition of request_id: u64 parameter is well-placed and properly typed.
114-122: LGTM: Clean invoice generation changes
The addition of request_id parameter and the multi-line formatting improve both functionality and readability.
99-105: Verify get_market_amount_and_fee signature update
The addition of request_id parameter looks correct. Let's verify that the function signature was properly updated in its definition.
#!/bin/bash
# Search for the function definition to verify signature
ast-grep --pattern 'fn get_market_amount_and_fee($$$) -> $$$'39-41: Verify consistent error handling pattern
The error handling for maker validation looks correct, but let's verify that this pattern is consistently applied across similar validation checks in the codebase.
✅ Verification successful
Let's analyze the error handling patterns more broadly by looking at all usages of the function and its definition. This will help us verify consistency in error handling across the codebase.
Error handling pattern is consistently applied across the codebase
The error handling pattern using send_cant_do_msg with request_id and Some(order.id) is consistently implemented across all validation checks in the codebase. The function is used uniformly in various scenarios:
- Validation failures in take_buy/take_sell operations
- Order status checks in rate_user, cancel, and dispute operations
- Permission checks in admin operations
- Invoice and fiat transaction validations
All occurrences follow the same pattern of passing the request_id, order ID (when available), and sender information, followed by an early return.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar validation patterns to ensure consistent error handling
rg -U "send_cant_do_msg.*request_id.*Some\(.*\.id\)" --type rust
Length of output: 157
Script:
#!/bin/bash
# Search for all occurrences of send_cant_do_msg to analyze error handling patterns
rg -U "send_cant_do_msg\(" --type rust -A 2
# Also search for the function definition to understand its parameters
ast-grep --pattern 'async fn send_cant_do_msg($$$)'
Length of output: 3787
src/app/order.rs (3)
17-17: LGTM: Request ID parameter addition
The addition of request_id: u64 parameter aligns with the broader changes across the codebase for better request tracking.
49-49: LGTM: Correct usage of request_id parameter
The range validation correctly uses the passed request_id parameter.
118-118: LGTM: Correct request_id forwarding
The request_id is correctly forwarded to the publish_order function.
src/flow.rs (1)
64-64: 🛠️ Refactor suggestion
Extract request_id handling to avoid repetition.
The pattern request_id.unwrap_or(1) is repeated four times. Consider extracting this to a local variable at the start of the function to follow the DRY principle.
pub async fn hold_invoice_paid(hash: &str, request_id: Option<u64>) -> Result<()> {
+ let request_id = request_id.unwrap_or(1);
let pool = crate::db::connect().await?;
// ... rest of the code ...
- send_new_order_msg(request_id.unwrap_or(1), /* other params */)
+ send_new_order_msg(request_id, /* other params */)Let's verify the message sending implementation:
Also applies to: 73-73, 89-89, 99-99
src/app/admin_settle.rs (4)
26-26: LGTM! Function signature updated appropriately.
The addition of request_id: u64 parameter aligns with the PR objectives and follows good Rust practices.
Line range hint 77-82: LGTM! Status check message updated correctly.
The request_id propagation is properly implemented while maintaining the existing status check logic.
63-68: 🛠️ Refactor suggestion
Consider improving error handling for message sending.
The message construction with request_id is correct, but the error handling could be more robust.
Consider this improvement:
- if let Ok(message) = message.as_json() {
- let _ = send_dm(&event.sender, message).await;
+ match message.as_json() {
+ Ok(msg) => {
+ if let Err(e) = send_dm(&event.sender, msg).await {
+ error!("Failed to send cooperative cancel message: {}", e);
+ }
+ }
+ Err(e) => error!("Failed to serialize message: {}", e),
+ }Likely invalid or redundant comment.
Line range hint 37-42: Verify error handling consistency across similar paths.
The request_id propagation looks correct. However, let's ensure this error handling pattern is consistent across the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response:
Error handling for IsNotYourDispute is consistent across admin actions
The error handling pattern for IsNotYourDispute is consistently implemented in both admin_settle.rs and admin_cancel.rs with identical structure:
- Both propagate request_id from input message
- Both include order_id in the response
- Both return early after sending the message
- Both use the same parameters in the same order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns to ensure consistency
rg -A 3 "send_new_order_msg.*Action::IsNotYourDispute"
Length of output: 56
Script:
#!/bin/bash
# Let's try a broader search for IsNotYourDispute usage
rg -A 5 "IsNotYourDispute"
# Also search for send_new_order_msg to understand error handling patterns
rg -A 5 "send_new_order_msg"
# Search for the Action enum definition to understand all error cases
ast-grep --pattern 'enum Action {
$$$
}'
Length of output: 21162
src/main.rs (1)
84-86: Consider enhancing request_id handling for held invoices
While passing None for held invoices is functionally correct, consider implementing proper request tracking for resubscribed invoices to maintain consistency with the new tracking system.
Let's verify the request_id handling implementation:
As mentioned in the PR description, consider using Rust's Arc<AtomicU32> for thread-safe request ID management, especially for these resubscription scenarios.
src/app/admin_cancel.rs (2)
25-25: LGTM: Function signature update is appropriate.
The addition of request_id: u64 parameter aligns with the PR objectives and follows good practices for parameter placement.
Line range hint 76-81: LGTM: Status check message handling is correct.
The request_id propagation in the status check message is properly implemented.
src/app/take_sell.rs (4)
22-22: LGTM: Request ID parameter addition
The addition of request_id: u64 parameter aligns with the PR's objective of improving request tracking.
97-97: LGTM: Proper request ID propagation
Correctly propagates the request_id from the message for status-related notifications.
143-143: LGTM: Proper error handling in invoice status update
The error handling and request_id propagation in the invoice status update is well implemented.
40-40: Verify error message handling
The request_id propagation looks correct. However, let's verify that the error message is properly handled across the system.
✅ Verification successful
The search results show consistent usage of send_cant_do_msg across the codebase. The function is called with the correct parameters in all instances:
request_idis always passed as the first parameterorder_idis properly wrapped inSome()when available, orNonewhen not applicable- The sender is always passed as the last parameter
Let's verify the implementation of the function itself to ensure it handles these parameters correctly:
Let's try a different approach to find the implementation using ripgrep:
Let's check the Message::cant_do implementation to complete our verification:
Error message handling is properly implemented
The verification shows that:
send_cant_do_msgis consistently used across all error cases with proper request_id propagation- The function correctly constructs error messages using
Message::cant_dowith the request_id and order_id - Error messages are properly delivered via DM to the intended recipient
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all instances of send_cant_do_msg to ensure consistent request_id usage
rg -A 2 "send_cant_do_msg"
Length of output: 5722
Script:
#!/bin/bash
# Get the implementation of send_cant_do_msg
ast-grep --pattern 'pub async fn send_cant_do_msg($$$)'
Length of output: 57
Script:
#!/bin/bash
# Get the implementation of send_cant_do_msg with more context
rg -A 10 "^pub async fn send_cant_do_msg"
Length of output: 510
Script:
#!/bin/bash
# Find the Message::cant_do implementation
rg -A 5 "cant_do.*request_id.*order_id"
Length of output: 730
src/app/admin_take_dispute.rs (2)
44-46: LGTM: Function signature updated appropriately.
The addition of request_id: u64 parameter aligns with the PR's objective of improving request tracking.
75-75: LGTM: Correct usage of request_id parameter.
The request_id parameter is properly used in the send_cant_do_msg call.
src/app/rate_user.rs (2)
Line range hint 134-146: Verify reputation calculation edge cases.
The reputation calculation logic handles complex scenarios, but please verify:
- Potential integer overflow in
total_reviews - Precision loss in floating-point calculations
- Race conditions when updating reputation concurrently
Consider adding guards for these cases:
let old_rating = reputation.total_rating;
let last_rating = reputation.last_rating;
+ // Guard against overflow
+ if reputation.total_reviews == u32::MAX {
+ return Err(Error::msg("Maximum review count reached"));
+ }
let new_rating =
old_rating + (last_rating as f64 - old_rating) / (reputation.total_reviews as f64);54-54: LGTM! Verify request_id propagation.
The addition of request_id parameter is well-placed and properly typed. This change aligns with the PR's objective to improve request tracking.
Let's verify the request_id propagation in caller functions:
✅ Verification successful
Request ID propagation is properly implemented
The verification confirms that:
- The
request_idparameter is correctly added to theupdate_user_reputation_actionfunction - The caller in
src/app.rsproperly passes therequest_idextracted from the message to the function - The
request_idis used appropriately in error handling and response messages within the function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to update_user_reputation_action to ensure request_id is properly passed
ast-grep --pattern 'update_user_reputation_action($$$request_id$$$)'
Length of output: 70
Script:
#!/bin/bash
# Let's try a broader search for the function name first
rg "update_user_reputation_action" -A 2 -B 2
Length of output: 914
Script:
#!/bin/bash
# Let's check the function signature and its call site in more detail
rg -U "update_user_reputation_action\(.*\)" --multiline
Length of output: 57
Script:
#!/bin/bash
# Let's examine the function definition and its usage with more context
cat src/app/rate_user.rs
Length of output: 5644
Script:
#!/bin/bash
# Let's check the caller in app.rs to see how request_id is passed
cat src/app.rs
Length of output: 10744
src/app/dispute.rs (3)
23-25: LGTM: Request ID parameter addition
The addition of request_id parameter aligns with the PR objectives to improve request tracking.
89-89: LGTM: Proper request ID propagation in error handling
The request ID is correctly propagated to error messages, maintaining consistent request tracking.
Line range hint 160-185: LGTM: Event publishing implementation
The event publishing logic with appropriate tags and dispute status is well-implemented.
src/app/add_invoice.rs (2)
20-21: LGTM: Function signature update is appropriate.
The addition of request_id: u64 parameter aligns with the PR's objective of improving request tracking.
57-59: LGTM: Error handling properly includes request tracking.
The authentication failure is properly communicated back to the sender with the request_id for correlation.
src/app/cancel.rs (2)
Line range hint 1-309: Code is functionally sound with room for improvements
The implementation is correct but could benefit from the following improvements:
- Consistent usage of the
request_idparameter - Reduced code duplication in message sending
- Enhanced error handling for Lightning Network operations
- Transaction support for database operations
These changes would improve maintainability and reliability without affecting the core functionality.
Line range hint 109-127: Enhance error handling for Lightning Network operations
The Lightning Network operations (e.g., cancel_hold_invoice) should have more robust error handling to ensure proper state transitions and user notification in case of failures.
Let's verify the error handling patterns in Lightning Network operations:
Consider wrapping the Lightning Network operation in a more detailed error handler:
if let Some(hash) = &order.hash {
match ln_client.cancel_hold_invoice(hash).await {
Ok(_) => {
info!("Cooperative cancel: Order Id {}: Funds returned to seller", &order.id);
}
Err(e) => {
error!("Failed to cancel hold invoice for order {}: {}", &order.id, e);
send_cant_do_msg(request_id, Some(order_id), Some("Failed to cancel hold invoice"), &event.sender).await;
return Ok(());
}
}
}src/scheduler.rs (1)
106-109: Verify error handling in do_payment.
The error from do_payment is logged but the payment attempt count might not be updated in case of certain errors.
Let's verify the error handling in the do_payment implementation:
✅ Verification successful
The payment attempt count is properly handled through check_failure_retries
The verification shows that the payment attempt count is correctly managed:
- When a payment fails,
check_failure_retriesis called which handles the attempt counting - For first failure, it sets
failed_payment = trueand initializespayment_attempts = 0 - For subsequent failures, it increments
payment_attemptsif belowretries_number - The error from
do_paymentis properly logged and doesn't affect the attempt counting logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in do_payment implementation
ast-grep --pattern 'fn do_payment($params) {
$$$
Err($e) => {
$$$
}
$$$
}'
Length of output: 88
Script:
#!/bin/bash
# Let's try a different pattern to find do_payment function and its error handling
rg "async fn do_payment" -A 20
Length of output: 1402
Script:
#!/bin/bash
# Let's find the check_failure_retries function implementation to understand how payment attempts are handled
rg "async fn check_failure_retries" -A 15
Length of output: 905
src/app/release.rs (2)
Line range hint 62-156: LGTM! Consistent request_id propagation
The changes properly propagate the request_id parameter to all message sending functions and maintain consistency in error handling paths.
Line range hint 246-371: LGTM! Consistent request_id usage
The request_id parameter is properly used across all message sending functions and the rate_counterpart call.
src/util.rs (18)
164-164: 'request_id' parameter added to 'publish_order' function appropriately
The addition of the request_id parameter enhances request tracking within the publish_order function and is implemented correctly.
225-225: Passing request_id to send_new_order_msg ensures proper message correlation
Including request_id in the call to send_new_order_msg allows for better tracking and correlation of the NewOrder action messages.
356-356: request_id parameter added to show_hold_invoice function appropriately
The inclusion of the request_id parameter in show_hold_invoice enhances request tracking and is correctly added to the function signature.
Line range hint 393-401: Passing request_id to send_new_order_msg for Action::PayInvoice
Including request_id when sending the PayInvoice action ensures that messages are properly correlated with the original request.
406-411: Passing request_id to notify buyer with Action::WaitingSellerToPay
The use of request_id in the message to the buyer enhances tracking of the waiting status for the seller to pay.
414-414: Passing request_id to invoice_subscribe improves tracking
Including Some(request_id) in the call to invoice_subscribe allows for better identification and handling of invoice-related events.
Line range hint 420-424: Addition of request_id parameter to invoice_subscribe enhances event tracking
The request_id parameter, added as an Option<u64>, improves the ability to track and manage invoice subscriptions.
440-440: Passing request_id to hold_invoice_paid improves flow handling
By including request_id, you enhance the tracking and handling of the invoice payment flow within hold_invoice_paid.
479-483: request_id parameter added to set_waiting_invoice_status function appropriately
The addition of request_id to set_waiting_invoice_status ensures that subsequent messages are correlated with the correct request.
510-510: Passing request_id to send_new_order_msg for Action::AddInvoice
Including request_id enhances message correlation when requesting the buyer to add an invoice.
526-526: request_id parameter added to rate_counterpart function appropriately
Adding request_id improves the tracking of rating actions between counterparts.
530-539: Sending rating requests with request_id to buyer and seller
Passing request_id in the send_new_order_msg calls ensures that both the buyer and seller can correlate the rating request with the original order.
552-552: request_id parameter added to settle_seller_hold_invoice function appropriately
The inclusion of request_id enhances request tracking during the settlement of the seller's hold invoice.
556-556: Consistent use of send_cant_do_msg with request_id for error responses
Using request_id in send_cant_do_msg ensures that error messages are properly correlated with the original request when the seller is not authorized or when there is no preimage.
Also applies to: 565-565
579-582: request_id parameter added to send_cant_do_msg function appropriately
The addition of request_id improves message correlation for situations where an action cannot be performed.
588-588: Creating cant_do message with request_id ensures proper tracking
Including request_id in the Message::cant_do constructor allows for accurate linkage between the error message and the initial request.
595-598: request_id parameter added to send_new_order_msg function appropriately
The inclusion of request_id enhances message tracking and correlation in the send_new_order_msg function.
602-602: Creating new_order message with request_id for accurate tracking
Including request_id in the Message::new_order constructor ensures that the messages are properly correlated with their original requests.
src/util.rs
Outdated
| if NOSTR_CLIENT.get().unwrap().send_event(event).await.is_err() { | ||
| tracing::warn!("order id : {} is expired", order_updated.id) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors when sending events more explicitly
Currently, if send_event returns an error, a warning is logged. Consider handling the error more explicitly, such as implementing retries or detailed error logging, to ensure proper recovery and improve robustness.
| @@ -461,6 +466,7 @@ | |||
| fiat_amount: i64, | |||
| fiat_code: &str, | |||
| premium: i64, | |||
| _request_id: u64, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused parameter _request_id in get_market_amount_and_fee
The parameter _request_id is unused in this function. If it's intended for future use or for signature consistency, consider documenting its purpose. Otherwise, it can be removed to clean up the code.
src/app/add_invoice.rs
Outdated
| send_new_order_msg( | ||
| msg.get_inner_message_kind().request_id, | ||
| Some(order.id), | ||
| Action::IncorrectInvoiceAmount, | ||
| None, | ||
| &event.sender, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Hi @arkanoider if you made this change please solve this conversation
src/app/add_invoice.rs
Outdated
| show_hold_invoice( | ||
| my_keys, | ||
| None, | ||
| &buyer_pubkey, | ||
| &seller_pubkey, | ||
| order, | ||
| msg.get_inner_message_kind().request_id, | ||
| ) | ||
| .await?; |
|
Yes! @grunch I will look tonite, I will read back rabbit suggestion and in case will remove some |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/app/add_invoice.rs(6 hunks)src/app/admin_take_dispute.rs(5 hunks)src/app/cancel.rs(14 hunks)src/app/fiat_sent.rs(5 hunks)src/app/order.rs(5 hunks)src/app/take_buy.rs(5 hunks)src/app/take_sell.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/add_invoice.rs
- src/app/admin_take_dispute.rs
- src/app/cancel.rs
- src/app/fiat_sent.rs
- src/app/take_buy.rs
- src/app/take_sell.rs
🔇 Additional comments (5)
src/app/order.rs (5)
17-17: LGTM: Function signature updated to include request tracking.
The addition of the request_id parameter enhances request tracking capabilities.
30-30: LGTM: Correctly using request_id parameter.
49-65: LGTM: Clean implementation of range order validation.
The code follows good practices with:
- Clear validation logic
- Early returns for error cases
- Consistent use of request_id
85-92: LGTM: Proper validation of negative amounts.
117-117: LGTM: Properly propagating request_id to publish_order.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/app/admin_take_dispute.rs (2)
Line range hint
108-115: Consider improving error handling for get_keys().The
unwrap()call onget_keys()could lead to a panic. Consider propagating the error using the?operator for more robust error handling.- let sender_keys = crate::util::get_keys().unwrap(); + let sender_keys = crate::util::get_keys()?;
Error handling improvement needed for
get_keys()callsThe verification reveals:
- Multiple instances of
get_keys().unwrap()across the codebase, including in the dispute handling code.- Other parts of the codebase (like
scheduler.rs) properly handleget_keys()errors usingmatch.- There's already logging for dispute events (
info!("Dispute event to be published: {event:#?}")) but no specific logging for message delivery.Suggested changes:
- Replace all
get_keys().unwrap()calls with proper error handling using?operator ormatch- Add message delivery logging as suggested
Example locations needing fixes:
src/app/admin_take_dispute.rssrc/app/admin_settle.rssrc/app/admin_cancel.rssrc/app/admin_add_solver.rs🔗 Analysis chain
Line range hint
120-134: Improve error handling and verify message delivery.Two suggestions:
- Replace
unwrap()with proper error handling.- Consider adding logging for successful message delivery to both parties.
Apply this change:
- let sender_keys = crate::util::get_keys().unwrap(); + let sender_keys = crate::util::get_keys()?; send_dm(&buyer_pubkey, sender_keys.clone(), msg_to_buyer.as_json()?).await?; + info!("Sent dispute notification to buyer: {}", buyer_pubkey); send_dm(&seller_pubkey, sender_keys, msg_to_seller.as_json()?).await?; + info!("Sent dispute notification to seller: {}", seller_pubkey);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify logging patterns for message delivery rg -A 2 "send_dm.*buyer_pubkey|send_dm.*seller_pubkey" | grep -B 2 "info!"Length of output: 76
Script:
#!/bin/bash # Let's first check if send_dm is used in the codebase and its context rg -A 5 "send_dm\(" # Also check for get_keys usage and implementation rg -A 3 "get_keys\(\)" # Check for existing logging patterns around dispute handling rg -A 3 "AdminTookDispute"Length of output: 10615
src/app.rs (1)
Line range hint
90-207: Consider future improvements for request_id synchronizationWhile the current implementation is functional, consider these future improvements:
- Use an
Arc<AtomicU64>for thread-safe request_id handling- Implement a dedicated RequestTracker struct to encapsulate request_id management
- Consider using a proper request context that can carry request_id along with other metadata
This would help:
- Improve thread safety
- Reduce parameter passing
- Make the code more maintainable
Example structure:
use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; struct RequestTracker { current_id: Arc<AtomicU64>, } impl RequestTracker { fn new() -> Self { Self { current_id: Arc::new(AtomicU64::new(0)), } } fn track_request(&self) -> u64 { self.current_id.fetch_add(1, Ordering::SeqCst) } }src/util.rs (4)
Line range hint
226-232: Improve error handling for event publishing.The event publishing at line 226 silently ignores errors. Consider logging the error or notifying the user when event publishing fails.
- NOSTR_CLIENT - .get() - .unwrap() - .send_event(event) - .await - .map(|_s| ()) - .map_err(|err| err.into()) + if let Some(client) = NOSTR_CLIENT.get() { + if let Err(err) = client.send_event(event).await { + tracing::error!("Failed to publish order event: {}", err); + return Err(err.into()); + } + } else { + return Err(anyhow::anyhow!("Nostr client not initialized")); + } + Ok(())
Line range hint
428-468: Consider improving the invoice subscription implementation.A few suggestions for improvement:
- The channel size (100) should be configurable through settings.
- Error handling in the subscription loop could be more robust.
- let (tx, mut rx) = channel(100); + let channel_size = Settings::get_mostro().invoice_channel_size; + let (tx, mut rx) = channel(channel_size); // In the subscription loop - if let Err(e) = flow::hold_invoice_paid(&hash, request_id).await { - info!("Invoice flow error {e}"); + if let Err(e) = flow::hold_invoice_paid(&hash, request_id).await { + error!("Failed to process paid invoice {}: {}", hash, e); + // Consider implementing retry logic or error recovery }
Line range hint
560-575: Enhance error messages for better debugging.Consider providing more descriptive error messages to aid in troubleshooting.
- return Err(Error::msg("Not allowed")); + return Err(Error::msg(format!( + "Unauthorized: Only seller {} can settle invoice", + order.seller_pubkey.as_ref().unwrap() + ))); - return Err(Error::msg("No preimage")); + return Err(Error::msg(format!( + "Missing preimage for order {}", + order.id + )));
Line range hint
587-615: Improve error handling in message sending utilities.Consider logging errors when message sending fails and possibly implementing retry logic for important messages.
if let Ok(message) = message.as_json() { let sender_keys = crate::util::get_keys().unwrap(); - let _ = send_dm(destination_key, sender_keys, message).await; + if let Err(err) = send_dm(destination_key, sender_keys, message).await { + tracing::error!( + "Failed to send message to {}: {}", + destination_key, + err + ); + // Consider implementing retry logic for critical messages + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/app.rs(5 hunks)src/app/admin_add_solver.rs(3 hunks)src/app/admin_cancel.rs(5 hunks)src/app/admin_settle.rs(7 hunks)src/app/admin_take_dispute.rs(5 hunks)src/app/dispute.rs(5 hunks)src/main.rs(3 hunks)src/scheduler.rs(1 hunks)src/util.rs(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/admin_add_solver.rs
- src/app/admin_cancel.rs
- src/app/admin_settle.rs
- src/app/dispute.rs
- src/main.rs
- src/scheduler.rs
🧰 Additional context used
📓 Learnings (1)
src/app.rs (2)
Learnt from: arkanoider
PR: MostroP2P/mostro#376
File: src/app.rs:82-83
Timestamp: 2024-11-07T09:44:16.945Z
Learning: In `src/app.rs`, the `request_id` field is an `Option`, ensuring safety against invalid or missing request IDs without the need for additional validation.
Learnt from: arkanoider
PR: MostroP2P/mostro#376
File: src/app/release.rs:47-54
Timestamp: 2024-11-07T09:38:52.196Z
Learning: In the codebase, `request_id` is now an `Option<u64>` and can be passed directly to functions without unwrapping, assuming the functions accept an `Option<u64>`.
🔇 Additional comments (8)
src/app/admin_take_dispute.rs (3)
44-44: LGTM: Function signature updated appropriately.
The addition of request_id: u64 parameter aligns with the PR objectives and follows good practices for parameter placement.
58-65: LGTM: Consistent request_id usage.
The implementation correctly uses the function parameter for request_id tracking.
74-74: LGTM: Error path correctly handles request_id.
The error handling path maintains request tracking consistency.
src/app.rs (2)
85-86: LGTM: Safe request_id extraction
The extraction of request_id as an Option type ensures safe handling of missing or invalid request IDs.
Line range hint 90-207: LGTM: Consistent request_id implementation across all actions
The implementation maintains consistency by:
- Adding request_id as the last parameter to all action functions
- Preserving existing error handling patterns
- Following a uniform approach across different action types
src/util.rs (3)
Line range hint 364-422: LGTM! Hold invoice handling is well-implemented.
The function properly handles invoice creation, updates order status, and sends notifications to both buyer and seller with appropriate request tracking.
Line range hint 477-484: Skip comment: Unused parameter issue already noted.
Line range hint 487-547: LGTM! Status updates and messaging are well-implemented.
The functions properly handle order status updates and notifications with appropriate request tracking.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/util.rs (3)
Line range hint
226-231: Add error handling for message sendingThe
send_new_order_msgcall should be handled more robustly. If message sending fails, it could leave the system in an inconsistent state where the order is created but the acknowledgment isn't sent.- send_new_order_msg( - request_id, - Some(order_id), - Action::NewOrder, - Some(Content::Order(order)), - &ack_pubkey, - ) - .await; + if let Err(e) = send_new_order_msg( + request_id, + Some(order_id), + Action::NewOrder, + Some(Content::Order(order)), + &ack_pubkey, + ) + .await + { + error!("Failed to send new order message: {}", e); + // Consider rolling back the order creation or retrying + }
Line range hint
604-615: Improve error handling in message sending functionsThe function silently ignores errors from
send_dm. This could lead to undetected communication failures.-pub async fn send_new_order_msg( +pub async fn send_new_order_msg( request_id: u64, order_id: Option<Uuid>, action: Action, content: Option<Content>, destination_key: &PublicKey, -) { +) -> Result<()> { // Send message to event creator let message = Message::new_order(request_id, order_id, action, content); - if let Ok(message) = message.as_json() { - let sender_keys = crate::util::get_keys().unwrap(); - let _ = send_dm(destination_key, sender_keys, message).await; - } + let message = message.as_json()?; + let sender_keys = crate::util::get_keys()?; + send_dm(destination_key, sender_keys, message).await }
744-747: Add more test cases for message handlingThe test only covers a basic scenario. Consider adding test cases for:
- Invalid request IDs
- Error conditions in message creation
- Edge cases in amount validation
#[tokio::test] async fn test_get_fiat_amount_requested_invalid_amount() { initialize(); let uuid = uuid!("308e1272-d5f4-47e6-bd97-3504baea9c23"); let order = Order { amount: 1000, min_amount: Some(500), max_amount: Some(2000), ..Default::default() }; let message = Message::Order(MessageKind::new( 1, Some(uuid), Action::TakeSell, Some(Content::Amount(3000)), // Amount exceeds max_amount )); let amount = get_fiat_amount_requested(&order, &message); assert_eq!(amount, None); }
| if let Err(e) = flow::hold_invoice_paid(&hash, request_id).await { | ||
| info!("Invoice flow error {e}"); | ||
| } else { | ||
| info!("Invoice with hash {hash} accepted!"); |
There was a problem hiding this comment.
Handle potential race condition in invoice subscription
The invoice state handling is susceptible to race conditions. Multiple concurrent subscriptions could lead to duplicate processing of the same invoice state.
Consider using a synchronization mechanism:
+ use std::sync::atomic::{AtomicBool, Ordering};
+ use std::sync::Arc;
+
+ let processing = Arc::new(AtomicBool::new(false));
if msg.state == InvoiceState::Accepted {
+ if !processing.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed).is_ok() {
+ return;
+ }
if let Err(e) = flow::hold_invoice_paid(&hash, request_id).await {
info!("Invoice flow error {e}");
} else {
info!("Invoice with hash {hash} accepted!");
}
+ processing.store(false, Ordering::Release);
}Committable suggestion skipped: line range outside the PR's diff.
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!
@grunch @bilthon
not the best implementation, in case we want to do a cleaner job i think and use it in mostrod in main trunk a proper sync variable like an
AtomicRust is the right way.Basically I get the
request_idwhen message comes in and propagate down to the code, a bit spaghetti now, but I should have cover all the request. I used in some case to simplify my life an option ( scheduler or main resubscribe invoice cases ).Take a look!
Will try to test now or tomorrow...
Summary by CodeRabbit
Release Notes
New Features
request_idparameter across multiple functions, improving message handling and traceability.request_id.Bug Fixes
Documentation
request_idfield.These changes collectively enhance the clarity and management of requests within the application, ensuring a more robust user experience.