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

channeldb+lnrpc: invoice state #2354

Merged
merged 2 commits into from Jan 4, 2019

Conversation

3 participants
@joostjager
Copy link
Collaborator

joostjager commented Dec 20, 2018

This PR converts the invoice settled boolean into a state. This is a preparation that enables creation of new invoice states.

@joostjager joostjager changed the title Invoice state channeldb+lnrpc: invoice state Dec 20, 2018

@joostjager joostjager force-pushed the joostjager:invoice-state branch 2 times, most recently from f6c8849 to f890ff7 Dec 20, 2018

Show resolved Hide resolved channeldb/invoices.go Outdated
@@ -2334,7 +2334,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
// TODO(conner): track ownership of settlements to
// properly recover from failures? or add batch invoice
// settlement
if invoice.Terms.Settled {
if invoice.Terms.State != channeldb.ContractOpen {

This comment has been minimized.

@halseth

halseth Dec 20, 2018

Collaborator

Is this intended? Comment should possibly be updated to reflect change?

This comment has been minimized.

@joostjager

joostjager Dec 20, 2018

Collaborator

Yes. In this PR both are equivalent, but I am inverting the condition as a preparation for the hold invoice.

This comment has been minimized.

@joostjager

joostjager Dec 20, 2018

Collaborator

Same for comment below

Show resolved Hide resolved invoiceregistry.go Outdated
Show resolved Hide resolved invoiceregistry.go Outdated
Show resolved Hide resolved invoiceregistry.go
Show resolved Hide resolved invoiceregistry.go

@joostjager joostjager force-pushed the joostjager:invoice-state branch from f890ff7 to 86e9ae7 Dec 20, 2018

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 20, 2018

comments processed

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 20, 2018

also added String() for ContractState type

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 21, 2018

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🐝

One minor nit/clarification, but generally good on my end!


// ContractSettled means the htlc is settled and the invoice has been
// paid.
ContractSettled = 1

This comment has been minimized.

@Roasbeef

Roasbeef Jan 3, 2019

Member

I think this one also needs to be explicitly specified as the prior one isn't using iota?

This comment has been minimized.

@joostjager

joostjager Jan 3, 2019

Collaborator

Fixed

@joostjager joostjager force-pushed the joostjager:invoice-state branch from 86e9ae7 to 320b775 Jan 3, 2019

High Priority automation moved this from In progress to Needs review Jan 3, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 4, 2019

Ready to land after sign+rebase! (may need to sync with some of the other PRs that also modify lnrpc protos)

joostjager added some commits Dec 19, 2018

channeldb: convert settled boolean to state
This commit is a preparation for the addition of new invoice
states. A database migration is not needed because we keep
the same field length and values.
lnrpc: report invoice state
Expose the new invoice state field over rpc.

@joostjager joostjager force-pushed the joostjager:invoice-state branch from 320b775 to 1199f17 Jan 4, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 4, 2019

Ptal @Roasbeef

@halseth

halseth approved these changes Jan 4, 2019

Copy link
Collaborator

halseth left a comment

LGTM 💯

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 4, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🍭

@Roasbeef Roasbeef merged commit 2e2d5fc into lightningnetwork:master Jan 4, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.04%) to 55.842%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment