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.

Sign up for GitHub

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

Jump to bottom

invoices: CancelInvoice #2457

Merged
merged 8 commits into from
Feb 6, 2019
Merged

invoices: CancelInvoice #2457

merged 8 commits into from
Feb 6, 2019

Conversation

joostjager
Copy link
Contributor

This PR builds on top of #2356. Review that PR first.

@joostjager joostjager changed the title Cancelinvoice [NO REVIEW] invoices: CancelInvoice [NO REVIEW] Jan 11, 2019
@joostjager joostjager force-pushed the cancelinvoice branch 5 times, most recently from 9033d36 to 59ade67 Compare January 14, 2019 11:44
@joostjager joostjager changed the title invoices: CancelInvoice [NO REVIEW] invoices: CancelInvoice Jan 14, 2019
@joostjager joostjager force-pushed the cancelinvoice branch 6 times, most recently from 50e6cdc to 08e9007 Compare January 15, 2019 12:54
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour database Related to the database/storage of LND payments Related to invoices/payments P2 should be fixed if one has time labels Jan 16, 2019
@Roasbeef Roasbeef added this to the 0.6 milestone Jan 16, 2019
@joostjager joostjager force-pushed the cancelinvoice branch 5 times, most recently from c86e158 to 36da109 Compare January 24, 2019 10:32
@joostjager joostjager force-pushed the cancelinvoice branch 3 times, most recently from 3a5c437 to 81a95f8 Compare January 29, 2019 13:58
@joostjager
Copy link
Contributor Author

@Roasbeef ptal

cfromknecht
cfromknecht reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

@joostjager completed an initial pass, design looks solid to me. also really like the refactorings made to the htlcswitch tests, as those have been in need of some love from a test framework perspective.

would be nice to rebase this on the master to visualize the channeldb coverage using go-acc, as right now the unit testing is separate from the implementation and appears uncovered

htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/mock.go Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
cmd/lncli/invoicesrpc_active.go Outdated Show resolved Hide resolved
channeldb/invoices.go Show resolved Hide resolved
lnrpc/invoicesrpc/invoices_server.go Show resolved Hide resolved
lnrpc/invoicesrpc/utils.go Outdated Show resolved Hide resolved
htlcswitch/mock.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the cancelinvoice branch 2 times, most recently from 3eeac06 to 4eda8d5 Compare January 30, 2019 07:48
@joostjager
Copy link
Contributor Author

Rebased on top of master with go-acc. Coverage reports shows cancel logic in link being hit.

ptal @cfromknecht

@joostjager joostjager force-pushed the cancelinvoice branch 2 times, most recently from c9d78d1 to ed40d53 Compare February 1, 2019 10:15
@joostjager
Copy link
Contributor Author

@cfromknecht ptal

@Roasbeef
Copy link
Member

Roasbeef commented Feb 2, 2019

Can be rebased now with single invoice in.

Roasbeef
Roasbeef previously approved these changes Feb 2, 2019
View reviewed changes
Copy link
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 🦑

htlcswitch/link_test.go Show resolved Hide resolved
@joostjager joostjager force-pushed the cancelinvoice branch 2 times, most recently from d269ee6 to 81d0922 Compare February 3, 2019 15:30
@joostjager
Copy link
Contributor Author

@Roasbeef stale review rejected by rebase

@cfromknecht unknown invoice cancel test added

ptal

cfromknecht
cfromknecht previously approved these changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM 🤓 one final nit re: logging

htlcswitch/link.go Outdated
@@ -2330,6 +2330,23 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
continue
}

// Reject htlcs for canceled invoices.
if invoice.Terms.State == channeldb.ContractCanceled {
log.Errorf("Rejecting htlc due to canceled " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use l.errorf instead so that the short channel id is prepended to the log message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this, but noticed that in many other places l.errorf isn't used. Also, I think error is not the right log level for something that can just happen normally. But don't want to tackle that in this pr.

@joostjager
Copy link
Contributor Author

@cfromknecht @Roasbeef
last comment addressed.
ptal

@Roasbeef
Copy link
Member

Roasbeef commented Feb 6, 2019

Needs rebase, something else went it before it.

@joostjager
Copy link
Contributor Author

Rebased ptal

@Roasbeef Roasbeef merged commit 27cfdf9 into lightningnetwork:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef left review comments

@cfromknecht cfromknecht cfromknecht left review comments

Assignees
No one assigned
Labels
database Related to the database/storage of LND enhancement Improvements to existing features / behaviour P2 should be fixed if one has time payments Related to invoices/payments
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.
3 participants
@joostjager @Roasbeef @cfromknecht