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

Add specific zero-amount-payment-request error message #364

Merged
merged 2 commits into from
Sep 26, 2021
Merged

Add specific zero-amount-payment-request error message #364

merged 2 commits into from
Sep 26, 2021

Conversation

vindard
Copy link
Contributor

@vindard vindard commented Sep 19, 2021 •
edited
Loading

Description

This PR introduces a more specific error message for the error case where a payment requests has an invalid zero-amount attached to it.

The prior error message suggests a more general and broad failure of the payment request that is very difficult to understand and correct by the end-user.

Note: The original error code was removed in a separate commit since it no longer is used anywhere else. If this isn't ok, we can simply drop that commit and force-push the branch (or revert the specific commit)

Motivation and Context

Issue: #363

How Has This Been Tested?

Has not been tested

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the Contribution document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@vindard
Copy link
Contributor Author

vindard commented Sep 19, 2021

I just found a similar issue/fix across in the Zap-Desktop repo: LN-Zap/zap-desktop#3385

@michaelWuensch
Copy link
Contributor

@vindard
Hi, thank you very much for the pull request!
The old message is indeed confusing.
Can you please use the wording from Zap desktop?
I like it more as it explains why 0 amount invoices are not supported.

@vindard vindard force-pushed the zero-amount-request-error-msg branch from d284ce9 to 65bf0c6 Compare September 25, 2021 16:21
@vindard
Copy link
Contributor Author

vindard commented Sep 25, 2021

@vindard
Hi, thank you very much for the pull request!
The old message is indeed confusing.
Can you please use the wording from Zap desktop?
I like it more as it explains why 0 amount invoices are not supported.

Sure no probs, change made here

@michaelWuensch
Copy link
Contributor

michaelWuensch commented Sep 25, 2021 •
edited
Loading

@vindard
Ah sorry, i just looked at the screenshot of that pull request you linked which showed another message that I actually meant. And I like it more:

"This is a zero amount payment request. Zap does not support zero amount payment requests due to security concerns associated with them."

Would you mind changing it again? I want to explain why we don't support them, not just say it is unsupported.

@vindard
Add a zero-amount error message for payment requests
f0d9826 
Change the error message for zero-amount requests to specifically
indicate amount failure instead of suggesting a general failure of the
payment request.
@vindard
Remove unused error code
ef0f12c
@vindard vindard force-pushed the zero-amount-request-error-msg branch from 65bf0c6 to ef0f12c Compare September 26, 2021 16:22
@vindard
Copy link
Contributor Author

vindard commented Sep 26, 2021

@vindard
Ah sorry, i just looked at the screenshot of that pull request you linked which showed another message that I actually meant. And I like it more:

"This is a zero amount payment request. Zap does not support zero amount payment requests due to security concerns associated with them."

Would you mind changing it again? I want to explain why we don't support them, not just say it is unsupported.

Ah makes sense, changed here

@michaelWuensch michaelWuensch merged commit f96296c into LN-Zap:master Sep 26, 2021
@vindard vindard deleted the zero-amount-request-error-msg branch September 27, 2021 10:43
@michaelWuensch michaelWuensch mentioned this pull request Sep 28, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@michaelWuensch michaelWuensch michaelWuensch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vindard @michaelWuensch