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

Ensure failed payments marked as failed #3145

Merged
merged 6 commits into from
Nov 11, 2019

Conversation

mrfelton
Copy link
Member

@mrfelton mrfelton commented Nov 8, 2019

Description:

Ensure failed payments marked as failed

Generate a unique payment id for sending payments to enable more accurate tracking of payments across retry attempts.

Motivation and Context:

Fix #3142

How Has This Been Tested?

Manually

Types of changes:

Fix

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: bug 🐛 Something isn't working label Nov 8, 2019
@mrfelton mrfelton added this to the v0.6.0-beta milestone Nov 8, 2019
@mrfelton mrfelton requested a review from korhaliv November 8, 2019 12:29
@mrfelton mrfelton self-assigned this Nov 8, 2019
@mrfelton mrfelton force-pushed the fix/payments-sending branch from d87618e to 6c79a0f Compare November 8, 2019 12:31
@coveralls
Copy link

coveralls commented Nov 8, 2019

Coverage Status

Coverage increased (+0.005%) to 22.7% when pulling f1600d1 on mrfelton:fix/payments-sending into ff4e7ba on LN-Zap:master.

@mrfelton mrfelton changed the base branch from next to master November 8, 2019 13:39
@mrfelton mrfelton force-pushed the fix/payments-sending branch from 6c79a0f to a0488d1 Compare November 8, 2019 13:56
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.

A couple of minor comments

renderer/reducers/invoice.js Outdated Show resolved Hide resolved
renderer/reducers/payment.js Outdated Show resolved Hide resolved
services/grpc/lightning.methods.js Outdated Show resolved Hide resolved
services/grpc/lightning.methods.js Outdated Show resolved Hide resolved
grpcLog.error('PAYMENT ERROR', res)
const error = new Error(res.payment_error)
error.details = res
reject(error)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why here we reject and in catch we re-throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

The rejection here is used to reject the Promise within the data event handler in the case that the sendPayment lightning call returns an error (which it does by returning a success but with payment_error set).

The catch at the end wraps the entire method and is triggered in the event that an unexpected error occurs anywhere within our sendPayment wrapper. It doesn't re throw the rejection from the data event handler.

renderer/reducers/payment.js Outdated Show resolved Hide resolved
services/grpc/lightning.methods.js Outdated Show resolved Hide resolved
@mrfelton mrfelton force-pushed the fix/payments-sending branch from e55c00f to f1600d1 Compare November 9, 2019 21:13
@mrfelton mrfelton requested a review from korhaliv November 9, 2019 21:32
@mrfelton mrfelton dismissed korhaliv’s stale review November 9, 2019 21:32

Addressed items raised

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.

Tested ACK f1600d1

@mrfelton mrfelton merged commit b8b74a8 into LN-Zap:master Nov 11, 2019
@mrfelton mrfelton deleted the fix/payments-sending branch November 11, 2019 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retried failed payment stuck in processing state
3 participants