Fix: missing order id in take-sell #95
Conversation
WalkthroughThe pull request modifies the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/cli/take_sell.rs (1)
60-61: Consider handling JSON serialization errors.The
unwrap()call onas_json()could potentially panic if serialization fails. Consider using proper error handling:- .as_json() - .unwrap(); + .as_json() + .map_err(|e| anyhow::anyhow!("Failed to serialize take sell message: {}", e))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/take_sell.rs(1 hunks)
🔇 Additional comments (2)
src/cli/take_sell.rs (2)
53-61: LGTM! The order_id is now properly included.
The change correctly includes the order_id in the Message::new_order construction, fixing the reported issue.
53-59: Verify order_id handling in related components.
Since this change affects the order cycle, let's verify the order_id handling in related components.
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response:
Order ID handling is consistent across all message types
The verification shows that order_id handling is consistent across all components:
- All take_sell/take_buy operations correctly pass Some(order_id)
- New orders appropriately use None for order_id (as they're creating new orders)
- All order-modifying actions (FiatSent, Release, Cancel, etc.) properly include the order_id
- The send_order_id_cmd utility is consistently used across all order-related operations
The changes in the take_sell.rs file align with the established pattern of order_id handling throughout the codebase, maintaining consistency in the order cycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Message::new_order usages to ensure consistent order_id handling
ast-grep --pattern 'Message::new_order($_,$_,$_,$_,$_)'
# Search for order cycle related tests
rg -l "order.*cycle.*test"
Length of output: 1614
Script:
#!/bin/bash
# Let's check the Message struct definition and new_order implementation
ast-grep --pattern 'struct Message'
# Let's also check for order cycle related tests with a broader pattern
rg -l "test.*order.*cycle|cycle.*order.*test"
# Let's check Action enum definition
ast-grep --pattern 'enum Action'
# Let's check if there are any order cycle related functions
rg -l "order.*cycle|cycle.*order"
Length of output: 156
Script:
#!/bin/bash
# Let's try a different approach to find Message and Action definitions
rg -A 5 "^(pub )?struct Message"
rg -A 10 "^(pub )?enum Action"
# Let's look for order_id usage patterns
rg "order_id" -A 2 -B 2
# Let's check for any test files
fd "test.*\.rs$"
Length of output: 12380
Hi @grunch,
starting back to test order cycle, this I think is a missing 'order-id' in take-sell.
Summary by CodeRabbit
Bug Fixes
order_idin the message.Chores