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

record+routing/route: add AMP record #3957

Merged
merged 3 commits into from Feb 3, 2020

Conversation

@cfromknecht
Copy link
Collaborator

cfromknecht commented Jan 25, 2020

This PR adds the currently proposed AMP record from lightningnetwork/lightning-rfc#658, and
implements the sanity checks when encoding a particular route.Hop. For now, this could will
remain dead until a number of other components are in place, but this serves as a starting point to
begin implementing the remainder of the proposal that depends on these primitive definitions
before ultimately enabling it externally.

Related to #3939.

@Roasbeef Roasbeef added this to In Progress in v0.10.0-beta via automation Jan 27, 2020
@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 27, 2020
Copy link
Member

Roasbeef left a comment

LGTM 🌋

No major comments, happy to see the ball start rolling once again. Even if this is just dead code atm, it'll make any subsequent changes easier as they won't need to continually rebase and manage conflicts for a series of PRs.

record/amp.go Show resolved Hide resolved
routing/route/route.go Show resolved Hide resolved
@Roasbeef Roasbeef moved this from In Progress to Review In Progress in v0.10.0-beta Jan 28, 2020
cfromknecht added 3 commits Jan 28, 2020
@cfromknecht cfromknecht force-pushed the cfromknecht:amp-record branch from ea87794 to 9fc197d Jan 28, 2020
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Jan 28, 2020

fixed the linter errors, build now passes

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 28, 2020

Does merging this now really make things easier? It doesn't look like this code has conflict potential. In general I think it is better to merge living code, unless we have for example two prs that build onto this. But not sure if that is already the case.

@Roasbeef Roasbeef merged commit 07977a2 into lightningnetwork:master Feb 3, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0001%) to 62.996%
Details
v0.10.0-beta automation moved this from Review In Progress to Done Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.10.0-beta
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.