From 4a497e0bdf482b4c9c3f308c893cdc41f8c72d4f Mon Sep 17 00:00:00 2001 From: Luke Rogerson Date: Wed, 20 Dec 2023 21:56:32 +0000 Subject: [PATCH] PR feedback --- data/evmrepo/get_tx.go | 4 +- data/redisrepo/get_pending_swaps.go | 6 +- .../process_completed_swap_orders.go | 2 +- data/redisrepo/store_new_pending_swap.go | 2 +- data/redisrepo/store_new_pending_swap_test.go | 8 +- data/redisrepo/store_open_order.go | 3 +- data/redisrepo/store_pending_swaps.go | 2 +- data/store/store.go | 8 +- mocks/service.go | 8 +- mocks/{pending.go => swap_tx.go} | 2 +- models/errors.go | 3 +- models/order.go | 27 ++++-- models/order_test.go | 95 ++++++++++++++++--- models/{pending.go => swap_tx.go} | 14 +-- scripts/redis-repo/main.go | 6 +- service/evm_check_pending_txs.go | 24 ++++- 16 files changed, 155 insertions(+), 59 deletions(-) rename mocks/{pending.go => swap_tx.go} (89%) rename models/{pending.go => swap_tx.go} (69%) diff --git a/data/evmrepo/get_tx.go b/data/evmrepo/get_tx.go index 8cf1537..1e3b6ae 100644 --- a/data/evmrepo/get_tx.go +++ b/data/evmrepo/get_tx.go @@ -48,13 +48,13 @@ func (e *evmRepository) GetTx(ctx context.Context, id string) (*models.Tx, error } else { // If receipt is found, check the status if receipt.Status == 1 { - logctx.Info(ctx, "Transaction %q succeeded", logger.String("txHash", txHash.String())) + logctx.Info(ctx, "Transaction succeeded", logger.String("txHash", txHash.String())) return &models.Tx{ Status: models.TX_SUCCESS, TxHash: receipt.TxHash.Hex(), }, nil } else { - logctx.Info(ctx, "Transaction %q failed", logger.String("txHash", txHash.String())) + logctx.Info(ctx, "Transaction failed", logger.String("txHash", txHash.String())) return &models.Tx{ Status: models.TX_FAILURE, TxHash: receipt.TxHash.Hex(), diff --git a/data/redisrepo/get_pending_swaps.go b/data/redisrepo/get_pending_swaps.go index 26c6f7c..7601296 100644 --- a/data/redisrepo/get_pending_swaps.go +++ b/data/redisrepo/get_pending_swaps.go @@ -8,8 +8,8 @@ import ( ) // GetPendingSwaps returns all pending swaps that are waiting to be checked for completion -func (r *redisRepository) GetPendingSwaps(ctx context.Context) ([]models.Pending, error) { - var pendingSwaps []models.Pending +func (r *redisRepository) GetPendingSwaps(ctx context.Context) ([]models.SwapTx, error) { + var pendingSwaps []models.SwapTx pendings, err := r.client.LRange(ctx, CreatePendingSwapTxsKey(), 0, -1).Result() if err != nil { @@ -17,7 +17,7 @@ func (r *redisRepository) GetPendingSwaps(ctx context.Context) ([]models.Pending } for _, pending := range pendings { - var p models.Pending + var p models.SwapTx err := p.FromJson([]byte(pending)) if err != nil { return nil, fmt.Errorf("failed to unmarshal pending: %s", err) diff --git a/data/redisrepo/process_completed_swap_orders.go b/data/redisrepo/process_completed_swap_orders.go index 22b0962..eb03448 100644 --- a/data/redisrepo/process_completed_swap_orders.go +++ b/data/redisrepo/process_completed_swap_orders.go @@ -46,7 +46,7 @@ func (r *redisRepository) ProcessCompletedSwapOrders(ctx context.Context, orders // 2. Remove the swap swapKey := CreateSwapKey(swapId) - transaction.Del(ctx, swapKey).Err() + transaction.Del(ctx, swapKey) // --- END TRANSACTION --- _, err := transaction.Exec(ctx) diff --git a/data/redisrepo/store_new_pending_swap.go b/data/redisrepo/store_new_pending_swap.go index 3fc0242..3f84dc1 100644 --- a/data/redisrepo/store_new_pending_swap.go +++ b/data/redisrepo/store_new_pending_swap.go @@ -11,7 +11,7 @@ import ( ) // StoreNewPendingSwap stores a new pending swap in order for its status (pending/complete) to be checked later -func (r *redisRepository) StoreNewPendingSwap(ctx context.Context, p models.Pending) error { +func (r *redisRepository) StoreNewPendingSwap(ctx context.Context, p models.SwapTx) error { key := CreatePendingSwapTxsKey() jsonData, err := json.Marshal(p) diff --git a/data/redisrepo/store_new_pending_swap_test.go b/data/redisrepo/store_new_pending_swap_test.go index b7b60fc..e08e09b 100644 --- a/data/redisrepo/store_new_pending_swap_test.go +++ b/data/redisrepo/store_new_pending_swap_test.go @@ -19,11 +19,11 @@ func TestRedisRepo_StoreNewPendingSwap(t *testing.T) { client: db, } - pendingJson, _ := mocks.Pending.ToJson() + pendingJson, _ := mocks.SwapTx.ToJson() mock.ExpectRPush(CreatePendingSwapTxsKey(), pendingJson).SetVal(1) - err := repo.StoreNewPendingSwap(ctx, mocks.Pending) + err := repo.StoreNewPendingSwap(ctx, mocks.SwapTx) assert.NoError(t, err) }) @@ -35,9 +35,9 @@ func TestRedisRepo_StoreNewPendingSwap(t *testing.T) { client: db, } - mock.ExpectRPush(CreatePendingSwapTxsKey(), mocks.Pending.PendingToMap()).SetErr(assert.AnError) + mock.ExpectRPush(CreatePendingSwapTxsKey(), mocks.SwapTx.ToMap()).SetErr(assert.AnError) - err := repo.StoreNewPendingSwap(ctx, mocks.Pending) + err := repo.StoreNewPendingSwap(ctx, mocks.SwapTx) assert.ErrorContains(t, err, "failed to store pending swap tx") }) diff --git a/data/redisrepo/store_open_order.go b/data/redisrepo/store_open_order.go index d577d7b..553dcb8 100644 --- a/data/redisrepo/store_open_order.go +++ b/data/redisrepo/store_open_order.go @@ -13,7 +13,8 @@ import ( // These methods should be used to store UNFILLED or PARTIALLY FILLED orders in Redis. // // `StoreFilledOrders` should be used to store completely filled orders. - +// +// TODO: combine `StoreOpenOrder` and `StoreFilledOrder` into a single `StoreOrder` method that checks order status and stores accordingly. func (r *redisRepository) StoreOpenOrder(ctx context.Context, order models.Order) error { // --- START TRANSACTION --- diff --git a/data/redisrepo/store_pending_swaps.go b/data/redisrepo/store_pending_swaps.go index fee7461..e66f319 100644 --- a/data/redisrepo/store_pending_swaps.go +++ b/data/redisrepo/store_pending_swaps.go @@ -11,7 +11,7 @@ import ( ) // StorePendingSwaps stores all swaps that are still pending completion in order for their status (pending/complete) to be checked again later -func (r *redisRepository) StorePendingSwaps(ctx context.Context, pendingSwaps []models.Pending) error { +func (r *redisRepository) StorePendingSwaps(ctx context.Context, pendingSwaps []models.SwapTx) error { key := CreatePendingSwapTxsKey() transaction := r.client.TxPipeline() diff --git a/data/store/store.go b/data/store/store.go index 41cbc80..7babf72 100644 --- a/data/store/store.go +++ b/data/store/store.go @@ -29,9 +29,9 @@ type OrderBookStore interface { GetSwap(ctx context.Context, swapId uuid.UUID) ([]models.OrderFrag, error) StoreSwap(ctx context.Context, swapId uuid.UUID, frags []models.OrderFrag) error RemoveSwap(ctx context.Context, swapId uuid.UUID) error - // Pending transactions - StoreNewPendingSwap(ctx context.Context, pendingSwap models.Pending) error - GetPendingSwaps(ctx context.Context) ([]models.Pending, error) - StorePendingSwaps(ctx context.Context, pendingSwaps []models.Pending) error + // Pending transactions (TODO: rename) + StoreNewPendingSwap(ctx context.Context, pendingSwap models.SwapTx) error + GetPendingSwaps(ctx context.Context) ([]models.SwapTx, error) + StorePendingSwaps(ctx context.Context, pendingSwaps []models.SwapTx) error ProcessCompletedSwapOrders(ctx context.Context, orders []*models.Order, swapId uuid.UUID, isSuccessful bool) error } diff --git a/mocks/service.go b/mocks/service.go index 375d70d..5849504 100644 --- a/mocks/service.go +++ b/mocks/service.go @@ -26,7 +26,7 @@ type MockOrderBookStore struct { // re-entrance Sets map[string]map[string]struct{} // Pending swaps - PendingSwaps []models.Pending + PendingSwaps []models.SwapTx } func (m *MockOrderBookStore) StoreOpenOrder(ctx context.Context, order models.Order) error { @@ -140,15 +140,15 @@ func (m *MockOrderBookStore) UpdateSwapTracker(ctx context.Context, swapStatus m return m.Error } -func (m *MockOrderBookStore) StoreNewPendingSwap(ctx context.Context, pendingSwap models.Pending) error { +func (m *MockOrderBookStore) StoreNewPendingSwap(ctx context.Context, pendingSwap models.SwapTx) error { return m.Error } -func (m *MockOrderBookStore) GetPendingSwaps(ctx context.Context) ([]models.Pending, error) { +func (m *MockOrderBookStore) GetPendingSwaps(ctx context.Context) ([]models.SwapTx, error) { return m.PendingSwaps, m.Error } -func (m *MockOrderBookStore) StorePendingSwaps(ctx context.Context, pendingSwaps []models.Pending) error { +func (m *MockOrderBookStore) StorePendingSwaps(ctx context.Context, pendingSwaps []models.SwapTx) error { return m.Error } diff --git a/mocks/pending.go b/mocks/swap_tx.go similarity index 89% rename from mocks/pending.go rename to mocks/swap_tx.go index c425f61..7b10886 100644 --- a/mocks/pending.go +++ b/mocks/swap_tx.go @@ -5,7 +5,7 @@ import ( "github.com/orbs-network/order-book/models" ) -var Pending = models.Pending{ +var SwapTx = models.SwapTx{ SwapId: uuid.MustParse("b3f9e3a0-5b7a-4b7a-8b0a-9b9b9b9b9b9b"), TxHash: "0x5dcbfe934287c50363e5c82502739aadd4d535a1f7c0ccd7a8088fb4dfd800da", } diff --git a/models/errors.go b/models/errors.go index 6c24c49..e5d0fd1 100644 --- a/models/errors.go +++ b/models/errors.go @@ -19,7 +19,8 @@ var ErrOrderFilled = errors.New("order is filled") var ErrInvalidInput = errors.New("invalid input") var ErrSignatureVerificationError = errors.New("signature verification error") var ErrSignatureVerificationFailed = errors.New("signature verification failed") -var ErrInvalidSize = errors.New("updated sizeFilled is greater than size") +var ErrUnexpectedSizeFilled = errors.New("unexpected sizeFilled") +var ErrUnexpectedSizePending = errors.New("unexpected sizePending") // store generic errors var ErrValAlreadyInSet = errors.New("the value is already a member of the set") diff --git a/models/order.go b/models/order.go index ec621b1..591a390 100644 --- a/models/order.go +++ b/models/order.go @@ -7,6 +7,8 @@ import ( "time" "github.com/google/uuid" + "github.com/orbs-network/order-book/utils/logger" + "github.com/orbs-network/order-book/utils/logger/logctx" "github.com/shopspring/decimal" ) @@ -215,21 +217,30 @@ func (o *Order) Status() string { return "OPEN" } -func (o *Order) MarkSwapSuccess() (isFilled bool, err error) { - - newSizeFilled := o.SizeFilled.Add(o.SizePending) +func (o *Order) Fill(ctx context.Context, fillSize decimal.Decimal) (isFilled bool, err error) { + newSizeFilled := o.SizeFilled.Add(fillSize) if newSizeFilled.GreaterThan(o.Size) { - return false, ErrInvalidSize + logctx.Error(ctx, "total size is less than requested fill size", logger.String("orderId", o.Id.String()), logger.String("orderSize", o.Size.String()), logger.String("requestedFillSize", fillSize.String())) + return false, ErrUnexpectedSizeFilled } - o.SizeFilled = newSizeFilled - o.SizePending = decimal.Zero + if fillSize.GreaterThan(o.SizePending) { + logctx.Error(ctx, "fillSize is greater than sizePending", logger.String("orderId", o.Id.String()), logger.String("pendingSize", o.SizePending.String()), logger.String("requestedFillSize", fillSize.String())) + return false, ErrUnexpectedSizePending + } + o.SizeFilled = o.SizeFilled.Add(fillSize) + o.SizePending = o.SizePending.Sub(fillSize) return o.IsFilled(), nil } -func (o *Order) MarkSwapFailed() error { - o.SizePending = decimal.Zero +func (o *Order) Kill(ctx context.Context, size decimal.Decimal) error { + if o.SizePending.LessThan(size) { + logctx.Error(ctx, "size to be rolled back is greater than sizePending", logger.String("orderId", o.Id.String()), logger.String("pendingSize", o.SizePending.String()), logger.String("requestedKillSize", size.String())) + return ErrUnexpectedSizeFilled + } + + o.SizePending = o.SizePending.Sub(size) return nil } diff --git a/models/order_test.go b/models/order_test.go index 34ec501..c56fbeb 100644 --- a/models/order_test.go +++ b/models/order_test.go @@ -1,6 +1,7 @@ package models import ( + "context" "testing" "time" @@ -126,12 +127,15 @@ func TestOrder_MapToOrder(t *testing.T) { }) } -func TestOrder_MarkSwapComplete(t *testing.T) { - // generate table test +func TestOrder_Fill(t *testing.T) { + + ctx := context.Background() + tests := []struct { name string order Order expected Order + fillSize decimal.Decimal isFilled bool error error }{ @@ -147,6 +151,7 @@ func TestOrder_MarkSwapComplete(t *testing.T) { SizeFilled: decimal.NewFromInt(1000), SizePending: decimal.Zero, }, + fillSize: decimal.NewFromInt(1000), isFilled: true, }, { @@ -161,10 +166,11 @@ func TestOrder_MarkSwapComplete(t *testing.T) { SizeFilled: decimal.NewFromInt(1000), SizePending: decimal.Zero, }, + fillSize: decimal.NewFromInt(500), isFilled: true, }, { - name: "size 23782378.50, sizeFilled 2.38, sizePending 1238.12", + name: "partial fill", order: Order{ Size: decimal.NewFromFloat(23782378.50), SizeFilled: decimal.NewFromFloat(2.38), @@ -172,31 +178,49 @@ func TestOrder_MarkSwapComplete(t *testing.T) { }, expected: Order{ Size: decimal.NewFromFloat(23782378.50), - SizeFilled: decimal.NewFromFloat(1240.50), - SizePending: decimal.Zero, + SizeFilled: decimal.NewFromFloat(2.38).Add(decimal.NewFromFloat(10)), + SizePending: decimal.NewFromFloat(1238.12).Sub(decimal.NewFromFloat(10)), }, + fillSize: decimal.NewFromFloat(10), isFilled: false, }, { - name: "size 1, sizeFilled 0.5, sizePending 1", + name: "total size is less than requested fill size", order: Order{ - Size: decimal.NewFromInt(1), - SizeFilled: decimal.NewFromFloat(0.5), - SizePending: decimal.NewFromInt(1), + Size: decimal.NewFromFloat(10.00), + SizeFilled: decimal.NewFromFloat(9.00), + SizePending: decimal.NewFromFloat(2.89), }, expected: Order{ - Size: decimal.NewFromInt(1), - SizeFilled: decimal.NewFromFloat(0.5), - SizePending: decimal.NewFromInt(1), + Size: decimal.NewFromFloat(10.00), + SizeFilled: decimal.NewFromFloat(9.00), + SizePending: decimal.NewFromFloat(2.89), }, + fillSize: decimal.NewFromFloat(2.00), isFilled: false, - error: ErrInvalidSize, + error: ErrUnexpectedSizeFilled, + }, + { + name: "size to be filled is greater than size pending", + order: Order{ + Size: decimal.NewFromFloat(10.00), + SizeFilled: decimal.NewFromFloat(2.00), + SizePending: decimal.NewFromFloat(2.00), + }, + expected: Order{ + Size: decimal.NewFromFloat(10.00), + SizeFilled: decimal.NewFromFloat(2.00), + SizePending: decimal.NewFromFloat(2.00), + }, + fillSize: decimal.NewFromFloat(4.00), + isFilled: false, + error: ErrUnexpectedSizePending, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - isFilled, err := test.order.MarkSwapSuccess() + isFilled, err := test.order.Fill(ctx, test.fillSize) assert.Equal(t, test.expected.Size.String(), test.order.Size.String(), "size should be equal") assert.Equal(t, test.expected.SizeFilled.String(), test.order.SizeFilled.String(), "sizeFilled should be equal") assert.Equal(t, test.expected.SizePending.String(), test.order.SizePending.String(), "sizePending should be equal") @@ -205,3 +229,46 @@ func TestOrder_MarkSwapComplete(t *testing.T) { }) } } + +func TestOrder_Kill(t *testing.T) { + + ctx := context.Background() + + tests := []struct { + name string + order Order + expected Order + killSize decimal.Decimal + error error + }{ + { + name: "sizePending 1000, killSize 1000", + order: Order{ + SizePending: decimal.NewFromFloat(1000), + }, + expected: Order{ + SizePending: decimal.Zero, + }, + killSize: decimal.NewFromInt(1000), + }, + { + name: "sizePending 500, killSize 1000", + order: Order{ + SizePending: decimal.NewFromInt(500), + }, + expected: Order{ + SizePending: decimal.NewFromInt(500), + }, + killSize: decimal.NewFromInt(1000), + error: ErrUnexpectedSizeFilled, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.order.Kill(ctx, test.killSize) + assert.Equal(t, test.expected.SizePending.String(), test.order.SizePending.String(), "sizePending should be equal") + assert.Equal(t, test.error, err, "error should be equal") + }) + } +} diff --git a/models/pending.go b/models/swap_tx.go similarity index 69% rename from models/pending.go rename to models/swap_tx.go index 9af1148..94c1aa3 100644 --- a/models/pending.go +++ b/models/swap_tx.go @@ -7,19 +7,19 @@ import ( "github.com/google/uuid" ) -type Pending struct { +type SwapTx struct { SwapId uuid.UUID `json:"swapId"` TxHash string `json:"txHash"` } -func (p *Pending) PendingToMap() map[string]interface{} { +func (p *SwapTx) ToMap() map[string]interface{} { return map[string]interface{}{ "swapId": p.SwapId.String(), "txHash": p.TxHash, } } -func (p *Pending) MapToPending(data map[string]string) error { +func (p *SwapTx) FromMap(data map[string]string) error { if len(data) == 0 { return fmt.Errorf("no data provided") } @@ -45,19 +45,19 @@ func (p *Pending) MapToPending(data map[string]string) error { return nil } -func (p *Pending) ToJson() ([]byte, error) { +func (p *SwapTx) ToJson() ([]byte, error) { jsonData, err := json.Marshal(p) if err != nil { - return nil, fmt.Errorf("failed to marshal pending: %s", err) + return nil, fmt.Errorf("failed to marshal swapTx: %s", err) } return jsonData, nil } -func (p *Pending) FromJson(data []byte) error { +func (p *SwapTx) FromJson(data []byte) error { err := json.Unmarshal(data, p) if err != nil { - return fmt.Errorf("failed to unmarshal pending: %s", err) + return fmt.Errorf("failed to unmarshal swapTx: %s", err) } return nil diff --git a/scripts/redis-repo/main.go b/scripts/redis-repo/main.go index 553bb9b..bc2d50e 100644 --- a/scripts/redis-repo/main.go +++ b/scripts/redis-repo/main.go @@ -400,7 +400,7 @@ func updateUser(newApiKey string) { } func storePendingSwap() { - p := models.Pending{ + p := models.SwapTx{ SwapId: uuid.New(), TxHash: "0x5dcbfe934287c50363e5c82502739aadd4d535a1f7c0ccd7a8088fb4dfd800da", } @@ -424,9 +424,9 @@ func getPendingSwaps() { } func storePendingSwaps() { - var pendingSwaps []models.Pending + var pendingSwaps []models.SwapTx for i := 0; i < 1000; i++ { - p := models.Pending{ + p := models.SwapTx{ SwapId: uuid.New(), TxHash: "0x5dcbfe934287c50363e5c82502739aadd4d535a1f7c0ccd7a8088fb4dfd800da", } diff --git a/service/evm_check_pending_txs.go b/service/evm_check_pending_txs.go index 88068be..c3fc5bc 100644 --- a/service/evm_check_pending_txs.go +++ b/service/evm_check_pending_txs.go @@ -9,8 +9,10 @@ import ( "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/shopspring/decimal" ) +// CheckPendingTxs checks all pending transactions and updates the order book accordingly func (e *EvmClient) CheckPendingTxs(ctx context.Context) error { logctx.Info(ctx, "Checking pending transactions...") pendingSwaps, err := e.orderBookStore.GetPendingSwaps(ctx) @@ -27,7 +29,7 @@ func (e *EvmClient) CheckPendingTxs(ctx context.Context) error { var wg sync.WaitGroup var mu sync.Mutex - ptxs := make([]models.Pending, 0) + ptxs := make([]models.SwapTx, 0) for i := 0; i < len(pendingSwaps); i++ { wg.Add(1) @@ -91,7 +93,7 @@ func (e *EvmClient) CheckPendingTxs(ctx context.Context) error { return nil } -func (e *EvmClient) processCompletedTransaction(ctx context.Context, p models.Pending, isSuccessful bool, mu *sync.Mutex) ([]models.Order, error) { +func (e *EvmClient) processCompletedTransaction(ctx context.Context, p models.SwapTx, isSuccessful bool, mu *sync.Mutex) ([]models.Order, error) { mu.Lock() defer mu.Unlock() @@ -102,8 +104,11 @@ func (e *EvmClient) processCompletedTransaction(ctx context.Context, p models.Pe } var orderIds []uuid.UUID + orderSizes := make(map[uuid.UUID]decimal.Decimal) + for _, frag := range orderFrags { orderIds = append(orderIds, frag.OrderId) + orderSizes[frag.OrderId] = frag.Size } orders, err := e.orderBookStore.FindOrdersByIds(ctx, orderIds) @@ -115,12 +120,23 @@ func (e *EvmClient) processCompletedTransaction(ctx context.Context, p models.Pe var swapOrders []*models.Order for _, order := range orders { + + size, found := orderSizes[order.Id] + if !found { + logctx.Error(ctx, "Failed to get order frag size", logger.String("orderId", order.Id.String())) + return []models.Order{}, fmt.Errorf("failed to get order frag size") + } + if isSuccessful { - if _, err := order.MarkSwapSuccess(); err != nil { + if _, err := order.Fill(ctx, size); err != nil { logctx.Error(ctx, "Failed to mark order as filled", logger.Error(err), logger.String("orderId", order.Id.String())) + continue } } else { - order.MarkSwapFailed() + if err := order.Kill(ctx, size); err != nil { + logctx.Error(ctx, "Failed to mark order as killed", logger.Error(err), logger.String("orderId", order.Id.String())) + continue + } } swapOrders = append(swapOrders, &order) }