fix(take_order): correct amount label and help text for takebuy/takesell#158
fix(take_order): correct amount label and help text for takebuy/takesell#158codaMW wants to merge 3 commits intoMostroP2P:mainfrom
Conversation
When taking a buy order, the -a flag represents a fiat amount, not a sats amount. The display table was incorrectly labeling it as 'Amount (sats)' for both takebuy and takesell. Also fixed action match to use references (&action) to avoid moving a non-Copy enum value before second match. Now shows 'Fiat Amount' for TakeBuy and 'Amount (sats)' for TakeSell.
- takebuy now shows 'Fiat Amount' instead of 'Amount (sats)' in the confirmation table, since -a represents fiat not sats for this command - Fixed action match to use &action references to avoid moving a non-Copy enum before the second match - Updated help text for takesell -a to clarify user is paying fiat to buy sats from the seller - Updated help text for takebuy -a to clarify user is receiving fiat in exchange for selling sats to the buyer Matches the language used in the Mostro mobile app where buy orders show 'Someone is Buying Sats' and sell orders show 'Someone is Selling Sats'
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughClarified CLI help text for TakeSell and TakeBuy to explain fiat amount semantics; adjusted take_order display logic to borrow the action value and conditionally label the amount row as "Fiat Amount" for TakeBuy/TakeSell and "Amount (sats)" for other actions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/take_order.rs`:
- Around line 82-89: The confirmation table's amount_label match in
take_order.rs currently uses "Fiat Amount" only for Action::TakeBuy and a
generic "Amount (sats)" for all other actions, which contradicts the updated
fiat semantics for TakeSell; update the match on action (used where amount_label
is defined) to explicitly handle Action::TakeSell (and Action::TakeBuy) so both
use "Fiat Amount", and keep "Amount (sats)" for true sat-denominated actions
only, ensuring the label shown in create_emoji_field_row matches the -a help
text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ac9478c-2c29-4f86-ace0-6192081209a2
📒 Files selected for processing (2)
src/cli.rssrc/cli/take_order.rs
TakeSell -a also accepts a fiat amount, not sats. Updated the amount label match to show 'Fiat Amount' for both TakeBuy and TakeSell actions as suggested in code review.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
The help text fixes and the borrow fix are correct. One issue worth clarifying before merging.
Issue: TakeSell with invoice + amount may show wrong label
The new label logic groups both TakeBuy and TakeSell under "Fiat Amount":
Action::TakeBuy | Action::TakeSell => "Fiat Amount",For takebuy, the -a flag always accepts a fiat amount, so "Fiat Amount" is correct.
For takesell, the semantics depend on whether an invoice is also provided. Looking at create_take_order_payload:
takesellwithout invoice + with amount → amount is fiat (correct,"Fiat Amount"applies)takesellwith invoice + with amount → amount maps toPayload::PaymentRequest(..., Some(amt)), which in that context represents the sats amount the buyer wants to receive, not a fiat amount
In that second case, showing "Fiat Amount" in the confirmation table is misleading.
Suggestion: either restrict the label change to TakeBuy only (since TakeSell already showed "Amount (sats)" which was closer to correct for the invoice+amount path), or differentiate the label based on both action and whether an invoice is present.
The PR description says takesell "correctly shows the sats amount" in the original code — so the original label for TakeSell may have actually been intentional.
Everything else LGTM: the borrow fix is correct, the help text improvements are clear and consistent with Mostro Mobile language.
1 participant
Problem
Two related UX issues were found when testing takebuy and takesell commands:
The confirmation table for
takebuyshowedAmount (sats)but the-aflag accepts a fiat amount, not sats. This was misleading andinconsistent with
takesellwhich correctly shows the sats amount.The help text for
-aon both commands was confusing:takesell -asaid "Amount of fiat to buy" (ambiguous)takebuy -asaid "Amount of fiat to sell" (ambiguous)Changes
takebuyconfirmation table now showsFiat Amountinstead ofAmount (sats)match actionto use&actionreferences to avoid moving anon-Copy enum before second match
takesell -ahelp text now reads: "Amount of fiat to pay (someone is selling sats, you are buying)"takebuy -ahelp text now reads: "Amount of fiat to receive (someone is buying sats, you are selling)"Testing
Tested against staging Mostro pubkey:
mostro-cli takebuy -o <order-id> -a 5confirms label showsFiat Amountmostro-cli takesell -o <order-id> -a 5confirms label still showsAmount (sats)mostro-cli takebuy -hshows updated help textmostro-cli takesell -hshows updated help textNotes
Help text language matches the Mostro mobile app which uses
"Someone is Buying Sats" for buy orders and "Someone is Selling Sats"
for sell orders.
Summary by CodeRabbit
Bug Fixes
Documentation