Skip to content

Commit

Permalink
Update cancel all orders for user to return cancelled order IDs, add …
Browse files Browse the repository at this point in the history
…find orders by IDs batch Redis method (#52)
  • Loading branch information
Luke-Rogerson authored Dec 7, 2023
1 parent 3c56554 commit 8925dc2
Show file tree
Hide file tree
Showing 18 changed files with 508 additions and 102 deletions.
56 changes: 34 additions & 22 deletions data/redisrepo/cancel_orders_for_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,44 +10,56 @@ import (
"github.com/orbs-network/order-book/utils/logger/logctx"
)

// TODO: this is a fairly expensive operation. We should use bulk operations to improve performance, and consider batching removals in case a of a large number of orders.
func (r *redisRepository) CancelOrdersForUser(ctx context.Context, userId uuid.UUID) error {
func (r *redisRepository) CancelOrdersForUser(ctx context.Context, userId uuid.UUID) ([]uuid.UUID, error) {
userOrdersKey := CreateUserOrdersKey(userId)

// Fetch all order IDs for the user
orderIdStrs, err := r.client.ZRange(ctx, userOrdersKey, 0, -1).Result()
if err != nil {
logctx.Error(ctx, "failed to get order IDs for user", logger.String("userId", userId.String()), logger.Error(err))
return fmt.Errorf("failed to fetch user order IDs. Reason: %v", err)
return nil, fmt.Errorf("failed to get order IDs for user: %v", err)
}

if len(orderIdStrs) == 0 {
logctx.Info(ctx, "no orders found for user", logger.String("userId", userId.String()))
return nil
logctx.Warn(ctx, "no orders found for user", logger.String("userId", userId.String()))
return nil, models.ErrNoOrdersFound
}

// --- START TRANSACTION ---
transaction := r.client.TxPipeline()

// Iterate through each order ID and remove associated data
// Convert string IDs to UUIDs
var orderIds []uuid.UUID
for _, orderIdStr := range orderIdStrs {
orderId, err := uuid.Parse(orderIdStr)
if err != nil {
logctx.Error(ctx, "failed to parse order ID", logger.String("orderId", orderIdStr), logger.Error(err))
return fmt.Errorf("failed to parse order ID: %v", err)
return nil, fmt.Errorf("failed to parse order ID: %v", err)
}
orderIds = append(orderIds, orderId)
}

// Remove client order ID
order, err := r.FindOrderById(ctx, orderId, false)
if err == models.ErrOrderNotFound {
logctx.Error(ctx, "order should exist but could not be found by ID. Continuing with user's other orders", logger.String("orderId", orderIdStr), logger.Error(err))
continue
// Fetch all orders for the user (in batches)
var ordersToCancel []models.Order
for i := 0; i < len(orderIds); i += MAX_ORDER_IDS {
end := i + MAX_ORDER_IDS
if end > len(orderIds) {
end = len(orderIds)
}

orders, err := r.FindOrdersByIds(ctx, orderIds[i:end])
if err != nil {
logctx.Error(ctx, "unexpected error finding order by ID", logger.String("orderId", orderIdStr), logger.Error(err))
return fmt.Errorf("unexpected error finding order by ID: %v", err)
logctx.Error(ctx, "failed to find orders by IDs", logger.String("userId", userId.String()), logger.Error(err))
return nil, fmt.Errorf("failed to find orders by IDs: %v", err)
}

ordersToCancel = append(ordersToCancel, orders...)
}

// --- START TRANSACTION ---
transaction := r.client.TxPipeline()

// Iterate through each order and remove associated data
for _, order := range ordersToCancel {

// Remove client order ID
clientOIDKey := CreateClientOIDKey(order.ClientOId)
transaction.Del(ctx, clientOIDKey)

Expand All @@ -60,10 +72,10 @@ func (r *redisRepository) CancelOrdersForUser(ctx context.Context, userId uuid.U
}

// Remove order details by order ID
orderIDKey := CreateOrderIDKey(orderId)
orderIDKey := CreateOrderIDKey(order.Id)
transaction.Del(ctx, orderIDKey)

logctx.Info(ctx, "removed order", logger.String("orderId", orderIdStr), logger.String("symbol", order.Symbol.String()), logger.String("side", order.Side.String()), logger.String("userId", userId.String()))
logctx.Info(ctx, "removed order", logger.String("orderId", order.Id.String()), logger.String("symbol", order.Symbol.String()), logger.String("side", order.Side.String()), logger.String("userId", userId.String()))
}

// Remove the user's orders key
Expand All @@ -73,10 +85,10 @@ func (r *redisRepository) CancelOrdersForUser(ctx context.Context, userId uuid.U
_, err = transaction.Exec(ctx)
if err != nil {
logctx.Error(ctx, "failed to remove user's orders in Redis", logger.String("userId", userId.String()), logger.Error(err))
return fmt.Errorf("failed to remove user's orders in Redis. Reason: %v", err)
return nil, fmt.Errorf("failed to remove user's orders in Redis. Reason: %v", err)
}
// --- END TRANSACTION ---

logctx.Info(ctx, "removed all orders for user", logger.String("userId", userId.String()))
return nil
logctx.Info(ctx, "removed all orders for user", logger.String("userId", userId.String()), logger.Int("numOrders", len(ordersToCancel)))
return orderIds, nil
}
56 changes: 22 additions & 34 deletions data/redisrepo/cancel_orders_for_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {
mock.ExpectDel(CreateUserOrdersKey(mocks.UserId)).SetVal(1)
mock.ExpectTxPipelineExec()

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.Equal(t, mocks.OrderId, orderIds[0])
assert.Len(t, orderIds, 1)
assert.NoError(t, err)
})

Expand All @@ -56,7 +58,10 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {
mock.ExpectDel(CreateUserOrdersKey(mocks.UserId)).SetVal(1)
mock.ExpectTxPipelineExec()

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.Equal(t, mocks.OrderId, orderIds[0])
assert.Equal(t, orderTwo.Id, orderIds[1])
assert.Len(t, orderIds, 2)
assert.NoError(t, err)
})

Expand All @@ -68,11 +73,13 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {
}

mock.ExpectZRange(CreateUserOrdersKey(mocks.UserId), 0, -1).SetErr(assert.AnError)
err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.ErrorContains(t, err, "failed to fetch user order IDs. Reason: assert.AnError")
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)

assert.Empty(t, orderIds)
assert.ErrorContains(t, err, "failed to get order IDs for user")
})

