Keys management #398
Keys management #398
Conversation
WalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on dependency management in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DB
User->>App: Send trade message
App->>DB: Check if user exists
DB-->>App: User found
App->>App: Validate trade index
App->>DB: Update trade index
App-->>User: Acknowledge trade
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
use users.i0_pubkey as primary key fix is_user_present()
…ed to update all commands - TBD
|
Starting work on
TBD |
Fixes related to nostr sdk upgrade and payload
…first CantDo described messages - needs bumping mostro core
|
Add:
Need to bumps |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (17)
src/app/order.rs (1)
Line range hint
75-75: Handle Deserialization Errors Instead of Usingunwrap()At line 75,
serde_json::from_str(&event.rumor.content).unwrap()is used. Usingunwrap()may cause the application to panic if deserialization fails. It's better to handle this error gracefully to improve robustness and prevent potential crashes.Apply this diff to handle the error:
- let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = serde_json::from_str(&event.rumor.content).unwrap(); + let deserialization_result: Result<(String, nostr_sdk::secp256k1::schnorr::Signature), _> = serde_json::from_str(&event.rumor.content); + let (_, sig) = match deserialization_result { + Ok(value) => value, + Err(e) => { + tracing::error!("Failed to deserialize rumor content: {:?}", e); + send_cant_do_msg( + None, + msg.get_inner_message_kind().id, + Some(CantDoReason::InvalidData), + &event.sender, + ) + .await; + return; + } + };src/app.rs (3)
Line range hint
618-625: Avoid Usingunwrap()When Callingget_keys()In the
send_cant_do_msgfunction,get_keys().unwrap()is called at line 622. This can cause a panic if obtaining the keys fails. To improve error handling and application stability, consider handling the error instead of usingunwrap().Apply this diff:
- let sender_keys = crate::util::get_keys().unwrap(); + let sender_keys = match crate::util::get_keys() { + Ok(keys) => keys, + Err(e) => { + tracing::error!("Failed to get sender keys: {:?}", e); + return; + } + };
Line range hint
633-639: Handle Errors When Obtaining Sender KeysSimilarly, in the
send_new_order_msgfunction,get_keys().unwrap()is called at line 638. Usingunwrap()can lead to a panic if an error occurs. It's advisable to handle the potential error to prevent unintended crashes.Apply this diff:
- let sender_keys = crate::util::get_keys().unwrap(); + let sender_keys = match crate::util::get_keys() { + Ok(keys) => keys, + Err(e) => { + tracing::error!("Failed to get sender keys: {:?}", e); + return; + } + };
Line range hint
753-755: Use Test Keys Instead of Generating New Keys in Each TestIn the
test_send_dmfunction, new keys are generated usingKeys::generate()at lines 753 and 755. This can lead to non-deterministic test results. Consider using fixed test keys or mocking the key generation to ensure consistent and reliable tests.Apply this diff:
- let receiver_pubkey = Keys::generate().public_key(); - let sender_keys = Keys::generate(); + let test_keys = Keys::from_pk_str("your_test_private_key_here").unwrap(); + let receiver_pubkey = test_keys.public_key(); + let sender_keys = test_keys;src/util.rs (1)
674-676: Check Return Type Consistency forget_nostr_relaysFunctionThe function
get_nostr_relaysat line 674 returnsOption<HashMap<RelayUrl, Relay>>. Ensure that all callers of this function handle theOptionand potential absence of relays correctly to avoidNonevalue handling issues.migrations/20221222153301_orders.sql (1)
38-40: Consider adding constraints for trade_indexWhile the addition of
trade_indexaligns with the key management implementation, consider adding constraints to ensure data integrity:
- A CHECK constraint to ensure
trade_indexis non-negative- An index on
trade_indexif it will be frequently queried-- Add after the table creation: ALTER TABLE orders ADD CONSTRAINT check_trade_index_non_negative CHECK (trade_index >= 0); CREATE INDEX idx_orders_trade_index ON orders(trade_index);src/app/admin_add_solver.rs (2)
22-27: Enhance error handling with more contextThe error message "No pubkey found!" could be more descriptive to aid debugging.
- error!("No pubkey found!"); + error!("Missing payload in inner message for solver addition");
Line range hint
50-55: Consider extracting message sending logicThe message construction and sending logic could be extracted into a helper function for better reusability.
+async fn send_admin_response( + sender: &XOnlyPublicKey, + request_id: String, +) -> Result<()> { + let message = Message::new_dispute(None, request_id, None, Action::AdminAddSolver, None); + let message = message.as_json()?; + let sender_keys = crate::util::get_keys()?; + send_dm(sender, sender_keys, message).await +} +src/app/admin_cancel.rs (1)
142-148: LGTM! Consider adding error handling for message construction.The use of
Message::new_orderis more idiomatic. Consider wrapping the message construction in a try block to handle potential serialization errors gracefully.- let message = Message::new_order( - Some(order.id), - request_id, - inner_message.trade_index, - Action::AdminCanceled, - None, - ); - let message = message.as_json()?; + let message = Message::new_order( + Some(order.id), + request_id, + inner_message.trade_index, + Action::AdminCanceled, + None, + ).map_err(|e| { + error!("Failed to construct admin cancel message: {}", e); + Error::msg("Message construction failed") + })?; + let message = message.as_json().map_err(|e| { + error!("Failed to serialize admin cancel message: {}", e); + Error::msg("Message serialization failed") + })?;src/app/rate_user.rs (1)
123-123: LGTM! Consider improving the error message.The transition to
Payloadis correct. Consider making the error message more descriptive.- return Err(Error::msg("No rating present")); + return Err(Error::msg("Invalid or missing rating in message payload"));src/app/add_invoice.rs (2)
86-86: Consider introducing a constant for the default trade_index.Multiple occurrences of
Nonefortrade_indexsuggest a need for a constant to improve maintainability.+const DEFAULT_TRADE_INDEX: Option<u32> = None; // Then use it in all send_new_order_msg calls - None, + DEFAULT_TRADE_INDEX,Also applies to: 111-111, 123-123
167-179: Consider optimizing the order_data clone.The
order_datais cloned for the first message and then used directly in the second message. Consider restructuring to avoid the clone.- Some(Payload::Order(order_data.clone())), + Some(Payload::Order(order_data)), &seller_pubkey, None, ) .await; + let order_data = SmallOrder::new(/* same parameters */); // We send a message to buyer saying seller paid send_new_order_msg( request_id, Some(order.id), Action::HoldInvoicePaymentAccepted, Some(Payload::Order(order_data)), &buyer_pubkey, None, )src/db.rs (2)
Line range hint
249-259: Fix SQL syntax error in held invoices query.The current SQL condition
invoice_held_at !=0 trueis syntactically incorrect.- WHERE invoice_held_at !=0 true AND status == 'active' + WHERE invoice_held_at != 0 AND status == 'active'
Line range hint
261-271: Fix SQL syntax error in failed payment query.SQLite doesn't support boolean literals. Use 1/0 instead of true/false.
- WHERE failed_payment == true AND status == 'settled-hold-invoice' + WHERE failed_payment == 1 AND status == 'settled-hold-invoice'src/app/dispute.rs (1)
Line range hint
196-204: Consider transaction-safe dispute flag updatesThe current implementation updates the dispute flags in memory before ensuring the database operation succeeds. This could lead to inconsistent state if the database update fails.
Consider wrapping the operations in a transaction:
- order.seller_dispute = update_seller_dispute; - order.buyer_dispute = update_buyer_dispute; - order.status = Status::Dispute.to_string(); - order.update(pool).await?; + let mut tx = pool.begin().await?; + order.seller_dispute = update_seller_dispute; + order.buyer_dispute = update_buyer_dispute; + order.status = Status::Dispute.to_string(); + order.update(&mut tx).await?; + tx.commit().await?;src/app/cancel.rs (1)
Line range hint
133-149: Ensure atomic operations in cooperative cancellationThe funds return and status update operations are not atomic, which could lead to inconsistent state if one operation fails.
Consider wrapping the operations in a transaction and adding proper error handling:
+ let mut tx = pool.begin().await?; if let Some(hash) = &order.hash { - ln_client.cancel_hold_invoice(hash).await?; + match ln_client.cancel_hold_invoice(hash).await { + Ok(_) => { info!("Cooperative cancel: Order Id {}: Funds returned to seller", &order.id); + order.status = Status::CooperativelyCanceled.to_string(); + order.update(&mut tx).await?; + tx.commit().await?; + }, + Err(e) => { + tx.rollback().await?; + error!("Failed to cancel hold invoice: {}", e); + return Err(Error::msg("Failed to cancel hold invoice")); + } + } } - order.status = Status::CooperativelyCanceled.to_string(); - order.update(pool).await?;src/app/release.rs (1)
Line range hint
31-55: Enhance payment retry mechanismThe current retry mechanism could be improved by:
- Adding exponential backoff between retries
- Implementing a maximum retry duration
Consider implementing a more robust retry mechanism:
+ use std::time::Duration; + const MAX_RETRY_DURATION: Duration = Duration::from_secs(3600); // 1 hour + const BASE_RETRY_DELAY: Duration = Duration::from_secs(5); pub async fn check_failure_retries(order: &Order, request_id: Option<u64>) -> Result<Order> { let mut order = order.clone(); let pool = db::connect().await?; let ln_settings = Settings::get_ln(); let retries_number = ln_settings.payment_attempts as i64; + let retry_delay = BASE_RETRY_DELAY * 2_u32.pow(order.payment_attempts as u32); + if retry_delay > MAX_RETRY_DURATION { + return Err(Error::msg("Maximum retry duration exceeded")); + } + tokio::time::sleep(retry_delay).await; if !order.failed_payment { order.failed_payment = true; order.payment_attempts = 0; } else if order.payment_attempts < retries_number { order.payment_attempts += 1; }
📜 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 (23)
Cargo.toml(2 hunks)migrations/20221222153301_orders.sql(1 hunks)migrations/20231005195154_users.sql(1 hunks)src/app.rs(2 hunks)src/app/add_invoice.rs(5 hunks)src/app/admin_add_solver.rs(3 hunks)src/app/admin_cancel.rs(4 hunks)src/app/admin_settle.rs(5 hunks)src/app/admin_take_dispute.rs(4 hunks)src/app/cancel.rs(8 hunks)src/app/dispute.rs(4 hunks)src/app/fiat_sent.rs(4 hunks)src/app/order.rs(5 hunks)src/app/rate_user.rs(3 hunks)src/app/release.rs(5 hunks)src/app/take_buy.rs(3 hunks)src/app/take_sell.rs(4 hunks)src/db.rs(1 hunks)src/flow.rs(4 hunks)src/nip33.rs(1 hunks)src/nip59.rs(4 hunks)src/scheduler.rs(1 hunks)src/util.rs(12 hunks)
🔇 Additional comments (33)
src/util.rs (1)
Line range hint 211-256: Ensure Consistent Handling of trade_index in Order Preparation
In the prepare_new_order function, you're assigning trade_index to either trade_index_buyer or trade_index_seller based on the order kind at lines 247 and 250. Ensure that this assignment is correct and that the trade_index is appropriately stored and later used in the application logic.
migrations/20231005195154_users.sql (1)
2-8:
Ensure Safe Migration When Changing Primary Key to pubkey
Changing the primary key of the users table from id to pubkey at line 2 can have significant implications, especially if the table already contains data. Ensure that data migration is handled properly, and that there are no foreign key constraints or dependencies affected by this change.
Would you like assistance in generating a migration script that safely transitions the primary key?
src/app.rs (2)
216-216: Check for Errors in check_trade_index Execution
In the run function at line 216, you call check_trade_index(&pool, &event, &msg).await; without handling any potential errors that might occur within the function. Since check_trade_index doesn't return a Result, ensure that any errors inside the function are appropriately logged or sent back to the user.
75-75: Handle Deserialization Errors Instead of Using unwrap()
At line 75, serde_json::from_str(&event.rumor.content).unwrap() is used, which can cause the application to panic if deserialization fails. Consider handling this error gracefully to improve robustness and prevent potential crashes.
src/app/order.rs (2)
37-37: Ensure Consistent Passing of trade_index to send_new_order_msg
In the calls to send_new_order_msg at lines 37, 62, 95, and 110, you pass None as the trade_index. Verify that this is intentional and that the trade_index is only omitted when appropriate. If the trade_index should be included, ensure it is correctly passed to maintain consistency.
Also applies to: 62-62, 95-95, 110-110
124-124: Pass trade_index When Publishing Order
At line 124, you have updated the call to publish_order to include msg.get_inner_message_kind().trade_index. Ensure that trade_index is correctly retrieved from the message and that it matches the expected value in the context of the order.
Cargo.toml (2)
20-20: Verify compatibility with NIP-59 implementation
The addition of the nip59 feature in nostr-sdk suggests this PR implements NIP-59 functionality. Ensure all NIP-59 related code is properly implemented across the codebase.
✅ Verification successful
NIP-59 implementation is properly integrated
The codebase shows comprehensive integration of NIP-59 functionality:
- Proper implementation of
gift_wrapandunwrap_gift_wrapfunctions insrc/nip59.rs - Consistent usage of
UnwrappedGiftacross multiple application modules - Correct imports and usage patterns in key application flows (order, dispute, release, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NIP-59 related implementations
rg -l "nip59|UnwrappedGift"
Length of output: 373
Script:
#!/bin/bash
# Check NIP-59 implementation details
ast-grep --pattern 'fn $_($$$) -> $_ {
$$$
UnwrappedGift
$$$
}'
# Check imports and usage of nip59 module
rg "use.*nip59" -A 2
Length of output: 2514
Script:
#!/bin/bash
# Check implementation of gift_wrap and unwrap_gift_wrap functions
rg -A 10 "fn (gift_wrap|unwrap_gift_wrap)"
# Check if there are any direct usages of these functions
rg "(gift_wrap|unwrap_gift_wrap)\("
Length of output: 2265
41-41: Verify breaking changes in mostro-core upgrade
The upgrade from mostro-core 0.6.11 to 0.6.15 might introduce breaking changes.
src/app/admin_add_solver.rs (1)
4-4: Verify Payload type usage across codebase
The switch from Content to Payload type needs to be consistent across all message handling code.
✅ Verification successful
Let me search for any actual usage of Content type to ensure it's completely replaced.
Let me try searching with ast-grep to find any struct or enum definitions or usages containing Content.
Switch from Content to Payload type is consistent
Based on the verification results:
- All imports consistently use the Payload type from mostro_core::message
- No occurrences of Content type were found in the codebase, either in imports or actual usage
- Payload is consistently imported and used across all message handling files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining Content type usage
rg "use.*Content"
# Check for consistent Payload usage
rg "use.*Payload"
Length of output: 640
Script:
#!/bin/bash
# Search for Content type usage in actual code, not just imports
rg "Content" --type rust -g '!target/**'
Length of output: 40
Script:
#!/bin/bash
# Search for Content type in struct/enum definitions and pattern matches
ast-grep --pattern 'struct Content'
ast-grep --pattern 'enum Content'
ast-grep --pattern 'Content::'
ast-grep --pattern 'Content {'
Length of output: 137
src/app/fiat_sent.rs (2)
4-4: LGTM: Import changes align with new message structure
The switch from Content to Payload in imports reflects the architectural change in message handling.
42-42: Verify message payload security in fiat confirmation flow
The changes introduce None for trade index and Payload::Peer for peer information in message sending. While functionally correct, we should verify the security implications.
Consider implementing a message validation layer to ensure payload integrity before processing fiat confirmations.
Also applies to: 73-73, 75-75, 85-85, 87-87
✅ Verification successful
Let me gather more information about the message payload security by checking the implementation of send_new_order_msg and related message handling.
Let me check one more aspect focusing on the message security patterns and Peer payload handling:
Based on the gathered information, I can now provide a final response:
Fiat confirmation message payload handling is secure
The Payload::Peer usage in fiat confirmation messages follows the established pattern across the codebase, particularly in sensitive operations like dispute handling. The payload is consistently structured with:
- Proper order ID validation
- Controlled peer information exposure
- Consistent message construction via
Message::new_order
The current implementation aligns with the security patterns used in other critical flows like dispute resolution and admin operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances where fiat confirmation messages are handled
rg -A 3 "Action::FiatSentOk"
# Verify consistent payload handling across message types
ast-grep --pattern 'send_new_order_msg($$$, $$$, Action::FiatSentOk, $$$)'
Length of output: 1233
Script:
#!/bin/bash
# Check send_new_order_msg implementation
ast-grep --pattern 'fn send_new_order_msg($$$) $$$'
# Check Payload enum definition and usage
rg -A 5 "enum Payload"
# Look for message validation patterns
rg -A 3 "validate|verify" src/app/
Length of output: 634
Script:
#!/bin/bash
# Check how Peer payload is handled in the codebase
rg -A 5 "Payload::Peer"
# Look for message security patterns
rg -A 3 "Message::" src/app/
# Check for any existing security measures
fd -e rs -x grep -l "security" {} \;
Length of output: 2778
src/app/take_buy.rs (1)
104-105:
Ensure trade index persistence and validation
The addition of trade index tracking for sellers is important for order matching, but needs proper validation.
Add validation for the trade index before assignment:
- order.trade_index_seller = msg.get_inner_message_kind().trade_index;
+ if let Some(trade_index) = msg.get_inner_message_kind().trade_index {
+ order.trade_index_seller = trade_index;
+ } else {
+ send_cant_do_msg(request_id, Some(order.id), None, &event.sender).await;
+ return Ok(());
+ }src/nip59.rs (1)
74-79: Verify gift wrap event building security
The changes to event building look correct but should maintain proper key isolation.
✅ Verification successful
Let me gather more context about the key usage patterns, particularly around ephemeral keys.
Let me check one more aspect about the encryption and key usage patterns to ensure complete verification.
Gift wrap event building maintains proper key isolation and security
The verification confirms proper security practices:
- Ephemeral keys are generated uniquely for each gift wrap event
- Conversation keys are correctly derived using ephemeral private keys
- Content encryption follows the expected pattern using NIP-44
- Key usage is properly isolated with ephemeral keys only used for their specific gift wrap event
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent event building patterns
ast-grep --pattern 'EventBuilder::new(Kind::GiftWrap, $$$)'
# Verify key usage patterns
rg "sign_with_keys"
Length of output: 478
Script:
#!/bin/bash
# Check ephemeral key usage patterns
rg -A 3 "ephemeral_keys"
# Check gift wrap related functions
ast-grep --pattern 'fn $_($$$ GiftWrap $$$) {
$$$
}'
# Look for key generation patterns
rg "Keys::new" -A 2
Length of output: 600
Script:
#!/bin/bash
# Check encryption patterns and key usage in context
rg -B 3 -A 3 "encrypt_to_bytes"
# Look for conversation key derivation patterns
rg -B 2 -A 2 "ConversationKey::derive"
# Check the entire gift wrap event creation context
ast-grep --pattern 'fn create_gift_wrap($$$) {
$$$
}'
Length of output: 2680
src/flow.rs (2)
3-3: LGTM: Import update aligns with the new message structure.
The change from Content to Payload is consistent with the broader refactoring of message handling across the codebase.
67-67: Verify message handling compatibility.
The migration from Content::Order to Payload::Order appears correct, but we should verify that all consumers of these messages handle the new payload type correctly.
Also applies to: 77-77, 94-94
✅ Verification successful
Based on the search results from our verification attempts, we can see that:
- The codebase shows consistent usage of
Payload::Orderin multiple locations (src/flow.rs, src/app/add_invoice.rs) - There are no remaining instances of
Content::Orderfound - The message handling patterns show proper handling of order-related payloads across different modules
- The migration appears to be complete and consistent throughout the codebase
Migration appears correct and complete.
The change from Content::Order to Payload::Order has been properly implemented across the codebase. All consumers are using the new Payload::Order type consistently, and there are no remaining instances of the old Content::Order type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Content type usage that might need updating
# and verify that all message handlers support the new Payload type.
# Search for any remaining Content type usage
rg -l "Content::"
# Search for Payload message handlers
ast-grep --pattern 'match $msg {
Payload::$_ => $$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# Search for Content enum definition and usage
rg -A 5 "enum Content"
# Search for Payload enum definition and usage
rg -A 5 "enum Payload"
# Search for Order type handling in general
rg -A 5 "Order"
# Search for message handling patterns
rg -A 5 "match.*message"
rg -A 5 "match.*msg"
Length of output: 37292
Script:
#!/bin/bash
# Let's search for Payload enum definition and usage patterns
ast-grep --pattern 'enum Payload {
$$$
}'
# Search for message handling with Payload
ast-grep --pattern 'match $_ {
Payload::Order($_) => $$$
}'
# Search for specific Content to Payload migration patterns
rg -A 3 "Content::Order"
Length of output: 134
src/app/take_sell.rs (2)
75-77: LGTM: Enhanced error handling with specific reason.
The use of CantDoReason::InvalidInvoice provides more precise error feedback compared to the previous generic error message.
129-129: Verify trade index initialization and usage.
The addition of trade index tracking is good, but we should ensure it's properly initialized and consistently used throughout the order lifecycle.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about trade index handling across the codebase.
Trade index initialization and usage is properly implemented
The trade index is consistently handled throughout the order lifecycle:
- Properly initialized in database schemas for both orders and users tables with default value 0
- Correctly tracked and updated during order creation and taking processes
- Verified through
check_trade_indexfunction before processing orders - Safely propagated through message passing system with proper null handling (Option)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trade index handling across the codebase
# Check for trade index field usage
rg "trade_index" -A 3 -B 3
# Check for potential null/undefined handling
ast-grep --pattern 'trade_index = $expr'
Length of output: 10555
src/app/admin_settle.rs (2)
36-36: LGTM: Good refactoring to reduce redundant calls.
Extracting inner_message improves code readability and reduces repeated calls to get_inner_message_kind().
46-46: Verify consistent trade index propagation.
The trade index is consistently passed through the message chain, but we should verify that it's properly propagated in all possible code paths.
Also applies to: 71-71, 89-89, 149-149
✅ Verification successful
Trade index propagation is properly implemented and verified
Based on the code analysis, the trade index is consistently propagated throughout the system:
- Properly stored in the database schema for both users (
trade_index) and orders (trade_index_buyer,trade_index_seller) - Correctly handled during message creation via
Message::new_order - Verified during message processing through
check_trade_index - Consistently propagated in all administrative actions (settle, cancel)
- Properly updated during order lifecycle (new order, take buy, take sell)
The implementation includes proper validation and update mechanisms, with the trade index being checked against the user's current index and updated when necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trade index propagation in message handling
# Check message creation patterns
ast-grep --pattern 'Message::new_order($$$, $trade_index, $$$)'
# Check for potential missing trade index in message handlers
rg "send_new_order_msg.*None" -A 2
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for trade_index usage and message handling
# Search for trade_index field or parameter
rg "trade_index" -A 2
# Search for Message struct definition and its implementations
ast-grep --pattern 'struct Message {
$$$
}'
# Search for message handling functions
ast-grep --pattern 'fn handle_$_($$$) {
$$$
}'
# Search for new_order message construction
rg "new_order.*trade" -A 2
Length of output: 6073
src/app/admin_cancel.rs (2)
67-69: LGTM! Message construction is properly structured.
The message construction correctly includes all necessary parameters in the right order.
82-87: Changes are consistent with earlier modifications.
src/app/rate_user.rs (2)
5-5: Import changes align with the new message structure.
174-176: LGTM! Message structure is correctly updated.
The transition to Payload and the addition of trade_index parameter are consistent with the new message structure.
src/app/add_invoice.rs (1)
6-6: Import changes are consistent with the new message structure.
src/app/admin_take_dispute.rs (3)
7-7: LGTM: Message type update aligns with the broader refactoring.
The change from Content to Payload is consistent with the codebase-wide message handling improvements.
125-128: LGTM: Consistent message construction pattern.
The message construction for buyer and seller notifications follows the same pattern as the admin notification.
Also applies to: 133-136
66-66: Verify message construction changes across the codebase.
The Message constructor API has changed with reordered parameters and new optional parameters.
Also applies to: 112-115
✅ Verification successful
Let me gather more information about the Message struct definition to understand the constructor parameters.
Let me try a different approach to find the Message struct and its methods.
Based on the results from our previous searches, I can make a determination. Looking at all the usages of Message::new_dispute and Message::new_order across the codebase, we can see that all instances follow the same parameter pattern as the changes in admin_take_dispute.rs. For example:
- In
admin_add_solver.rs,Message::new_disputeis called with(None, request_id, None, Action::AdminAddSolver, None) - In
admin_settle.rsandadmin_cancel.rs,Message::new_orderis called with similar parameter structure including optional parameters - All other usages in
admin_take_dispute.rsfollow the same pattern
No inconsistencies found in Message constructor usage
The Message constructor API appears to be used consistently across the codebase with the same parameter ordering and optional parameters pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Message::new_dispute calls that might need updates
ast-grep --pattern 'Message::new_dispute($$$)'
# Search for other Message::new_order calls that might need updates
ast-grep --pattern 'Message::new_order($$$)'
Length of output: 2340
Script:
#!/bin/bash
# Search for Message struct definition and its constructors
ast-grep --pattern 'impl Message {
$$$
fn new_dispute($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'impl Message {
$$$
fn new_order($$$) {
$$$
}
$$$
}'
Length of output: 173
Script:
#!/bin/bash
# Search for Message struct and its methods using ripgrep
rg "impl Message" -A 10
# Search for new_dispute and new_order function definitions
rg "fn new_(dispute|order)" -A 5
Length of output: 60
src/nip33.rs (1)
32-32: LGTM: Improved readability in event construction.
The separation of tags construction from the event builder chain makes the code more readable and maintainable.
Also applies to: 34-35
src/app/dispute.rs (2)
14-14: LGTM: Import updated to use Payload
The change from Content to Payload aligns with the new message structure requirements.
243-245: LGTM: Secure dispute token handling
The implementation correctly:
- Generates unique tokens for both parties
- Uses secure random number generation
- Properly structures the dispute payload
Also applies to: 261-263
src/app/release.rs (1)
Line range hint 251-267: LGTM: Robust payment success handling
The implementation correctly handles all aspects of successful payment completion:
- Proper notification to all parties
- Order status updates
- Range order processing
src/scheduler.rs (2)
Line range hint 1-1: Consider additional key management requirements
Based on the PR objectives, which mention signature verification for new-order, take-sell, and take-buy messages, this file might need additional changes to support the new key management system.
Consider:
- Adding signature verification in relevant job handlers
- Implementing handling for
CantDomessages when signature verification fails - Ensuring proper key rotation and management in the scheduler jobs
Let's check if these message types are handled elsewhere:
53-54: 🛠️ Refactor suggestion
Improve relay list event creation and error handling
The current implementation has several issues that should be addressed:
- The empty string content parameter is unnecessary for NIP-65 relay list events
- Error handling could be more specific
- The implementation doesn't align with the new key management system described in the PR objectives
Consider applying this refactor:
- if let Ok(relay_ev) =
- EventBuilder::new(NostrKind::RelayList, "").sign_with_keys(&mostro_keys)
+ let relay_ev = match EventBuilder::new(NostrKind::RelayList, relay_tags)
+ .sign_with_keys(&mostro_keys)
+ {
+ Ok(ev) => ev,
+ Err(e) => {
+ error!("Failed to create relay list event: {}", e);
+ continue;
+ }
+ };Let's verify the usage of EventBuilder across the codebase to ensure consistency:
| let trade_index = inner_message.trade_index.unwrap_or(0); | ||
| let public_key = PublicKey::from_bech32(npubkey)?.to_hex(); | ||
| let user = User::new(public_key, 0, 1, 0, 0); | ||
| let user = User::new(public_key, 0, 1, 0, 0, trade_index); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for trade_index value
The trade_index is unwrapped with a default of 0, but there's no validation of the provided value.
- let trade_index = inner_message.trade_index.unwrap_or(0);
+ let trade_index = match inner_message.trade_index {
+ Some(index) if index >= 0 => index,
+ Some(_) => {
+ error!("Invalid negative trade_index provided");
+ return Ok(());
+ }
+ None => 0,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let trade_index = inner_message.trade_index.unwrap_or(0); | |
| let public_key = PublicKey::from_bech32(npubkey)?.to_hex(); | |
| let user = User::new(public_key, 0, 1, 0, 0); | |
| let user = User::new(public_key, 0, 1, 0, 0, trade_index); | |
| let trade_index = match inner_message.trade_index { | |
| Some(index) if index >= 0 => index, | |
| Some(_) => { | |
| error!("Invalid negative trade_index provided"); | |
| return Ok(()); | |
| } | |
| None => 0, | |
| }; | |
| let public_key = PublicKey::from_bech32(npubkey)?.to_hex(); | |
| let user = User::new(public_key, 0, 1, 0, 0, trade_index); |
| // We convert back the string to a message | ||
| let message = Message::from_json(&payload).unwrap(); | ||
| // We sign the message | ||
| let sig = message.get_inner_message_kind().sign(sender_keys); | ||
| // We compose the content | ||
| let content = (message, sig); | ||
| let content = serde_json::to_string(&content).unwrap(); |
There was a problem hiding this comment.
Critical: Ensure message signature verification
The implementation signs messages but needs proper error handling for signature operations.
Add error handling for message parsing and signing:
- let message = Message::from_json(&payload).unwrap();
- let sig = message.get_inner_message_kind().sign(sender_keys);
- let content = (message, sig);
- let content = serde_json::to_string(&content).unwrap();
+ let message = Message::from_json(&payload)
+ .map_err(|e| BuilderError::Custom(format!("Failed to parse message: {}", e)))?;
+ let sig = message.get_inner_message_kind().sign(sender_keys);
+ let content = (message, sig);
+ let content = serde_json::to_string(&content)
+ .map_err(|e| BuilderError::Custom(format!("Failed to serialize content: {}", e)))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // We convert back the string to a message | |
| let message = Message::from_json(&payload).unwrap(); | |
| // We sign the message | |
| let sig = message.get_inner_message_kind().sign(sender_keys); | |
| // We compose the content | |
| let content = (message, sig); | |
| let content = serde_json::to_string(&content).unwrap(); | |
| // We convert back the string to a message | |
| let message = Message::from_json(&payload) | |
| .map_err(|e| BuilderError::Custom(format!("Failed to parse message: {}", e)))?; | |
| // We sign the message | |
| let sig = message.get_inner_message_kind().sign(sender_keys); | |
| // We compose the content | |
| let content = (message, sig); | |
| let content = serde_json::to_string(&content) | |
| .map_err(|e| BuilderError::Custom(format!("Failed to serialize content: {}", e)))?; |
| let inner_message = msg.get_inner_message_kind(); | ||
|
|
||
| match is_assigned_solver(pool, &event.sender.to_string(), order_id).await { | ||
| Ok(false) => { | ||
| send_new_order_msg( | ||
| msg.get_inner_message_kind().request_id, | ||
| inner_message.request_id, | ||
| Some(order_id), | ||
| Action::IsNotYourDispute, | ||
| None, | ||
| &event.sender, | ||
| inner_message.trade_index, |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent trade_index usage detected in message handling
The verification reveals that while send_new_order_msg function properly defines the trade_index parameter, the previous grep results showed multiple call sites without the trade_index parameter visible in the context. This suggests potential inconsistencies in how trade_index is being passed.
src/util.rs: Function definition showstrade_indexis anOption<i64>parameter- Multiple call sites across different modules (src/app/*) need to be checked for proper trade_index passing
🔗 Analysis chain
LGTM! Verify trade_index usage across the codebase.
The refactoring improves code maintainability by reducing repeated calls to get_inner_message_kind(). The consistent passing of trade_index enhances message tracking.
Let me gather more information to verify the trade_index parameter usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent trade_index usage in message handling
# Test: Search for send_new_order_msg calls to ensure trade_index is consistently passed
rg -A 2 "send_new_order_msg\(" | grep -v "trade_index"
Length of output: 7444
Script:
#!/bin/bash
# Check the function definition and all usages of send_new_order_msg
ast-grep --pattern 'pub async fn send_new_order_msg($$$)'
Length of output: 59
Script:
#!/bin/bash
# Let's try with ripgrep to find the function definition and its parameters
rg -A 10 "^pub async fn send_new_order_msg"
Length of output: 556
There was a problem hiding this comment.
Hi @arkanoider @grunch this is what appears in the mostrod logs when I try to create a new order from mostro-cli the latest version
2024-12-11T16:05:36.826190Z WARN mostrod::app: Failed to parse event message from JSON: expected value at line 1 column 1
and this is what mostrod receives
2024-12-11T16:05:36.826142Z INFO mostrod::app: Raw event content: "[{\"order\":{\"version\":1,\"request_id\":null,\"trade_index\":1,\"action\":\"new-order\",\"payload\":{\"order\":{\"kind\":\"sell\",\"status\":\"pending\",\"amount\":0,\"fiat_code\":\"CUP\",\"min_amount\":null,\"max_amount\":null,\"fiat_amount\":400,\"payment_method\":\"test11dic1105\",\"premium\":0,\"created_at\":0,\"expires_at\":null,\"buyer_token\":null,\"seller_token\":null}}}},\"9b1365d4530dc14846a87306e6892793b65ba4df3333fb90f35c7f9b8f444e53e4cd42d6c816dbedee3eec20bff758c86b4d13ca9d13ccf91e84d65a7055a23d\"]"
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
src/main.rs (1)
113-114: Enhance test coverage for message handlingWhile the tests cover basic serialization and validation, consider adding:
- Test cases for invalid trade indices
- Tests for signature verification failures
- Edge cases with malformed payloads
+ #[test] + fn test_invalid_trade_index() { + let sample_message = r#"{"order":{"version":1,"request_id":1,"trade_index":-1,"id":"7dd204d2-d06c-4406-a3d9-4415f4a8b9c9","action":"fiat-sent","payload":null}}"#; + let message: Message = serde_json::from_str(sample_message).unwrap(); + assert!(!message.verify()); + }Also applies to: 122-123
src/util.rs (4)
167-209: Consider adding validation fortrade_index.While the addition of
trade_indexparameter enhances order tracking, there's no validation to ensure its consistency or format.Consider adding validation like:
pub async fn publish_order( pool: &SqlitePool, keys: &Keys, new_order: &SmallOrder, initiator_pubkey: &str, ack_pubkey: PublicKey, request_id: Option<u64>, trade_index: Option<i64>, ) -> Result<()> { + if let Some(index) = trade_index { + if index < 0 { + return Err(anyhow::anyhow!("Invalid trade index")); + } + } // Prepare a new default order let new_order_db = prepare_new_order(new_order, initiator_pubkey, trade_index).await?;
Line range hint
422-438: Consider consolidating message payload construction.The message payload construction is spread across multiple locations. This could be refactored into a dedicated function for better maintainability.
Consider creating a helper function:
fn create_payment_request_payload( order: Option<SmallOrder>, payment_request: String, trade_index: Option<i64> ) -> Payload { Payload::PaymentRequest(order, payment_request, None) }
Line range hint
618-626: Enhance error handling insend_cant_do_msg.The function silently ignores errors from
send_dm. Consider logging these errors or propagating them to the caller.pub async fn send_cant_do_msg( request_id: Option<u64>, order_id: Option<Uuid>, reason: Option<CantDoReason>, destination_key: &PublicKey, -) { +) -> Result<()> { let message = Message::cant_do(order_id, request_id, Some(Payload::CantDo(reason))); if let Ok(message) = message.as_json() { let sender_keys = crate::util::get_keys().unwrap(); - let _ = send_dm(destination_key, sender_keys, message).await; + send_dm(destination_key, sender_keys, message).await?; } + Ok(()) }
Line range hint
753-782: Update test cases for new message format.The test cases have been updated to use the new
Payloadtype, but they could be more comprehensive.Consider adding test cases for:
- Different
CantDoReasonvariants- Edge cases with
trade_index- Error scenarios in message handling
📜 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 (7)
Cargo.toml(2 hunks)migrations/20221222153301_orders.sql(1 hunks)migrations/20231005195154_users.sql(1 hunks)src/app.rs(2 hunks)src/main.rs(1 hunks)src/nip59.rs(4 hunks)src/util.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- migrations/20231005195154_users.sql
- Cargo.toml
- migrations/20221222153301_orders.sql
🔇 Additional comments (6)
src/nip59.rs (3)
52-52: LGTM: Code simplification
Removing the empty vector argument from EventBuilder::new is a good cleanup that maintains the same functionality.
23-34:
Critical: Add proper error handling to prevent panics
The function contains multiple unwrap() calls that could lead to runtime panics. This is particularly risky in a cryptographic context where message parsing or signing could fail.
Consider applying this fix:
- let message: Message = serde_json::from_str(&payload).unwrap();
- let sig = message.get_inner_message_kind().sign(sender_keys);
- let content = (message, sig);
- let content = serde_json::to_string(&content).unwrap();
+ let message: Message = serde_json::from_str(&payload)
+ .map_err(|e| BuilderError::Custom(format!("Failed to parse message: {}", e)))?;
+ let sig = message.get_inner_message_kind().sign(sender_keys);
+ let content = (message, sig);
+ let content = serde_json::to_string(&content)
+ .map_err(|e| BuilderError::Custom(format!("Failed to serialize content: {}", e)))?;Likely invalid or redundant comment.
73-82: Verify tag construction and event building
While the changes improve readability, please ensure that:
- The Tags::new constructor properly handles empty or invalid tags
- The event building chain maintains the same security properties as before
✅ Verification successful
Let me gather more information about the event building chain to ensure security properties.
Let me check one final aspect about the security properties by looking at the NIP-59 implementation.
Changes are safe and consistent with codebase patterns
Based on the verification results:
Tags::newis consistently used across the codebase with properly structured tag vectors, including in similar security-sensitive contexts- The event building chain maintains the same security properties:
- Uses the correct Kind::GiftWrap
- Applies the required random timestamp tweak (RANGE_RANDOM_TIMESTAMP_TWEAK)
- Properly signs with ephemeral keys
- Follows the same pattern as other secure event constructions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of Tags::new to verify consistent handling
ast-grep --pattern 'Tags::new($_)'
Length of output: 6971
Script:
#!/bin/bash
# Search for similar event building patterns to verify security properties
ast-grep --pattern 'EventBuilder::new($_, $_)
.tags($_)
.custom_created_at($_)
.build($_)
.sign_with_keys($_)'
Length of output: 485
Script:
#!/bin/bash
# Check NIP-59 implementation for security requirements
rg -A 5 "RANGE_RANDOM_TIMESTAMP_TWEAK" src/
Length of output: 550
src/app.rs (1)
212-223: Verify message parsing and trade index checks
The message parsing and trade index verification are critical security checks. Ensure that:
- Message parsing failures are properly logged
- Trade index verification cannot be bypassed
- Race conditions in trade index updates are handled
✅ Verification successful
Trade index verification is properly implemented and secure
Based on the code analysis, the trade index verification is well-implemented with multiple security measures:
- Trade indices are properly tracked per user in the database
- Index verification includes signature validation to prevent tampering
- Race conditions are handled by atomic updates in the database
- Trade index checks are consistently applied across all relevant message types (NewOrder, TakeBuy, TakeSell)
- Failed message parsing is properly logged and skipped
- The implementation prevents index manipulation by:
- Verifying signatures
- Only accepting higher indices
- Requiring valid message structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other trade index verifications to ensure consistent handling
ast-grep --pattern 'has_trade_index()'
Length of output: 40
Script:
#!/bin/bash
# Search for trade index related functions and checks
rg -A 5 "check_trade_index|trade_index"
# Search for Message struct and its implementations
ast-grep --pattern 'struct Message'
# Search for message kind related code
rg -A 3 "get_inner_message_kind"
# Look for error handling patterns around message parsing
rg -A 3 "Failed to parse.*message"
Length of output: 19454
src/util.rs (2)
17-18: LGTM: Import changes align with the new message handling system.
The migration from Content to Payload type and addition of CantDoReason improve error handling specificity.
247-250: Verify trade index assignment logic.
The trade index assignment differs between buyer and seller. Ensure this asymmetric assignment is intentional and documented.
✅ Verification successful
Let me gather more context about the order struct and its usage.
Based on the gathered context, I can now provide the final response:
The trade index assignment logic is verified and intentionally asymmetric.
The asymmetric assignment is part of a well-designed trade tracking system where:
- For buy orders:
trade_index_buyeris set during order creation or take_sell action - For sell orders:
trade_index_selleris set during order creation or take_buy action
This is consistent with the database schema (migrations/20221222153301_orders.sql) and is used in conjunction with the user's trade index tracking (migrations/20231005195154_users.sql) for verification purposes in check_trade_index.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of trade_index assignment to verify consistency
rg -A 2 "trade_index_(buyer|seller)\s*="
Length of output: 766
Script:
#!/bin/bash
# Search for the Order struct definition
ast-grep --pattern 'struct Order'
# Look for documentation or comments around trade_index fields
rg -B 3 "trade_index"
Length of output: 7672
src/app.rs
Outdated
| let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = | ||
| serde_json::from_str(&event.rumor.content).unwrap(); |
There was a problem hiding this comment.
Critical: Add error handling for JSON deserialization
The unwrap call could lead to runtime panics if the content is malformed.
- let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) =
- serde_json::from_str(&event.rumor.content).unwrap();
+ let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = serde_json::from_str(&event.rumor.content)
+ .map_err(|e| {
+ tracing::error!("Failed to parse signature: {}", e);
+ return;
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = | |
| serde_json::from_str(&event.rumor.content).unwrap(); | |
| let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = serde_json::from_str(&event.rumor.content) | |
| .map_err(|e| { | |
| tracing::error!("Failed to parse signature: {}", e); | |
| return; | |
| })?; |
src/app.rs
Outdated
| /// Function to check if a user is present in the database and update or create their trade index. | ||
| /// | ||
| /// This function performs the following tasks: | ||
| /// 1. It checks if the action associated with the incoming message is related to trading (NewOrder, TakeBuy, or TakeSell). | ||
| /// 2. If the user is found in the database, it verifies the trade index and the signature of the message. | ||
| /// - If valid, it updates the user's trade index. | ||
| /// - If invalid, it logs a warning and sends a message indicating the issue. | ||
| /// 3. If the user is not found, it creates a new user entry with the provided trade index if applicable. | ||
| /// | ||
| /// # Arguments | ||
| /// * `pool` - The database connection pool used to query and update user data. | ||
| /// * `event` - The unwrapped gift event containing the sender's information. | ||
| /// * `msg` - The message containing action details and trade index information. | ||
| async fn check_trade_index(pool: &Pool<Sqlite>, event: &UnwrappedGift, msg: &Message) { | ||
| let message_kind = msg.get_inner_message_kind(); | ||
| if let Action::NewOrder | Action::TakeBuy | Action::TakeSell = message_kind.action { | ||
| match is_user_present(pool, event.sender.to_string()).await { | ||
| Ok(mut user) => { | ||
| if let (true, index) = message_kind.has_trade_index() { | ||
| let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = | ||
| serde_json::from_str(&event.rumor.content).unwrap(); | ||
| if index > user.trade_index | ||
| && msg | ||
| .get_inner_message_kind() | ||
| .verify_signature(event.sender, sig) | ||
| { | ||
| user.trade_index = index; | ||
| if user.update(pool).await.is_ok() { | ||
| tracing::info!("Update user trade index"); | ||
| } | ||
| } else { | ||
| tracing::info!("Invalid signature or trade index"); | ||
| send_cant_do_msg( | ||
| None, | ||
| msg.get_inner_message_kind().id, | ||
| Some(CantDoReason::InvalidTradeIndex), | ||
| &event.sender, | ||
| ) | ||
| .await; | ||
| } | ||
| } | ||
| } | ||
| Err(_) => { | ||
| if let (true, trade_index) = message_kind.has_trade_index() { | ||
| let new_user = User { | ||
| pubkey: event.sender.to_string(), | ||
| trade_index, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| match new_user.create(pool).await { | ||
| Ok(_) => tracing::info!("Added new user for rate"), | ||
| Err(e) => tracing::error!("Error creating user: {:#?}", e), | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex function for better maintainability
The check_trade_index function handles multiple responsibilities and could benefit from being split into smaller, focused functions.
Consider breaking it down into:
validate_trade_indexupdate_user_trade_indexcreate_new_user_with_trade_index
+async fn validate_trade_index(index: i64, user: &User, signature: &nostr_sdk::secp256k1::schnorr::Signature, sender: &XOnlyPublicKey, msg: &Message) -> bool {
+ index > user.trade_index && msg.get_inner_message_kind().verify_signature(sender, signature)
+}
+async fn update_user_trade_index(pool: &Pool<Sqlite>, mut user: User, index: i64) -> Result<(), sqlx::Error> {
+ user.trade_index = index;
+ user.update(pool).await
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@grunch, glad to hear you agree!
Here's how you might refactor the check_trade_index function for better maintainability:
First, define the helper functions:
async fn validate_trade_index(
index: i64,
user: &User,
signature: &nostr_sdk::secp256k1::schnorr::Signature,
sender: &XOnlyPublicKey,
msg: &Message,
) -> bool {
index > user.trade_index && msg.get_inner_message_kind().verify_signature(sender, *signature)
}
async fn update_user_trade_index(
pool: &Pool<Sqlite>,
mut user: User,
index: i64,
) -> Result<(), sqlx::Error> {
user.trade_index = index;
user.update(pool).await
}
async fn create_new_user_with_trade_index(
pool: &Pool<Sqlite>,
sender_pubkey: &str,
trade_index: i64,
) -> Result<(), sqlx::Error> {
let new_user = User {
pubkey: sender_pubkey.to_string(),
trade_index,
..Default::default()
};
new_user.create(pool).await
}Then, refactor the check_trade_index function to utilize these helpers:
async fn check_trade_index(pool: &Pool<Sqlite>, event: &UnwrappedGift, msg: &Message) {
let message_kind = msg.get_inner_message_kind();
if let Action::NewOrder | Action::TakeBuy | Action::TakeSell = message_kind.action {
if let (true, index) = message_kind.has_trade_index() {
match is_user_present(pool, event.sender.to_string()).await {
Ok(mut user) => {
let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) =
serde_json::from_str(&event.rumor.content).unwrap();
if validate_trade_index(index, &user, &sig, &event.sender, msg).await {
if let Err(e) = update_user_trade_index(pool, user, index).await {
tracing::error!("Error updating user: {:#?}", e);
} else {
tracing::info!("Updated user trade index");
}
} else {
tracing::info!("Invalid signature or trade index");
send_cant_do_msg(
None,
message_kind.id,
Some(CantDoReason::InvalidTradeIndex),
&event.sender,
)
.await;
}
}
Err(_) => {
if let Err(e) =
create_new_user_with_trade_index(pool, &event.sender.to_string(), index)
.await
{
tracing::error!("Error creating user: {:#?}", e);
} else {
tracing::info!("Added new user for rate");
}
}
}
}
}
}This refactoring breaks down check_trade_index into smaller, focused functions:
validate_trade_index: Validates the trade index and signature.update_user_trade_index: Updates the user's trade index in the database.create_new_user_with_trade_index: Creates a new user with the provided trade index.
This approach improves code readability and maintainability by separating concerns and making each function responsible for a single task.
There was a problem hiding this comment.
@arkanoider please give a look to this 😃
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
force-pushed
the
keys-management
branch
from
7c7fd6e to
617548e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app.rs (2)
209-215: Refactor: Extract message verification logicThe message verification logic could be extracted into a separate function to improve readability and reusability.
Suggested refactor:
+async fn verify_message(event: &UnwrappedGift) -> Option<(Message, Signature)> { + match serde_json::from_str(&event.rumor.content) { + Ok((message, sig)) => { + let inner_message = message.get_inner_message_kind(); + if !inner_message.verify_signature(event.rumor.pubkey, sig) { + tracing::warn!("Error in event verification"); + return None; + } + Some((message, sig)) + } + Err(e) => { + tracing::error!("Failed to parse message: {}", e); + None + } + } +} - let (message, sig): (Message, Signature) = - serde_json::from_str(&event.rumor.content).unwrap(); - let inner_message = message.get_inner_message_kind(); - if !inner_message.verify_signature(event.rumor.pubkey, sig) { - tracing::warn!("Error in event verification"); - continue; - } + let (message, sig) = match verify_message(&event).await { + Some(result) => result, + None => continue, + }; + let inner_message = message.get_inner_message_kind();Also applies to: 217-218
55-67: Add documentation for error casesWhile the function is well-documented for the happy path, it's missing documentation about error cases and the conditions under which
CantDomessages are sent.Add error cases to the documentation:
/// Function to check if a user is present in the database and update or create their trade index. /// /// This function performs the following tasks: /// 1. It checks if the action associated with the incoming message is related to trading (NewOrder, TakeBuy, or TakeSell). /// 2. If the user is found in the database, it verifies the trade index and the signature of the message. /// - If valid, it updates the user's trade index. /// - If invalid, it logs a warning and sends a message indicating the issue. /// 3. If the user is not found, it creates a new user entry with the provided trade index if applicable. +/// +/// # Error Handling +/// The function sends CantDo messages in the following cases: +/// - Invalid trade index (when the new index is not greater than the current one) +/// - Invalid signature (when message signature verification fails) +/// - Invalid message format (when message deserialization fails) /// /// # Arguments /// * `pool` - The database connection pool used to query and update user data. /// * `event` - The unwrapped gift event containing the sender's information. /// * `msg` - The message containing action details and trade index information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app.rs(2 hunks)
🔇 Additional comments (4)
src/app.rs (4)
37-37: LGTM: Import changes are well-structured
The new imports are properly organized and align with the added functionality for trade index verification and user management.
Also applies to: 41-41, 43-44, 47-47
74-75:
Critical: Replace unwrap() with proper error handling
The JSON deserialization could panic if the content is malformed. This is a critical reliability issue.
Apply this fix:
- let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) =
- serde_json::from_str(&event.rumor.content).unwrap();
+ let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) = match serde_json::from_str(&event.rumor.content) {
+ Ok(result) => result,
+ Err(e) => {
+ tracing::error!("Failed to parse signature: {}", e);
+ send_cant_do_msg(
+ None,
+ msg.get_inner_message_kind().id,
+ Some(CantDoReason::InvalidSignature),
+ &event.sender,
+ )
+ .await;
+ return;
+ }
+ };209-215:
Critical: Add error handling for message deserialization
Similar to the previous issue, using unwrap() here could cause runtime panics.
Apply this fix:
- let (message, sig): (Message, Signature) =
- serde_json::from_str(&event.rumor.content).unwrap();
+ let (message, sig): (Message, Signature) = match serde_json::from_str(&event.rumor.content) {
+ Ok(result) => result,
+ Err(e) => {
+ tracing::error!("Failed to parse message: {}", e);
+ continue;
+ }
+ };Likely invalid or redundant comment.
68-111: 🛠️ Refactor suggestion
Refactor: Split complex function into smaller units
The check_trade_index function has multiple responsibilities and deep nesting, making it harder to maintain and test. Consider breaking it down into smaller, focused functions.
Suggested refactor:
+async fn validate_trade_index(
+ index: i64,
+ user: &User,
+ sender: &XOnlyPublicKey,
+ sig: nostr_sdk::secp256k1::schnorr::Signature,
+ msg: &Message,
+) -> bool {
+ index > user.trade_index && msg.get_inner_message_kind().verify_signature(sender, sig)
+}
+
+async fn update_user_trade_index(
+ pool: &Pool<Sqlite>,
+ user: &mut User,
+ index: i64,
+) -> Result<(), sqlx::Error> {
+ user.trade_index = index;
+ user.update(pool).await
+}
+
+async fn create_new_user(
+ pool: &Pool<Sqlite>,
+ sender: String,
+ trade_index: i64,
+) -> Result<(), sqlx::Error> {
+ let new_user = User {
+ pubkey: sender,
+ trade_index,
+ ..Default::default()
+ };
+ new_user.create(pool).await
+}
+
async fn check_trade_index(pool: &Pool<Sqlite>, event: &UnwrappedGift, msg: &Message) {
let message_kind = msg.get_inner_message_kind();
if let Action::NewOrder | Action::TakeBuy | Action::TakeSell = message_kind.action {
match is_user_present(pool, event.sender.to_string()).await {
Ok(mut user) => {
if let (true, index) = message_kind.has_trade_index() {
- let (_, sig): (String, nostr_sdk::secp256k1::schnorr::Signature) =
- serde_json::from_str(&event.rumor.content).unwrap();
- if index > user.trade_index
- && msg
- .get_inner_message_kind()
- .verify_signature(event.sender, sig)
- {
- user.trade_index = index;
- if user.update(pool).await.is_ok() {
- tracing::info!("Update user trade index");
- }
+ let (_, sig) = match serde_json::from_str(&event.rumor.content) {
+ Ok(result) => result,
+ Err(e) => {
+ tracing::error!("Failed to parse signature: {}", e);
+ send_cant_do_msg(
+ None,
+ msg.get_inner_message_kind().id,
+ Some(CantDoReason::InvalidSignature),
+ &event.sender,
+ )
+ .await;
+ return;
+ }
+ };
+
+ if validate_trade_index(index, &user, &event.sender, sig, msg).await {
+ if let Err(e) = update_user_trade_index(pool, &mut user, index).await {
+ tracing::error!("Failed to update user trade index: {}", e);
+ } else {
+ tracing::info!("Updated user trade index");
+ }
} else {
tracing::info!("Invalid signature or trade index");
send_cant_do_msg(
None,
msg.get_inner_message_kind().id,
Some(CantDoReason::InvalidTradeIndex),
&event.sender,
)
.await;
}
}
}
Err(_) => {
if let (true, trade_index) = message_kind.has_trade_index() {
- let new_user = User {
- pubkey: event.sender.to_string(),
- trade_index,
- ..Default::default()
- };
- if new_user.create(pool).await.is_ok() {
- tracing::info!("Added new user for rate");
+ if let Err(e) = create_new_user(pool, event.sender.to_string(), trade_index).await {
+ tracing::error!("Failed to create new user: {}", e);
+ } else {
+ tracing::info!("Added new user");
}
}
}
}
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/db.rs (2)
314-328: Consider renaming the function to better reflect its behavior.The function name
is_user_presentsuggests a boolean return, but it actually returns the fullUserobject. Consider renaming it toget_user_by_pubkeyorfind_userto better reflect its behavior.-pub async fn is_user_present(pool: &SqlitePool, public_key: String) -> anyhow::Result<User> { +/// Retrieves a user by their public key +pub async fn get_user_by_pubkey(pool: &SqlitePool, public_key: String) -> anyhow::Result<User> {
Line range hint
279-293: Fix invalid SQL syntax in query conditions.There are syntax errors in the WHERE clauses that will cause query failures:
- WHERE invoice_held_at !=0 true AND status == 'active' + WHERE invoice_held_at != 0 AND status == 'active'- WHERE failed_payment == true AND status == 'settled-hold-invoice' + WHERE failed_payment == true AND status == 'settled-hold-invoice'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/db.rs(2 hunks)
🔇 Additional comments (2)
src/db.rs (2)
304-304: LGTM! Good optimization.
Adding LIMIT 1 is a good optimization since we only need one result, preventing unnecessary table scanning.
Line range hint 279-328: Verify the usage of these database queries.
Let's verify how these queries are used across the codebase to ensure all callers handle potential errors correctly.
✅ Verification successful
Let me gather more information about the error handling patterns and the Order struct to ensure completeness of the verification.
Based on the gathered information, I can now generate the final response:
Database queries are properly handled across the codebase
The verification shows that:
- All database queries return
anyhow::Resultand callers properly handle potential errors usingif let Ok(...)pattern - Query usage is consistent in both scheduler and main application logic
- Error cases are appropriately handled in all identified usage locations:
src/scheduler.rs: Safely handles failed payments with retry logicsrc/main.rs: Properly handles held invoices with null safety checkssrc/app.rs: Correctly handles user presence verificationsrc/app/admin_take_dispute.rs: Appropriately handles solver verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of affected database queries
# Check for calls to find_held_invoices and find_failed_payment
echo "Checking query usage patterns:"
rg -A 2 "find_held_invoices|find_failed_payment"
# Check for error handling patterns around these calls
echo "Checking error handling patterns:"
rg -B 2 -A 2 "find_held_invoices\(.*\)\.await|find_failed_payment\(.*\)\.await"
Length of output: 1847
Script:
#!/bin/bash
# Check Order struct definition and its usage
echo "Checking Order struct definition:"
ast-grep --pattern 'struct Order {
$$$
}'
# Check for any other potential error handling patterns
echo "Checking additional error handling patterns:"
rg -B 2 -A 2 "is_user_present|find_solver_pubkey"
# Check for any transaction or rollback patterns
echo "Checking transaction patterns:"
rg "begin_transaction|rollback|commit"
Length of output: 2861
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
You have to relaunch |
Sorry @Catrya we need to add back the two values in db |
There was a problem hiding this comment.
Creating a new order works! But:
-
When doing
getdmin mostro-cli it saysNo new messagesbut I see in the mostrod logs the messages being sent, so I don't know if it's a mostrod or mostro-cli problem -
When the user sends
take-buy, mostrod responds withcant-do, mostro cli doesn't show the messages but it appears in the mostrod logs:
2024-12-11T22:25:20.677121Z INFO mostrod::util: Sending DM, Event ID: c9c78890117ef6b0255412466adb2228cc49b9c61719774103fd1eef824c6a93 with payload: "{\"cant-do\,\"id\":\"02e688c5-1c00-46b2-87b8-9f0d44033166\",\"action\":\"cant-do\",\"payload\":{\"cant_do\":null}}}" -
When the user sends
take-sell, mostrod does nothing, nothing appears in the logs, the message being sent with mostro-cli is:
{
"order": {
"version": 1,
"request_id": null,
"trade_index": 1,
"action": "take-sell",
"payload": null
}
}
( I see that because I put this in my mostro-cli:
if let Ok(json_content) = serde_json::from_str::<Value>(&payload) {
if let Ok(pretty_content) = serde_json::to_string_pretty(&json_content){
println!("{}", pretty_content);}
}
)
Great! Thanks @Catrya will try as soon as I am at home. Probably some of my checks for trade index are not ok now. |
* bumps to mostro core v0.6.16 * add new fields on users table, fields related to user rating * fix bug parsing message and content on app.rs * change all dm destination, now is sending to the trade key
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
migrations/20231005195154_users.sql (1)
8-8: Consider adding an index on last_trade_indexSince
last_trade_indexwill likely be frequently queried for trade operations, consider adding an index to improve query performance.+ CREATE INDEX idx_users_last_trade_index ON users(last_trade_index);src/app/admin_add_solver.rs (2)
22-27: Improve error message for payload validationThe error message "No pubkey found!" doesn't clearly indicate whether the payload is missing or invalid.
- error!("No pubkey found!"); + error!("Invalid or missing payload in message");
50-52: Consider extracting message construction to a separate functionThe message construction logic could be moved to a dedicated function for better reusability and maintainability.
+ fn create_admin_solver_message(request_id: Option<String>) -> Result<String> { + let message = Message::new_dispute(None, request_id, None, Action::AdminAddSolver, None); + message.as_json() + } + - let message = Message::new_dispute(None, request_id, None, Action::AdminAddSolver, None); - let message = message.as_json()?; + let message = create_admin_solver_message(request_id)?;src/app/rate_user.rs (2)
82-82: Consider adding specific error messagesWhile the
send_cant_do_msgcalls are functionally correct, consider adding more specific error messages to help with debugging:-send_cant_do_msg(request_id, Some(order.id), None, &event.rumor.pubkey).await; +send_cant_do_msg(request_id, Some(order.id), Some("Invalid order status for rating"), &event.rumor.pubkey).await;Also applies to: 103-103
174-176: Consider potential race condition in rating updatesThe rating update logic could be subject to race conditions if multiple ratings are processed simultaneously. Consider implementing optimistic locking:
+ // Add version field to Rating struct for optimistic locking + let current_version = reputation.version; + reputation.version += 1; + + // Use a transaction to ensure atomic updates + let mut tx = pool.begin().await?; + update_user_rating_event( &counterpart, update_buyer_rate, update_seller_rate, reputation, order.id, my_keys, pool, rate_list, ).await?; + + tx.commit().await?;src/app/add_invoice.rs (1)
Line range hint
71-91: Enhance invoice validation with amount toleranceThe current invoice validation is strict about matching the exact amount. Consider adding a small tolerance for lightning network fee variations:
match is_valid_invoice( payment_request.clone(), - Some(order.amount as u64), + Some((order.amount as f64 * 1.01) as u64), // Allow 1% tolerance Some(order.fee as u64), ).awaitsrc/app/dispute.rs (1)
Line range hint
219-227: Enhance security token generationThe current implementation uses a 3-digit random number for security tokens, which might be insufficient for security purposes. Consider:
- Increasing the token length
- Using a cryptographically secure random number generator
- Adding expiration time for tokens
- dispute.buyer_token = Some(rng.gen_range(100..=999)); - dispute.seller_token = Some(rng.gen_range(100..=999)); + use rand::rngs::OsRng; + use rand::distributions::Uniform; + + let token_range = Uniform::new(100_000, 999_999); + let mut rng = OsRng; + dispute.buyer_token = Some(token_range.sample(&mut rng)); + dispute.seller_token = Some(token_range.sample(&mut rng)); + dispute.token_expiry = Some(chrono::Utc::now() + chrono::Duration::hours(24));src/app.rs (3)
55-111: Consider refactoring for better maintainabilityThe function has high cyclomatic complexity with nested conditionals. Consider breaking it down into smaller, focused functions:
validate_trade_index_and_signatureupdate_user_trade_indexcreate_new_user_with_trade_indexAlso, improve error handling for database operations:
- if user.update(pool).await.is_ok() { + match user.update(pool).await { + Ok(_) => { tracing::info!("Update user trade index"); + } + Err(e) => { + tracing::error!("Failed to update user trade index: {}", e); + } }
Line range hint
167-167: Consider adding validation for trade_index parameterThe
publish_orderfunction now accepts atrade_indexparameter but doesn't validate its value. Consider adding validation to ensure the trade index is positive when provided.+ if let Some(index) = trade_index { + if index < 0 { + return Err(anyhow::anyhow!("Trade index must be positive")); + } + }Also applies to: 169-209
Line range hint
593-593: Improve error messaging in send_cant_do_msg callsThe
send_cant_do_msgcalls don't provide specific reasons for the failure. Consider adding appropriateCantDoReasonvalues:- send_cant_do_msg(request_id, Some(order.id), None, &event.rumor.pubkey).await; + send_cant_do_msg( + request_id, + Some(order.id), + Some(CantDoReason::Unauthorized), + &event.rumor.pubkey + ).await; - send_cant_do_msg(request_id, Some(order.id), None, &event.rumor.pubkey).await; + send_cant_do_msg( + request_id, + Some(order.id), + Some(CantDoReason::InvalidPreimage), + &event.rumor.pubkey + ).await;Also applies to: 602-602
src/util.rs (2)
167-167: Add documentation for the trade_index parameter.The
trade_indexparameter has been added to both functions, but its purpose and usage should be documented. Consider adding doc comments explaining:
- The purpose of the trade index
- When it's expected to be Some/None
- How it's used differently for buyers vs sellers
Also applies to: 211-215
752-763: Add test cases for error scenarios in send_dm.The current test only covers the success path. Consider adding test cases for:
- Invalid receiver public key
- Failed event wrapping
- Failed client initialization
📜 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 (15)
Cargo.toml(2 hunks)migrations/20231005195154_users.sql(1 hunks)src/app.rs(2 hunks)src/app/add_invoice.rs(6 hunks)src/app/admin_add_solver.rs(3 hunks)src/app/admin_take_dispute.rs(5 hunks)src/app/cancel.rs(11 hunks)src/app/dispute.rs(5 hunks)src/app/fiat_sent.rs(4 hunks)src/app/order.rs(6 hunks)src/app/rate_user.rs(5 hunks)src/app/release.rs(5 hunks)src/app/take_buy.rs(4 hunks)src/app/take_sell.rs(5 hunks)src/util.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/fiat_sent.rs
- src/app/take_buy.rs
- src/app/release.rs
- src/app/take_sell.rs
- src/app/admin_take_dispute.rs
- src/app/cancel.rs
- src/app/order.rs
🔇 Additional comments (13)
migrations/20231005195154_users.sql (1)
3-3: Verify existing data before adding unique constraint
Adding a unique constraint to the existing pubkey column could fail if duplicate values exist. Ensure the migration handles existing data appropriately.
Cargo.toml (1)
20-20: Verify compatibility and security of dependency updates
Several key dependencies have been updated:
- nostr-sdk: 0.36.0 → 0.37.0 (+ nip59 feature)
- mostro-core: 0.6.11 → 0.6.16
- lnurl-rs: 0.8.0 → 0.9.0
- Added bitcoin: 0.32.5
Also applies to: 41-41, 46-46, 49-49
✅ Verification successful
Dependencies updated with minimal risk
Based on the evidence gathered:
- nostr-sdk 0.37.0 is a feature release with NIP support additions and no breaking changes
- The new bitcoin dependency and updated dependencies are used consistently in the codebase
- Code changes show proper integration with bitcoin price functionality
- The version increments are minor, suggesting backward compatibility
The dependency updates appear safe to proceed with. The nostr-sdk changelog explicitly confirms no breaking changes, and the codebase changes align with the dependency updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories and breaking changes
echo "Checking for security advisories..."
cargo audit
# Check compatibility of updated dependencies
echo "Checking dependency compatibility..."
cargo check
Length of output: 383
Script:
#!/bin/bash
# Let's check the git diff to understand the exact dependency changes
git diff origin/main Cargo.toml
# Also check if there are any breaking changes mentioned in the PR description or commit messages
git log --grep="BREAKING" --grep="breaking" -i HEAD^..HEAD
Length of output: 1013
Script:
#!/bin/bash
# Let's check the changelog or release notes for these dependencies in their repositories
gh api repos/rust-nostr/nostr/releases/tags/v0.37.0 || true
gh api repos/MostroP2P/mostro-core/releases/tags/v0.6.16 || true
gh api repos/rust-bitcoin/rust-bitcoin/releases/tags/0.32.5 || true
gh api repos/lnurl-rs/lnurl-rs/releases/tags/v0.9.0 || true
# Also check if there are any related changes in the codebase that might indicate breaking changes
rg -A 3 "nostr-sdk|mostro-core|lnurl-rs|bitcoin" --type rust
Length of output: 4609
src/app/admin_add_solver.rs (1)
41-43:
Add validation for trade_index value
The trade_index is unwrapped without proper validation.
- let trade_index = inner_message.trade_index.unwrap_or(0);
+ let trade_index = match inner_message.trade_index {
+ Some(index) if index >= 0 => index,
+ Some(_) => {
+ error!("Invalid negative trade_index provided");
+ return Ok(());
+ }
+ None => 0,
+ };src/app/rate_user.rs (1)
5-5: LGTM: Import statement updated consistently
The change from Content to Payload aligns with the codebase-wide transition in message handling.
src/app/dispute.rs (1)
14-14: Verify consistent Payload usage across the codebase
Let's verify that all message handling has been updated to use the new Payload type consistently.
✅ Verification successful
All message payloads consistently use the new Payload type
The verification shows that all message handling across the codebase has been properly updated to use the new Payload enum variants (RatingUser, Order, and Dispute). No instances of the old Content type were found, and the Payload usage is consistent across all relevant files:
src/app/dispute.rs: UsesPayload::Disputesrc/app/rate_user.rs: UsesPayload::RatingUsersrc/app/add_invoice.rsandsrc/app/admin_take_dispute.rs: UsePayload::Ordersrc/flow.rsandsrc/util.rs: Consistently usePayload::Order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining uses of the old Content type
rg "use mostro_core::message::.*Content"
# Check for consistent Payload usage in message handling
rg "Payload::(RatingUser|Order|Dispute)"
Length of output: 946
src/app.rs (5)
Line range hint 247-247: LGTM: Trade index assignment is correctly handled
The trade index is properly assigned based on the order type (buy/sell).
Also applies to: 250-250
Line range hint 674-680: LGTM: Proper type usage for relay URLs
The function now correctly uses RelayUrl instead of Url for better type safety.
Line range hint 753-763: LGTM: Tests updated to reflect new message structure
The test cases have been properly updated to use the new message structure and payload types.
74-75:
Critical: Add error handling for JSON deserialization
The unwrap call could lead to runtime panics if the content is malformed.
209-215:
Add proper error handling for message parsing
The unwrap call on JSON deserialization could cause runtime panics. Consider adding proper error handling:
- let (message, sig): (Message, Signature) =
- serde_json::from_str(&event.rumor.content).unwrap();
+ let (message, sig): (Message, Signature) = serde_json::from_str(&event.rumor.content)
+ .map_err(|e| {
+ tracing::error!("Failed to parse message content: {}", e);
+ return;
+ })?;Likely invalid or redundant comment.
src/util.rs (3)
17-18: LGTM! Dependencies updated correctly.
The new imports for CantDoReason and Payload from mostro-core align with the changes in message handling.
Line range hint 615-626: LGTM! Error handling improved with CantDoReason.
The update to use CantDoReason instead of a generic message improves error handling by providing more structured and type-safe error responses.
Line range hint 674-680: LGTM! Type safety improved with RelayUrl.
The change from using generic Url to specific RelayUrl type improves type safety and makes the API more precise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/util.rs (1)
167-209: Add documentation for the trade_index parameter.The new
trade_indexparameter's purpose and usage should be documented for better maintainability.+/// Publishes a new order to the network +/// +/// # Arguments +/// * `pool` - The database connection pool +/// * `keys` - The nostr keys for signing +/// * `new_order` - The order details to publish +/// * `initiator_pubkey` - The public key of the order initiator +/// * `ack_pubkey` - The public key to send acknowledgment to +/// * `request_id` - Optional request identifier +/// * `trade_index` - Optional trade sequence number for order tracking pub async fn publish_order( pool: &SqlitePool, keys: &Keys, new_order: &SmallOrder, initiator_pubkey: &str, ack_pubkey: PublicKey, request_id: Option<u64>, trade_index: Option<i64>, ) -> Result<()> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/rate_user.rs(5 hunks)src/util.rs(14 hunks)
🔇 Additional comments (9)
src/util.rs (4)
Line range hint 211-256: LGTM! Trade index handling is implemented correctly.
The function properly stores the trade_index for both buyer and seller scenarios.
271-273: Error handling improvement implemented as suggested.
The changes implement the previously suggested error handling for the send_event operation.
Line range hint 619-629: LGTM! Good improvement in error handling structure.
The change from using raw strings to a structured CantDoReason enum improves type safety and maintainability.
Line range hint 676-683: Verify RelayUrl type compatibility across the codebase.
The change from Url to RelayUrl improves type safety but requires verification of all usage sites.
✅ Verification successful
Let me gather more information to verify the type compatibility thoroughly.
Let me check one more aspect to ensure we have a complete picture of the type usage.
Based on the gathered information, I can now provide the final response:
The RelayUrl type change is safe and properly integrated
The change from Url to RelayUrl is verified to be safe because:
- The
get_nostr_relays()function is only used injob_relay_list()in scheduler.rs - The return type
HashMap<RelayUrl, Relay>is properly handled in the consuming code - No other direct usages of the old
Urltype with relays were found - The relay data is only used for metadata tags and connection status checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of generic Url type with relays
rg -l 'use.+url::Url' | xargs -I {} rg 'Url.*Relay' {}
# Search for all relay URL handling code
rg -l 'RelayUrl' | xargs -I {} rg 'Relay.*Url' {}
Length of output: 182
Script:
#!/bin/bash
# Search for all usages of RelayUrl type
ast-grep --pattern 'RelayUrl'
# Search for any function calls to get_nostr_relays
ast-grep --pattern 'get_nostr_relays()'
# Search for any HashMap usage with Relay types
rg 'HashMap.*Relay'
Length of output: 256
Script:
#!/bin/bash
# Check imports and type definitions related to RelayUrl
rg -A 3 'type RelayUrl'
# Check the scheduler.rs file for how it handles the relay data
cat src/scheduler.rs
# Look for any other Relay related type definitions
rg 'struct.*Relay|enum.*Relay'
Length of output: 14251
src/app/rate_user.rs (5)
5-5: LGTM: Import addition aligns with payload structure changes
The addition of Payload import aligns with the codebase-wide transition from Content to Payload type for message handling.
82-82: LGTM: Appropriate error handling for invalid order status
The error handling correctly sends a CantDo message and logs the error when the order status is invalid.
103-103: LGTM: Appropriate error handling for invalid counterpart
The error handling correctly sends a CantDo message when no valid counterpart is found.
123-129: LGTM: Rating validation implemented as suggested
The implemented rating validation matches the previous review suggestion, ensuring ratings fall within the defined range with clear error messages.
180-182: Verify the updated send_new_order_msg parameters
The changes to send_new_order_msg include new parameters for payload and an additional None value. Let's verify the function signature matches these changes.
✅ Verification successful
Based on the search results, I can now verify the function signature and its usage. The send_new_order_msg function in src/util.rs has the following signature:
pub async fn send_new_order_msg(
request_id: Option<u64>,
order_id: Option<Uuid>,
action: Action,
payload: Option<Payload>,
destination_key: &PublicKey,
)Looking at the code changes in the review and comparing with other usages in the codebase, particularly the RateReceived action cases, the parameters match the function signature correctly:
Some(Payload::RatingUser(rating)), // payload parameter
&event.sender, // destination_key parameter
None, // request_id parameterThe parameter changes are correct and consistent with the function signature
The changes align with other similar usages in the codebase, particularly for rating-related actions where Payload::RatingUser is used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for send_new_order_msg function definition
ast-grep --pattern 'fn send_new_order_msg($$$)'
Length of output: 49
Script:
#!/bin/bash
# Search for send_new_order_msg function with ripgrep for better coverage
rg "send_new_order_msg" -A 5
Length of output: 19854
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util.rs (1)
266-266: Avoid logging sensitive information in production logsWhile public keys are public, logging them may expose user identities and could be a privacy concern. Consider removing or sanitizing the log statement or adjusting the log level to debug to prevent unnecessary exposure of user information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/take_sell.rs(7 hunks)src/util.rs(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/take_sell.rs
|
looked back to all new management of keys in CLI and mostro to get all the new things in my mind, now i put in the above commit a small fix that should solve the issue that when a user Going on with understanding of all the flow with new Please take a look! |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/util.rs (1)
Based on the gathered information, I can now provide the final response:
CantDoReason is underutilized in error handling
The codebase shows inconsistent usage of the CantDoReason enum. While structured reasons like
InvalidTradeIndex,InvalidOrderKind, andInvalidInvoiceare used in some places, mostsend_cant_do_msgcalls passNonefor the reason parameter, missing opportunities for more descriptive error handling. Key observations:
- Only 3 instances use structured CantDoReason variants
- 20+ calls to send_cant_do_msg pass None as the reason
- Common error scenarios like "Not allowed", "No preimage", and "wrong status" lack corresponding CantDoReason variants
Suggested improvements:
- Add new CantDoReason variants for common error cases (NotAllowed, NoPreimage, InvalidStatus, etc.)
- Replace None reasons with appropriate CantDoReason variants throughout the codebase
- Consider adding a reason parameter to Error::msg calls that lead to cant_do messages
🔗 Analysis chain
Line range hint
645-655: Verify CantDoReason handling across the codebase.The change from generic messages to structured
CantDoReasonis a good improvement. Let's verify that all error cases are properly mapped to the new enum.
Let me gather more context about the CantDoReason enum definition and its variants to ensure all error cases are properly mapped.
Let me try a different approach to find the enum definition and error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances where send_cant_do_msg is called to verify CantDoReason usage # Search for send_cant_do_msg calls rg "send_cant_do_msg" -A 2 # Search for CantDoReason enum usage ast-grep --pattern 'CantDoReason::$_'Length of output: 6368
Script:
#!/bin/bash # Find the CantDoReason enum definition rg "enum CantDoReason" -A 10 # Find all error messages in the codebase that lead to send_cant_do_msg calls rg "Error::msg\(.*\).*send_cant_do_msg" -B 2 -A 2Length of output: 82
Script:
#!/bin/bash # Search for CantDoReason in the entire codebase to find its definition rg "CantDoReason" -B 2 -A 2 # Find error patterns near send_cant_do_msg calls rg "send_cant_do_msg" -B 3 -A 1Length of output: 11277
🧹 Nitpick comments (5)
src/error.rs (2)
19-19: Consider adding context to the InvalidOrderKind error variantWhile the error variant is well-named, consider enhancing it to include the actual invalid order kind for better debugging and logging:
- InvalidOrderKind, + InvalidOrderKind(String),This would allow you to provide more specific error messages about which order kind was invalid and why.
41-41: Enhance error message to include more contextIf you implement the suggested enhancement to
InvalidOrderKind, update the Display implementation to include the context:- MostroError::InvalidOrderKind => write!(f, "Invalid order kind"), + MostroError::InvalidOrderKind(kind) => write!(f, "Invalid order kind: {}", kind),This would provide more detailed error messages for debugging and logging purposes.
src/util.rs (3)
232-235: Remove redundant default OrderKind assignment.The default OrderKind::Sell assignment is immediately overwritten by the match statement below. Consider removing it from the struct initialization.
let mut new_order_db = Order { id: Uuid::new_v4(), - kind: OrderKind::Sell.to_string(), + kind: String::new(), status: Status::Pending.to_string(),
251-272: Consider using Result instead of Option for better error handling.The function returns
Option<Order>and uses early returns withNone. Consider usingResult<Order, MostroError>to provide more context about why the order preparation failed.-async fn prepare_new_order( +async fn prepare_new_order( new_order: &SmallOrder, initiator_pubkey: &str, trade_index: Option<i64>, trade_pubkey: PublicKey, -) -> Option<Order> { +) -> Result<Order, MostroError> { // ... existing code ... match new_order.kind { Some(OrderKind::Buy) => { new_order_db.kind = OrderKind::Buy.to_string(); new_order_db.buyer_pubkey = Some(trade_pubkey.to_string()); new_order_db.trade_index_buyer = trade_index; } Some(OrderKind::Sell) => { new_order_db.kind = OrderKind::Sell.to_string(); new_order_db.seller_pubkey = Some(trade_pubkey.to_string()); new_order_db.trade_index_seller = trade_index; } None => { send_cant_do_msg( None, None, Some(CantDoReason::InvalidOrderKind), &trade_pubkey, ) .await; - return None; + return Err(MostroError::InvalidOrderKind); } } - Some(new_order_db) + Ok(new_order_db)
781-791: Add test coverage for prepare_new_order function.The new
prepare_new_orderfunction lacks dedicated test coverage. Consider adding test cases for:
- Valid Buy/Sell orders
- Invalid order kind
- Trade index handling
#[tokio::test] async fn test_prepare_new_order() { let new_order = SmallOrder { kind: Some(OrderKind::Buy), // ... other fields }; let trade_pubkey = Keys::generate().public_key(); let result = prepare_new_order(&new_order, "pubkey", Some(1), trade_pubkey).await; assert!(result.is_some()); // Add more assertions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/error.rs(2 hunks)src/util.rs(15 hunks)
🔇 Additional comments (2)
src/error.rs (1)
19-19: Verify the usage of InvalidOrderKind error
Let's ensure this error is properly integrated with the order processing logic mentioned in the PR objectives.
Also applies to: 41-41
✅ Verification successful
Let me gather more context about the usage of CantDoReason and its relationship with order processing.
Let me check for the order kind validation and error handling patterns in the codebase.
Based on the comprehensive analysis of the codebase, I can now provide a final response:
InvalidOrderKind error is properly integrated with order processing
The error is correctly used in the order validation flow:
- It's defined in the error enum and has a proper error message implementation
- It's used through
CantDoReason::InvalidOrderKindwhen validating order kinds - The validation occurs during order processing, specifically when handling new orders and verifying their kind (buy/sell)
- The error is properly propagated to users through the
send_cant_do_msgmechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how InvalidOrderKind is used in order processing code
# Look for files handling order types
echo "Files potentially handling order types:"
fd -e rs | rg -l "new-order|take-sell|take-buy"
# Check for InvalidOrderKind usage
echo -e "\nInvalidOrderKind usage:"
rg "InvalidOrderKind" -B 2 -A 2
Length of output: 949
Script:
#!/bin/bash
# Check CantDoReason enum definition and usage
echo "CantDoReason definition and usage:"
ast-grep --pattern 'enum CantDoReason { $$$ }'
echo -e "\nContext around CantDoReason::InvalidOrderKind usage:"
rg "CantDoReason::InvalidOrderKind" -B 5 -A 5
echo -e "\nFunctions handling order kinds:"
rg "fn.*order.*kind" -i
Length of output: 787
Script:
#!/bin/bash
# Look for order kind validation and related error handling
echo "Order kind validation patterns:"
rg "order.*kind" -B 3 -A 3
echo -e "\nError handling patterns with InvalidOrderKind:"
rg "InvalidOrderKind" --type rust -C 10
echo -e "\nCant-do message handling:"
rg "send_cant_do_msg" -B 5 -A 5
Length of output: 34408
src/util.rs (1)
297-299: LGTM! Error handling improvement implemented.
The error handling for send_event has been improved as suggested in the past review.
|
Some sparse questions, now in the struct below we still need Another one, in pub struct Order {
pub id: Uuid,
pub kind: String,
pub event_id: String,
pub hash: Option<String>,
pub preimage: Option<String>,
pub creator_pubkey: String,
pub cancel_initiator_pubkey: Option<String>,
pub buyer_pubkey: Option<String>,
pub master_buyer_pubkey: Option<String>,
pub seller_pubkey: Option<String>,
pub master_seller_pubkey: Option<String>,
pub status: String,
pub price_from_api: bool,
pub premium: i64,
pub payment_method: String,
pub amount: i64,
pub min_amount: Option<i64>,
pub max_amount: Option<i64>,
pub buyer_dispute: bool,
pub seller_dispute: bool,
pub buyer_cooperativecancel: bool,
pub seller_cooperativecancel: bool,
pub fee: i64,
pub routing_fee: i64,
pub fiat_code: String,
pub fiat_amount: i64,
pub buyer_invoice: Option<String>,
pub range_parent_id: Option<Uuid>,
pub invoice_held_at: i64,
pub taken_at: i64,
pub created_at: i64,
pub buyer_sent_rate: bool,
pub seller_sent_rate: bool,
pub failed_payment: bool,
pub payment_attempts: i64,
pub expires_at: i64,
pub trade_index_seller: Option<i64>,
pub trade_index_buyer: Option<i64>,
} |
Summary by CodeRabbit
Release Notes
New Features
trade_indexparameter for better tracking in multiple functions.is_user_presentfunction to check user existence in the database.Improvements
ContentwithPayload.Database Changes
ordersanduserstables to include new columns.Bug Fixes