From ea4b4833fef5a26aea5c0032b4bc1a907466787b Mon Sep 17 00:00:00 2001 From: Alex Browne Date: Tue, 19 May 2020 17:24:22 -0700 Subject: [PATCH] Return db.ErrNotFound where appropriate. Remove old TODOs --- core/core.go | 3 +-- db/{models.go => common.go} | 17 ++++++++++++++++ db/constants.go | 21 -------------------- db/db_test.go | 25 +++++++++++++++++++++--- db/sql_implementation.go | 26 +++++++++++++------------ zeroex/orderwatch/order_watcher.go | 24 ++++++++++------------- zeroex/orderwatch/order_watcher_test.go | 3 --- 7 files changed, 64 insertions(+), 55 deletions(-) rename db/{models.go => common.go} (82%) delete mode 100644 db/constants.go diff --git a/core/core.go b/core/core.go index eba59bebe..e56c5715c 100644 --- a/core/core.go +++ b/core/core.go @@ -458,8 +458,7 @@ func initPrivateKey(path string) (p2pcrypto.PrivKey, error) { func initMetadata(chainID int, database *db.DB) (*types.Metadata, error) { metadata, err := database.GetMetadata() if err != nil { - // TODO(albrow): Handle not found error. - if err == db.ErrMetadataNotFound { + if err == db.ErrNotFound { // No stored metadata found (first startup) metadata = &types.Metadata{ EthereumChainID: chainID, diff --git a/db/models.go b/db/common.go similarity index 82% rename from db/models.go rename to db/common.go index 8b76fc621..4e7ad7271 100644 --- a/db/models.go +++ b/db/common.go @@ -1,13 +1,30 @@ package db import ( + "errors" "fmt" + "time" "github.com/0xProject/0x-mesh/common/types" "github.com/0xProject/0x-mesh/ethereum" "github.com/0xProject/0x-mesh/zeroex" ) +const ( + // The default miniHeaderRetentionLimit used by Mesh. This default only gets overwritten in tests. + defaultMiniHeaderRetentionLimit = 20 + // The maximum MiniHeaders to query per page when deleting MiniHeaders + miniHeadersMaxPerPage = 1000 + // The amount of time to wait before timing out when connecting to the database for the first time. + connectTimeout = 10 * time.Second +) + +var ( + ErrDBFilledWithPinnedOrders = errors.New("the database is full of pinned orders; no orders can be removed in order to make space") + ErrMetadataAlreadyExists = errors.New("metadata already exists in the database (use UpdateMetadata instead?)") + ErrNotFound = errors.New("could not find existing model or row in database") +) + func ParseContractAddressesAndTokenIdsFromAssetData(assetData []byte, contractAddresses ethereum.ContractAddresses) ([]*types.SingleAssetData, error) { if len(assetData) == 0 { return []*types.SingleAssetData{}, nil diff --git a/db/constants.go b/db/constants.go deleted file mode 100644 index 1d84d15c3..000000000 --- a/db/constants.go +++ /dev/null @@ -1,21 +0,0 @@ -package db - -import ( - "errors" - "time" -) - -const ( - // The default miniHeaderRetentionLimit used by Mesh. This default only gets overwritten in tests. - defaultMiniHeaderRetentionLimit = 20 - // The maximum MiniHeaders to query per page when deleting MiniHeaders - miniHeadersMaxPerPage = 1000 - // The amount of time to wait before timing out when connecting to the database for the first time. - connectTimeout = 10 * time.Second -) - -var ( - ErrDBFilledWithPinnedOrders = errors.New("the database is full of pinned orders; no orders can be removed in order to make space") - ErrMetadataAlreadyExists = errors.New("metadata already exists in the database (use UpdateMetadata instead?)") - ErrMetadataNotFound = errors.New("could not find existing metadata in database") -) diff --git a/db/db_test.go b/db/db_test.go index 472ab13be..9fc015a59 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -62,6 +62,9 @@ func TestGetOrder(t *testing.T) { require.NoError(t, err) require.NotNil(t, foundOrder, "found order should not be nil") assertOrdersAreEqual(t, originalOrder, foundOrder) + + _, err = db.GetOrder(common.Hash{}) + assert.EqualError(t, err, ErrNotFound.Error(), "calling GetOrder with a hash that doesn't exist should return ErrNotFound") } func TestUpdateOrder(t *testing.T) { @@ -69,6 +72,11 @@ func TestUpdateOrder(t *testing.T) { defer cancel() db := newTestDB(t, ctx) + err := db.UpdateOrder(common.Hash{}, func(existingOrder *types.OrderWithMetadata) (*types.OrderWithMetadata, error) { + return existingOrder, nil + }) + assert.EqualError(t, err, ErrNotFound.Error(), "calling UpdateOrder with a hash that doesn't exist should return ErrNotFound") + // Note(albrow): We create more than one order to make sure that // UpdateOrder only updates one of them and does not affect the // others. @@ -77,7 +85,7 @@ func TestUpdateOrder(t *testing.T) { for i := 0; i < numOrders; i++ { originalOrders = append(originalOrders, newTestOrder()) } - _, _, err := db.AddOrders(originalOrders) + _, _, err = db.AddOrders(originalOrders) require.NoError(t, err) orderToUpdate := originalOrders[0] @@ -1315,6 +1323,9 @@ func TestGetMiniHeader(t *testing.T) { foundMiniHeader, err := db.GetMiniHeader(originalMiniHeader.Hash) require.NoError(t, err) assertMiniHeadersAreEqual(t, originalMiniHeader, foundMiniHeader) + + _, err = db.GetMiniHeader(common.Hash{}) + assert.EqualError(t, err, ErrNotFound.Error(), "calling GetMiniHeader with a hash that doesn't exist should return ErrNotFound") } func TestFindMiniHeaders(t *testing.T) { @@ -2116,8 +2127,11 @@ func TestGetMetadata(t *testing.T) { defer cancel() db := newTestDB(t, ctx) + _, err := db.GetMetadata() + assert.EqualError(t, err, ErrNotFound.Error(), "calling GetMetadata when it hasn't been saved yet should return ErrNotFound") + originalMetadata := newTestMetadata() - err := db.SaveMetadata(originalMetadata) + err = db.SaveMetadata(originalMetadata) require.NoError(t, err) foundMetadata, err := db.GetMetadata() @@ -2131,8 +2145,13 @@ func TestUpdateMetadata(t *testing.T) { defer cancel() db := newTestDB(t, ctx) + err := db.UpdateMetadata(func(existingMetadata *types.Metadata) *types.Metadata { + return existingMetadata + }) + assert.EqualError(t, err, ErrNotFound.Error(), "calling UpdateMetadata when it hasn't been saved yet should return ErrNotFound") + originalMetadata := newTestMetadata() - err := db.SaveMetadata(originalMetadata) + err = db.SaveMetadata(originalMetadata) require.NoError(t, err) updatedMaxExpirationTime := originalMetadata.MaxExpirationTime.Add(originalMetadata.MaxExpirationTime, big.NewInt(500)) diff --git a/db/sql_implementation.go b/db/sql_implementation.go index b3f1cc743..273a774c1 100644 --- a/db/sql_implementation.go +++ b/db/sql_implementation.go @@ -311,8 +311,9 @@ func (db *DB) AddOrders(orders []*types.OrderWithMetadata) (added []*types.Order func (db *DB) GetOrder(hash common.Hash) (*types.OrderWithMetadata, error) { var order sqltypes.Order if err := db.sqldb.GetContext(db.ctx, &order, "SELECT * FROM orders WHERE hash = $1", hash); err != nil { - // TODO(albrow): Specifically handle not found error. - // - Maybe wrap other types of errors for consistency with Dexie.js implementation? + if err == sql.ErrNoRows { + return nil, ErrNotFound + } return nil, err } return sqltypes.OrderToCommonType(&order), nil @@ -540,8 +541,6 @@ func whereConditionsFromOrderFilterOpts(opts []OrderFilter) ([]sqlz.WhereConditi func (db *DB) DeleteOrder(hash common.Hash) error { if _, err := db.sqldb.ExecContext(db.ctx, "DELETE FROM orders WHERE hash = $1", hash); err != nil { - // TODO(albrow): Specifically handle not found error. - // - Maybe wrap other types of errors for consistency with Dexie.js implementation? return err } return nil @@ -591,8 +590,9 @@ func (db *DB) UpdateOrder(hash common.Hash, updateFunc func(existingOrder *types var existingOrder sqltypes.Order if err := txn.GetContext(db.ctx, &existingOrder, "SELECT * FROM orders WHERE hash = $1", hash); err != nil { - // TODO(albrow): Specifically handle not found error. - // - Maybe wrap other types of errors for consistency with Dexie.js implementation? + if err == sql.ErrNoRows { + return ErrNotFound + } return err } @@ -654,8 +654,9 @@ func (db *DB) AddMiniHeaders(miniHeaders []*types.MiniHeader) (added []*types.Mi func (db *DB) GetMiniHeader(hash common.Hash) (*types.MiniHeader, error) { var miniHeader sqltypes.MiniHeader if err := db.sqldb.GetContext(db.ctx, &miniHeader, "SELECT * FROM miniHeaders WHERE hash = $1", hash); err != nil { - // TODO(albrow): Specifically handle not found error. - // - Maybe wrap other types of errors for consistency with Dexie.js implementation? + if err == sql.ErrNoRows { + return nil, ErrNotFound + } return nil, err } return sqltypes.MiniHeaderToCommonType(&miniHeader), nil @@ -812,12 +813,12 @@ func (db *DB) DeleteMiniHeaders(opts *MiniHeaderQuery) ([]*types.MiniHeader, err return sqltypes.MiniHeadersToCommonType(miniHeadersToDelete), nil } -// GetMetadata returns the metadata (or a db.NotFoundError if no metadata has been found). +// GetMetadata returns the metadata (or db.ErrNotFound if no metadata has been saved). func (db *DB) GetMetadata() (*types.Metadata, error) { var metadata sqltypes.Metadata if err := db.sqldb.GetContext(db.ctx, &metadata, "SELECT * FROM metadata LIMIT 1"); err != nil { if err == sql.ErrNoRows { - return nil, ErrMetadataNotFound + return nil, ErrNotFound } return nil, err } @@ -861,8 +862,9 @@ func (db *DB) UpdateMetadata(updateFunc func(oldmetadata *types.Metadata) (newMe var existingMetadata sqltypes.Metadata if err := txn.GetContext(db.ctx, &existingMetadata, "SELECT * FROM metadata LIMIT 1"); err != nil { - // TODO(albrow): Specifically handle not found error. - // - Maybe wrap other types of errors for consistency with Dexie.js implementation? + if err == sql.ErrNoRows { + return ErrNotFound + } return err } diff --git a/zeroex/orderwatch/order_watcher.go b/zeroex/orderwatch/order_watcher.go index 7e5b7bd57..38d8f3533 100644 --- a/zeroex/orderwatch/order_watcher.go +++ b/zeroex/orderwatch/order_watcher.go @@ -817,7 +817,6 @@ func (w *Watcher) Cleanup(ctx context.Context, lastUpdatedBuffer time.Duration) // _ = ordersColTxn.Discard() // }() lastUpdatedCutOff := time.Now().Add(-lastUpdatedBuffer) - // orders, err := w.meshDB.FindOrdersLastUpdatedBefore(lastUpdatedCutOff) orders, err := w.db.FindOrders(&db.OrderQuery{ Filters: []db.OrderFilter{ { @@ -1059,19 +1058,17 @@ func (w *Watcher) Subscribe(sink chan<- []*zeroex.OrderEvent) event.Subscription return w.orderScope.Track(w.orderFeed.Subscribe(sink)) } -// TODO(albrow): Do we even still need this method? func (w *Watcher) findOrder(orderHash common.Hash) *types.OrderWithMetadata { order, err := w.db.GetOrder(orderHash) if err != nil { - // TODO(albrow): Implement NotFoundError - // if _, ok := err.(db.NotFoundError); ok { - // // short-circuit. We expect to receive events from orders we aren't actively tracking - // return nil - // } + if err == db.ErrNotFound { + // short-circuit. We expect to receive events from orders we aren't actively tracking + return nil + } logger.WithFields(logger.Fields{ "error": err.Error(), "orderHash": orderHash, - }).Warning("Unexpected error using FindByID for order") + }).Warning("Unexpected error from db.GetOrder") return nil } return order @@ -1560,12 +1557,11 @@ func (w *Watcher) meshSpecificOrderValidation(orders []*zeroex.SignedOrder, chai // Check if order is already stored in DB dbOrder, err := w.db.GetOrder(orderHash) if err != nil { - // TODO(albrow): Handle not found error. - // if _, ok := err.(db.NotFoundError); !ok { - // logger.WithField("error", err).Error("could not check if order was already stored") - // return nil, nil, err - // } - // If the error is a db.NotFoundError, it just means the order is not currently stored in + if err != db.ErrNotFound { + logger.WithField("error", err).Error("could not check if order was already stored") + return nil, nil, err + } + // If the error is db.ErrNotFound, it just means the order is not currently stored in // the database. There's nothing else in the database to check, so we can continue. } else { // If stored but flagged for removal, reject it diff --git a/zeroex/orderwatch/order_watcher_test.go b/zeroex/orderwatch/order_watcher_test.go index f0bb3333d..b0611f707 100644 --- a/zeroex/orderwatch/order_watcher_test.go +++ b/zeroex/orderwatch/order_watcher_test.go @@ -109,7 +109,6 @@ func init() { } } -// TODO(albrow): Figure out why this test is failing. func TestOrderWatcherUnfundedInsufficientERC20Balance(t *testing.T) { if !serialTestsEnabled { t.Skip("Serial tests (tests which cannot run in parallel) are disabled. You can enable them with the --serial flag") @@ -804,7 +803,6 @@ func TestOrderWatcherERC20PartiallyFilled(t *testing.T) { assert.Equal(t, halfAmount, orders[0].FillableTakerAssetAmount) } -// TODO(albrow): Needs more MiniHeader methods. func TestOrderWatcherOrderExpiredThenUnexpired(t *testing.T) { if !serialTestsEnabled { t.Skip("Serial tests (tests which cannot run in parallel) are disabled. You can enable them with the --serial flag") @@ -1437,7 +1435,6 @@ func setupOrderWatcher(ctx context.Context, t *testing.T, ethRPCClient ethrpccli blockWatcherClient, err := blockwatch.NewRpcClient(ethRPCClient) require.NoError(t, err) topics := GetRelevantTopics() - // TODO(albrow): May need to be updated. blockWatcherConfig := blockwatch.Config{ DB: meshDB, PollingInterval: blockPollingInterval,