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

lnrpc: deprecate SendToRoute with more than one route #2521

Merged
merged 2 commits into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@joostjager
Copy link
Collaborator

joostjager commented Jan 21, 2019

The current SendToRoute RPC call offers the option of passing in multiple routes. Especially in the streaming variant, this makes the structure of the response complicated, because it is double nested (multiple streamed requests each containing multiple routes for each of which a result needs to be reported). The advantage of this in terms of network round trips is negligible, so the proposal is to limit every SendToRoute request to a single route.

@joostjager joostjager requested a review from halseth Jan 21, 2019

@halseth
Copy link
Collaborator

halseth left a comment

I'm fine with doing this change, also something that should be done to the sync methods eventually IMO.

Get this in to v0.6? LGTM 👍

@@ -4072,7 +4070,7 @@ func unmarshallHop(graph *channeldb.ChannelGraph, hop *lnrpc.Hop,

// unmarshallRoute unmarshalls an rpc route. For hops that don't specify a
// pubkey, the channel graph is queried.
func (r *rpcServer) unmarshallRoute(rpcroute *lnrpc.Route,

This comment has been minimized.

Copy link
@halseth

halseth Jan 22, 2019

Collaborator

ooh, nice

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 23, 2019

@joostjager joostjager requested a review from Roasbeef Jan 24, 2019

}

var routes []*routing.Route
if len(req.Routes) > 0 {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 1, 2019

Member

Why even distinguish these two cases? If the case of the else statement, it completes after a single loop iteration.

This comment has been minimized.

Copy link
@joostjager

joostjager Feb 1, 2019

Author Collaborator

I chose not to restrict the Routes field to a single element, but to add a new non-list field Route. Therefore the else construct here.

The intention behind this is to make one change to the interface and not two (first restrict to a single element and then later make it a non-list field).

@joostjager joostjager force-pushed the joostjager:sendtosingleroute branch from 28b9b96 to 3ce228c Feb 1, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Feb 1, 2019

ptal @Roasbeef

joostjager added some commits Jan 21, 2019

@joostjager joostjager force-pushed the joostjager:sendtosingleroute branch from 3ce228c to 17645fe Feb 4, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🔌

@Roasbeef Roasbeef merged commit e5d660d into lightningnetwork:master Feb 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on sendtosingleroute at 59.668%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.