Add `ListBurns` RPC #1178

GeorgeTsagk · 2024-11-07T18:08:03Z

Description

We're currently able to provably burn assets with tapd but we have no (easy) way of retrieving burn related historical data. This PR adds a DB table and exposes some new RPC methods/parameters to help keep track of burns.

When completing an asset burn, we add an entry to the new burns table. We also add a new ListBurns method which returns a list of all burns that pass the user-provided filters (asset id, group id, anchor txid)

Closes #1095

coveralls · 2024-11-07T18:18:02Z

Pull Request Test Coverage Report for Build 11816308677

Details

30 of 167 (17.96%) changed or added relevant lines in 4 files are covered.
28 unchanged lines in 7 files lost coverage.
Overall coverage decreased (-0.05%) to 40.56%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapdb/sqlc/transfers.sql.go	30	38	78.95%
rpcserver.go	0	31	0.0%
cmd/tapcli/assets.go	0	32	0.0%
tapdb/assets_store.go	0	66	0.0%

Files with Coverage Reduction	New Missed Lines	%
tapdb/addrs.go	2	79.04%
rpcserver.go	2	0.0%
tapchannel/aux_leaf_signer.go	2	35.92%
asset/asset.go	2	80.2%
tapgarden/caretaker.go	4	68.87%
commitment/tap.go	6	83.64%
universe/interface.go	10	51.12%

Totals
Change from base Build 11804776121:	-0.05%
Covered Lines:	24721
Relevant Lines:	60949

💛 - Coveralls

guggero

Looks pretty good. I wonder if we should add a Golang based "migration" of sort that queries existing assets in the DB, checks if they're burns (cannot be done on SQL level alone) and then inserts them into the new table retroactively?
Perhaps we could add a mechanism that detects if any DB level migrations were executed. And if they were, we can assume an update happened and could run these Golang-level checks (which would need to be idempotent).

tapdb/assets_store.go

rpcserver.go

taprpc/taprootassets.proto

taprpc/taprootassets.yaml

cmd/tapcli/assets.go

tapdb/sqlc/queries/transfers.sql

guggero

Super close!

guggero · 2024-11-13T12:08:38Z

tapdb/assets_store.go

