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.

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

bug: cln paymentstate check on error #2151

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Conversation

callebtc
Copy link
Collaborator

@callebtc callebtc commented Dec 5, 2023

Payment state check should only return False if the payment 100% failed. Uncheckable entries should not enter the db in the first place, which we will be working on next.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.54%. Comparing base (6083948) to head (e01c45f).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #2151   +/-   ##
=======================================
  Coverage   58.53%   58.54%           
=======================================
  Files          61       61           
  Lines        9187     9181    -6     
=======================================
- Hits         5378     5375    -3     
+ Misses       3809     3806    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

prusnak
prusnak previously requested changes Dec 5, 2023
Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

@callebtc callebtc marked this pull request as draft December 5, 2023 11:47
@callebtc callebtc marked this pull request as ready for review December 5, 2023 11:57
@dni dni self-requested a review December 5, 2023 11:57
@prusnak
Copy link
Collaborator

prusnak commented Dec 6, 2023

Is the commit 09b91b8 that removes the time block intentional? cc @dni (author of that feature)

Copy link
Member

@dni dni left a comment

Choose a reason for hiding this comment

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

i am scared, i think also the debug log when deleting was removed,and that actually saved us this time discovering the issue at all. so at least the log should be readded.

@prusnak
Copy link
Collaborator

prusnak commented Dec 6, 2023

i am scared, i think also the debug log when deleting was removed

It was re-added in 7dfbabf

lnbits/core/models.py Outdated Show resolved Hide resolved
lnbits/core/models.py Outdated Show resolved Hide resolved
@dni dni requested a review from prusnak December 19, 2023 10:58
@dni dni added this to the 0.12.0 milestone Dec 19, 2023
@prusnak prusnak force-pushed the wallets/safer_paymentstate_check branch from 0b480a3 to 092a32c Compare December 19, 2023 15:08
@prusnak
Copy link
Collaborator

prusnak commented Dec 19, 2023

Squashed and rebased

@prusnak prusnak force-pushed the wallets/safer_paymentstate_check branch from dd15558 to 2bd392b Compare December 19, 2023 16:01
@dni dni modified the milestones: 0.12.0, 0.12.1 Jan 30, 2024
@motorina0 motorina0 marked this pull request as draft February 13, 2024 13:24
@motorina0 motorina0 removed this from the 0.12.2 milestone Feb 13, 2024
@dni dni marked this pull request as ready for review March 12, 2024 08:03
@dni dni changed the title CLN: paymentstate check on error bug: cln paymentstate check on error Mar 12, 2024
@dni dni force-pushed the wallets/safer_paymentstate_check branch from 2bd392b to 7877be8 Compare March 12, 2024 10:30
@dni dni force-pushed the wallets/safer_paymentstate_check branch from a995392 to e01c45f Compare March 12, 2024 12:30
@dni dni merged commit 4c0bd13 into dev Mar 12, 2024
23 checks passed
@dni dni deleted the wallets/safer_paymentstate_check branch March 12, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants