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

Rename PaymentReceived to PaymentClaimable #1891

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Dec 1, 2022

As discussed in lightningdevkit/ldk-node#34, the current naming of PaymentReceived/PaymentClaimed events is rather confusing since PaymentReceived by itself sounds like the final event, however, the user still needs to claim the payment before it ... well ... is actually received.

In order to make the naming more consistent and intuitive, in this PR we therefore simply rename PaymentReceived to PaymentClaimable and PaymentClaimed to PaymentReceived.

@TheBlueMatt
Copy link
Collaborator

I wonder if we shouldn't rename PaymentClaimed here - just renaming PaymentRecieved resolves a lot of the confusion, and renaming PaymentClaimed is kinda confusing for existing users given there's still a PaymentReceived but it has totally different semantics now.

@arik-so
Copy link
Contributor

arik-so commented Dec 3, 2022

I agree with Matt. Leaving PaymentClaimed should be less confusing.

@tnull
Copy link
Contributor Author

tnull commented Dec 3, 2022

Yup, I had the same thought (see lightningdevkit/ldk-node#34 (comment)), but couldn't keep myself from including the second commit. Would have been nice to clean it up once and for all...

Alright, will drop the second commit.

@tnull tnull force-pushed the 2022-12-rename-payment-events branch from 64524d4 to 1d807c8 Compare December 3, 2022 06:06
@tnull tnull changed the title Rename PaymentReceived to PaymentClaimable and PaymentClaimed to PaymentReceived Rename PaymentReceived to PaymentClaimable Dec 3, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2022

Codecov Report

Base: 90.70% // Head: 91.01% // Increases project coverage by +0.30% 🎉

Coverage data is based on head (c17e9fb) compared to base (5e577cb).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1891      +/-   ##
==========================================
+ Coverage   90.70%   91.01%   +0.30%     
==========================================
  Files          91       91              
  Lines       48392    51224    +2832     
  Branches    48392    51224    +2832     
==========================================
+ Hits        43894    46621    +2727     
- Misses       4498     4603     +105     
Impacted Files Coverage Δ
lightning-invoice/src/utils.rs 97.81% <100.00%> (ø)
lightning/src/ln/chanmon_update_fail_tests.rs 97.59% <100.00%> (ø)
lightning/src/ln/channelmanager.rs 88.98% <100.00%> (+2.71%) ⬆️
lightning/src/ln/functional_test_utils.rs 93.73% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.07% <100.00%> (-0.07%) ⬇️
lightning/src/ln/monitor_tests.rs 99.56% <100.00%> (ø)
lightning/src/ln/onion_route_tests.rs 97.49% <100.00%> (ø)
lightning/src/ln/payment_tests.rs 98.45% <100.00%> (-0.28%) ⬇️
lightning/src/ln/priv_short_conf_tests.rs 96.54% <100.00%> (ø)
lightning/src/ln/reload_tests.rs 95.24% <100.00%> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, basically.

lightning/src/util/events.rs Outdated Show resolved Hide resolved
lightning/src/util/events.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2022-12-rename-payment-events branch from 1d807c8 to c17e9fb Compare December 4, 2022 16:44
@TheBlueMatt TheBlueMatt merged commit de2acc0 into lightningdevkit:main Dec 4, 2022
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