+	var writeTxOpts AssetStoreTxOptions
+	return a.db.ExecTx(ctx, &writeTxOpts, func(q ActiveAssetsStore) error {
+		// Look up the transfer that relates to this txid.
+		// TODO(george): We assume there's a single burn in every txn,


I don't think we need this TODO. If there are multiple burns in a transaction, we'd call InsertBurn multiple times with the same txid. But because they would all still be in the same transfer, we'd query the same transfer ID which is correct.

^ makes sense

guggero · 2024-11-13T12:08:57Z

tapdb/assets_store.go

+			return fmt.Errorf("unable to query asset transfers: %w",
+				err)
+		}
+		assetTransfer := assetTransfers[0]


Should check that we have exactly one result (which should always be the case when we query with an anchor TX hash).

Yeah otherwise, we'll panic here.

guggero · 2024-11-13T12:12:29Z

tapdb/sqlc/queries/transfers.sql

+    abt.asset_id,
+    abt.group_key,
+    abt.amount,
+    ct.txid AS anchor_txid -- Retrieving the txid from chain_txns


nit: missing full stop, here and below.

guggero · 2024-11-13T12:13:06Z

tapdb/sqlc/queries/transfers.sql

+    abt.amount,
+    ct.txid AS anchor_txid -- Retrieving the txid from chain_txns
+FROM asset_burn_transfers abt
+JOIN asset_transfers at ON abt.transfer_id = at.id


Looks like we don't need to join on asset_transfers? Since we don't seem to be using at anywhere.

Perhaps the idea was to return the full transfer information alongside the burn?

From the PoV of a future query CLI, I think that would be desirable.

guggero · 2024-11-13T12:15:56Z

tapdb/assets_store_test.go

+	// Let's insert a burn.
+	assetID := inputAsset.ID()
+	transferID := assetTransfers[0].ID
+	_, err = assetsStore.db.InsertBurn(ctx, sqlc.InsertBurnParams{


We'll want to test the actual AssetStore's QueryBurns and InsertBurn methods, not directly call into the DB-level methods.

guggero · 2024-11-13T12:17:46Z

rpcserver.go

+	in *taprpc.ListBurnsRequest) (*taprpc.ListBurnsResponse, error) {
+
+	burns, err := r.cfg.AssetStore.QueryBurns(
+		ctx, sqlc.QueryBurnsParams{


We should use the alias tapdb.QueryBurnsFilters here.

guggero · 2024-11-13T12:20:13Z

cmd/tapcli/assets.go

+	anchorTxidStr := ctx.String(anchorTxidName)
+	anchorTxid, err := hex.DecodeString(anchorTxidStr)
+	if err != nil {
+		return fmt.Errorf("invalid anchor txid")


nit: missing wrapped error.

Roasbeef · 2024-11-14T03:00:55Z

tapdb/assets_store.go

+	var writeTxOpts AssetStoreTxOptions
+	return a.db.ExecTx(ctx, &writeTxOpts, func(q ActiveAssetsStore) error {
+		// Look up the transfer that relates to this txid.
+		// TODO(george): We assume there's a single burn in every txn,


^ makes sense

Roasbeef · 2024-11-14T03:01:09Z

tapdb/assets_store.go

+			return fmt.Errorf("unable to query asset transfers: %w",
+				err)
+		}
+		assetTransfer := assetTransfers[0]


Yeah otherwise, we'll panic here.

Roasbeef · 2024-11-14T03:01:42Z

tapdb/assets_store.go

+
+		return nil
+	})
+


Style nit: extra space.

Roasbeef · 2024-11-14T03:04:31Z

tapdb/assets_store_test.go

@@ -2204,6 +2205,44 @@ func TestTransferOutputProofDeliveryStatus(t *testing.T) {
 	require.Equal(
 		t, randBlockHash[:], assetTransfers[0].AnchorTxBlockHash,
 	)
+
+	// Let's insert a burn.


How about we make another isolated unit test for this? This exiting test is already rather long/large.

Roasbeef · 2024-11-14T03:06:43Z

tapdb/sqlc/queries/transfers.sql

+    abt.amount,
+    ct.txid AS anchor_txid -- Retrieving the txid from chain_txns
+FROM asset_burn_transfers abt
+JOIN asset_transfers at ON abt.transfer_id = at.id


Perhaps the idea was to return the full transfer information alongside the burn?

From the PoV of a future query CLI, I think that would be desirable.

Roasbeef · 2024-11-14T03:07:36Z

rpcserver.go

@@ -3312,12 +3314,56 @@ func (r *rpcServer) BurnAsset(ctx context.Context,
 		}
 	}

+	// At this point everything completed correctly, so we log this burn in


Hmm, perhaps we should do this further in the pipeline? So in the same db transaction that we insert the transfer.

Otherwise, if we crash here, then the burn is never inserted in the db, and we exit in an inconsistent state.

Roasbeef · 2024-11-14T03:08:12Z

rpcserver.go

+	rpcBurns := fn.Map(burns, marshalRpcBurn)
+
+	return &taprpc.ListBurnsResponse{
+		Burns: rpcBurns,


Re other comment, I think it would be useful to include the full transfer here, also maybe the raw tx itself?

GeorgeTsagk self-assigned this Nov 7, 2024

GeorgeTsagk force-pushed the track-burns branch from 5c69a1e to 8037aff Compare November 7, 2024 18:08

GeorgeTsagk force-pushed the track-burns branch from 8037aff to dcb3eee Compare November 7, 2024 18:25

GeorgeTsagk requested review from ffranr and guggero November 7, 2024 18:26

GeorgeTsagk force-pushed the track-burns branch from dcb3eee to a1ddf99 Compare November 7, 2024 18:31

guggero reviewed Nov 8, 2024

View reviewed changes

dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 11, 2024

dstadulis added the burning label Nov 11, 2024

tapdb: add asset burns table and queries

a9eb3ec

GeorgeTsagk force-pushed the track-burns branch from a1ddf99 to 0055981 Compare November 13, 2024 11:09

GeorgeTsagk added 2 commits November 13, 2024 12:13

taprpc: track burned assets

7e8716a

itest: use ListBurns in burn test

b9c8ce7

GeorgeTsagk force-pushed the track-burns branch from 0055981 to 7f7febe Compare November 13, 2024 11:13

cmd: add listburns command

ba5db4b

GeorgeTsagk force-pushed the track-burns branch from 7f7febe to ba5db4b Compare November 13, 2024 11:26

GeorgeTsagk requested a review from guggero November 13, 2024 11:32

guggero reviewed Nov 13, 2024

View reviewed changes

Roasbeef requested review from Roasbeef and removed request for ffranr November 14, 2024 03:00

Roasbeef requested changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ListBurns` RPC #1178

Add `ListBurns` RPC #1178

GeorgeTsagk commented Nov 7, 2024

coveralls commented Nov 7, 2024 •

edited

Loading

guggero left a comment

guggero left a comment

guggero Nov 13, 2024

Roasbeef Nov 14, 2024

guggero Nov 13, 2024

Roasbeef Nov 14, 2024

guggero Nov 13, 2024

guggero Nov 13, 2024

Roasbeef Nov 14, 2024

guggero Nov 13, 2024

guggero Nov 13, 2024

guggero Nov 13, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Roasbeef Nov 14, 2024

Add ListBurns RPC #1178

Are you sure you want to change the base?

Add ListBurns RPC #1178

Conversation

GeorgeTsagk commented Nov 7, 2024

Description

coveralls commented Nov 7, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11816308677

Details

💛 - Coveralls

guggero left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `ListBurns` RPC #1178

Add `ListBurns` RPC #1178

coveralls commented Nov 7, 2024 •

edited

Loading