Return db.ErrNotFound where appropriate. Remove old TODOs

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,

db/models.go → 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

db/db_test.go

@@ -62,13 +62,21 @@ 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) {
 	ctx, cancel := context.WithCancel(context.Background())
 	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))

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
 	}
 

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

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,