From 71f33363870b2b4aad2cfa3e8daf4f199f66c0dc Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 16 Jun 2026 17:27:39 +0700 Subject: [PATCH] fix(coin): sell command accepts USD amount with dust and zero-price guards - Change /coin_sell semantics from quantity to USD amount - Add price-validity and dust-quantity guards to buy/sell - Improve usage text and error messages - Add tests for invalid amount, dust, zero price, and insufficient holdings --- aws/telegram-commands.json | 2 +- internal/modules/coin/coin.go | 2 +- internal/modules/coin/handlers.go | 32 ++++--- internal/modules/coin/handlers_order_test.go | 2 +- internal/modules/coin/handlers_test.go | 98 +++++++++++++++++++- internal/modules/coin/trade_args.go | 5 +- 6 files changed, 119 insertions(+), 22 deletions(-) diff --git a/aws/telegram-commands.json b/aws/telegram-commands.json index 028c473..df4a742 100644 --- a/aws/telegram-commands.json +++ b/aws/telegram-commands.json @@ -138,7 +138,7 @@ }, { "command": "coin_sell", - "description": "Sell coin quantity back to USD" + "description": "Sell coin back to USD amount" }, { "command": "coin_stats", diff --git a/internal/modules/coin/coin.go b/internal/modules/coin/coin.go index 6c251a4..18ceae2 100644 --- a/internal/modules/coin/coin.go +++ b/internal/modules/coin/coin.go @@ -29,7 +29,7 @@ func New(deps modules.Deps) modules.Module { { Name: "coin_sell", Visibility: modules.VisibilityPublic, - Description: "Sell coin quantity back to USD", + Description: "Sell coin back to USD amount", Handler: s.handleSell, }, { diff --git a/internal/modules/coin/handlers.go b/internal/modules/coin/handlers.go index 4b1578d..0f42e77 100644 --- a/internal/modules/coin/handlers.go +++ b/internal/modules/coin/handlers.go @@ -70,7 +70,7 @@ func (s *state) handleBuy(ctx context.Context, b *bot.Bot, update *models.Update } args := argsAfterCommand(update.Message.Text) if len(args) != 2 { - return chathelper.Reply(ctx, b, update.Message, "Usage: /coin_buy \nExample: /coin_buy BTC 10") + return chathelper.Reply(ctx, b, update.Message, "Usage: /coin_buy \nAlternative: /coin_buy \nExample: /coin_buy BTC 10") } parsed, err := parseCoinValueArgs(args, isSafeUSD, errInvalidUSDAmount) if errors.Is(err, errInvalidUSDAmount) { @@ -85,9 +85,12 @@ func (s *state) handleBuy(ctx context.Context, b *bot.Bot, update *models.Update if err != nil { return s.replyPriceError(ctx, b, update, err) } + if !isPositiveFinite(price.USD) { + return s.replyPriceError(ctx, b, update, ErrNoCoinPrice) + } qty := amount / price.USD - if !isPositiveFinite(qty) { - return chathelper.Reply(ctx, b, update.Message, "Trade value is invalid.") + if !isPositiveFinite(qty) || qty < coinDustEpsilon { + return chathelper.Reply(ctx, b, update.Message, "USD amount is too small to convert to a tradeable coin quantity at the current price.") } defer s.locks.Acquire(strconv.FormatInt(userID, 10))() var insufficientBalance *float64 @@ -121,24 +124,27 @@ func (s *state) handleSell(ctx context.Context, b *bot.Bot, update *models.Updat } args := argsAfterCommand(update.Message.Text) if len(args) != 2 { - return chathelper.Reply(ctx, b, update.Message, "Usage: /coin_sell \nExample: /coin_sell BTC 0.01") + return chathelper.Reply(ctx, b, update.Message, "Usage: /coin_sell \nAlternative: /coin_sell \nExample: /coin_sell BTC 10") } - parsed, err := parseCoinValueArgs(args, isPositiveFinite, errInvalidQuantity) - if errors.Is(err, errInvalidQuantity) { - return chathelper.Reply(ctx, b, update.Message, "Quantity must be a positive finite number.") + parsed, err := parseCoinValueArgs(args, isSafeUSD, errInvalidUSDAmount) + if errors.Is(err, errInvalidUSDAmount) { + return chathelper.Reply(ctx, b, update.Message, "USD amount must be a positive finite number within the supported range.") } if err != nil { return s.replyPriceError(ctx, b, update, err) } coin := parsed.coin - qty := parsed.value + amount := parsed.value price, err := s.prices.FetchUSD(ctx, coin) if err != nil { return s.replyPriceError(ctx, b, update, err) } - revenue := qty * price.USD - if !isSafeUSD(revenue) { - return chathelper.Reply(ctx, b, update.Message, "Trade value is too large.") + if !isPositiveFinite(price.USD) { + return s.replyPriceError(ctx, b, update, ErrNoCoinPrice) + } + qty := amount / price.USD + if !isPositiveFinite(qty) || qty < coinDustEpsilon { + return chathelper.Reply(ctx, b, update.Message, "USD amount is too small to convert to a tradeable coin quantity at the current price.") } defer s.locks.Acquire(strconv.FormatInt(userID, 10))() var insufficientHeld *float64 @@ -148,7 +154,7 @@ func (s *state) handleSell(ctx context.Context, b *bot.Bot, update *models.Updat insufficientHeld = &held return errInsufficientCoin } - p.AddUSD(revenue) + p.AddUSD(amount) return nil }) if errors.Is(err, errInsufficientCoin) && insufficientHeld != nil { @@ -161,5 +167,5 @@ func (s *state) handleSell(ctx context.Context, b *bot.Bot, update *models.Updat } return chathelper.Reply(ctx, b, update.Message, "Sold "+FormatCoinQty(qty)+" "+coin.Symbol+" @ "+FormatUSD(price.USD)+" ("+price.Source+")"+ - "\nRevenue: "+FormatUSD(revenue)+"\nRemaining: "+FormatUSD(p.USD)) + "\nProceeds: "+FormatUSD(amount)+"\nRemaining: "+FormatUSD(p.USD)) } diff --git a/internal/modules/coin/handlers_order_test.go b/internal/modules/coin/handlers_order_test.go index bd571e4..6c20a4e 100644 --- a/internal/modules/coin/handlers_order_test.go +++ b/internal/modules/coin/handlers_order_test.go @@ -33,7 +33,7 @@ func TestHandleSellAcceptsCoinFirstOrder(t *testing.T) { _ = s.handleBuy(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_buy 500 BTC")) rb.Reset() - if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell BTC 0.01")); err != nil { + if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell BTC 500")); err != nil { t.Fatalf("handleSell: %v", err) } diff --git a/internal/modules/coin/handlers_test.go b/internal/modules/coin/handlers_test.go index 07a1036..8470d48 100644 --- a/internal/modules/coin/handlers_test.go +++ b/internal/modules/coin/handlers_test.go @@ -124,7 +124,7 @@ func TestHandleBuyAndSell(t *testing.T) { t.Fatalf("after buy = %+v", p) } rb.Reset() - if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 0.01 BTC")); err != nil { + if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 500 BTC")); err != nil { t.Fatalf("handleSell: %v", err) } rb.AssertSentText(t, "Sold 0.01 BTC") @@ -146,12 +146,106 @@ func TestHandleBuyInsufficientUSD(t *testing.T) { func TestHandleSellInsufficientCoin(t *testing.T) { s := newTestState(map[string]CoinPrice{"ETH": {USD: 3000, Source: "Coinbase"}}, nil) rb := testutil.NewRecordingBot(t) - if err := s.handleSell(context.Background(), rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 1 ETH")); err != nil { + if err := s.handleSell(context.Background(), rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 10 ETH")); err != nil { t.Fatalf("handleSell: %v", err) } rb.AssertSentText(t, "Insufficient ETH") } +func TestHandleSellRejectsInvalidUSDAmount(t *testing.T) { + s := newTestState(map[string]CoinPrice{"BTC": {USD: 50000, Source: "Binance"}}, nil) + rb := testutil.NewRecordingBot(t) + if err := s.handleSell(context.Background(), rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell BTC -5")); err != nil { + t.Fatalf("handleSell: %v", err) + } + rb.AssertSentText(t, "USD amount must be a positive finite number within the supported range.") +} + +func TestHandleSellRejectsAmountTooSmall(t *testing.T) { + s := newTestState(map[string]CoinPrice{"BTC": {USD: 1e300, Source: "Test"}}, nil) + rb := testutil.NewRecordingBot(t) + if err := s.handleSell(context.Background(), rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell BTC 1e-300")); err != nil { + t.Fatalf("handleSell: %v", err) + } + rb.AssertSentText(t, "too small") +} + +func TestHandleSellInsufficientCoinWithHoldings(t *testing.T) { + ctx := context.Background() + s := newTestState(map[string]CoinPrice{"BTC": {USD: 50000, Source: "Binance"}}, nil) + rb := testutil.NewRecordingBot(t) + _ = s.handleTopup(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_topup 1000")) + _ = s.handleBuy(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_buy 500 BTC")) + rb.Reset() + + if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 600 BTC")); err != nil { + t.Fatalf("handleSell: %v", err) + } + rb.AssertSentText(t, "Insufficient BTC") + + p, _ := LoadPortfolio(ctx, s.kv, 7, 999) + if p.USD != 500 || p.Assets["BTC"] != 0.01 { + t.Fatalf("portfolio mutated on failed sell = %+v", p) + } +} + +func TestHandleSellRejectsDustQuantity(t *testing.T) { + ctx := context.Background() + s := newTestState(map[string]CoinPrice{"BTC": {USD: 1e9, Source: "Test"}}, nil) + rb := testutil.NewRecordingBot(t) + _ = s.handleTopup(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_topup 1000")) + _ = s.handleBuy(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_buy 1 BTC")) + rb.Reset() + + if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 0.5 BTC")); err != nil { + t.Fatalf("handleSell: %v", err) + } + rb.AssertSentText(t, "too small") + + p, _ := LoadPortfolio(ctx, s.kv, 7, 999) + if p.USD != 999 || len(p.Assets) != 1 { + t.Fatalf("portfolio mutated on dust sell = %+v", p) + } +} + +func TestHandleBuyRejectsDustQuantity(t *testing.T) { + ctx := context.Background() + s := newTestState(map[string]CoinPrice{"BTC": {USD: 1e9, Source: "Test"}}, nil) + rb := testutil.NewRecordingBot(t) + _ = s.handleTopup(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_topup 1000")) + rb.Reset() + + if err := s.handleBuy(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_buy 0.5 BTC")); err != nil { + t.Fatalf("handleBuy: %v", err) + } + rb.AssertSentText(t, "too small") + + p, _ := LoadPortfolio(ctx, s.kv, 7, 999) + if p.USD != 1000 || len(p.Assets) != 0 { + t.Fatalf("portfolio mutated on dust buy = %+v", p) + } +} + +func TestHandleSellRejectsZeroPrice(t *testing.T) { + ctx := context.Background() + s := newTestState(map[string]CoinPrice{"BTC": {USD: 0, Source: "Test"}}, nil) + rb := testutil.NewRecordingBot(t) + if err := s.handleSell(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_sell 10 BTC")); err != nil { + t.Fatalf("handleSell: %v", err) + } + rb.AssertSentText(t, "No coin price available") +} + +func TestHandleBuyRejectsZeroPrice(t *testing.T) { + ctx := context.Background() + s := newTestState(map[string]CoinPrice{"BTC": {USD: 0, Source: "Test"}}, nil) + rb := testutil.NewRecordingBot(t) + if err := s.handleBuy(ctx, rb.Bot, testutil.NewPrivateMessage(7, "/coin_buy 10 BTC")); err != nil { + t.Fatalf("handleBuy: %v", err) + } + rb.AssertSentText(t, "No coin price available") +} + func TestPriceErrorDoesNotMutatePortfolio(t *testing.T) { ctx := context.Background() s := newTestState(nil, errors.New("upstream down")) diff --git a/internal/modules/coin/trade_args.go b/internal/modules/coin/trade_args.go index 07d3c5e..2669be3 100644 --- a/internal/modules/coin/trade_args.go +++ b/internal/modules/coin/trade_args.go @@ -2,10 +2,7 @@ package coin import "errors" -var ( - errInvalidUSDAmount = errors.New("coin: invalid USD amount") - errInvalidQuantity = errors.New("coin: invalid quantity") -) +var errInvalidUSDAmount = errors.New("coin: invalid USD amount") type coinValueArgs struct { coin CoinSymbol