Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Support for Hold invoices #3558

Merged
merged 11 commits into from Jun 12, 2020
Merged

Support for Hold invoices #3558

merged 11 commits into from Jun 12, 2020

Conversation

mrfelton
Copy link
Member

Description:

  • Adds support for Hold invoices.
  • Currently disabled behind feature flag.

Motivation and Context:

Support more LN features

How Has This Been Tested?

Manually

Screenshots:

image

image

image

image

Types of changes:

Feature

Checklist:

  • My code follows the code style of this project.
  • I have reviewed and updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes where needed.
  • All new and existing tests passed.
  • My commits have been squashed into a concise set of changes.

@mrfelton mrfelton added the type: enhancement New feature or request label Jun 11, 2020
@mrfelton mrfelton self-assigned this Jun 11, 2020
@coveralls
Copy link

coveralls commented Jun 11, 2020

Coverage Status

Coverage decreased (-0.1%) to 22.11% when pulling f145097 on mrfelton:feat/hold-invoices into 4bb0c2a on LN-Zap:master.

@mrfelton mrfelton added this to the v0.7.0-beta milestone Jun 12, 2020
@mrfelton mrfelton merged commit 18c4873 into LN-Zap:master Jun 12, 2020
@mrfelton mrfelton deleted the feat/hold-invoices branch June 12, 2020 08:54
Copy link
Member

@korhaliv korhaliv left a comment

Choose a reason for hiding this comment

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

some minor feedback

const isSettled = invoice.state ? invoice.state === 'SETTLED' : invoice.settled

const isCancelled = invoice.state === 'CANCELED'
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to extract a constant because string is 'CANCE_L_E' (one L) while the var name is cancelled (2 Ls), so while both of these are correct it might be easy to confuse later.

@@ -169,11 +187,12 @@ const ACTION_HANDLERS = {
[SET_MODALS]: (state, { modals }) => {
state.modals = modals
},

[SET_TOP_MODAL]: (state, { modal }) => {
state.modals[0] = modal
Copy link
Member

Choose a reason for hiding this comment

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

is it desired that we overwrite the topmost modal as opposed to shifting the queue ?

<FormattedMessage {...messages.payment_request_keysend} />
) : (
<QRCode value={paymentRequest.toUpperCase()} />
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check for paymentRequest to be != null ? Looks like it's possible (isKeySend being false doesn't automatically mean that paymentRequest is set or does it?).

</Text>
) : (
<Text color={getStatusColor()} fontWeight="light">
<>{stateDisplayName}</>
Copy link
Member

Choose a reason for hiding this comment

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

is fragment needed here?

@@ -123,10 +123,22 @@ const commitString = createSelector(version, v => {
return c ? c.replace('commit=', '') : undefined
})

/**
* hasRouterSupport - Check whether node has support for Invoices service
Copy link
Member

Choose a reason for hiding this comment

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

hasRouterSupport -> hasInvoicesSupport

return (
<Box>
<Flex alignItems="center" justifyContent="space-between">
<Flex>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to have "double" <Flex> here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants