Updated ListAssetRequest to include additional filter options #1199

itsrachelfish · 2024-11-16T06:11:09Z

Resolves #876

This PR adds new filter options to the ListAssets RPC call.

The ListAssetRequest proto definition was updated to include the new filter options.
The fetchRpcAssets function was refactored to accept a AssetQueryFilters argument so that separate arguments don't need to be added for each filter option.
The assets store constraintsToDbFilter function was updated to use the new filter options.
And the assets.sql file was updated to include a new maxAmount option.

coveralls · 2024-11-20T09:09:34Z

Pull Request Test Coverage Report for Build 13776255223

Details

45 of 110 (40.91%) changed or added relevant lines in 3 files are covered.
30 unchanged lines in 10 files lost coverage.
Overall coverage decreased (-0.02%) to 54.564%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapdb/assets_store.go	27	34	79.41%
taprpc/taprootassets.pb.go	0	25	0.0%
rpcserver.go	18	51	35.29%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	85.0%
tapdb/assets_store.go	1	78.14%
asset/asset.go	2	80.04%
internal/test/helpers.go	2	86.95%
asset/mock.go	3	72.3%
itest/assertions.go	3	90.36%
itest/multisig.go	3	97.43%
rpcserver.go	4	63.82%
tapchannel/aux_leaf_signer.go	5	43.08%
tapgarden/custodian.go	6	75.5%

Totals
Change from base Build 13731570279:	-0.02%
Covered Lines:	49450
Relevant Lines:	90627

💛 - Coveralls

GeorgeTsagk

looking good!
it's in the right direction 💯

rpcserver.go

sample-tapd.conf

taprpc/taprootassets.proto

GeorgeTsagk

lots of improvements! ⬆️
I have one comment on the types of filters

We'll also need that unit test coverage ✔️

GeorgeTsagk · 2024-12-10T15:42:44Z

tapdb/assets_store.go

+				Int64: int64(query.MaxAmt),
+				Valid: true,
+			}
+		} else {


this whole block is not needed, you can just leave query.MaxAmt initialized to default values (Valid: false)

^ can just remove whole else block

GeorgeTsagk · 2024-12-10T16:06:18Z

tapdb/assets_store.go

@@ -980,6 +1008,10 @@ type AssetQueryFilters struct {
 	// MinAnchorHeight is the minimum block height the asset's anchor tx
 	// must have been confirmed at.
 	MinAnchorHeight int32
+
+	ScriptKeyID *int64


missing godocs (see MinAnchorHeight above)

GeorgeTsagk · 2024-12-10T16:10:24Z

tapdb/assets_store.go

 		if query.MinAnchorHeight != 0 {
 			assetFilter.MinAnchorHeight = sqlInt32(
 				query.MinAnchorHeight,
 			)
 		}

+		if query.ScriptKeyID != nil {


we can't really have user-facing arguments that reference database IDs

the ScriptKeyID is the DB identifier for a script key and there's no possible way a user could (...easily) get their hands on one of them in order to filter the assets

Instead we could have *AssetQueryFilters expose scriptKey []byte and anchorOutpoint []byte filters, which the user can easily acquire, then on this point in the code we could map these values to the unique db ID that the underlying layer requires for filtering (that is QueryAssetFilters struct)

if you add some tests with the current way things work, you'll see that when you'll try to query a specific script key you'll have trouble finding the ID to filter by

rpcserver.go

Roasbeef · 2024-12-11T10:49:50Z

I think this function can help resolve the issue w.r.t getting the internal ID, you can just specify all the normal bytes (script key, anchor point, etc):

taproot-assets/tapdb/assets_store.go

Lines 904 to 935 in ec84760

    
           // specificAssetFilter maps the given asset parameters to the set of filters 
        
           // we use in the SQL queries. 
        
           func (a *AssetStore) specificAssetFilter(id asset.ID, anchorPoint wire.OutPoint, 
        
           	groupKey *asset.GroupKey, 
        
           	scriptKey *asset.ScriptKey) (QueryAssetFilters, error) { 
        
           	anchorPointBytes, err := encodeOutpoint(anchorPoint) 
        
           	if err != nil { 
        
           		return QueryAssetFilters{}, fmt.Errorf("unable to encode "+ 
        
           			"outpoint: %w", err) 
        
           	} 
        
           	filter := QueryAssetFilters{ 
        
           		AssetIDFilter: id[:], 
        
           		AnchorPoint:   anchorPointBytes, 
        
           		Now: sql.NullTime{ 
        
           			Time:  a.clock.Now().UTC(), 
        
           			Valid: true, 
        
           		}, 
        
           	} 
        
           	if groupKey != nil { 
        
           		key := groupKey.GroupPubKey 
        
           		filter.KeyGroupFilter = key.SerializeCompressed() 
        
           	} 
        
           	if scriptKey != nil { 
        
           		key := scriptKey.PubKey 
        
           		filter.TweakedScriptKey = key.SerializeCompressed() 
        
           	} 
        
           	return filter, nil 
        
           }

lightninglabs-deploy · 2025-03-05T16:38:20Z

@GeorgeTsagk: review reminder

itsrachelfish · 2025-03-10T19:57:59Z

@GeorgeTsagk @guggero I updated the code to filter assets based on the tweaked script key instead of the internal script key ID. I ran into some weird issues with the automated tests, for some reason I was unable to filter the 10th asset:

assetGen:    assetGen.assetGens[9],
anchorPoint: assetGen.anchorPoints[4],
amt:         777,
scriptKey:   scriptKeyWithScript,

Oli said this might have something to do with an underlying issue with the specific anchor point or asset generation, but I'm not sure why this particular asset is "cursed" 😅

I worked around the issue by adding a script key to the 3rd asset and was able to filter it as expected.

GeorgeTsagk

super close 💯
mostly styling related comments

should also add the filter options to the cmd command but since they weren't there in the first place this can be done in a different PR

GeorgeTsagk · 2025-03-11T16:06:40Z

tapdb/assets_store.go

+				Int64: int64(query.MaxAmt),
+				Valid: true,
+			}
+		} else {


^ can just remove whole else block

GeorgeTsagk · 2025-03-11T16:08:00Z

tapdb/assets_store.go

@@ -970,6 +1002,14 @@ type AssetQueryFilters struct {
 	// MinAnchorHeight is the minimum block height the asset's anchor tx
 	// must have been confirmed at.
 	MinAnchorHeight int32
+
+	// ScriptKey allows filtering by the pubkey that individual assets


Suggested change

// ScriptKey allows filtering by the pubkey that individual assets

// ScriptKey allows filtering by asset script key.

GeorgeTsagk · 2025-03-11T16:08:31Z

tapdb/assets_store.go

+	// are locked behind.
+	ScriptKey *asset.ScriptKey
+
+	// AnchorPoint allows filtering by the specific on-chain UTXO which


Suggested change

// AnchorPoint allows filtering by the specific on-chain UTXO which

// AnchorPoint allows filtering by the outpoint the asset is anchored to.

GeorgeTsagk · 2025-03-11T16:10:12Z

tapfreighter/interface.go

@@ -35,6 +35,9 @@ type CommitmentConstraints struct {
 	// to satisfy the constraints.
 	MinAmt uint64

+	// MaxAmt specifies the maximum amount of minted assets to be selected.


Suggested change

// MaxAmt specifies the maximum amount of minted assets to be selected.

// MaxAmt specifies the maximum amount that an asset commitment needs to hold

// to satisfy the constraints.

GeorgeTsagk · 2025-03-11T16:10:51Z

rpcserver.go

+				err)
+		}
+
+		// Construct an asset specifier using the requested group key


missing . at the end of comment

GeorgeTsagk · 2025-03-11T16:12:26Z

rpcserver.go

+	rpcAssets, err := r.fetchRpcAssets(
+		ctx, false, false, req.IncludeLeased, nil,
+	)
+


unnecessary newline

GeorgeTsagk · 2025-03-11T16:14:21Z

tapdb/assets_store_test.go

+	makeFilter := func(
+		minAmt, maxAmt uint64,
+		anchorHeight int32,
+		scriptKey *asset.ScriptKey,
+		anchorPoint *wire.OutPoint,
+		coinSelectType tapsend.CoinSelectType,
+	) *AssetQueryFilters {


need to fix styling of func definition

GeorgeTsagk · 2025-03-11T16:18:27Z

tapdb/assets_store_test.go

+		name: "script key",
+		filter: makeFilter(
+			0, 0, 0, scriptKeyWithScript,
+			nil, tapsend.ScriptTreesAllowed,


no need to break the line early if it can fit in the same line

also applies to makeFilter calls below

itsrachelfish force-pushed the listassets-filters branch from 84ac4c8 to b6e90aa Compare November 16, 2024 18:38

dstadulis assigned itsrachelfish Nov 17, 2024

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

Roasbeef modified the milestones: v0.5 (v0.4.2 rename), v0.6 Nov 19, 2024

itsrachelfish force-pushed the listassets-filters branch 4 times, most recently from f31d9f4 to f189364 Compare November 20, 2024 08:56

itsrachelfish force-pushed the listassets-filters branch from f189364 to ccf4b76 Compare November 20, 2024 10:13

GeorgeTsagk self-requested a review November 20, 2024 10:16

itsrachelfish force-pushed the listassets-filters branch 2 times, most recently from 6abd778 to 3c0141b Compare November 20, 2024 10:23

GeorgeTsagk reviewed Nov 20, 2024

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

rpcserver.go Show resolved Hide resolved

rpcserver.go Outdated Show resolved Hide resolved

sample-tapd.conf Outdated Show resolved Hide resolved

taprpc/taprootassets.proto Outdated Show resolved Hide resolved

itsrachelfish force-pushed the listassets-filters branch 7 times, most recently from 13eabd1 to 241d98d Compare November 27, 2024 11:47

itsrachelfish force-pushed the listassets-filters branch 5 times, most recently from f5d1985 to 9c7b7e9 Compare December 4, 2024 07:29

itsrachelfish force-pushed the listassets-filters branch from 9c7b7e9 to 4566b97 Compare December 7, 2024 10:31

itsrachelfish changed the title ~~[WIP] Updated ListAssetRequest to include additional filter options~~ Updated ListAssetRequest to include additional filter options Dec 7, 2024

itsrachelfish marked this pull request as ready for review December 7, 2024 10:54

itsrachelfish requested a review from GeorgeTsagk December 7, 2024 10:55

GeorgeTsagk reviewed Dec 10, 2024

View reviewed changes

dstadulis modified the milestones: v0.6, v0.6 - Post MVP Features Dec 17, 2024

itsrachelfish force-pushed the listassets-filters branch 3 times, most recently from ccaf4e4 to ffa733e Compare December 20, 2024 21:53

itsrachelfish requested a review from GeorgeTsagk January 7, 2025 17:38

itsrachelfish force-pushed the listassets-filters branch from ffa733e to 485cbad Compare February 8, 2025 02:23

itsrachelfish force-pushed the listassets-filters branch 3 times, most recently from c74c44a to 8b0d382 Compare February 25, 2025 22:58

itsrachelfish force-pushed the listassets-filters branch from 8b0d382 to 6951f28 Compare March 10, 2025 19:53

itsrachelfish force-pushed the listassets-filters branch from 6951f28 to 856d9e3 Compare March 10, 2025 21:11

guggero self-requested a review March 10, 2025 22:06

itsrachelfish added 3 commits March 10, 2025 16:42

tapdb: Update AssetQueryFilters to handle additional filter options

c4fce13

taprpc: Update ListAssetRequest to include additional filter options

4a7e63f

tapdb: Update assets store tests to include new filter options

cf0ef8e

itsrachelfish force-pushed the listassets-filters branch from 856d9e3 to cf0ef8e Compare March 10, 2025 22:44

GeorgeTsagk reviewed Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated ListAssetRequest to include additional filter options #1199

Updated ListAssetRequest to include additional filter options #1199

itsrachelfish commented Nov 16, 2024 •

edited

Loading

coveralls commented Nov 20, 2024 •

edited

Loading

GeorgeTsagk left a comment

GeorgeTsagk left a comment

GeorgeTsagk Dec 10, 2024

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Dec 10, 2024

GeorgeTsagk Dec 10, 2024

GeorgeTsagk Dec 10, 2024

Roasbeef commented Dec 11, 2024

lightninglabs-deploy commented Mar 5, 2025

itsrachelfish commented Mar 10, 2025

GeorgeTsagk left a comment

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

GeorgeTsagk Mar 11, 2025

	// ScriptKey allows filtering by the pubkey that individual assets
	// ScriptKey allows filtering by asset script key.

	// AnchorPoint allows filtering by the specific on-chain UTXO which
	// AnchorPoint allows filtering by the outpoint the asset is anchored to.

	// MaxAmt specifies the maximum amount of minted assets to be selected.
	// MaxAmt specifies the maximum amount that an asset commitment needs to hold
	// to satisfy the constraints.

Updated ListAssetRequest to include additional filter options #1199

Are you sure you want to change the base?

Updated ListAssetRequest to include additional filter options #1199

Conversation

itsrachelfish commented Nov 16, 2024 • edited Loading

coveralls commented Nov 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 13776255223

Details

💛 - Coveralls

GeorgeTsagk left a comment

Choose a reason for hiding this comment

GeorgeTsagk 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

Roasbeef commented Dec 11, 2024

lightninglabs-deploy commented Mar 5, 2025

itsrachelfish commented Mar 10, 2025

GeorgeTsagk 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

itsrachelfish commented Nov 16, 2024 •

edited

Loading

coveralls commented Nov 20, 2024 •

edited

Loading