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

tarofreighter: introduce new package to house batched sending logic, add coin selection interface + impl #113

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Sep 10, 2022

In this PR, we add a new package tarofreighter that'll house the upcoming batched sending logic. The first addition in this package is the CommitmentSelector interface that abstracts away the process of coin selection. In addition to an asset input, we also need: the internal key, the anchor point, and also the taro commitment of all other assets along side it.

This is the half of the DB level changes we need to complete the state machine. The second half will come in the form of "spend" logic to modify an input asset, and also re-anchor (along w/ a proof file extension) any other assets that were spent along the way.

Missing components:

  • Unit tests

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Nice to know the trick with length(hex(sqlc.narg())) works (even though it was costly to find out 😬 ).

Found one potential issue about multiple "coins" of the same asset. At least I think it's an issue, could be wrong.

tarodb/sqlite/queries/assets.sql Show resolved Hide resolved
tarodb/sqlite/queries/assets.sql Outdated Show resolved Hide resolved
tarodb/assets_store.go Outdated Show resolved Hide resolved
// input in a transaction.
//
// To obtain this, we'll first do another query to fetch all
// the _other_ assets that are anchored at the anchor point for
Copy link
Member

Choose a reason for hiding this comment

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

Won't this query also contain the asset already selected in matchingAssets as that has the same anchor point? I guess it doesn't really matter for the logic here (as far as I can tell), it's just that the comment is a bit misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah it would unless we extend the query to exclude things. Below this is taken into account, as we'll take everything here to construct the new taro root for this particular output. Otherwise we'd need to remember to add this asset again. So for now it's duplicate in memory.

@@ -912,6 +913,7 @@ func (a *AssetStore) SelectCommitment(

// First, we'll map the commitment constraints to our database query
// filters.
//var assetFilter QueryAssetFilters
Copy link
Member

Choose a reason for hiding this comment

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

nit: commented out code.

@Roasbeef
Copy link
Member Author

Nice to know the trick with length(hex(sqlc.narg())) works (even though it was costly to find out 😬 ).

Heh yeh, I think eventually I'll make an issue that a *[]byte should be emitted for structs in that scenario. We can do this manauylly ourselves ofc (another run of that sed script), but seems easier for now to juse have this length work around.

I think we'll also want to eventually break out that mega query into a view. Then all the new clauses can be a super set of that, putting it all in a new WHERE clause.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

log.go Outdated Show resolved Hide resolved
tarofreighter/interface.go Outdated Show resolved Hide resolved
tarodb/sqlite/queries/assets.sql Show resolved Hide resolved
tarodb/assets_store.go Show resolved Hide resolved
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 just a few nits and perhaps a bit of grooming on the commits that could be done.

tarofreighter/interface.go Outdated Show resolved Hide resolved
tarodb/sqlite/queries/assets.sql Outdated Show resolved Hide resolved
tarodb/sqlite/queries/assets.sql Show resolved Hide resolved
tarodb/assets_store.go Show resolved Hide resolved
tarodb/sqlite/assets.sql.go Show resolved Hide resolved
tarodb/assets_store.go Outdated Show resolved Hide resolved
tarodb/assets_store.go Outdated Show resolved Hide resolved
tarodb/assets_store_test.go Outdated Show resolved Hide resolved
tarodb/sqlite/queries/assets.sql Show resolved Hide resolved
tarodb/assets_store.go Show resolved Hide resolved
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good overall, left notes on the missing amount field 📈

tarofreighter/interface.go Show resolved Hide resolved
tarodb/assets_store.go Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the coin-select branch 2 times, most recently from 6b248c0 to 4feb85f Compare September 15, 2022 23:02
…ly key

This'll be useful for coin selection as we'll be able to search through
the assets based on some key parameters. This call can also eventually
serve as the basis for other queries like the balance of all the assets
with a given ID, etc, etc.
We'll start to extend with even more queries, so we might as well switch
up the name.
This sets up for an upcoming new query to use all this functionality
without duplicating it all.
In this commit, we add an implementation of CommitmentSelector for the
main assets store. Along the way, we extend the new QueryAsset method to
allow us to select all the assets anchored to a given outpoint. We can
then use this to fetch all the other assets that are committed alongside
a main asset, as we need to also return the full taro commitment for
each selected asset.

This implementation will be used to do proper coin selection for the
upcoming batched send logic.
@Roasbeef
Copy link
Member Author

Thx for the review y'all, I've made some follow up issues to address other items, so we can move forward with the integration elsewhere.

Related to #48 I think we prob want to make that mega query into a simpler view, then make the other future queries (balance for assets, etc) on top of that.

@Roasbeef Roasbeef merged commit 58c474b into main Sep 15, 2022
@guggero guggero deleted the coin-select branch September 16, 2022 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants