Support group keys for RFQ negotiation flows #1382
Conversation
GeorgeTsagk
force-pushed
the
rfq-negotiation-groupkey
branch
from
ded24ba to
a76775a
Compare
|
Rebased on |
There was a problem hiding this comment.
Great set of commits! Minimal, yet impactful diff.
I think I've spotted a bug where we'll start to compare sats balance instead of assets balance when trying to select the "best" channel. We should follow up there with a unit test to confirm the error, then correct it (if it does indeed exist).
In order to properly test this e2e, I think we may need to expose a group key arg during asset invoice creation. I don't think this requires the full on multi asset group channels (we just want to verify that we can make invoices based no the group key).
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @GeorgeTsagk)
rfq/manager.go line 963 at r3 (raw file):
// Store the result for future calls. m.assetIDToGroup.Store(id, groupKeyBytes)
Not something we need to worry about today, but eventually we'll want have some sort of eviction policy here.
rpcserver.go line 7865 at r6 (raw file):
// the best local balance. fn.ForEach(balances, func(b channelWithSpecifier) { if b.channelInfo.LocalBalance >
Is .LocalBalance here actually the asset, or sats balance? I ask as it's lnclient.ChannelInfo, so I assume it would be a sats balance.
We should ensure that for the best balance here, we're looking at the asset balance
rpcserver.go line 7909 at r6 (raw file):
for assetIdx := range assets { assetOutput := assets[assetIdx]
Looks like the inner loop here could be an actual Filter, you'd need to contend with the extra return error value, but that's where the Result type can be nice when working with higher order functions/types.
itest/rfq_test.go line 429 at r7 (raw file):
// testRfqNegotiationGroupKey checks that two nodes can negotiate and register // quotes based on a specifier that only uses a group key. func testRfqNegotiationGroupKey(t *harnessTest) {
We should complement this test with one on litd that makes RFQ quotes with a group key.
Think aloud, in order to properly test that, I think we may need to support creating asset invoices with a group key at the very least.
rpcserver.go line 7886 at r6 (raw file):
type channelWithAsset struct { // assetInfo is the information about one of the assets in a channel. assetInfo rfqmsg.JsonAssetChanInfo
Interesting how this seems to duplicate the information in the attribute below it?
| @@ -245,6 +245,11 @@ func (i ID) String() string { | |||
| return hex.EncodeToString(i[:]) | |||
| } | |||
|
|
|||
| // IsEqual returns true if the ID matches the provided ID. | |||
| func (i ID) IsEqual(a ID) bool { | |||
There was a problem hiding this comment.
just a drive-by comment: asset.ID is an array, so it supports the == operator.
There was a problem hiding this comment.
Ah, I see. I'd argue that we still should keep this method, it's a top-level way of comparing without having to dive into the type and see what it's made of
We add a simple helper to the asset ID struct which helps us compare it to another ID. We also add a helper to check whether an asset specifier is empty. This is going to be used in a follow-up commit.
Previously our sell & buy request message validators would consider the message invalid if the included specifier didn't set an ID. Now we want to support group keys, so having either a group key or ID defined is fine.
This commit adds a new interface to the rfq manager which helps us look up the group an asset ID may belong to. The reason we can't reuse the same interface from tapsend is a circular import issue. We also introduce an in memory look-up map to skip a roundtrip to the db, since this information is static.
This helper is needed in various checks around the rfq/rpc codebase. When we want to check an asset ID against a specifier we can now call this method directly. This is used in a follow-up commit.
When trying to add a local scid alias we would look up all channels and filter them based on whether the asset ID matches the one of the specifier. We now call the previously introduced helper method to easily check the asset against the specifier.
When querying for channels we now provide the specifier, allowing the helper methods to check against the specifier with all the helpers that we introduced in the previous commits. Now a channel with an asset ID that belongs to a group, may qualify as valid "balance" of that group.
This commit adds a simple itest which checks that Alice and Bob can negotiate an rfq quote, using a group key as the specifier.
There was a problem hiding this comment.
Reviewable status: 7 of 8 files reviewed, 5 unresolved discussions (waiting on @guggero and @Roasbeef)
rpcserver.go line 7865 at r6 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Is
.LocalBalancehere actually the asset, or sats balance? I ask as it'slnclient.ChannelInfo, so I assume it would be a sats balance.We should ensure that for the best balance here, we're looking at the asset balance
good catch, it's actually the sats balance
rpcserver.go line 7886 at r6 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Interesting how this seems to duplicate the information in the attribute below it?
this should not be removed, it includes the decoded data of the CustomChannelData field of lndclient.ChannelInfo
rpcserver.go line 7909 at r6 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Looks like the inner loop here could be an actual
Filter, you'd need to contend with the extra return error value, but that's where theResulttype can be nice when working with higher order functions/types.
you mean an fn.Filter? Seems like it would have to obfuscate some errors we want to send upstream, will see if I can pull it through
itest/rfq_test.go line 429 at r7 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
We should complement this test with one on
litdthat makes RFQ quotes with a group key.Think aloud, in order to properly test that, I think we may need to support creating asset invoices with a group key at the very least.
Trying to keep the scope of this PR small, focusing on the establishment of quotes on group keys, before involving send/receive flows anyhow.
Could implement your recommendation in a follow up PR, taking advantage of the imported LitD oracle harness too
rfq/manager.go line 963 at r3 (raw file):
Previously, Roasbeef (Olaoluwa Osuntokun) wrote…
Not something we need to worry about today, but eventually we'll want have some sort of eviction policy here.
True, will throw a todo, or just do something simple like flushing on intervals
| @@ -245,6 +245,11 @@ func (i ID) String() string { | |||
| return hex.EncodeToString(i[:]) | |||
| } | |||
|
|
|||
| // IsEqual returns true if the ID matches the provided ID. | |||
| func (i ID) IsEqual(a ID) bool { | |||
There was a problem hiding this comment.
Ah, I see. I'd argue that we still should keep this method, it's a top-level way of comparing without having to dive into the type and see what it's made of
GeorgeTsagk
force-pushed
the
rfq-negotiation-groupkey
branch
from
a76775a to
269ad4b
Compare
Pull Request Test Coverage Report for Build 13504659506Details
💛 - Coveralls |
| // look up of the group an asset belongs to. Since this information is | ||
| // static and generated during minting, it is not possible for an asset | ||
| // to change groups. | ||
| assetIDToGroup lnutils.SyncMap[asset.ID, []byte] |
There was a problem hiding this comment.
nit: rename this to groupKeyLookupCache? Just to signify that this isn't an authoritative source (containing the full truth) but just a cache to prevent multiple database lookups for the same asset ID?
| @@ -917,6 +935,36 @@ func (m *Manager) RemoveSubscriber( | |||
| return nil | |||
| } | |||
|
|
|||
| // GetAssetGroupKey retrieves the group key of an asset based on its ID. | |||
| func (m *Manager) GetAssetGroupKey(ctx context.Context, | |||
| id asset.ID) ([]byte, error) { | |||
There was a problem hiding this comment.
Why do we store raw bytes in the cache and return them here? Seems like *btcec.PublicKey would be more appropriate.
| case specifier.HasId(): | ||
| specifierID := specifier.UnwrapIdToPtr() | ||
|
|
||
| return specifierID.IsEqual(id), nil |
There was a problem hiding this comment.
Okay, I guess the IsEqual is convenient because we can call it on a pointer as well as on a struct. But it kind of also hides the fact that specifierID might be nil here (it's not because of .HasId() above.
IMO *specifierID == id still is more explicit.
|
|
||
| switch { | ||
| case specifier.HasGroupPubKey(): | ||
| group, err := m.GetAssetGroupKey(ctx, id) |
There was a problem hiding this comment.
Yeah, I think we should return the public key instead of the bytes here. Then can use .IsEqual() on it.
| return specifierID.IsEqual(id), nil | ||
| } | ||
|
|
||
| return false, fmt.Errorf("specifier is empty") |
There was a problem hiding this comment.
style nit: I find it easier to read if a switch has a default case that has the error return. But personal preference I guess.
| @@ -602,8 +614,8 @@ func (m *Manager) addScidAlias(scidAlias uint64, assetSpecifier asset.Specifier, | |||
| // At this point, if the base SCID is still not found, we return an | |||
| // error. We can't map the SCID alias to a base SCID. | |||
| if baseSCID == 0 { | |||
| return fmt.Errorf("add alias: base SCID not found for asset: "+ | |||
| "%v", assetID) | |||
| return fmt.Errorf("add alias: base SCID not found for %v", | |||
There was a problem hiding this comment.
I think we need to use %s and &assetSpecifier to invoke the .String() method here.
| "asset %s", id.String()) | ||
| if len(balances) == 0 { | ||
| return nil, fmt.Errorf("no asset channel balance found for %s", | ||
| specifier.String()) |
There was a problem hiding this comment.
I think because .String() has a pointer receiver, it's enough to just do &specifier.
There was a problem hiding this comment.
Can be applied to the other error messages below as well (%v and pointer).
| assetGen := assetOutput.AssetInfo.AssetGenesis | ||
| assetIDBytes, err := hex.DecodeString( | ||
| assetGen.AssetID, | ||
| ) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error "+ | ||
| "decoding asset ID: %w", err) | ||
| } | ||
|
|
||
| var assetID asset.ID | ||
| copy(assetID[:], assetIDBytes) | ||
|
|
||
| match, err := r.cfg.RfqManager.AssetMatchesSpecifier( | ||
| ctx, specifier, assetID, | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
This is the exact same code as in the previous commit. Perhaps we should extract that into a re-usable method in the Manager:
func (m *Manager) ChannelCompatible(ctx context.Context,
jsonAsset rfqmsg.JsonAssetChanInfo, specifier asset.Specifier) (bool,
error) {
gen := jsonAsset.AssetInfo.AssetGenesis
assetIDBytes, err := hex.DecodeString(
gen.AssetID,
)
if err != nil {
return false, fmt.Errorf("error decoding asset ID: %w", err)
}
var assetID asset.ID
copy(assetID[:], assetIDBytes)
match, err := m.AssetMatchesSpecifier(ctx, specifier, assetID)
if err != nil {
return false, err
}
// TODO(george): Instead of returning the first result,
// try to pick the best channel for what we're trying to
// do (receive/send). Binding a baseSCID means we're
// also binding the asset liquidity on that channel.
return match, nil
}
| event, err := aliceEventNtfns.Recv() | ||
| require.NoError(t.t, err) | ||
|
|
||
| _, ok := event.Event.(*rfqrpc.RfqEvent_PeerAcceptedSellQuote) |
There was a problem hiding this comment.
Since we now have the initial request message available in the event, I wonder if we should add the asset specifier to the RFQ quote messages and verify here that the group key is set properly.
See
Line 6889 in 7d7cec6
Description
This PR introduces the minimum functionality for two nodes to negotiate and register a buy or sell quote, which identifies the underlying asset via a group key instead of an ID.
It includes a basic itest which lets Alice & Bob negotiate a quote based on a specifier which only includes a group key.
Related to #1028, but does not close it.
This change is