disallow premium with fixed order #400
Conversation
WalkthroughThe changes made in Changes
Assessment against linked issues
📜 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/order.rs(1 hunks)
🔇 Additional comments (2)
src/app/order.rs (2)
70-74: Implementation successfully addresses PR objectives.
The validation check successfully prevents the creation of orders with both fixed price and premium, directly addressing issue #397. The placement of the check is appropriate within the order processing flow.
70-70: Consider additional validation for premium orders.
The current check only validates when all three fields are non-zero, but we might want to prevent any premium when fixed prices are set.
Let's verify if there are other premium-related validations in the codebase:
Consider a more strict validation:
- if order.premium != 0 && order.amount !=0 && order.fiat_amount !=0 {
+ if order.premium != 0 && (order.amount != 0 || order.fiat_amount != 0) {This would prevent any premium when either fixed amount is set, which might better align with the business logic.
src/app/order.rs
Outdated
| if order.premium != 0 && order.amount !=0 && order.fiat_amount !=0 { | ||
| send_cant_do_msg(None, None, &event.sender).await; | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for premium validation.
While the validation logic correctly implements the business rule, the error handling could be more informative and maintain context.
Consider applying these improvements:
- if order.premium != 0 && order.amount !=0 && order.fiat_amount !=0 {
- send_cant_do_msg(None, None, &event.sender).await;
+ if order.premium != 0 && order.amount != 0 && order.fiat_amount != 0 {
+ send_new_order_msg(
+ request_id,
+ order.id,
+ Action::InvalidOrderCombination,
+ Some("Orders cannot have both fixed price and premium".to_string()),
+ &event.sender,
+ )
return Ok(());
}This change:
- Preserves the request_id and order.id context
- Uses a more specific error message
- Uses
send_new_order_msgfor consistent error handling
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
waiting for it to be decided whether to do #393 (comment)
There was a problem hiding this comment.
Apart from cosmetics of the code it makes sense to me.
Waiting for @grunch opinion
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!
|
@coderabbitai can you refactor the new code of this PR to be more rust idiomatic? |
|
Tip For best results, initiate chat on the files or code changes. @grunch: Certainly! Here’s how you can refactor the new code to be more idiomatic in Rust: Encapsulate the condition in a method By creating a method within the impl Order {
fn has_premium_and_fixed_amounts(&self) -> bool {
self.premium != 0 && self.amount != 0 && self.fiat_amount != 0
}
}Then, update the condition in your if order.has_premium_and_fixed_amounts() {
send_cant_do_msg(request_id, None, None, &event.sender).await;
return Ok(());
}Use pattern matching with If if let (Some(premium), Some(amount), Some(fiat_amount)) = (order.premium, order.amount, order.fiat_amount) {
send_cant_do_msg(request_id, None, None, &event.sender).await;
return Ok(());
}This ensures the code only executes when all fields are Leverage the Alternatively, you can use the if order.premium.is_some() && order.amount.is_some() && order.fiat_amount.is_some() {
send_cant_do_msg(request_id, None, None, &event.sender).await;
return Ok(());
}This approach is clean and directly expresses the intent to check for the presence of values. Let me know if you need further assistance! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 613 |
fix #397
Now Mostro will not allow the creation of orders that have a fixed price and a premium at the same time
Summary by CodeRabbit