rpcserver: better rfq channel selection #1357
Conversation
Pull Request Test Coverage Report for Build 13238262829Details
💛 - Coveralls |
rpcserver.go
Outdated
| @@ -7786,12 +7791,30 @@ func (r *rpcServer) DecDisplayForAssetID(ctx context.Context, | |||
| return meta.DecDisplayOption() | |||
| } | |||
|
|
|||
| // rfqChannelIntention defines the intention of calling rfqChannel. This helps | |||
| // with returning the channel that is most suitable for what we want to do. | |||
| type rfqChannelIntention uint8 | |||
There was a problem hiding this comment.
This should probably be moved into the rfq package. Then the name prefix isn't required and it just becomes rfq.ChannelIntention (or maybe even just rfq.Intention?). That would also make it an exported type (which makes more sense, given that the individual constants are also exported).
There was a problem hiding this comment.
Not sure I follow here
This enum is specifically defined for the call func below named rpcServer.rfqChannel which just picks a channel to use for either sending or receiving. Having its definition be remote to it, in a diff package, seems quite hard to keep track of. Also it doesn't seem like something that can be used in another point in the code.
Will rename to chanIntention to better reflect the scope. As the "rfq" part for this type was just a naive copy of the func name "rfqChannel"
rpcserver.go
Outdated
| case SendIntention: | ||
| // If the intention is to send, return the channel with | ||
| // the biggest local balance. | ||
| fn.ForEach(assetBalances, func(b channelWithAsset) { |
There was a problem hiding this comment.
Just to check my understanding: When sending, this code path only really matters if the peerPubKey isn't set and you have multiple channels with a given asset to different peers. Then we select the peer with the best inbound liquidity to us (the actual channel is ignored on the send related call sites).
But if peerPubKey is specified, this is basically a no-op, since it doesn't matter which channel of a peer we choose, as we only need the pubKey anyway.
There was a problem hiding this comment.
Our code currently fails if we have multiple candidate channels with different peers, asking the user to specify a peer.
This code really matters when user specifies a peer, and that peer has multiple channels with us.
There was a problem hiding this comment.
I could also address the issue of selecting a peer automatically, but that's a bit more involved and is part of #1052 so leaving it out of the picture for now
GeorgeTsagk
force-pushed
the
better-chan-selection
branch
from
086553d to
7eab028
Compare
Previously, callers of rfqChannel would just get the first channel from the list of candidates. This is sufficient, but we could easily optimise this depending on the intention. Depending on whether we want to receive or send over the channel, we return the channel with the best remote or local balance accordingly.
GeorgeTsagk
force-pushed
the
better-chan-selection
branch
from
7eab028 to
dbef1d4
Compare
Previously, callers of rfqChannel would just get the first channel from the list of candidates. This is sufficient, but we could easily optimise this depending on the intention. Depending on whether we want to receive or send over the channel, we return the channel with the best remote or local balance accordingly.