Refactoring #431
Refactoring #431
Conversation
WalkthroughThis pull request standardizes error handling and messaging across the codebase while refactoring many control flows. In Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
|
Hi @arkanoider cACK, I like it, let's go further with this implementation 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
src/app/release.rs (1)
213-246: Review Outcome – Potential Issue in do_payment HandlingThe investigation confirms that in two locations (specifically, in src/app/release.rs and src/app/admin_settle.rs), the result of the asynchronous
do_paymentcall is discarded by assigning it to_. In contrast, src/scheduler.rs properly checks and logs errors fromdo_payment. This inconsistency could obscure unexpected payment errors, especially if partial failures are critical to workflow integrity.
- src/app/release.rs:
do_payment(order, request_id).awaitis assigned to_, which discards its outcome.- src/app/admin_settle.rs: Similarly,
do_payment(order_updated, request_id).awaitis ignored.- src/scheduler.rs: The function's error is explicitly checked and logged if the call fails.
Please review whether ignoring the outcome in the first two instances is intended or if further error handling/logging should be implemented.
🔗 Analysis chain
Consider verifying do_payment’s outcome.
do_payment(order, request_id).awaitis called and its result is assigned to_. If partial failures in payment logic are critical to your workflow, ensure that unexpected errors are handled or logged appropriately. Otherwise, ignoring the result might obscure possible failures.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Script to identify any place where `do_payment` is called # but its result is discarded in the codebase. rg 'do_payment\(.*\)' -A 2Length of output: 746
src/lightning/mod.rs (2)
149-167: Standardize error mapping incancel_hold_invoice.The error handling pattern in this function is inconsistent with the rest of the file. Line 161 maps the error to a string directly, while other functions use
MostroInternalErrdirectly.Apply this diff to standardize the error handling:
let cancel = self .client .invoices() .cancel_invoice(cancel_message) .await - .map_err(|e| e.to_string()); + .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string()))); match cancel { Ok(cancel) => Ok(cancel.into_inner()), - Err(e) => Err(MostroInternalErr(ServiceError::LnNodeError(e.to_string()))), + Err(e) => Err(e), }
169-262: Improve error handling consistency insend_payment.The function uses different error types (
LnNodeErrorandLnPaymentError) inconsistently. Consider standardizing to useLnPaymentErrorfor all payment-related errors.Apply this diff to standardize the error handling:
listener .clone() .send(msg) .await - .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))? + .map_err(|e| MostroInternalErr(ServiceError::LnPaymentError(e.to_string())))?
🧹 Nitpick comments (32)
src/app/admin_add_solver.rs (3)
5-9: Remove wildcard import
Usinguse mostro_core::error::{MostroError, ServiceError};might be more explicit and avoids polluting the namespace with unused items.
45-47: Consider logging underlying error
When converting from bech32, capturing the actual parsing error could aid debugging.- .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?; + .map_err(|e| MostroInternalErr(ServiceError::InvalidPubkey(e.to_string())))?;
55-57: Preserve JSON serialization cause
Currently returning a genericMessageSerializationError—consider inserting the underlying serialization error for better troubleshooting.src/app/take_buy.rs (3)
5-9: Unified error handling
AdoptingMostroError::{self, *}and removing usage ofanyhow::Resultfosters consistency. However, consider avoiding wildcard imports for clarity.
46-46: Wrong amount error
MostroInternalErr(ServiceError::WrongAmountError)may be better replaced with a user-facing error if the user provided an invalid amount rather than an internal configuration error.
60-62: map_err overshadowing
Mapping errors fromget_buyer_pubkey()toMostroInternalErralone might obscure important debug details.src/app/order.rs (2)
4-8: Wildcard import caution
While including all variants ofMostroErrorcan be convenient, consider enumerating them for clarity and to avoid accidental collisions.
92-93: Don't lose original error
.map_err(|_| MostroError::MostroInternalErr(ServiceError::InvalidOrderId))?;discards the original error context. Includingecould simplify debugging..map_err(|e| MostroError::MostroInternalErr( ServiceError::InvalidOrderId(format!("Invalid order: {}", e)) ))?;src/app/take_sell.rs (1)
17-39:update_order_statusfunction
Centralizing the status update logic fosters improved readability and reusability. However, consider handling potentialunwrap()on buyer_pubkey (line 24) more gracefully to avoid panics if an order is malformed.Here’s an optional diff to ensure robust error handling:
- let buyer_pubkey = order.get_buyer_pubkey().unwrap(); + let buyer_pubkey = match order.get_buyer_pubkey() { + Ok(pk) => pk, + Err(cause) => return Err(MostroCantDo(cause)), + };src/app/admin_settle.rs (1)
49-60: Conditional check on cooperative cancellation.
ReturningMostroCantDo(IsNotYourDispute)if the order is not cooperatively canceled might be confusing. Consider using a more specific reason, such asCantDoReason::InvalidOrderStatus, for better clarity.-if order.check_status(Status::CooperativelyCanceled).is_err() { - return Err(MostroCantDo( - CantDoReason::IsNotYourDispute, - )); +if order.check_status(Status::CooperativelyCanceled).is_err() { + return Err(MostroCantDo(CantDoReason::InvalidOrderStatus)); }src/app/admin_cancel.rs (1)
49-60: Cooperative cancellation validation.
Similar toadmin_settle.rs, returningMostroCantDo(cause)for mismatched status is valid, though consider ifcauseshould be more descriptive (e.g.,InvalidOrderStatus).src/app/rate_user.rs (1)
17-65: New helper functionprepare_variables_for_vote.
Creating a dedicated function to manage buyer/seller identification and error handling is a great step towards modularity. Consider returning a small struct instead of a tuple for enhanced readability and maintainability.-pub fn prepare_variables_for_vote(...) -> Result<(String, String, bool, bool), MostroError> { +#[derive(Debug)] +pub struct VoteVariables { + pub counterpart: String, + pub counterpart_trade_pubkey: String, + pub buyer_rating: bool, + pub seller_rating: bool, +} + +pub fn prepare_variables_for_vote(...) -> Result<VoteVariables, MostroError> { ... - Ok((counterpart, counterpart_trade_pubkey, buyer_rating, seller_rating)) + Ok(VoteVariables { + counterpart, + counterpart_trade_pubkey, + buyer_rating, + seller_rating, + }) }src/app/admin_take_dispute.rs (2)
114-116: Preserve the underlying error message when serialization fails.
You're currently discarding the original error value by usingmap_err(|_| ...). Retaining the original error details can help with debugging.- .map_err(|_| MostroInternalErr(ServiceError::MessageSerializationError))?; + .map_err(|err| MostroInternalErr(ServiceError::MessageSerializationError( + err.to_string() + )))?;
152-160: Consolidate repeated DM-sending logic.
The logic for sending direct messages to buyer and seller is almost duplicated. A helper function or shared method could reduce duplication and simplify future maintenance.Also applies to: 162-172
src/app/dispute.rs (1)
53-53: Retain the original error context when creating dispute events.
You're discarding the details by usingmap_err(|_| ...); consider preserving the underlying error for easier debugging.- .map_err(|_| MostroInternalErr(ServiceError::DisputeEventError))?; + .map_err(|err| MostroInternalErr(ServiceError::DisputeEventError( + err.to_string() + )))?;src/app.rs (2)
293-293: Consider handling the result ofcheck_trade_index.
You're discarding the possible error fromcheck_trade_indexby assigning it to_. This may obscure user feedback or diagnostics.- let _ = check_trade_index(&pool, &event, &message).await; + if let Err(err) = check_trade_index(&pool, &event, &message).await { + tracing::warn!("Trade index check error: {:?}", err); + // Optionally handle or escalate the error here + }
324-325: Complete thetodo!()block for error handling.
Atodo!()indicates an unimplemented code path. Use an appropriate error variant or implement the necessary logic.Would you like help implementing this logic or returning a specialized error?
src/app/release.rs (1)
82-88: Avoid repeated status checks.
These two blocks check the same set of statuses (Active, FiatSent, Dispute). Consider consolidating them into a single helper function or removing the duplication if only one check is needed here.- // Check if order is active, fiat sent or dispute - if order.check_status(Status::Active).is_err() && order.check_status(Status::FiatSent).is_err() - && order.check_status(Status::Dispute).is_err() - { - return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); - } ... (some lines later) ... - // Check if order is active, fiat sent or dispute - if order.check_status(Status::Active).is_err() && order.check_status(Status::FiatSent).is_err() - && order.check_status(Status::Dispute).is_err() - { - return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); - } + // Consider merging these checks into a single function or call: + fn ensure_valid_release_status(order: &Order) -> Result<(), MostroError> { + if order.check_status(Status::Active).is_err() + && order.check_status(Status::FiatSent).is_err() + && order.check_status(Status::Dispute).is_err() + { + return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); + } + Ok(()) + } + ensure_valid_release_status(&order)?;Also applies to: 97-102
src/app/cancel.rs (3)
70-185: Complex cancellation logic could be simplified.
The code handlingWaitingPaymentvsWaitingBuyerInvoicewith maker/taker checks is sprawling. Consider extracting these checks into small helper functions (e.g.,is_cancellable_by_maker,is_cancellable_by_taker). This would reduce nesting, improve testability, and better convey intent.
282-360: Unit tests recommended.
cancel_add_invoiceadds important logic around returning seller funds and reverting the order state. Currently, there's no direct test coverage for it. Automated tests would help ensure correctness, especially for edge cases (repeated calls, invalid pubkeys, etc.).Do you want me to provide a sample unit test you can adapt for verifying edge cases?
362-444: Similar flows incancel_pay_hold_invoice.
Likecancel_add_invoice, thecancel_pay_hold_invoicefunction has repeated logic for resetting the order. Consider extracting a shared helper to handle the revert-to-pending scenario.src/util.rs (4)
109-120: Clear error handling for Yadio request.
ReturningMostroInternalErr(ServiceError::NoAPIResponse)orNoCurrencyorMalformedAPIResneatly indicates the root cause. To reduce branching, consider combining repeated checks into a single match if the logic is invariably linear.
124-131: Preserve JSON parsing errors for debugging.
When callingq.json::<Yadio>(), you discard the original error detail. Including it in theMessageSerializationErroror logging it could greatly help debugging malformed responses.-.map_err(|_| MostroError::MostroInternalErr(ServiceError::MessageSerializationError))? +.map_err(|e| { + tracing::error!("Failed parsing Yadio response: {e}"); + MostroError::MostroInternalErr(ServiceError::MessageSerializationError) +})?
761-774: Helpfulget_orderfunction.
This function neatly wraps DB lookups in a type-safe manner. It might be beneficial to mention which upstream code is responsible for verifying user ownership or permissions to access the returned order.
776-798: Efficient invoice validation withvalidate_invoice.
This is a nice modular approach. One minor suggestion: log invalid invoice details (at least partially or hashed) for troubleshooting, if not considered sensitive.src/app/trade_pubkey.rs (3)
12-16: Consider adding documentation for the function parameters.While the function signature is clear, adding documentation for
msg,event, andpoolparameters would improve code maintainability.+/// Handles the trading of public keys between buyers and sellers +/// +/// # Arguments +/// * `msg` - The message containing the request details +/// * `event` - The unwrapped gift event containing sender information +/// * `pool` - The database connection pool +/// +/// # Returns +/// * `Result<(), MostroError>` - Success or a MostroError pub async fn trade_pubkey_action( msg: Message, event: &UnwrappedGift, pool: &Pool<Sqlite>, ) -> Result<(), MostroError> {
27-38: Consider using early returns for better readability.The nested match pattern could be simplified using early returns, making the code more maintainable.
- match ( - order.master_buyer_pubkey.as_ref(), - order.master_seller_pubkey.as_ref(), - ) { - (Some(master_buyer_pubkey), _) if master_buyer_pubkey == &event.sender.to_string() => { - order.buyer_pubkey = Some(event.rumor.pubkey.to_string()); - } - (_, Some(master_seller_pubkey)) if master_seller_pubkey == &event.sender.to_string() => { - order.seller_pubkey = Some(event.rumor.pubkey.to_string()); - } - _ => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), - }; + if let Some(master_buyer_pubkey) = order.master_buyer_pubkey.as_ref() { + if master_buyer_pubkey == &event.sender.to_string() { + order.buyer_pubkey = Some(event.rumor.pubkey.to_string()); + return Ok(()); + } + } + + if let Some(master_seller_pubkey) = order.master_seller_pubkey.as_ref() { + if master_seller_pubkey == &event.sender.to_string() { + order.seller_pubkey = Some(event.rumor.pubkey.to_string()); + return Ok(()); + } + } + + return Err(MostroInternalErr(ServiceError::InvalidPubkey));
52-55: Consider adding error context for better debugging.The database error could include more context about what operation failed.
order .update(pool) .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError( + format!("Failed to update order {}: {}", order.id, e) + )))?;src/lnurl.rs (1)
20-34: Consider adding error context for better debugging.While the error handling is correct, adding context about which validation failed would help with debugging.
let (user, domain) = match address.split_once('@') { Some((user, domain)) => (user, domain), - None => return Err(MostroInternalErr(ServiceError::LnAddressParseError)), + None => return Err(MostroInternalErr(ServiceError::LnAddressParseError( + format!("Invalid lightning address format: {}", address) + ))), };src/flow.rs (1)
90-98: Consider grouping related messages for better maintainability.The buyer and seller messages could be grouped into a helper function for better code organization.
+async fn send_order_messages( + request_id: Option<u64>, + order_id: i64, + buyer_action: Action, + seller_action: Action, + order_data: Option<Payload>, + buyer_pubkey: PublicKey, + seller_pubkey: PublicKey, +) { + enqueue_order_msg( + request_id, + Some(order_id), + buyer_action, + order_data, + buyer_pubkey, + None, + ).await; + + enqueue_order_msg( + request_id, + Some(order_id), + seller_action, + None, + seller_pubkey, + None, + ).await; +}Also applies to: 101-109
src/lightning/invoice.rs (1)
11-17: Consider adding specific error messages for better debugging.While the error handling is correct, adding context about why the invoice is invalid would help with debugging.
let invoice = Bolt11Invoice::from_str(payment_request) - .map_err(|_| MostroInternalErr(ServiceError::InvoiceInvalidError))?; + .map_err(|e| MostroInternalErr(ServiceError::InvoiceInvalidError( + format!("Failed to parse invoice: {}", e) + )))?;src/main.rs (1)
37-45: Consider adding documentation for the message queue system.The new message queue system is a critical component for handling various types of messages. Please add documentation to explain:
- The purpose and lifecycle of each queue type
- The thread-safety guarantees provided by the implementation
- The expected message flow and processing patterns
+/// A thread-safe container for managing different types of message queues in the system. +/// +/// # Fields +/// * `queue_order_msg` - Queue for regular order-related messages +/// * `queue_order_cantdo` - Queue for order rejection/failure messages +/// * `queue_order_rate` - Queue for order rating events #[derive(Debug, Clone, Default)] pub struct MessageQueues { pub queue_order_msg: Arc<Mutex<Vec<(Message, PublicKey)>>>, pub queue_order_cantdo: Arc<Mutex<Vec<(Message, PublicKey)>>>, pub queue_order_rate: Arc<Mutex<Vec<Event>>>, } +/// Global instance of message queues, lazily initialized and protected by a read-write lock. static MESSAGE_QUEUES: LazyLock<RwLock<MessageQueues>> = LazyLock::new(|| RwLock::new(MessageQueues::default()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.toml(1 hunks)src/app.rs(7 hunks)src/app/add_invoice.rs(1 hunks)src/app/admin_add_solver.rs(3 hunks)src/app/admin_cancel.rs(5 hunks)src/app/admin_settle.rs(4 hunks)src/app/admin_take_dispute.rs(6 hunks)src/app/cancel.rs(5 hunks)src/app/dispute.rs(6 hunks)src/app/fiat_sent.rs(1 hunks)src/app/order.rs(2 hunks)src/app/rate_user.rs(8 hunks)src/app/release.rs(10 hunks)src/app/take_buy.rs(1 hunks)src/app/take_sell.rs(1 hunks)src/app/trade_pubkey.rs(1 hunks)src/db.rs(1 hunks)src/error.rs(0 hunks)src/flow.rs(3 hunks)src/lightning/invoice.rs(6 hunks)src/lightning/mod.rs(12 hunks)src/lnurl.rs(1 hunks)src/main.rs(3 hunks)src/scheduler.rs(4 hunks)src/util.rs(17 hunks)
💤 Files with no reviewable changes (1)
- src/error.rs
✅ Files skipped from review due to trivial changes (1)
- src/db.rs
🔇 Additional comments (122)
src/app/admin_add_solver.rs (4)
2-2: No concerns
New usage ofsend_dmis consistent with your approach to direct messaging.
21-21: Consistent error type
ReturningResult<(), MostroError>aligns with your new error handling approach across the codebase.
42-42: Confirm error type
You're returning aMostroInternalErrfor an invalid pubkey. Consider whether this scenario is purely internal or if a user-friendly error (MostroCantDo) might be more appropriate.
60-62: Good approach
Usingmap_errwithMostroInternalErrinsend_dmensures unified error handling for potential Nostr-specific failures.src/app/take_buy.rs (8)
2-2: Helpful utility usage
Adding references toget_fiat_amount_requested,get_market_amount_and_fee,get_order, andshow_hold_invoicealigns well with your refactoring efforts.
20-20: Harmonized return type
ReturningResult<(), MostroError>fits the new approach of explicit error handling.
23-23: Verifyget_orderusage
get_order(&msg, pool).await?might fail if the order doesn't exist or is invalid. Confirm that scenarios where the order is missing are handled elsewhere or tested comprehensively.
28-34: Properly blocking invalid orders
UsingMostroCantDo(cause)if the order isn't a buy or hasn't pending status is a straightforward approach that informs the user appropriately.
37-40: Security measure
Ensuring the event is from the original maker prevents unauthorized manipulation of the order. Good practice.
49-57: Market amount fallback
Falling back toget_market_amount_and_fee()for a zero-amount order is sensible, but confirm that invalid or unavailable exchange rates are surfaced to the user appropriately.
68-69: Timestamping
order.set_timestamp_now();ensures the take action is recorded—this is a good approach for order tracking.
71-85: Consistent user feedback
Allshow_hold_invoicefailures become internal errors. Verify if any scenario could be more informative to end users, for instance, a "can't do" message if invoice creation fails due to user input.src/app/order.rs (7)
2-2: Centralized invoice validation
Importingvalidate_invoicealong with other related utilities consolidates invoice handling logic neatly.
9-10: Expanded core references
ImportingMessage,Order, andSmallOrderindicates enhanced utility for multiple order operations in this module.
55-55: Unified error type
-> Result<(), MostroError>improves consistency across the system, making error handling more predictable.
60-61: Direct invoice validation
validate_invoice(&msg, &Order::from(order.clone())).await?is a neat approach; ensure partial or malformed invoices yield clear user feedback.
67-69: Range order limits
UsingMostroCantDoifcheck_range_order_limitsfails gives the user explicit feedback on invalid range requests.
71-73: Zero amount with premium
Rejecting an order that sets an impossible premium with zero amount is sensible.
78-78: Quote validation
Invokingcalculate_and_check_quotefor each fiat amount ensures robust verification of multi-amount orders.src/app/fiat_sent.rs (13)
1-9: Imports look consistent with new error handling and queue utilities.
They align well with the refactoring goals of centralizing error definitions inmostro_core::errorand usingenqueue_order_msgfor message dispatch.
15-15: Doc comment addition is clear.
The new comment clarifies this function's purpose without affecting logic.
21-23: ReturningResult<(), MostroError>is consistent with the project's new error strategy.
This signature change follows the refactoring scope: any function that may fail should return aMostroError.
25-26: Status validation is appropriate.
ReturningMostroCantDofor invalid statuses ensures consistent error semantics.
31-32: Buyer identity check.
Verifyingorder.get_buyer_pubkey()matchesevent.rumor.pubkeyprevents mismatched ownership. Good approach.
35-39: Careful handling of next trade key.
Graceful fallback toNoneor returningMostroInternalErrhelps avoid undefined states.
44-46: Order and event update integration.
Mapping errors to an internal error type makes the flow consistent with the newMostroErrorscheme.
48-51: Database update error handling is well-defined.
Usingmap_errensures that anysqlxfailure is translated into aMostroInternalErr.
54-54: Seller pubkey retrieval is correct.
Mapping any retrieval error directly toMostroInternalErris consistent.
60-65: Enqueuing seller message.
This centralized queuing approach fits the PR's goal of simplifying message dispatch.
72-77: Buyer notification logic.
Similar to the seller, properly enqueued with minimal duplication.
84-96: Synchronized next trade handling.
Updatingnext_trade_pubkeyandnext_trade_indexonly after verifying thesent_from_makercondition is a solid approach.
98-99: Final return.
Function cleanly concludes withOk(()), maintaining consistent error-handling patterns.src/app/add_invoice.rs (18)
1-3: New import references for invoice validation.
Bringingvalidate_invoiceinto scope aligns with the refactor that centralizes invoice checks.
5-5: Use ofanyhow::Resultreplaced byMostroErrorbelow.
Retaininganyhow::Resultfor local convenience is fine, given final errors map toMostroError.
7-11: Consolidated error and order modules.
Imports group relevant modules (MostroCantDo,ServiceError,Status, etc.) for the new approach.
17-45:check_order_statusfunction fosters clarity and modularity.
It encapsulates status checks and messages, reducing duplication inadd_invoice_action. Good reusability.
52-54: ReturningResult<(), MostroError>inadd_invoice_action.
Consistent with the application-wide change to unify error handling types.
56-58: Validating order status.
Mapping the status fetch failure to an internal error ensures uniform error propagation.
60-63: Validating order kind is essential.
ReturningMostroInternalErrenforces consistent error semantics.
65-66: Buyer public key resolution.
Gracefully propagates any data retrieval failure throughMostroInternalErr.
69-70: Restricting add-invoice action to buyer only.
Rejecting mismatched pubkeys asMostroCantDois correct for user-facing error messages.
73-74:validate_invoiceusage.
Offers a neat encapsulation for invoice correctness checks, now integrated with the main logic.
76-77: Delegating status check tocheck_order_status.
Enhances readability by separating status logic from main invoice flow.
78-79: Seller pubkey retrieval.
Fetching the counterpart's pubkey with error mapping is consistent with the new pattern.
83-84: Creating a smallerSmallOrderstruct.
This smaller variant is beneficial for message dispatch, reducing data overhead.
86-88: Replacing the order event.
Uses the same approach seen in other modules to keep order data in sync with on-chain/off-chain changes.
90-100: Enqueuing message for seller.
Refactor fromsend_new_order_msgtoenqueue_order_msgunifies messaging in the codebase.
101-110: Enqueuing message for buyer.
Mirrors the seller logic, ensuring both parties remain informed at each stage.
110-119: Fallback toshow_hold_invoicefor missing preimage.
Properly extends the same error handling strategy, returningMostroInternalErron failure.
124-125: Conclusion ofadd_invoice_action.
The function ends cleanly withOk(()), fulfilling the new error pattern.src/app/take_sell.rs (14)
2-3: New utility imports.
Includingvalidate_invoiceandget_fiat_amount_requestedcentralizes order data retrieval and invoice checks.
6-11: Errors consolidated underMostroErrorandServiceErrorplus order definitions.
This import strategy is consistent with the shift to typed error returns.
41-46: Updated signature to returnResult<(), MostroError>.
Aligns with the new approach for typed error handling within the application.
48-49: Order retrieval.
Pulling order from the database viaget_orderis consistent with the rest of the refactor.
53-60: Ensuring it's asellorder and inPendingstatus.
ReturningMostroCantDofor user-facing rejections preserves uniform error semantics.
62-65: Validates the maker identity.
Requiring that theevent.rumor.pubkeymatch ensures only the original maker can finalize the order.
67-67: Fetching seller pubkey.
Maps potential errors toMostroInternalErr, consistent with the approach in other modules.
70-70: Invoice validation viavalidate_invoice.
Streamlines the logic by delegating invoice checks to a single utility function.
77-77: ReturningMostroInternalErrfor a wrong amount.
Ensures the function terminates gracefully when the user-submitted or derived fiat amount is invalid.
81-81: Populating the buyer’s pubkey.
Ensures the order data remains consistent with the actual user taking the sell offer.
87-87: Timestamp update.
Captures the time at which the buyer interacts with the order. Fits the new logic for tracking.
90-98: Market-based amount and fee checks.
This is a convenient approach to handle unfilledamountthrough external logic inget_market_amount_and_fee.
100-106: Branch when no payment request is present.
Callsupdate_order_statusto setWaitingBuyerInvoice, simplifying the flow for late invoice provisioning.
109-123: Showing a hold invoice if payment request exists.
This fallback suitably handles the invoice interaction, with consistent error mapping toMostroInternalErr.src/app/admin_settle.rs (16)
5-5: Imports updated to align with refactored helpers.
The inclusion ofenqueue_order_msg,get_nostr_client,get_order,settle_seller_hold_invoice, andupdate_order_eventin one place is consistent with the PR’s goal of centralizing logic and improving readability.
9-10: Explicit error imports for clarity.
Using theMostroErrorvariants andServiceErrorfosters more expressive error handling and keeps error definitions centralized.
28-28: Refined return type for improved error reporting.
Changing the function signature to returnResult<(), MostroError>instead of a genericResult<()>adds clarity and consistency.
31-32: Centralized order retrieval.
Switching toget_orderis a clean approach, ensuring uniform access to order data and simplifying error management.
34-38: Solver assignment verification.
This block neatly checks whether the user handling the dispute is indeed the assigned solver. Good use ofMostroCantDo(IsNotYourDispute)to clarify unauthorized access attempts.
41-43: Database access errors are properly wrapped.
Converting the database error intoMostroInternalErr(ServiceError::DbAccessError)clarifies the nature of DB failures and simplifies error propagation.
65-67: Dispute status enforcement.
Ensuring the order is inDisputestate before proceeding helps avoid unintended flows. This is a solid guard clause.
68-71: Seller hold invoice settlement.
Callingsettle_seller_hold_invoicewith a specializedAction::AdminSettledaccurately reflects the administrative override. Good usage of the new error type.
72-75: Consistent event update after invoice settlement.
Leveragingupdate_order_eventwithStatus::SettledHoldInvoicemaintains uniform status transitions and clear event history.
78-78: Potential silent failure when fetching dispute.
Iffind_dispute_by_order_idfails, the code silently ignores that error. Consider logging or explicitly handling the error to maintain clear visibility into DB issues.
84-86: Dispute update with robust error handling.
Wrapping DB update errors underMostroInternalErr(ServiceError::DbAccessError(...))ensures clarity when diagnosing dispute-related failures.
104-105: Constructing Nostr event with meaningful error checks.
Wrapping potential Nostr errors intoMostroInternalErr(ServiceError::NostrError(...))is a good approach to unify error reporting.
118-128: Message dispatch to the event creator.
Centralizing calls toenqueue_order_msgaligns with the PR objective of unifying message handling logic.
130-142: Seller notification viaenqueue_order_msg.
Parsing theseller_pubkeyand mapping any parse error toInvalidPubkeyis straightforward and consistent with other error flows.
143-155: Buyer notification mirrored to seller flow.
Reusing the same pattern for the buyer ensures consistency, avoids code duplication, and cleanly handles pubkey parsing.
158-159: Ignoring the result ofdo_payment.
Storingdo_payment(...)into_swallows any potential errors. If payments occasionally fail, consider handling or logging the outcome for greater transparency.src/app/admin_cancel.rs (14)
7-7: Extended utility imports for centralized message handling.
Integratingenqueue_order_msg,get_order, andupdate_order_eventsupports the PR’s aim of unifying helper calls.
10-14: Expanded error variants for precise problem reporting.
IntroducingCantDoReasonand distinctMostroErrortypes (e.g.,MostroCantDo,MostroInternalErr) clarifies error origin and severity.
29-29: Method signature clarified withResult<(), MostroError>.
Propagating domain-specific error types elevates code maintainability and debugging clarity.
33-33: Consistent order retrieval.
get_ordersystematically fetches the order, matching the approach across the codebase.
35-37: Immediate solver assignment check.
ReturningMostroCantDo(CantDoReason::IsNotYourDispute)quickly halts unauthorized actions, promoting security.
40-43: Proper DB error wrapping.
UsingMostroInternalErr(ServiceError::DbAccessError(e.to_string()))ensures that database issues are uniformly handled.
63-65: Dispute check ensures correct workflow.
Restricting actions for a disputed order helps avoid conflicting states.
76-76: Potential silent error for dispute retrieval.
If the retrieval fails, theOk(...)pattern is not triggered, leaving an error unchecked. Consider adding logs for debugging.
82-84: Dispute update with robust DB error conversion.
This maintains consistency with the rest of the system’s error management.
101-103: Creating a new Nostr event for dispute status.
Wrapping potential Nostr errors inMostroInternalErr(ServiceError::NostrError(...))is a clear, consistent approach.
116-122: Order event update.
Updating both the event in Nostr and the local DB withCanceledByAdminthoroughly reflects the final state.
127-128: Trade index usage.
Ensuring the correcttrade_indexis passed along preserves traceability for front-end recipients or logs.
144-150: Seller/buyer pubkey parsing.
ReturningMostroInternalErr(ServiceError::InvalidPubkey)on parse failure is more explicit than older generic errors, improving debug speed.
152-157: Direct messaging the parties.
Usingsend_dmfor admin, seller, and buyer not only improves clarity but also aligns with the new unified error approach.src/app/rate_user.rs (15)
1-1: Unified helpers for rating logic.
Pulling inenqueue_order_msgandget_orderharmonizes the approach to message dispatch and data retrieval.
5-5: Transition away from genericanyhow::Result.
Thoughanyhow::Resultremains in scope, shifting toMostroErrorin function signatures fosters domain-centric error handling.
6-8: Expanded error coverage for rating process.
ImportingMostroError,CantDoReason, andServiceErrorensures the code can differentiate between user-facing and internal failures.
67-98: Refined user reputation fetching.
Switching to-> Result<Option<Rating>, MostroError>allows consistent error handling across external calls (e.g., Nostr). Sorting events by creation time ensures the latest rating is returned.
105-105: Specific error type for rating updates.
UtilizingResult<(), MostroError>clarifies that rating failures are part of a well-defined error domain.
106-107: Single call for uniform order retrieval.
get_order(&msg, pool)usage furthers the goal of consistent data access and error reporting.
109-111: Status check ensures the order succeeded.
Rejecting attempts to rate an incomplete or invalid order withInvalidOrderStatushelps avoid inconsistent data.
114-116: Variable extraction for rating.
prepare_variables_for_voteis invoked here, providing a clean separation of logic.
131-135: Safe retrieval of user-supplied rating.
Theget_rating()function call is gracefully wrapped withmap_err(MostroInternalErr), ensuring rating data is validated.
138-141: Database check for user presence.
is_user_presentplus descriptive error mapping ensures that an invalid or non-existent user flow is handled predictably.
143-143: Adjusting rating on the found user record.
user_to_vote.update_rating(new_rating)elegantly modifies user stats in memory before persisting.
153-154: Rating tagging logic.
to_tags()method and associated error handling unify the rating data structure with Nostr’s event-based model.
168-171: Detailed DB update error context.
Embedding the specific DB error string inServiceError::DbAccessErroris beneficial for debugging.
181-191: Precise error handling when updating rating event.
Wrapping potential DB issues underMostroInternalErr(ServiceError::DbAccessError(...))maintains a consistent error approach across the codebase.
194-202: Final rating acknowledgment message.
PostingRateReceivedpromptly notifies the user that the rating was successfully processed. Including thePayload::RatingUser(new_rating)is a clean way to convey relevant data.src/app/admin_take_dispute.rs (1)
3-3: Imports and updated function signature look good!
No major issues identified with the new imports, updated function signature, or error definitions.Also applies to: 6-10, 47-47
src/app/dispute.rs (1)
10-11: Overall error handling refactor looks consistent.
No major issues identified in these changed lines. The new error types and usage appear correct.Also applies to: 12-18, 19-19, 69-69, 74-74, 84-88, 93-93, 101-101, 103-103, 105-109, 129-129, 136-136, 139-139, 150-152, 156-160, 165-166, 169-172, 177-178, 182-188, 193-198, 200-207, 210-212
src/app.rs (1)
18-19: General improvements are correct.
No major issues found in these line changes. The new module references and refined error handling match the broader refactoring goals.Also applies to: 33-33, 39-39, 44-48, 53-53, 70-82, 97-99, 107-112, 131-132, 159-166, 170-178, 182-185, 189-190, 193-196, 197-201, 202-203, 205-207, 307-323
src/app/release.rs (1)
24-28: Good introduction ofcheck_failure_retries.
This function decouples the retry logic from the main payment flow, improving maintainability. Consider adding a short doc example that clarifies how retry counts are persisted and how clients should interpret partial retry attempts.src/app/cancel.rs (1)
35-39: Properly preventing redundant cancellations.
ReturningMostroCantDo(CantDoReason::OrderAlreadyCanceled)is clear and straightforward. This prevents confusion over canceled orders being canceled again.src/util.rs (2)
178-201: Validate partial failures inpublish_order.
If database insertion succeeds (lines 196–201) but publishing the Nostr event fails, you could end up with inconsistent states. Consider implementing a rollback or additional logging to handle partial failures.
353-385: Event queue approach for rating updates looks neat.
Offloading Nostr event emission viaMESSAGE_QUEUESis a clean pattern. Ensure concurrency on rating updates is tested, especially if multiple rating requests can happen simultaneously.src/app/trade_pubkey.rs (1)
1-11: LGTM! Clean and well-organized imports.The imports are logically grouped and necessary for the functionality.
src/lnurl.rs (1)
8-8: LGTM! Clean error type imports.The error type imports are well-organized and follow the new error handling strategy.
src/flow.rs (1)
63-71: LGTM! Consistent message handling.The transition to
enqueue_order_msgimproves message handling consistency.Also applies to: 73-81
src/lightning/invoice.rs (1)
96-105: LGTM! Well-structured test case.The test case is clear and properly validates the error handling.
Cargo.toml (2)
54-55: Review the use of local path dependency.Using a local path for
mostro-corein production could cause issues. This should be temporary for development/testing only.Please confirm:
- Is this patch intended for development only?
- Should it be removed before deploying to production?
- Are there plans to publish the local changes to crates.io?
41-47: Verify the impact of downgrading mostro-core.The downgrade from version 0.6.25 to 0.6.23 could affect the error handling changes. Also, the
openssldependency has been commented out.Run the following script to check for breaking changes:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/fiat_sent.rs (1)
81-96: Consider extracting the next trade fields update logic.The next trade fields update logic could be moved to a separate function for better maintainability.
+async fn update_next_trade_fields( + order: &mut Order, + next_trade: Option<(String, u32)>, + pool: &Pool<Sqlite>, +) -> Result<(), MostroError> { + if let Some((pubkey, index)) = next_trade { + order.next_trade_pubkey = Some(pubkey); + order.next_trade_index = Some(index as i64); + order.update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + } + Ok(()) +}src/app/release.rs (2)
212-284: Consider extracting payment status handling logic.The payment status handling logic in
do_paymentis complex and could be extracted into separate functions for better readability and maintainability.+async fn handle_payment_status( + status: PaymentStatus, + order: &mut Order, + msg: PaymentUpdate, + buyer_pubkey: PublicKey, + my_keys: &Keys, + request_id: Option<u64>, +) -> Result<()> { + match status { + PaymentStatus::Succeeded => { + info!( + "Order Id {}: Invoice with hash: {} paid!", + order.id, msg.payment.payment_hash + ); + payment_success(order, buyer_pubkey, my_keys, request_id).await?; + } + PaymentStatus::Failed => { + info!( + "Order Id {}: Invoice with hash: {} has failed!", + order.id, msg.payment.payment_hash + ); + if let Ok(failed_payment) = check_failure_retries(order, request_id).await { + info!( + "Order id {} has {} failed payments retries", + failed_payment.id, failed_payment.payment_attempts + ); + } + } + _ => {} + } + Ok(()) +}
436-442: Consider consolidating duplicate message sending logic.The message sending logic in
notify_invalid_amountis duplicated for buyer and seller. Consider consolidating it into a single function.+async fn send_invalid_amount_message( + request_id: Option<u64>, + order_id: uuid::Uuid, + pubkey: PublicKey, +) { + enqueue_cant_do_msg( + request_id, + Some(order_id), + CantDoReason::InvalidAmount, + pubkey, + ) + .await; +}Also applies to: 444-450
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/fiat_sent.rs(1 hunks)src/app/release.rs(10 hunks)
🔇 Additional comments (4)
src/app/fiat_sent.rs (4)
1-7: LGTM! Clean imports organization.The imports are well-organized, with clear separation between internal and external dependencies.
15-21: LGTM! Function signature updated for better error handling.The return type change from
Result<()>toResult<(), MostroError>improves error specificity.
35-39: LGTM! Improved error handling for next trade key.The error handling is now more specific with
map_err(MostroInternalErr).
25-33: Verify buyer's public key validation.The code checks if the pubkey matches the buyer, but consider adding logging before returning the error to help with debugging.
if order.get_buyer_pubkey().ok() != Some(event.rumor.pubkey) { + tracing::debug!("Invalid pubkey: expected {:?}, got {:?}", order.get_buyer_pubkey().ok(), event.rumor.pubkey); return Err(MostroCantDo(CantDoReason::InvalidPubkey)); }✅ Verification successful
I'll start by checking if there is an existing logging statement for the buyer's public key validation in the file. This will help us determine whether the suggested logging should be added. For example, we can search for any usage of
tracing::debugwithinsrc/app/fiat_sent.rs.
Action: Add debug logging for buyer's public key validation
The search confirms that no debug logging is present in the buyer pubkey validation block, so incorporating a
tracing::debug!call—as suggested—is indeed a valuable improvement for debugging.
- File: src/app/fiat_sent.rs (lines 25-33) lacks a logging statement before returning the error for an invalid pubkey.
- Suggestion: Add a logging statement to capture the expected pubkey and the supplied value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
Script:
#!/bin/bash rg "tracing::debug" src/app/fiat_sent.rs -n .Length of output: 45
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/app/release.rs (1)
243-272: Enhance payment status handling.The payment status handling could be more robust:
- Consider handling intermediate states like
InFlight- Add timeout handling for stuck payments
- Log payment amounts for better debugging
if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { match status { PaymentStatus::Succeeded => { + info!( + "Order Id {}: Invoice with hash: {} paid! Amount: {}", + order.id, msg.payment.payment_hash, msg.payment.value + ); let _ = payment_success(&mut order, buyer_pubkey, &my_keys, request_id).await; } PaymentStatus::Failed => { info!( - "Order Id {}: Invoice with hash: {} has failed!", + "Order Id {}: Invoice with hash: {} has failed! Amount: {}", - order.id, msg.payment.payment_hash + order.id, msg.payment.payment_hash, msg.payment.value ); // Mark payment as failed if let Ok(failed_payment) = check_failure_retries(&order, request_id).await { info!( "Order id {} has {} failed payments retries", failed_payment.id, failed_payment.payment_attempts ); } } + PaymentStatus::InFlight => { + info!( + "Order Id {}: Payment in flight. Hash: {}, Amount: {}", + order.id, msg.payment.payment_hash, msg.payment.value + ); + } _ => {} } }
🧹 Nitpick comments (2)
src/app/release.rs (2)
32-34: Consider using the?operator for cleaner error handling.The error mapping can be simplified using the
?operator with a type conversion implementation.- let pool = db::connect() - .await - .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; + let pool = db::connect().await.map_err(|e| e.into())?;
175-188: Consider using an enum for trade roles.The repeated checks against creator_pubkey could be replaced with an enum to improve type safety and readability.
enum TradeRole { Seller, Buyer, } impl Order { fn get_trade_role(&self) -> TradeRole { if &self.creator_pubkey == self.seller_pubkey.as_ref().unwrap() { TradeRole::Seller } else { TradeRole::Buyer } } }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/order.rs (1)
16-48:⚠️ Potential issueFix potential floating-point precision issues in quote calculation.
The quote calculation uses floating-point arithmetic and casts to
i64, which could lead to precision loss in financial calculations.Consider using fixed-point decimal arithmetic or a decimal library like
rust_decimal. Here's a suggested fix:-let quote = *fiat_amount as f64 / price; - (quote * 1E8) as i64 +use rust_decimal::Decimal; +let fiat_decimal = Decimal::from(*fiat_amount); +let price_decimal = Decimal::from_f64(price).ok_or(MostroInternalErr(ServiceError::InvalidAmount))?; +let quote = (fiat_decimal / price_decimal * Decimal::from(100_000_000)).round().to_i64() + .ok_or(MostroInternalErr(ServiceError::InvalidAmount))?;src/app/take_sell.rs (1)
18-40:⚠️ Potential issueAdd synchronization to prevent race conditions in order status updates.
While the function nicely encapsulates order status update logic, it lacks synchronization mechanisms to prevent race conditions during concurrent order updates.
Consider implementing one of these solutions:
- Use database transactions:
async fn update_order_status( order: &mut Order, my_keys: &Keys, pool: &Pool<Sqlite>, request_id: Option<u64>, ) -> Result<(), MostroError> { + let mut tx = pool.begin().await.map_err(|_| MostroInternalErr(ServiceError::DatabaseError))?; // Get buyer pubkey let buyer_pubkey = order.get_buyer_pubkey().unwrap(); // Set order status to waiting buyer invoice match set_waiting_invoice_status(order, buyer_pubkey, request_id).await { Ok(_) => { // Update order status match update_order_event(my_keys, Status::WaitingBuyerInvoice, order).await { Ok(order_updated) => { - let _ = order_updated.update(pool).await; + let _ = order_updated.update(&mut *tx).await; + tx.commit().await.map_err(|_| MostroInternalErr(ServiceError::DatabaseError))?; Ok(()) } Err(_) => Err(MostroInternalErr(ServiceError::UpdateOrderStatusError)), } } Err(_) => Err(MostroInternalErr(ServiceError::UpdateOrderStatusError)), } }
- Use an application-level lock:
use tokio::sync::Mutex; use std::collections::HashMap; use std::sync::Arc; lazy_static! { static ref ORDER_LOCKS: Arc<Mutex<HashMap<String, ()>>> = Arc::new(Mutex::new(HashMap::new())); } async fn update_order_status( order: &mut Order, my_keys: &Keys, pool: &Pool<Sqlite>, request_id: Option<u64>, ) -> Result<(), MostroError> { let mut locks = ORDER_LOCKS.lock().await; locks.insert(order.id.clone(), ()); // ... existing code ... locks.remove(&order.id); Ok(()) }
🧹 Nitpick comments (7)
src/app/take_buy.rs (2)
71-79: Consider using structured logging for trade index errors.The error logging could be enhanced with structured fields for better observability.
- tracing::error!("Error updating user trade index: {}", e); + tracing::error!( + error = %e, + user = %event.sender.to_string(), + trade_index = %msg.get_inner_message_kind().trade_index.unwrap(), + "Error updating user trade index" + );
82-95: Enhance error context for hold invoice failures.The error message could provide more context about the failure.
- return Err(MostroInternalErr(ServiceError::HoldInvoiceError( - cause.to_string(), - ))); + return Err(MostroInternalErr(ServiceError::HoldInvoiceError( + format!("Failed to show hold invoice for buyer {}: {}", + buyer_pubkey, cause) + )));src/app.rs (3)
206-206: Implement the PayInvoice action.The
PayInvoiceaction is currently marked astodo!(). This should be implemented to complete the payment functionality.Would you like me to help implement the
PayInvoiceaction handler? I can generate a basic implementation that follows the same error handling pattern as other actions.
337-348: Consider using structured logging for unexpected errors.The error logging for unexpected errors could be enhanced with structured fields.
- tracing::error!("Unexpected error type: {}", e); + tracing::error!( + error = %e, + action = %action, + "Unexpected error while handling message action" + );
143-146: Commented code should be removed.The commented-out code for updating user trade index should be removed since it's no longer needed.
- // if let Err(e) = update_user_trade_index(pool, event.sender.to_string(), index).await - // { - // tracing::error!("Error updating user trade index: {}", e); - // }src/app/order.rs (2)
81-89: Consider propagating trade index update errors.The error from
update_user_trade_indexis only logged but not propagated. This could hide issues that might need attention.Consider propagating the error or adding more context to the error log:
-if let Err(e) = update_user_trade_index( +match update_user_trade_index( pool, event.sender.to_string(), msg.get_inner_message_kind().trade_index.unwrap(), ) .await -{ - tracing::error!("Error updating user trade index: {}", e); +{ + Ok(_) => (), + Err(e) => { + tracing::error!( + "Error updating user trade index for sender {}: {}", + event.sender.to_string(), + e + ); + return Err(MostroInternalErr(ServiceError::DatabaseError)); + } }
91-104: Improve error mapping for publish_order.The current error mapping from
publish_ordertoInvalidOrderIdmight not accurately represent all possible failure cases.Consider mapping specific error cases:
-publish_order( +match publish_order( pool, my_keys, order, event.rumor.pubkey, event.sender, event.rumor.pubkey, request_id, msg.get_inner_message_kind().trade_index, ) .await -.map_err(|_| MostroError::MostroInternalErr(ServiceError::InvalidOrderId))?; +{ + Ok(_) => Ok(()), + Err(e) => { + tracing::error!("Failed to publish order: {}", e); + Err(match e { + PublishError::InvalidId(_) => MostroInternalErr(ServiceError::InvalidOrderId), + PublishError::Database(_) => MostroInternalErr(ServiceError::DatabaseError), + _ => MostroInternalErr(ServiceError::Unknown), + }) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(1 hunks)src/app.rs(6 hunks)src/app/order.rs(2 hunks)src/app/take_buy.rs(1 hunks)src/app/take_sell.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
src/app.rs (1)
Learnt from: arkanoider
PR: MostroP2P/mostro#431
File: src/app.rs:115-118
Timestamp: 2025-02-09T11:32:27.570Z
Learning: In the `check_trade_index` function, errors from `update_user_trade_index` should only be logged and not propagated, as decided by the maintainer. This is to prevent disruption of the main flow for non-critical database updates.
🔇 Additional comments (5)
src/app/order.rs (1)
2-8: LGTM! Improved error handling structure.The introduction of structured error types (
MostroError,CantDoReason,ServiceError) and utility functions improves error handling clarity and maintainability.src/app/take_sell.rs (4)
1-17: LGTM! Imports align with error handling improvements.The imports are well-organized and support the PR's objective of standardizing error handling across the codebase.
42-67: LGTM! Improved error handling in order validation.The validation logic is well-structured with clear error handling using
MostroErrortypes.
68-99: Consider adding validation for negative amounts.While the market amount calculation is handled well, there's no explicit validation to prevent negative amounts.
Add validation before setting the amount:
if order.has_no_amount() { match get_market_amount_and_fee(order.fiat_amount, &order.fiat_code, order.premium).await { Ok(amount_fees) => { + if amount_fees.0 <= 0 { + return Err(MostroInternalErr(ServiceError::WrongAmountError)); + } order.amount = amount_fees.0; order.fee = amount_fees.1 } Err(_) => return Err(MostroInternalErr(ServiceError::WrongAmountError)), }; }
111-126: LGTM! Clear separation of payment request handling paths.The logic for handling payment requests is well-structured with clear branching between presence and absence of payment requests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/app/cancel.rs (3)
84-91:⚠️ Potential issueAdd maker validation to prevent unauthorized cancellations.
According to the PR objectives, only makers should be able to cancel orders. The current implementation needs validation at the beginning of the function.
Add maker validation at the beginning of the function:
pub async fn cancel_order_by_taker( pool: &Pool<Sqlite>, event: &UnwrappedGift, order: &mut Order, my_keys: &Keys, request_id: Option<u64>, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { + // Only maker should be able to cancel orders + if order.sent_from_maker(event.rumor.pubkey).is_ok() { + return Err(MostroCantDo(CantDoReason::IsNotYourOrder)); + }
167-169:⚠️ Potential issueImprove error handling for maker cancellation.
The current implementation silently ignores errors from
update_order_event. This could lead to inconsistent state if the update fails.Apply this diff to properly handle errors:
- if let Ok(order_updated) = update_order_event(my_keys, Status::Canceled, order).await { - let _ = order_updated.update(pool).await; - } + let order_updated = update_order_event(my_keys, Status::Canceled, order) + .await + .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + order_updated + .update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
335-498: 🛠️ Refactor suggestionReduce code duplication between cancel functions.
There's significant code duplication between
cancel_add_invoiceandcancel_pay_hold_invoice. This makes the code harder to maintain and more prone to bugs.Extract common logic into shared helper functions:
// Helper function to handle hold invoice cancellation async fn cancel_hold_invoice_if_exists( ln_client: &mut LndConnector, order: &Order, ) -> Result<(), MostroError> { if let Some(hash) = &order.hash { ln_client .cancel_hold_invoice(hash) .await .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))?; info!("Order Id {}: Funds returned to seller", &order.id); } Ok(()) } // Helper function to get and validate pubkeys fn get_validated_pubkeys( order: &Order, user_pubkey: &str, is_buyer_check: bool, ) -> Result<(PublicKey, String), MostroError> { let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { (Some(seller), Some(buyer)) => ( PublicKey::from_str(seller) .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, buyer.clone(), ), _ => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), }; // Validate if user is buyer or seller based on is_buyer_check if is_buyer_check && &buyer_pubkey != user_pubkey { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } else if !is_buyer_check && seller_pubkey.to_string() != user_pubkey { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } Ok((seller_pubkey, buyer_pubkey)) } // Helper function to handle creator cancellation async fn handle_creator_cancellation( order: &mut Order, my_keys: &Keys, event: &UnwrappedGift, seller_pubkey: PublicKey, status: Status, request_id: Option<u64>, ) -> Result<(), MostroError> { update_order_event(my_keys, status, order) .await .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; // Send cancel messages enqueue_order_msg( request_id, Some(order.id), Action::Canceled, None, event.rumor.pubkey, None, ) .await; enqueue_order_msg( None, Some(order.id), Action::Canceled, None, seller_pubkey, None, ) .await; Ok(()) } // Helper function to handle non-creator cancellation async fn handle_non_creator_cancellation( order: &mut Order, pool: &Pool<Sqlite>, my_keys: &Keys, is_buyer: bool, ) -> Result<(), MostroError> { if order.price_from_api { order.amount = 0; order.fee = 0; } // Edit pubkey based on order type if is_buyer { edit_buyer_pubkey_order(pool, order.id, None) } else { edit_seller_pubkey_order(pool, order.id, None) } .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; update_order_to_initial_state(pool, order.id, order.amount, order.fee) .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; update_order_event(my_keys, Status::Pending, order) .await .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; Ok(()) }Then refactor the main functions to use these helpers:
pub async fn cancel_add_invoice( ln_client: &mut LndConnector, order: &mut Order, event: &UnwrappedGift, pool: &Pool<Sqlite>, my_keys: &Keys, request_id: Option<u64>, ) -> Result<(), MostroError> { cancel_hold_invoice_if_exists(ln_client, order).await?; let user_pubkey = event.rumor.pubkey.to_string(); let (seller_pubkey, buyer_pubkey) = get_validated_pubkeys(order, &user_pubkey, true)?; if &order.creator_pubkey == &buyer_pubkey { handle_creator_cancellation( order, my_keys, event, seller_pubkey, Status::CooperativelyCanceled, request_id, ) .await } else { handle_non_creator_cancellation(order, pool, my_keys, true).await } } pub async fn cancel_pay_hold_invoice( ln_client: &mut LndConnector, order: &mut Order, event: &UnwrappedGift, pool: &Pool<Sqlite>, my_keys: &Keys, request_id: Option<u64>, ) -> Result<(), MostroError> { cancel_hold_invoice_if_exists(ln_client, order).await?; let user_pubkey = event.rumor.pubkey.to_string(); let (seller_pubkey, buyer_pubkey) = get_validated_pubkeys(order, &user_pubkey, false)?; if order.creator_pubkey == seller_pubkey.to_string() { handle_creator_cancellation( order, my_keys, event, seller_pubkey, Status::Canceled, request_id, ) .await } else { handle_non_creator_cancellation(order, pool, my_keys, false).await } }
🧹 Nitpick comments (1)
src/app/cancel.rs (1)
423-432: Simplify nested if conditions.The nested if conditions for checking and canceling the hold invoice can be simplified.
Apply this diff to simplify the logic:
- if order.hash.is_some() { - // We return funds to seller - if let Some(hash) = order.hash.as_ref() { - ln_client - .cancel_hold_invoice(hash) - .await - .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))?; - info!("Order Id {}: Funds returned to seller", &order.id); - } - } + // We return funds to seller + if let Some(hash) = &order.hash { + ln_client + .cancel_hold_invoice(hash) + .await + .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))?; + info!("Order Id {}: Funds returned to seller", &order.id); + }
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/app/cancel.rs (1)
199-333: 🛠️ Refactor suggestionImprove error handling and simplify logic.
The function has silent error handling and complex nested logic that could be simplified.
pub async fn cancel_action( msg: Message, event: &UnwrappedGift, my_keys: &Keys, pool: &Pool<Sqlite>, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { let request_id = msg.get_inner_message_kind().request_id; let mut order = get_order(&msg, pool).await?; // Check if order is already canceled if order.check_status(Status::Canceled).is_ok() || order.check_status(Status::CooperativelyCanceled).is_ok() || order.check_status(Status::CanceledByAdmin).is_ok() { return Err(MostroCantDo(CantDoReason::OrderAlreadyCanceled)); } // Handle pending order cancellation if order.check_status(Status::Pending).is_ok() { order .sent_from_maker(event.rumor.pubkey) .map_err(|_| MostroCantDo(CantDoReason::IsNotYourOrder))?; - if let Ok(order_updated) = update_order_event(my_keys, Status::Canceled, &order).await { - let _ = order_updated.update(pool).await; - } + let order_updated = update_order_event(my_keys, Status::Canceled, &order) + .await + .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + order_updated + .update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; enqueue_order_msg( request_id, Some(order.id), Action::Canceled, None, event.rumor.pubkey, None, ) .await; return Ok(()); } // Get seller and buyer pubkey let seller_pubkey = order.get_seller_pubkey().map_err(MostroInternalErr)?; let buyer_pubkey = order.get_buyer_pubkey().map_err(MostroInternalErr)?; // Handle order cancellation in WaitingPayment or WaitingBuyerInvoice status if order.check_status(Status::WaitingPayment).is_ok() || order.check_status(Status::WaitingBuyerInvoice).is_ok() { let taker_pubkey = match order.creator_pubkey.as_str() { s if s == seller_pubkey.to_string() => buyer_pubkey, b if b == buyer_pubkey.to_string() => seller_pubkey, _ => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), }; if order.sent_from_maker(event.rumor.pubkey).is_ok() { cancel_order_by_maker( pool, event, &mut order, taker_pubkey, my_keys, request_id, ln_client, ) .await?; } else if event.rumor.pubkey == taker_pubkey { cancel_order_by_taker(pool, event, &mut order, my_keys, request_id, ln_client).await?; } else { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } } // Handle cooperative cancellation if order.check_status(Status::Active).is_ok() || order.check_status(Status::FiatSent).is_ok() || order.check_status(Status::Dispute).is_ok() { let counterparty_pubkey = if buyer_pubkey == event.rumor.pubkey { order.buyer_cooperativecancel = true; seller_pubkey.to_string() } else { order.seller_cooperativecancel = true; buyer_pubkey.to_string() }; match order.cancel_initiator_pubkey { Some(_) => { cancel_cooperative_execution( pool, event, request_id, &mut order, counterparty_pubkey, my_keys, ln_client, ) .await?; } None => { order.cancel_initiator_pubkey = Some(event.rumor.pubkey.to_string()); let order = order.update(pool).await?; // Send messages to both parties let counterparty_pubkey = PublicKey::from_str(&counterparty_pubkey)?; for (pubkey, action) in [ (event.rumor.pubkey, Action::CooperativeCancelInitiatedByYou), (counterparty_pubkey, Action::CooperativeCancelInitiatedByPeer), ] { enqueue_order_msg( if pubkey == event.rumor.pubkey { request_id } else { None }, Some(order.id), action, None, pubkey, None, ) .await; } } } } Ok(()) }
♻️ Duplicate comments (2)
src/app/cancel.rs (2)
84-156:⚠️ Potential issueAdd maker validation and simplify pubkey updates.
According to the PR objectives, only makers should be able to cancel orders. Additionally, the pubkey update logic can be simplified.
pub async fn cancel_order_by_taker( pool: &Pool<Sqlite>, event: &UnwrappedGift, order: &mut Order, my_keys: &Keys, request_id: Option<u64>, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { + // Only maker should be able to cancel orders + if order.sent_from_maker(event.rumor.pubkey).is_ok() { + return Err(MostroCantDo(CantDoReason::IsNotYourOrder)); + } if let Some(hash) = &order.hash { ln_client.cancel_hold_invoice(hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } let buyer_pubkey = order.get_buyer_pubkey().map_err(MostroInternalErr)?; //We notify the creator that the order was cancelled only if the taker had already done his part before if (order.is_sell_order().is_ok() && order.check_status(Status::WaitingPayment).is_ok()) || (order.is_buy_order().is_ok() && order.check_status(Status::WaitingBuyerInvoice).is_ok()) { enqueue_order_msg( request_id, Some(order.id), Action::Canceled, None, order.get_creator_pubkey().map_err(MostroInternalErr)?, None, ) .await; if order.price_from_api { order.amount = 0; order.fee = 0; } - if order.is_buy_order().is_ok() { - edit_seller_pubkey_order(pool, order.id, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - edit_master_seller_pubkey_order(pool, order.id, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - } - if order.is_sell_order().is_ok() { - edit_buyer_pubkey_order(pool, order.id, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - edit_master_buyer_pubkey_order(pool, order.id, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - } + // Update pubkeys based on order type + let (edit_pubkey, edit_master_pubkey) = if order.is_buy_order().is_ok() { + (edit_seller_pubkey_order, edit_master_seller_pubkey_order) + } else { + (edit_buyer_pubkey_order, edit_master_buyer_pubkey_order) + }; + edit_pubkey(pool, order.id, None).await?; + edit_master_pubkey(pool, order.id, None).await?; update_order_to_initial_state(pool, order.id, order.amount, order.fee) .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; update_order_event(my_keys, Status::Pending, order) .await .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; info!( "{}: Canceled order Id {} republishing order", buyer_pubkey, order.id ); } enqueue_order_msg( request_id, Some(order.id), Action::Canceled, None, event.rumor.pubkey, None, ) .await; Ok(()) }
158-197:⚠️ Potential issueImprove error handling for maker cancellation.
The current implementation silently ignores errors from
update_order_eventand database updates.pub async fn cancel_order_by_maker( pool: &Pool<Sqlite>, event: &UnwrappedGift, order: &mut Order, taker_pubkey: PublicKey, my_keys: &Keys, request_id: Option<u64>, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { - if let Ok(order_updated) = update_order_event(my_keys, Status::Canceled, order).await { - let _ = order_updated.update(pool).await; - } + let order_updated = update_order_event(my_keys, Status::Canceled, order) + .await + .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + order_updated + .update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; if let Some(hash) = &order.hash { ln_client.cancel_hold_invoice(hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } // Send messages to both parties for (pubkey, req_id) in [(event.rumor.pubkey, request_id), (taker_pubkey, None)] { enqueue_order_msg( req_id, Some(order.id), Action::Canceled, None, pubkey, None, ) .await; } Ok(()) }
🧹 Nitpick comments (1)
src/app/cancel.rs (1)
22-82: Simplify error handling and messaging logic.The error handling and messaging logic can be simplified to improve readability and maintainability.
pub async fn cancel_cooperative_execution( pool: &Pool<Sqlite>, event: &UnwrappedGift, request_id: Option<u64>, order: &mut Order, counterparty_pubkey: String, my_keys: &Keys, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { if let Some(initiator) = &order.cancel_initiator_pubkey { if *initiator == event.rumor.pubkey.to_string() { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } } else { - if let Some(hash) = &order.hash { - ln_client.cancel_hold_invoice(hash).await?; - info!( - "Cooperative cancel: Order Id {}: Funds returned to seller", - &order.id - ); - } - order.status = Status::CooperativelyCanceled.to_string(); - let order = order - .clone() - .update(pool) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - update_order_event(my_keys, Status::CooperativelyCanceled, &order) - .await - .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + // Cancel invoice if exists + if let Some(hash) = &order.hash { + ln_client.cancel_hold_invoice(hash).await?; + info!("Cooperative cancel: Order Id {}: Funds returned to seller", &order.id); + } + + // Update order status + order.status = Status::CooperativelyCanceled.to_string(); + let order = order.clone().update(pool).await?; + update_order_event(my_keys, Status::CooperativelyCanceled, &order).await?; + // Send messages to both parties + let counterparty_pubkey = PublicKey::from_str(&counterparty_pubkey)?; + for pubkey in [event.rumor.pubkey, counterparty_pubkey] { + enqueue_order_msg( + if pubkey == event.rumor.pubkey { request_id } else { None }, + Some(order.id), + Action::CooperativeCancelAccepted, + None, + pubkey, + None, + ).await; + } - enqueue_order_msg( - request_id, - Some(order.id), - Action::CooperativeCancelAccepted, - None, - event.rumor.pubkey, - None, - ) - .await; - let counterparty_pubkey = PublicKey::from_str(&counterparty_pubkey) - .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?; - enqueue_order_msg( - None, - Some(order.id), - Action::CooperativeCancelAccepted, - None, - counterparty_pubkey, - None, - ) - .await; info!("Cancel: Order Id {} canceled cooperatively!", order.id); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/cancel.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/app/cancel.rs (1)
84-91:⚠️ Potential issueAdd validation to prevent taker cancellation.
According to the PR objectives, only makers should be able to cancel orders. Add validation at the beginning of the function.
pub async fn cancel_order_by_taker( pool: &Pool<Sqlite>, event: &UnwrappedGift, order: &mut Order, my_keys: &Keys, request_id: Option<u64>, ln_client: &mut LndConnector, ) -> Result<(), MostroError> { + // Only maker should be able to cancel orders + if order.sent_from_maker(event.rumor.pubkey).is_ok() { + return Err(MostroCantDo(CantDoReason::IsNotYourOrder)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/cancel.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (1)
src/app/cancel.rs (1)
167-172: Fix error handling for maker cancellation.The current implementation silently ignores errors from
update_order_event. This matches a past review comment that's still valid.- if let Ok(order_updated) = update_order_event(my_keys, Status::Canceled, order).await { - order_updated - .update(pool) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - } + let order_updated = update_order_event(my_keys, Status::Canceled, order) + .await + .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + order_updated + .update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
There was a problem hiding this comment.
Cancel still fails, when the taker cancels it should go to Pending status, but it stays in the same status as when it took the order
To reproduce:
A creates a sales order
B takes the order (goes to status waiting-buyer-invoice)
B sends a message to cancel the order
Mostrod sent B a canceled message. The order stays in status waiting-buyer-invoice
It should go to pending status and delete B's data from the database
The same error with a purchase order, when the buyer cancels it remains in a waiting-payment status, it should go to pending
The hold invoice is cancelled, that is correct, what is missing is the change of status and updating of the database
There was a problem hiding this comment.
When a buyer sends an invoice with an incorrect amount, this appears in the Mostro logs
2025-02-12T23:35:19.514655Z WARN mostrod::app: Error in AddInvoice with context Invoice is invalidBut Mostro should also send the buyer an invalid invoice amount message. Mostro is not sending that message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/util.rs (1)
904-907: Improve thesend_dmtest implementation.The current test is a temporary solution that only verifies error cases. Consider implementing a proper test that:
- Mocks the nostr client
- Tests successful message sending
- Verifies the message content
- // Now error is well manager this call will fail now, previously test was ok becuse error was not managed - // now just make it ok and then will make a better test - let result = send_dm(receiver_pubkey, sender_keys, payload, None).await; - assert!(result.is_err()); + // Mock the nostr client + let client = Client::default(); + NOSTR_CLIENT.set(client).unwrap(); + + // Test successful message sending + let result = send_dm(receiver_pubkey, sender_keys, payload.clone(), None).await; + assert!(result.is_ok()); + + // Test error case (invalid payload) + let result = send_dm(receiver_pubkey, sender_keys, "invalid payload".to_string(), None).await; + assert!(result.is_err());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs(24 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (3)
src/util.rs (3)
65-67: Great improvements to error handling!The refactoring enhances error handling by:
- Using more specific error types (
MostroError,ServiceError)- Providing better context for errors
- Maintaining consistent error handling patterns across functions
Also applies to: 108-109, 116-120, 127-129, 189-191, 787-796
709-724: Excellent message handling refactoring!The changes improve message handling by:
- Centralizing message queuing
- Using more descriptive function names
- Consistent pattern for message enqueueing
Also applies to: 726-743
784-821: Well-structured new utility functions!The new
get_orderandvalidate_invoicefunctions:
- Extract common functionality
- Follow consistent error handling patterns
- Improve code reusability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/app/cancel.rs (4)
31-35: 🛠️ Refactor suggestionAdd validation for order status before checking initiator.
The function should validate the order status before checking the initiator to prevent race conditions.
84-158:⚠️ Potential issueFix taker cancellation logic.
According to the PR objectives, there are issues with taker cancellation:
- Orders are incorrectly marked as canceled instead of pending
- Only maker should be able to cancel orders
- Orders should revert to pending if taker has fulfilled their part
170-175:⚠️ Potential issueImprove error handling for maker cancellation.
The function silently ignores errors from
update_order_event.
22-82: 🛠️ Refactor suggestionReduce code duplication between cancel functions.
There's significant code duplication between the cancellation functions.
Also applies to: 84-158, 160-203
src/app.rs (1)
332-343:⚠️ Potential issueImplement the unhandled error case.
The error handling includes a TODO comment for an unhandled case.
🧹 Nitpick comments (2)
src/app/order.rs (1)
91-102: Make error mapping more specific inpublish_order.The current error mapping discards the original error context by mapping any error to
ServiceError::InvalidOrderId.Consider preserving the original error context:
- .map_err(|_| MostroError::MostroInternalErr(ServiceError::InvalidOrderId))?; + .map_err(|e| MostroError::MostroInternalErr(ServiceError::OrderPublishError(e.to_string())))?;This requires adding a new variant to
ServiceError:pub enum ServiceError { // ... existing variants ... OrderPublishError(String), }src/app/take_buy.rs (1)
42-58: Consider adding validation for negative amounts.While the code handles zero amounts and out-of-range amounts, it should also validate for negative amounts to prevent potential issues.
if let Some(am) = get_fiat_amount_requested(&order, &msg) { + if am < 0.0 { + return Err(MostroCantDo(CantDoReason::InvalidAmount)); + } order.fiat_amount = am; } else { return Err(MostroCantDo(CantDoReason::OutOfRangeSatsAmount)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app.rs(6 hunks)src/app/cancel.rs(2 hunks)src/app/order.rs(2 hunks)src/app/take_buy.rs(1 hunks)src/app/take_sell.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/app.rs (1)
Learnt from: arkanoider
PR: MostroP2P/mostro#431
File: src/app.rs:115-118
Timestamp: 2025-02-09T11:32:27.570Z
Learning: In the `check_trade_index` function, errors from `update_user_trade_index` should only be logged and not propagated, as decided by the maintainer. This is to prevent disruption of the main flow for non-critical database updates.
🔇 Additional comments (9)
src/app/order.rs (2)
23-28: Avoid floating-point arithmetic for financial calculations.Using floating-point arithmetic (
f64) and casting toi64can lead to precision issues in financial calculations.Consider using fixed-point decimal arithmetic or a dedicated money-handling library. Example:
- let quote = *fiat_amount as f64 / price; - (quote * 1E8) as i64 + // Using rust_decimal for precise financial calculations + use rust_decimal::Decimal; + let fiat_decimal = Decimal::from(*fiat_amount); + let price_decimal = Decimal::from_f64(price).unwrap(); + let quote_decimal = fiat_decimal / price_decimal; + (quote_decimal * Decimal::from(100_000_000)).to_i64().unwrap()
50-55: LGTM! Improved error handling structure.The function now returns
Result<(), MostroError>, providing better error context and type safety.src/app/take_buy.rs (3)
22-40: LGTM! Improved error handling for order validation.The validation chain is well-structured with clear error propagation using
MostroErrorvariants.
71-78: Add retry logic for trade index updates.The trade index update is critical for order tracking but lacks retry logic.
80-94: Improve error handling for hold invoice generation.The error handling for
show_hold_invoiceis thorough and properly maps toMostroInternalErr.src/app/take_sell.rs (2)
101-108: Add retry logic for trade index updates.The trade index update is critical for order tracking but lacks retry logic.
110-125: Potential race condition in order status updates.The current implementation invokes
update_order_eventwithout any explicit synchronization.src/app.rs (2)
52-77: LGTM! Improved error handling with centralized management.The
manage_errorsfunction effectively centralizes error handling and message dispatch.
102-104: Consider adding TradePubkey to the trading actions list.Since
TradePubkeyis a new action, it should be included in the trading actions check if it requires trade index validation.if !matches!( message_kind.action, - Action::NewOrder | Action::TakeBuy | Action::TakeSell + Action::NewOrder | Action::TakeBuy | Action::TakeSell | Action::TradePubkey ) {
@Catrya you cant test this scenario again, it should be fixed now. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (1)
Cargo.toml (1)
53-53: LGTM!The tokio dev-dependency configuration remains unchanged.
Fixed now |
There was a problem hiding this comment.
Hi @arkanoider cancellation logic works fine now!
There are just a few details left with the messages that mostrod sends
-
When the taker takes an order and he cancels it, it goes into pending (this is fine) but he sends a cancellation message to the maker. That message should not be sent, only to the taker (since it was not the maker's turn to do anything)
-
When the maker cancels the order before the taker completes his part, he does not send the cancellation message to the taker, he should send it to him so that he knows that he is not going to continue the order anymore
There was a problem hiding this comment.
Now the cooperative cancellation does not work, when the counterparty also sends cancellation, mostrod does not respond, does not change the status of the order to canceled (it stays active) and does not send the CooperativeCancelAccepted messages to the 2 users.
Hi @grunch ,
ok starting with a draft to let you review what i am doing and see if we can really get something better in the end.
I will try to describe the big parts i am trying to do:
I DON'T want to touch any part of the logic, there are many changes but only as "cosmetics" modifications.
I will try to describe a bit more the three main points starting from errors:
errors.rsis now insidemostro-coreand now mostro error is an enum with two variants as below:This errors enum can be splitted and updated to have more enum splitting errors in different categories, this was an old idea to have a more handwritten approach with Rust errors. You will see much more
map_erraround the code, but this is the way to do that.Now cantDo message are enqued in main loop, here the part in
app.rswhere the error messages are queued:There is dequeue in
scheduler.rsthat checks every second and in case dequeues messages. This has a nice effect on many part of the code like here intake_sell.rsJust returning the error should automatically enqueue the message in main loop, and basically after max 1s scheduler will clean queue....
Finally compiles, all the work in these days was related to add other helpers in
mostro-coreand compiling and started testing, did some basic testing:Will work for other testing in the next days, will make this PR official so rabbit can sniff it and see what he sees, also if it's quite big.
Then @grunch and @Catrya please use your eyes to check everything and see if you like my modifications, still a lot of room, maybe start with takesell, takebuy and order files and check if they seem more ordered now.
Note: you need to use the same
error-managebranch for mostro-core to compile.Now let's make rabbit speak...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores