Merge pull request #1549 from bhandras/upsert-addr

   tapdb: allow reinsertion of an existing address to the book

Author: guggero

.gitignore

@@ -4,6 +4,7 @@
 *.dll
 *.so
 *.dylib
+.DS_Store
 tapcli-debug
 tapd-debug
 /tapcli

tapdb/addrs.go

@@ -32,7 +32,7 @@ type (
 	AddrQuery = sqlc.FetchAddrsParams
 
 	// NewAddr is a type alias for the params to create a new address.
-	NewAddr = sqlc.InsertAddrParams
+	NewAddr = sqlc.UpsertAddrParams
 
 	// Addresses is a type alias for the full address row with key locator
 	// information.
@@ -82,6 +82,14 @@ type (
 	AssetMeta = sqlc.FetchAssetMetaForAssetRow
 )
 
+var (
+	// ErrConflictingAddress is returned when an address (with the same
+	// output key) already exists in the database and we're attempting to
+	// re-insert it but at least one of the fields (except creation_time)
+	// is different.
+	ErrConflictingAddress = errors.New("failed to add conflicting address")
+)
+
 // AddrBook is an interface that represents the storage backed needed to create
 // the TapAddressBook book. We need to be able to insert/fetch addresses, and
 // also make internal keys since each address has an internal key and a script
@@ -109,8 +117,9 @@ type AddrBook interface {
 	FetchAddrByTaprootOutputKey(ctx context.Context,
 		arg []byte) (AddrByTaprootOutput, error)
 
-	// InsertAddr inserts a new address into the database.
-	InsertAddr(ctx context.Context, arg NewAddr) (int64, error)
+	// UpsertAddr upserts a new address into the database returning the
+	// primary key.
+	UpsertAddr(ctx context.Context, arg NewAddr) (int64, error)
 
 	// UpsertInternalKey inserts a new or updates an existing internal key
 	// into the database and returns the primary key.
@@ -302,7 +311,7 @@ func (t *TapAddressBook) InsertAddrs(ctx context.Context,
 				addr.Tap.ProofCourierAddr.String(),
 			)
 
-			_, err = db.InsertAddr(ctx, NewAddr{
+			_, err = db.UpsertAddr(ctx, NewAddr{
 				Version:          int16(addr.Version),
 				AssetVersion:     int16(addr.AssetVersion),
 				GenesisAssetID:   genAssetID,
@@ -319,6 +328,10 @@ func (t *TapAddressBook) InsertAddrs(ctx context.Context,
 				ProofCourierAddr: proofCourierAddrBytes,
 			})
 			if err != nil {
+				if errors.Is(err, sql.ErrNoRows) {
+					return ErrConflictingAddress
+				}
+
 				return fmt.Errorf("unable to insert addr: %w",
 					err)
 			}

tapdb/addrs_test.go

@@ -172,6 +172,23 @@ func TestAddressInsertion(t *testing.T) {
 	}
 	require.NoError(t, addrBook.InsertAddrs(ctx, addrs...))
 
+	// Now insert the addresses again, to test that upsert works as well.
+	require.NoError(t, addrBook.InsertAddrs(ctx, addrs...))
+
+	// Changing an address' creation time should not affect the insertion
+	// process but should not change the database either.
+	addrTime := addrs[0].CreationTime
+	addrs[0].CreationTime = addrTime.Add(time.Second)
+	require.NoError(t, addrBook.InsertAddrs(ctx, addrs[0]))
+	addrs[0].CreationTime = addrTime
+
+	// Now change the amount, which upon upsert should trigger an error.
+	addrs[0].Amount += 1
+	require.Error(t,
+		addrBook.InsertAddrs(ctx, addrs[0]), ErrConflictingAddress,
+	)
+	addrs[0].Amount -= 1
+
 	// Now we should be able to fetch the complete set of addresses with
 	// the query method without specifying any special params.
 	dbAddrs, err := addrBook.QueryAddrs(ctx, address.QueryParams{})

tapdb/sqlc/addrs.sql.go

@@ -1,9 +1,45 @@
--- name: InsertAddr :one
+-- name: UpsertAddr :one
 INSERT INTO addrs (
-    version, asset_version, genesis_asset_id, group_key, script_key_id,
-    taproot_key_id, tapscript_sibling, taproot_output_key, amount, asset_type,
-    creation_time, proof_courier_addr
-) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING id;
+    version,
+    asset_version,
+    genesis_asset_id,
+    group_key,
+    script_key_id,
+    taproot_key_id,
+    tapscript_sibling,
+    taproot_output_key,
+    amount,
+    asset_type,
+    creation_time,
+    proof_courier_addr
+) VALUES (
+    $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12
+) 
+ON CONFLICT (taproot_output_key) DO UPDATE
+SET
+    -- If the WHERE clause below is true (exact match on all other fields,
+    -- except for creation_time), we set taproot_output_key to its current
+    -- conflicting value. This is a no-op in terms of data change but allows
+    -- RETURNING id to work on the existing row.
+    taproot_output_key = excluded.taproot_output_key
+WHERE 
+    addrs.version = excluded.version
+    AND addrs.asset_version = excluded.asset_version
+    AND addrs.genesis_asset_id = excluded.genesis_asset_id
+    AND (
+        (addrs.group_key IS NULL AND excluded.group_key IS NULL)
+        OR addrs.group_key = excluded.group_key
+    )
+    AND addrs.script_key_id = excluded.script_key_id
+    AND addrs.taproot_key_id = excluded.taproot_key_id
+    AND (
+        (addrs.tapscript_sibling IS NULL AND excluded.tapscript_sibling IS NULL)
+        OR addrs.tapscript_sibling = excluded.tapscript_sibling
+    )
+    AND addrs.amount = excluded.amount
+    AND addrs.asset_type = excluded.asset_type
+    AND addrs.proof_courier_addr = excluded.proof_courier_addr
+RETURNING id;
 
 -- name: FetchAddrs :many
 SELECT