Remove order (data layer)

data/redisrepo/remove_order.go

@@ -2,15 +2,14 @@ package redisrepo
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/orbs-network/order-book/models"
 	"github.com/orbs-network/order-book/utils/logger"
 	"github.com/orbs-network/order-book/utils/logger/logctx"
 )
 
 // Removes an order from the order book.
-// Order is removed from the prices sorted set, user's orders set and order hash is updated to `status: CANCELED`
+// Order is removed from the prices sorted set, user's order set and order hash is updated to `status: CANCELED`
 // SHOULD ONLY BE USED WHEN ORDER STATUS IS STILL `OPEN`
 func (r *redisRepository) RemoveOrder(ctx context.Context, order models.Order) error {
 
@@ -34,13 +33,13 @@ func (r *redisRepository) RemoveOrder(ctx context.Context, order models.Order) e
 
 	// update order status to CANCELED
 	orderIDKey := CreateOrderIDKey(order.Id)
-	transaction.HSet(ctx, orderIDKey, "status", models.STATUS_CANCELED.String()).Result()
+	transaction.HSet(ctx, orderIDKey, "status", models.STATUS_CANCELLED.String()).Result()
 
 	_, err := transaction.Exec(ctx)
 
 	if err != nil {
 		logctx.Error(ctx, "failed to remove order from Redis", logger.Error(err), logger.String("orderId", order.Id.String()))
-		return fmt.Errorf("transaction failed. Reason: %v", err)
+		return models.ErrTransactionFailed
 	}
 	// --- END TRANSACTION ---
 

data/redisrepo/remove_order_test.go

@@ -11,11 +11,20 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestRedisRepository_RemoveOrder(t *testing.T) {
+var ctx = context.Background()
+var orderId = uuid.MustParse("00000000-0000-0000-0000-000000000001")
+var size, _ = decimal.NewFromString("10000324.123456789")
 
-	ctx := context.Background()
-	orderId := uuid.MustParse("00000000-0000-0000-0000-000000000001")
-	size, _ := decimal.NewFromString("10000324.123456789")
+var order = models.Order{
+	Id:     orderId,
+	Price:  price,
+	Size:   size,
+	Symbol: symbol,
+	Side:   models.BUY,
+	Status: models.STATUS_OPEN,
+}
+
+func TestRedisRepository_RemoveOrder(t *testing.T) {
 
 	t.Run("only open orders can be removed", func(t *testing.T) {
 		db, _ := redismock.NewClientMock()
@@ -29,21 +38,29 @@ func TestRedisRepository_RemoveOrder(t *testing.T) {
 		assert.ErrorIs(t, err, models.ErrOrderNotOpen)
 	})
 
-	t.Run("succesfully removes order", func(t *testing.T) {
+	t.Run("fails to remove order", func(t *testing.T) {
 		db, mock := redismock.NewClientMock()
 
 		repo := &redisRepository{
 			client: db,
 		}
 
-		order := models.Order{
-			Id:     orderId,
-			UserId: uuid.New(),
-			Price:  price,
-			Size:   size,
-			Symbol: symbol,
-			Side:   models.BUY,
-			Status: models.STATUS_OPEN,
+		buyPricesKey := CreateBuySidePricesKey(order.Symbol)
+
+		mock.ExpectTxPipeline()
+		mock.ExpectZRem(buyPricesKey, order.Id.String()).SetErr(assert.AnError)
+
+		err := repo.RemoveOrder(ctx, order)
+
+		assert.ErrorIs(t, err, models.ErrTransactionFailed)
+		assert.NoError(t, mock.ExpectationsWereMet())
+	})
+
+	t.Run("succesfully removes order", func(t *testing.T) {
+		db, mock := redismock.NewClientMock()
+
+		repo := &redisRepository{
+			client: db,
 		}
 
 		buyPricesKey := CreateBuySidePricesKey(order.Symbol)
@@ -53,7 +70,7 @@ func TestRedisRepository_RemoveOrder(t *testing.T) {
 		mock.ExpectTxPipeline()
 		mock.ExpectZRem(buyPricesKey, order.Id.String()).SetVal(1)
 		mock.ExpectSRem(userOrdersKey, order.Id.String()).SetVal(1)
-		mock.ExpectHSet(orderIDKey, "status", models.STATUS_CANCELED.String()).SetVal(1)
+		mock.ExpectHSet(orderIDKey, "status", models.STATUS_CANCELLED.String()).SetVal(1)
 		mock.ExpectTxPipelineExec()
 
 		err := repo.RemoveOrder(ctx, order)

models/errors.go

@@ -8,3 +8,4 @@ var ErrUnexpectedError = errors.New("unexpected error")
 var ErrNoUserInContext = errors.New("no user in context")
 var ErrUnauthorized = errors.New("user not allowed to perform this action")
 var ErrOrderNotOpen = errors.New("order must be status open to perform this action")
+var ErrTransactionFailed = errors.New("transaction failed")

models/status.go

@@ -5,10 +5,10 @@ import "errors"
 type Status string
 
 const (
-	STATUS_OPEN     Status = "OPEN"
-	STATUS_PENDING  Status = "PENDING"
-	STATUS_FILLED   Status = "FILLED"
-	STATUS_CANCELED Status = "CANCELED"
+	STATUS_OPEN      Status = "OPEN"
+	STATUS_PENDING   Status = "PENDING"
+	STATUS_FILLED    Status = "FILLED"
+	STATUS_CANCELLED Status = "CANCELLED"
 )
 
 func (s Status) String() string {
@@ -26,7 +26,7 @@ func StrToStatus(s string) (Status, error) {
 	case "FILLED":
 		return STATUS_FILLED, nil
 	case "CANCELED":
-		return STATUS_CANCELED, nil
+		return STATUS_CANCELLED, nil
 	default:
 		return "", ErrInvalidStatus
 	}

service/cancel_order.go

@@ -29,6 +29,11 @@ func (s *Service) CancelOrder(ctx context.Context, orderId uuid.UUID) error {
 		return models.ErrOrderNotFound
 	}
 
+	if order.Status != models.STATUS_OPEN {
+		logctx.Warn(ctx, "trying to cancel order that is not open", logger.String("orderId", orderId.String()), logger.String("status", order.Status.String()))
+		return models.ErrOrderNotOpen
+	}
+
 	if user.ID != order.UserId {
 		logctx.Warn(ctx, "user trying to cancel another user's order", logger.String("orderId", orderId.String()), logger.String("requestUserId", user.ID.String()), logger.String("orderUserId", order.UserId.String()))
 		return models.ErrUnauthorized

service/cancel_order_test.go

@@ -16,7 +16,7 @@ func TestService_CancelOrder(t *testing.T) {
 
 	userId := uuid.MustParse("a577273e-12de-4acc-a4f8-de7fb5b86e37")
 	orderId := uuid.MustParse("e577273e-12de-4acc-a4f8-de7fb5b86e37")
-	order := &models.Order{UserId: userId}
+	order := &models.Order{UserId: userId, Status: models.STATUS_OPEN}
 
 	t.Run("no user in context - returns `ErrNoUserInContext` error", func(t *testing.T) {
 		svc, _ := service.New(&mocks.MockOrderBookStore{})
@@ -32,10 +32,10 @@ func TestService_CancelOrder(t *testing.T) {
 		err         error
 		expectedErr error
 	}{
-
 		{name: "unexpected error when finding order - returns error", err: models.ErrOrderNotFound, expectedErr: models.ErrOrderNotFound},
 		{name: "order not found - returns `ErrOrderNotFound` error", order: nil, err: nil, expectedErr: models.ErrOrderNotFound},
-		{name: "user trying to cancel another user's order - returns `ErrUnauthorized` error", order: &models.Order{UserId: uuid.MustParse("00000000-0000-0000-0000-000000000009")}, expectedErr: models.ErrUnauthorized},
+		{name: "trying to cancel order that is not open - returns `ErrOrderNotOpen` error", order: &models.Order{Status: models.STATUS_PENDING}, err: nil, expectedErr: models.ErrOrderNotOpen},
+		{name: "user trying to cancel another user's order - returns `ErrUnauthorized` error", order: &models.Order{UserId: uuid.MustParse("00000000-0000-0000-0000-000000000009"), Status: models.STATUS_OPEN}, expectedErr: models.ErrUnauthorized},
 		{name: "unexpected error when removing order - returns error", order: order, err: assert.AnError, expectedErr: assert.AnError},
 		{name: "order removed successfully - returns nil", order: order, err: nil, expectedErr: nil},
 	}

transport/rest/cancel_order.go

@@ -44,10 +44,23 @@ func (h *Handler) CancelOrder(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if err == models.ErrOrderNotFound {
+		logctx.Warn(userCtx, "order not found", logger.String("orderId", orderId.String()))
 		http.Error(w, "Order not found", http.StatusNotFound)
 		return
 	}
 
+	if err == models.ErrOrderNotOpen {
+		logctx.Warn(userCtx, "user trying to cancel order that is not open", logger.String("orderId", orderId.String()))
+		http.Error(w, "Order not found", http.StatusNotFound)
+		return
+	}
+
+	if err == models.ErrUnauthorized {
+		logctx.Warn(userCtx, "user not authorized to cancel order", logger.String("orderId", orderId.String()))
+		http.Error(w, "Not authorized", http.StatusUnauthorized)
+		return
+	}
+
 	if err != nil {
 		logctx.Error(userCtx, "failed to cancel order", logger.Error(err))
 		http.Error(w, "Error cancelling order. Try again later", http.StatusInternalServerError)

transport/rest/cancel_order_test.go

@@ -14,114 +14,81 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-var orderId = uuid.MustParse("00000000-0000-0000-0000-000000000001")
-
 func TestHandler_CancelOrder(t *testing.T) {
-
-	t.Run("invalid orderId - should return `http.StatusBadRequest`", func(t *testing.T) {
-		mockService := &mocks.MockOrderBookService{}
-		router := mux.NewRouter()
-
-		h, _ := rest.NewHandler(mockService, router)
-
-		req, err := http.NewRequest("DELETE", "/order/invalid", nil)
-
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		rr := httptest.NewRecorder()
-		router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
-
-		router.ServeHTTP(rr, req)
-
-		assert.Equal(t, http.StatusBadRequest, rr.Code)
-		assert.Equal(t, "invalid order ID\n", rr.Body.String())
-
-	})
-
-	t.Run("no user in context - should return `http.StatusUnauthorized`", func(t *testing.T) {
-		mockService := &mocks.MockOrderBookService{Error: models.ErrNoUserInContext}
-		router := mux.NewRouter()
-
-		h, _ := rest.NewHandler(mockService, router)
-
-		req, err := http.NewRequest("DELETE", fmt.Sprintf("/order/%s", orderId.String()), nil)
-
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		rr := httptest.NewRecorder()
-		router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
-
-		router.ServeHTTP(rr, req)
-
-		assert.Equal(t, http.StatusUnauthorized, rr.Code)
-		assert.Equal(t, "User not found\n", rr.Body.String())
-	})
-
-	t.Run("order not found - should return `http.StatusNotFound`", func(t *testing.T) {
-		mockService := &mocks.MockOrderBookService{Error: models.ErrOrderNotFound}
-		router := mux.NewRouter()
-
-		h, _ := rest.NewHandler(mockService, router)
-
-		req, err := http.NewRequest("DELETE", fmt.Sprintf("/order/%s", orderId.String()), nil)
-
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		rr := httptest.NewRecorder()
-		router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
-
-		router.ServeHTTP(rr, req)
-
-		assert.Equal(t, http.StatusNotFound, rr.Code)
-		assert.Equal(t, "Order not found\n", rr.Body.String())
-	})
-
-	t.Run("unexpected error from service - should return `http.StatusInternalServerError`", func(t *testing.T) {
-		mockService := &mocks.MockOrderBookService{Error: assert.AnError}
-		router := mux.NewRouter()
-
-		h, _ := rest.NewHandler(mockService, router)
-
-		req, err := http.NewRequest("DELETE", fmt.Sprintf("/order/%s", orderId.String()), nil)
-
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		rr := httptest.NewRecorder()
-		router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
-
-		router.ServeHTTP(rr, req)
-
-		assert.Equal(t, http.StatusInternalServerError, rr.Code)
-		assert.Equal(t, "Error cancelling order. Try again later\n", rr.Body.String())
-	})
-
-	t.Run("successful cancel - should return `http.StatusOK`", func(t *testing.T) {
-		mockService := &mocks.MockOrderBookService{}
-		router := mux.NewRouter()
-
-		h, _ := rest.NewHandler(mockService, router)
-
-		req, err := http.NewRequest("DELETE", fmt.Sprintf("/order/%s", orderId.String()), nil)
-
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		rr := httptest.NewRecorder()
-		router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
-
-		router.ServeHTTP(rr, req)
-
-		assert.Equal(t, http.StatusOK, rr.Code)
-		assert.Equal(t, "{\"orderId\":\"00000000-0000-0000-0000-000000000001\"}", rr.Body.String())
-	})
-
+	var orderId = uuid.MustParse("00000000-0000-0000-0000-000000000001")
+
+	tests := []struct {
+		name         string
+		mockService  *mocks.MockOrderBookService
+		url          string
+		expectedCode int
+		expectedBody string
+	}{
+		{
+			"invalid orderId",
+			&mocks.MockOrderBookService{},
+			"/order/invalid",
+			http.StatusBadRequest,
+			"invalid order ID\n",
+		},
+		{
+			"no user in context",
+			&mocks.MockOrderBookService{Error: models.ErrNoUserInContext},
+			fmt.Sprintf("/order/%s", orderId.String()),
+			http.StatusUnauthorized,
+			"User not found\n",
+		},
+		{
+			"order not found",
+			&mocks.MockOrderBookService{Error: models.ErrOrderNotFound},
+			fmt.Sprintf("/order/%s", orderId.String()),
+			http.StatusNotFound,
+			"Order not found\n",
+		},
+		{
+			"trying to cancel order that is not open",
+			&mocks.MockOrderBookService{Error: models.ErrOrderNotOpen},
+			fmt.Sprintf("/order/%s", orderId.String()),
+			http.StatusNotFound,
+			"Order not found\n",
+		},
+		{
+			"unexpected error from service",
+			&mocks.MockOrderBookService{Error: assert.AnError},
+			fmt.Sprintf("/order/%s", orderId.String()),
+			http.StatusInternalServerError,
+			"Error cancelling order. Try again later\n",
+		},
+		{
+			"successful cancel",
+			&mocks.MockOrderBookService{},
+			fmt.Sprintf("/order/%s", orderId.String()),
+			http.StatusOK,
+			fmt.Sprintf("{\"orderId\":\"%s\"}", orderId.String()),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+
+			fmt.Println(test.name)
+
+			router := mux.NewRouter()
+
+			h, _ := rest.NewHandler(test.mockService, router)
+
+			req, err := http.NewRequest("DELETE", test.url, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			rr := httptest.NewRecorder()
+			router.HandleFunc("/order/{orderId}", h.CancelOrder).Methods("DELETE")
+
+			router.ServeHTTP(rr, req)
+
+			assert.Equal(t, test.expectedCode, rr.Code)
+			assert.Equal(t, test.expectedBody, rr.Body.String())
+		})
+	}
 }