Skip to content

lnrpc+routing: fix unmarshallRoute and simplify route structure#1888

Merged
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
joostjager:routestruct
Oct 24, 2018
Merged

lnrpc+routing: fix unmarshallRoute and simplify route structure#1888
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
joostjager:routestruct

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Sep 12, 2018

In this PR the dependency of unmarshallRoute on edge policies being
available is removed. Edge policies may be unknown and reported as nil.
SendToRoute does not need the policies, but it does need pubkeys of the
route hops. In this commit, unmarshallRoute is modified so that it
takes the pubkeys from edgeInfo instead of channelEdgePolicy.

In addition to this, the route structure is simplified. No more connection
to the database at that point. Fees are determined based on incoming and
outgoing amounts.

@halseth
Copy link
Copy Markdown
Contributor

halseth commented Sep 25, 2018

@joostjager Possible to split into smaller commits?

@joostjager
Copy link
Copy Markdown
Contributor Author

@halseth It is a set of changes to all belong together to make the project build again. Possible the only thing that can be split off functionally is the fix to unmarshallRoute. But that is just a very small part of this PR.

In general, if I split in multiple commits, I want to do it in such a way that the project builds in between every commit. Have them really build on top of eachother. In this case, I don't see how it is possible. Modifying the Route struct just has a big impact in many places.

I could split off the test changes from the rest, but it will result in a non-building intermediate step.

Do you see a good partitioning of the changes into multiple commits?

@halseth
Copy link
Copy Markdown
Contributor

halseth commented Sep 25, 2018

Sure, if they cannot be split into individual changes without introducing compile errors, then it is fine! 👍

Copy link
Copy Markdown
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

More or less LGTM now, just a few small things! 😄

Comment thread routing/heap.go Outdated
Comment thread routing/pathfind.go Outdated
Comment thread routing/pathfind_test.go Outdated
Comment thread routing/pathfind_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can be make one line now?

Comment thread routing/pathfind_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread routing/pathfind_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread rpcserver.go Outdated
@halseth halseth added the first pass review done PR has had first pass of review, needs more tho label Sep 26, 2018
halseth
halseth previously approved these changes Oct 4, 2018
Copy link
Copy Markdown
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 🚗

@wpaulino
Copy link
Copy Markdown
Contributor

Needs a rebase.

@joostjager
Copy link
Copy Markdown
Contributor Author

Rebased

In this commit the dependency of unmarshallRoute on edge policies being
available is removed. Edge policies may be unknown and reported as nil.
SendToRoute does not need the policies, but it does need pubkeys of the
route hops. In this commit, unmarshallRoute is modified so that it
takes the pubkeys from edgeInfo instead of channelEdgePolicy.

In addition to this, the route structure is simplified. No more connection
to the database at that point. Fees are determined based on incoming and
outgoing amounts.
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💹

@Roasbeef Roasbeef merged commit fbd91fe into lightningnetwork:master Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first pass review done PR has had first pass of review, needs more tho

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants