Support group keys on HTLC Interceptor & Invoice HTLC Modifier#1416
Support group keys on HTLC Interceptor & Invoice HTLC Modifier#1416GeorgeTsagk wants to merge 4 commits intorfq-negotiation-groupkeyfrom
Conversation
Pull Request Test Coverage Report for Build 13656341913Details
💛 - Coveralls |
GeorgeTsagk
force-pushed
the
rfq-negotiation-groupkey
branch
from
269ad4b to
7c660bd
Compare
GeorgeTsagk
force-pushed
the
group-htlc-interceptor-acceptor
branch
from
b1ee2ba to
43afd50
Compare
We add a new interface to the HTLC SumAssetBalance method, which helps check the identifier of the asset against a specifier. This allows for checking asset inclusion in a group, which is a bit involved and not the responsibility of the HTLC model.
We extend the interface of the rfq Policy in order to allow the specifier checker to be involved. This extends certain checks, and allows us to use asset specifiers that only have a group key.
We add the specifier checker interface to the AuxInvoiceManager too, as it is needed to validate incoming HTLCs which may use asset IDs that belong to a group, while the RFQ is based on a group key.
Adds some coverage to the invoice manager unit tests, which involve an RFQ quote over a group key, plus an HTLC with multiple asset balances, which may belong or not to the group.
GeorgeTsagk
force-pushed
the
group-htlc-interceptor-acceptor
branch
from
43afd50 to
e3be2a0
Compare
| // AssetMatchesSpecifier checks whether the passed specifier and asset | ||
| // ID match. If the specifier contains a group key, it will check | ||
| // whether the asset belongs to that group. | ||
| AssetMatchesSpecifier(ctx context.Context, |
There was a problem hiding this comment.
I think we should remove the other version of this interface added (in rfqmsg) and just keep this one.
There was a problem hiding this comment.
importing this in rfqmsg causes import cycle
| // we just need to provide a dummy value as the asset ID. The | ||
| // real asset IDs will be carefully picked in a later step in | ||
| // the process. What really matters now is the total amount. | ||
| assetID = sha256.Sum256(groupKey.SerializeCompressed()) |
There was a problem hiding this comment.
Why isn't the asset ID relevant here? It looks like it's used to pass along which custom records to use for the outgoing HTLC transformation.
IIUC, we need to select the asset ID amongst the set of active channels the outgoing link has that may all be diff, but have the same asset group.
There was a problem hiding this comment.
We don't know of specific asset IDs on this level, and we don't need do. What we want to do here is signal the amount that needs to be sent to the other side.
Channel selection was already taken care of when the receiver negotiated & embedded a quote in the invoice. At this point in the code, we're only intercepting the HTLC which has a predetermined outgoing channel.
| // AssetMatchesSpecifier checks if the provided asset satisfies the | ||
| // provided specifier. If the specifier includes a group key, we will | ||
| // check if the asset belongs to that group. | ||
| AssetMatchesSpecifier(ctx context.Context, specifier asset.Specifier, |
There was a problem hiding this comment.
Seems the rfq package is already imported here, so we can embed that interface. Mainly a question re style.
There was a problem hiding this comment.
I usually stick with the interface approach, which also serves as a hint w.r.t what parts of rfq manager we'll be using.
|
|
||
| switch { | ||
| case specifier.HasGroupPubKey(): | ||
| return true, nil |
There was a problem hiding this comment.
Why do we just return true for group keys? We should expand the unit tests here and earlier in the PR to make sure to matches properly re same group key but diff asset IDs.
There was a problem hiding this comment.
this was a placeholder just for the commit to compile. In a later commit this transforms to
case specifier.HasGroupPubKey():
if id == groupAssetID_1 || id == groupAssetID_2 {
return true, nil
}
return false, nil
github-project-automation
bot
moved this from 🏗 In progress
to 👀 In review
in Taproot-Assets Project Board
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
3 participants
Description
With this PR we extend the
rfq.OrderHandlerandtapchannel.AuxInvoiceManagersub systems to be able to read groupkey specifiers and handle HTLCs with multiple asset balances.Based on #1382