Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated ListAssetRequest to include additional filter options #1199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

itsrachelfish
Copy link

@itsrachelfish itsrachelfish commented Nov 16, 2024

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.

@dstadulis dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 17, 2024
@Roasbeef Roasbeef modified the milestones: v0.5 (v0.4.2 rename), v0.6 Nov 19, 2024
@itsrachelfish itsrachelfish force-pushed the listassets-filters branch 4 times, most recently from f31d9f4 to f189364 Compare November 20, 2024 08:56
@coveralls
Copy link

coveralls commented Nov 20, 2024

Pull Request Test Coverage Report for Build 12439004519

Details

  • 17 of 86 (19.77%) changed or added relevant lines in 3 files are covered.
  • 20 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.03%) to 40.935%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapdb/assets_store.go 16 33 48.48%
rpcserver.go 0 52 0.0%
Files with Coverage Reduction New Missed Lines %
asset/asset.go 2 81.08%
asset/mock.go 3 92.68%
tapgarden/caretaker.go 4 68.5%
universe/interface.go 5 50.65%
tapchannel/aux_invoice_manager.go 6 83.25%
Totals Coverage Status
Change from base Build 12417405641: -0.03%
Covered Lines: 26423
Relevant Lines: 64548

💛 - Coveralls

@GeorgeTsagk GeorgeTsagk self-requested a review November 20, 2024 10:16
@itsrachelfish itsrachelfish force-pushed the listassets-filters branch 2 times, most recently from 6abd778 to 3c0141b Compare November 20, 2024 10:23
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good!
it's in the right direction 💯

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 itsrachelfish force-pushed the listassets-filters branch 7 times, most recently from 13eabd1 to 241d98d Compare November 27, 2024 11:47
@itsrachelfish itsrachelfish force-pushed the listassets-filters branch 5 times, most recently from f5d1985 to 9c7b7e9 Compare December 4, 2024 07:29
@itsrachelfish itsrachelfish changed the title [WIP] Updated ListAssetRequest to include additional filter options Updated ListAssetRequest to include additional filter options Dec 7, 2024
@itsrachelfish itsrachelfish marked this pull request as ready for review December 7, 2024 10:54
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We'll also need that unit test coverage ✔️

Int64: int64(query.MaxAmt),
Valid: true,
}
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing godocs (see MinAnchorHeight above)

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

if query.ScriptKeyID != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


assets, err := r.cfg.AssetStore.FetchAllAssets(
ctx, includeSpent, includeLeased, nil,
ctx, includeSpent, includeLeased, queryFilters,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add some unit test coverage in tapdb to test the behavior of AssetStore.FetchAllAssets

@Roasbeef
Copy link
Member

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):

// 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
}

@itsrachelfish itsrachelfish force-pushed the listassets-filters branch 2 times, most recently from 850c549 to ccaf4e4 Compare December 20, 2024 21:44
@lightninglabs-deploy
Copy link

@itsrachelfish, remember to re-request review from reviewers when ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

[feature]: Add filters to ListAssets
6 participants