From 48a2d5b6dcc5a71f85b9cd69ed7f3d31324498b6 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Thu, 9 Jan 2020 14:58:47 +0000 Subject: [PATCH] use only one coins field in MsgPlaceBid --- x/auction/abci_test.go | 2 +- x/auction/client/cli/tx.go | 15 +++++---------- x/auction/client/rest/tx.go | 11 ++--------- x/auction/handler.go | 2 +- x/auction/keeper/auctions.go | 23 +++++++++++------------ x/auction/keeper/auctions_test.go | 12 ++++++------ x/auction/types/msg.go | 20 +++++++------------- x/auction/types/msg_test.go | 24 +++++++++++++++++------- 8 files changed, 50 insertions(+), 59 deletions(-) diff --git a/x/auction/abci_test.go b/x/auction/abci_test.go index 79f526b5..19bc1792 100644 --- a/x/auction/abci_test.go +++ b/x/auction/abci_test.go @@ -39,7 +39,7 @@ func TestKeeper_EndBlocker(t *testing.T) { auctionID, err := keeper.StartForwardReverseAuction(ctx, sellerModName, c("token1", 20), c("token2", 50), returnAddrs, returnWeights) require.NoError(t, err) - require.NoError(t, keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 30), c("token1", 20))) + require.NoError(t, keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 30))) // Run the endblocker, simulating a block time 1ns before auction expiry preExpiryTime := ctx.BlockTime().Add(auction.DefaultBidDuration - 1) diff --git a/x/auction/client/cli/tx.go b/x/auction/client/cli/tx.go index 2ced6610..e705eb74 100644 --- a/x/auction/client/cli/tx.go +++ b/x/auction/client/cli/tx.go @@ -33,9 +33,9 @@ func GetTxCmd(cdc *codec.Codec) *cobra.Command { // GetCmdPlaceBid cli command for creating and modifying cdps. func GetCmdPlaceBid(cdc *codec.Codec) *cobra.Command { return &cobra.Command{ - Use: "placebid [AuctionID] [Bidder] [Bid] [Lot]", + Use: "placebid [auctionID] [amount]", Short: "place a bid on an auction", - Args: cobra.ExactArgs(4), + Args: cobra.MinimumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { cliCtx := context.NewCLIContext().WithCodec(cdc) txBldr := auth.NewTxBuilderFromCLI().WithTxEncoder(utils.GetTxEncoder(cdc)) @@ -46,18 +46,13 @@ func GetCmdPlaceBid(cdc *codec.Codec) *cobra.Command { return err } - bid, err := sdk.ParseCoin(args[2]) + amt, err := sdk.ParseCoin(args[2]) if err != nil { - fmt.Printf("invalid bid amount - %s \n", string(args[2])) + fmt.Printf("invalid amount - %s \n", string(args[2])) return err } - lot, err := sdk.ParseCoin(args[3]) - if err != nil { - fmt.Printf("invalid lot - %s \n", string(args[3])) - return err - } - msg := types.NewMsgPlaceBid(id, cliCtx.GetFromAddress(), bid, lot) + msg := types.NewMsgPlaceBid(id, cliCtx.GetFromAddress(), amt) err = msg.ValidateBasic() if err != nil { return err diff --git a/x/auction/client/rest/tx.go b/x/auction/client/rest/tx.go index 5773f24a..910714ca 100644 --- a/x/auction/client/rest/tx.go +++ b/x/auction/client/rest/tx.go @@ -33,7 +33,7 @@ const ( func registerTxRoutes(cliCtx context.CLIContext, r *mux.Router) { r.HandleFunc( - fmt.Sprintf("/auction/bid/{%s}/{%s}/{%s}/{%s}", restAuctionID, restBidder, restBid, restLot), bidHandlerFn(cliCtx)).Methods("PUT") + fmt.Sprintf("/auction/bid/{%s}/{%s}/{%s}", restAuctionID, restBidder, restBid), bidHandlerFn(cliCtx)).Methods("PUT") } func bidHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { @@ -44,7 +44,6 @@ func bidHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { strAuctionID := vars[restAuctionID] bechBidder := vars[restBidder] strBid := vars[restBid] - strLot := vars[restLot] auctionID, err := strconv.ParseUint(strAuctionID, 10, 64) if err != nil { @@ -64,13 +63,7 @@ func bidHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { return } - lot, err := sdk.ParseCoin(strLot) - if err != nil { - rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) - return - } - - msg := types.NewMsgPlaceBid(auctionID, bidder, bid, lot) + msg := types.NewMsgPlaceBid(auctionID, bidder, bid) if err := msg.ValidateBasic(); err != nil { rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) return diff --git a/x/auction/handler.go b/x/auction/handler.go index ec238957..f9b7c137 100644 --- a/x/auction/handler.go +++ b/x/auction/handler.go @@ -21,7 +21,7 @@ func NewHandler(keeper Keeper) sdk.Handler { func handleMsgPlaceBid(ctx sdk.Context, keeper Keeper, msg MsgPlaceBid) sdk.Result { - err := keeper.PlaceBid(ctx, msg.AuctionID, msg.Bidder, msg.Bid, msg.Lot) + err := keeper.PlaceBid(ctx, msg.AuctionID, msg.Bidder, msg.Amount) if err != nil { return err.Result() } diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 38108079..6aa27fd6 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -68,8 +68,7 @@ func (k Keeper) StartForwardReverseAuction(ctx sdk.Context, seller string, lot s } // PlaceBid places a bid on any auction. -// TODO passing bid and lot is weird when only one needed -func (k Keeper) PlaceBid(ctx sdk.Context, auctionID uint64, bidder sdk.AccAddress, bid sdk.Coin, lot sdk.Coin) sdk.Error { +func (k Keeper) PlaceBid(ctx sdk.Context, auctionID uint64, bidder sdk.AccAddress, newAmount sdk.Coin) sdk.Error { // get auction from store auction, found := k.GetAuction(ctx, auctionID) @@ -81,32 +80,26 @@ func (k Keeper) PlaceBid(ctx sdk.Context, auctionID uint64, bidder sdk.AccAddres if ctx.BlockTime().After(auction.GetEndTime()) { return sdk.ErrInternal("auction has closed") } - if auction.GetBid().Denom != bid.Denom { - return sdk.ErrInternal("bid has incorrect denom") - } - if auction.GetLot().Denom != lot.Denom { - return sdk.ErrInternal("lot has incorrect denom") - } // place bid var err sdk.Error var a types.Auction switch auc := auction.(type) { case types.ForwardAuction: - a, err = k.PlaceBidForward(ctx, auc, bidder, bid) + a, err = k.PlaceBidForward(ctx, auc, bidder, newAmount) if err != nil { return err } case types.ReverseAuction: - a, err = k.PlaceBidReverse(ctx, auc, bidder, lot) + a, err = k.PlaceBidReverse(ctx, auc, bidder, newAmount) if err != nil { return err } case types.ForwardReverseAuction: if !auc.IsReversePhase() { - a, err = k.PlaceBidForwardReverseForward(ctx, auc, bidder, bid) + a, err = k.PlaceBidForwardReverseForward(ctx, auc, bidder, newAmount) } else { - a, err = k.PlaceBidForwardReverseReverse(ctx, auc, bidder, lot) + a, err = k.PlaceBidForwardReverseReverse(ctx, auc, bidder, newAmount) } if err != nil { return err @@ -163,6 +156,9 @@ func (k Keeper) PlaceBidForward(ctx sdk.Context, a types.ForwardAuction, bidder // TODO naming func (k Keeper) PlaceBidForwardReverseForward(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, bid sdk.Coin) (types.ForwardReverseAuction, sdk.Error) { // Validate bid + if bid.Denom != a.Bid.Denom { + return a, sdk.ErrInternal("bid denom doesn't match auction") + } if a.IsReversePhase() { return a, sdk.ErrInternal("auction is not in forward phase") } @@ -201,6 +197,9 @@ func (k Keeper) PlaceBidForwardReverseForward(ctx sdk.Context, a types.ForwardRe func (k Keeper) PlaceBidForwardReverseReverse(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, lot sdk.Coin) (types.ForwardReverseAuction, sdk.Error) { // Validate bid + if lot.Denom != a.Lot.Denom { + return a, sdk.ErrInternal("lot denom doesn't match auction") + } if !a.IsReversePhase() { return a, sdk.ErrInternal("auction not in reverse phase") } diff --git a/x/auction/keeper/auctions_test.go b/x/auction/keeper/auctions_test.go index 1f10a28c..7d7f3073 100644 --- a/x/auction/keeper/auctions_test.go +++ b/x/auction/keeper/auctions_test.go @@ -43,14 +43,14 @@ func TestForwardAuctionBasic(t *testing.T) { tApp.CheckBalance(t, ctx, sellerAddr, cs(c("token1", 80), c("token2", 100))) // PlaceBid (bid: 10 token, lot: same as starting) - require.NoError(t, keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 10), c("token1", 20))) // bid, lot + require.NoError(t, keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 10))) // Check buyer's coins have decreased tApp.CheckBalance(t, ctx, buyer, cs(c("token1", 100), c("token2", 90))) // Check seller's coins have not increased (because proceeds are burned) tApp.CheckBalance(t, ctx, sellerAddr, cs(c("token1", 80), c("token2", 100))) // increment bid same bidder - err = keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 20), c("token1", 20)) + err = keeper.PlaceBid(ctx, auctionID, buyer, c("token2", 20)) require.NoError(t, err) // Close auction at just at auction expiry time @@ -85,7 +85,7 @@ func TestReverseAuctionBasic(t *testing.T) { tApp.CheckBalance(t, ctx, buyerAddr, nil) // zero coins // Place a bid - require.NoError(t, keeper.PlaceBid(ctx, 0, seller, c("token1", 20), c("token2", 10))) // bid, lot + require.NoError(t, keeper.PlaceBid(ctx, 0, seller, c("token2", 10))) // Check seller's coins have decreased tApp.CheckBalance(t, ctx, seller, cs(c("token1", 80), c("token2", 100))) // Check buyer's coins have increased @@ -129,7 +129,7 @@ func TestForwardReverseAuctionBasic(t *testing.T) { tApp.CheckBalance(t, ctx, sellerAddr, cs(c("token1", 80), c("token2", 100))) // Place a forward bid - require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 10), c("token1", 20))) // bid, lot + require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 10))) // Check bidder's coins have decreased tApp.CheckBalance(t, ctx, buyer, cs(c("token1", 100), c("token2", 90))) // Check seller's coins have increased @@ -140,8 +140,8 @@ func TestForwardReverseAuctionBasic(t *testing.T) { } // Place a reverse bid - require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50), c("token1", 15))) // first bid up to max bid to switch phases - require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50), c("token1", 15))) + require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50))) // first bid up to max bid to switch phases + require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token1", 15))) // Check bidder's coins have decreased tApp.CheckBalance(t, ctx, buyer, cs(c("token1", 100), c("token2", 50))) // Check seller's coins have increased diff --git a/x/auction/types/msg.go b/x/auction/types/msg.go index 45c10472..dbaf43b2 100644 --- a/x/auction/types/msg.go +++ b/x/auction/types/msg.go @@ -9,17 +9,15 @@ var _ sdk.Msg = &MsgPlaceBid{} type MsgPlaceBid struct { AuctionID uint64 Bidder sdk.AccAddress // This can be a buyer (who increments bid), or a seller (who decrements lot) TODO rename to be clearer? - Bid sdk.Coin - Lot sdk.Coin + Amount sdk.Coin // The new bid or lot to set on the auction } // NewMsgPlaceBid returns a new MsgPlaceBid. -func NewMsgPlaceBid(auctionID uint64, bidder sdk.AccAddress, bid sdk.Coin, lot sdk.Coin) MsgPlaceBid { +func NewMsgPlaceBid(auctionID uint64, bidder sdk.AccAddress, amt sdk.Coin) MsgPlaceBid { return MsgPlaceBid{ AuctionID: auctionID, Bidder: bidder, - Bid: bid, - Lot: lot, + Amount: amt, } } @@ -29,18 +27,14 @@ func (msg MsgPlaceBid) Route() string { return RouterKey } // Type returns a human-readable string for the message, intended for utilization within tags. func (msg MsgPlaceBid) Type() string { return "place_bid" } -// ValidateBasic does a simple validation check that doesn't require access to any other information. +// ValidateBasic does a simple validation check that doesn't require access to state. func (msg MsgPlaceBid) ValidateBasic() sdk.Error { if msg.Bidder.Empty() { return sdk.ErrInternal("invalid (empty) bidder address") } - if msg.Bid.Amount.LT(sdk.ZeroInt()) { - return sdk.ErrInternal("invalid (negative) bid amount") + if !msg.Amount.IsValid() { + return sdk.ErrInternal("invalid bid amount") } - if msg.Lot.Amount.LT(sdk.ZeroInt()) { - return sdk.ErrInternal("invalid (negative) lot amount") - } - // TODO check coin denoms return nil } @@ -53,4 +47,4 @@ func (msg MsgPlaceBid) GetSignBytes() []byte { // GetSigners returns the addresses of signers that must sign. func (msg MsgPlaceBid) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{msg.Bidder} -} \ No newline at end of file +} diff --git a/x/auction/types/msg_test.go b/x/auction/types/msg_test.go index cfb51b5c..f78e7102 100644 --- a/x/auction/types/msg_test.go +++ b/x/auction/types/msg_test.go @@ -14,19 +14,29 @@ func TestMsgPlaceBid_ValidateBasic(t *testing.T) { msg MsgPlaceBid expectPass bool }{ - {"normal", MsgPlaceBid{0, addr, sdk.NewInt64Coin("usdx", 10), sdk.NewInt64Coin("kava", 20)}, true}, - {"emptyAddr", MsgPlaceBid{0, sdk.AccAddress{}, sdk.NewInt64Coin("usdx", 10), sdk.NewInt64Coin("kava", 20)}, false}, - {"negativeBid", MsgPlaceBid{0, addr, sdk.Coin{"usdx", sdk.NewInt(-10)}, sdk.NewInt64Coin("kava", 20)}, false}, - {"negativeLot", MsgPlaceBid{0, addr, sdk.NewInt64Coin("usdx", 10), sdk.Coin{"kava", sdk.NewInt(-20)}}, false}, - {"zerocoins", MsgPlaceBid{0, addr, sdk.NewInt64Coin("usdx", 0), sdk.NewInt64Coin("kava", 0)}, true}, + {"normal", + NewMsgPlaceBid(0, addr, c("token", 10)), + true}, + {"emptyAddr", + NewMsgPlaceBid(0, sdk.AccAddress{}, c("token", 10)), + false}, + {"negativeAmount", + NewMsgPlaceBid(0, addr, sdk.Coin{Denom: "token", Amount: sdk.NewInt(-10)}), + false}, + {"zeroAmount", + NewMsgPlaceBid(0, addr, c("token", 0)), + true}, } + for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { if tc.expectPass { - require.Nil(t, tc.msg.ValidateBasic()) + require.NoError(t, tc.msg.ValidateBasic()) } else { - require.NotNil(t, tc.msg.ValidateBasic()) + require.Error(t, tc.msg.ValidateBasic()) } }) } } + +func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) }