t.Run("should exit without error if no orders found for user", func(t *testing.T) {
t.Run("should return `ErrNoOrdersFound` error if no orders are found for the user", func(t *testing.T) {
db, mock := redismock.NewClientMock()

repo := &redisRepository{
Expand All @@ -81,8 +88,9 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {

mock.ExpectZRange(CreateUserOrdersKey(mocks.UserId), 0, -1).SetVal([]string{})

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.NoError(t, err)
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.Empty(t, orderIds)
assert.ErrorIs(t, err, models.ErrNoOrdersFound)
})

t.Run("should exit with error if failed to parse order ID", func(t *testing.T) {
Expand All @@ -93,12 +101,11 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {
}

mock.ExpectZRange(CreateUserOrdersKey(mocks.UserId), 0, -1).SetVal([]string{"invalid"})
err := repo.CancelOrdersForUser(ctx, mocks.UserId)
_, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.ErrorContains(t, err, "failed to parse order ID: invalid UUID length: 7")
})

// TODO: confirm whether this is the behaviour we want
t.Run("should continue removing other orders if one order is not found by ID", func(t *testing.T) {
t.Run("should immediately return error if one order is not found by ID", func(t *testing.T) {
db, mock := redismock.NewClientMock()

repo := &redisRepository{
Expand All @@ -107,30 +114,10 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {

mock.ExpectZRange(CreateUserOrdersKey(mocks.UserId), 0, -1).SetVal([]string{mocks.OrderId.String(), orderTwo.Id.String()})
mock.ExpectHGetAll(CreateOrderIDKey(orderTwo.Id)).SetErr(models.ErrOrderNotFound) // order not found - break out of loop iteration
mock.ExpectHGetAll(CreateOrderIDKey(orderTwo.Id)).SetVal(mocks.Order.OrderToMap())
mock.ExpectTxPipeline()
mock.ExpectDel(CreateClientOIDKey(orderTwo.ClientOId)).SetVal(1)
mock.ExpectZRem(CreateBuySidePricesKey(orderTwo.Symbol), orderTwo.Id.String()).SetVal(1)
mock.ExpectDel(CreateOrderIDKey(orderTwo.Id)).SetVal(1)
mock.ExpectDel(CreateUserOrdersKey(orderTwo.UserId)).SetVal(1)
mock.ExpectTxPipelineExec()

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.NoError(t, err)
})

t.Run("should exit with error if failed to get order by ID", func(t *testing.T) {
db, mock := redismock.NewClientMock()

repo := &redisRepository{
client: db,
}

mock.ExpectZRange(CreateUserOrdersKey(mocks.UserId), 0, -1).SetVal([]string{mocks.OrderId.String()})
mock.ExpectHGetAll(CreateOrderIDKey(mocks.OrderId)).SetErr(assert.AnError)

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.ErrorContains(t, err, "unexpected error finding order by ID: assert.AnError")
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.Empty(t, orderIds)
assert.ErrorContains(t, err, "failed to find orders by IDs")
})

t.Run("an error should be returned if transaction failed", func(t *testing.T) {
Expand All @@ -149,7 +136,8 @@ func TestRedisRepository_CancelAllOrdersForUser(t *testing.T) {
mock.ExpectDel(CreateUserOrdersKey(mocks.UserId)).SetErr(assert.AnError)
mock.ExpectTxPipelineExec().SetErr(assert.AnError)

err := repo.CancelOrdersForUser(ctx, mocks.UserId)
orderIds, err := repo.CancelOrdersForUser(ctx, mocks.UserId)
assert.Empty(t, orderIds)
assert.ErrorContains(t, err, "failed to remove user's orders in Redis. Reason: assert.AnError")
})

Expand Down
1 change: 1 addition & 0 deletions data/redisrepo/find_order_by_id.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,6 @@ func (r *redisRepository) FindOrderById(ctx context.Context, id uuid.UUID, isCli
return nil, err
}

logctx.Info(ctx, "found order", logger.String("orderId", order.Id.String()))
return order, nil
}
64 changes: 64 additions & 0 deletions data/redisrepo/find_orders_by_ids.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package redisrepo

import (
"context"
"errors"
"fmt"

"github.com/google/uuid"
"github.com/orbs-network/order-book/models"
"github.com/orbs-network/order-book/utils/logger"
"github.com/orbs-network/order-book/utils/logger/logctx"
"github.com/redis/go-redis/v9"
)

const MAX_ORDER_IDS = 100

// FindOrdersByIds finds orders by their IDs. If an order is not found for any of the provided IDs, an error is returned.
//
// Only finding orders by their IDs is supported, not by their clientOIds.
func (r *redisRepository) FindOrdersByIds(ctx context.Context, ids []uuid.UUID) ([]models.Order, error) {

if len(ids) > MAX_ORDER_IDS {
return nil, fmt.Errorf("exceeded maximum number of IDs: %d", MAX_ORDER_IDS)
}

pipeline := r.client.Pipeline()

cmds := make([]*redis.MapStringStringCmd, len(ids))
for i, id := range ids {
cmds[i] = pipeline.HGetAll(ctx, CreateOrderIDKey(id))
}

_, err := pipeline.Exec(ctx)
if err != nil {
logctx.Error(ctx, "failed to execute pipeline getting orders by IDs", logger.Error(err), logger.Int("numIds", len(ids)))
return nil, fmt.Errorf("failed to execute pipeline: %v", err)
}

orders := make([]models.Order, 0, len(ids))
for _, cmd := range cmds {
orderMap, err := cmd.Result()
if err != nil {
logctx.Error(ctx, "could not get order", logger.Error(err))
return nil, fmt.Errorf("could not get order: %v", err)
}

if len(orderMap) == 0 {
logctx.Warn(ctx, "order not found but was expected to exist", logger.String("orderId", cmd.Args()[1].(string)))
return nil, errors.New("order not found but was expected to exist")
}

order := models.Order{}
err = order.MapToOrder(orderMap)
if err != nil {
logctx.Error(ctx, "could not map order", logger.Error(err))
return nil, fmt.Errorf("could not map order: %v", err)
}

orders = append(orders, order)
}

logctx.Info(ctx, "found orders by IDs", logger.Int("numIdsProvided", len(ids)), logger.Int("numOrders", len(orders)))
return orders, nil
}
Loading

0 comments on commit 8925dc2

Please sign in to comment.