search

lightningdevkit / ldk-node

A ready-to-go node implementation built using LDK.
Other
147 stars 73 forks source link

Emit `PaymentReceived` on upstream `PaymentReceived` or `PaymentClaimed`? #34

Closed tnull closed 1 year ago

tnull commented 1 year ago

Having PaymentReceived and PaymentClaimed in LDK Lite might be a bit verbose for the 'simplified' API.

If we only want to have one, i.e., PaymentReceived event, it raises the question when we should emit it: on receiving PaymentReceived or PaymentClaimed?

Or do we want to mirror both events here after all?

TheBlueMatt commented 1 year ago

LDKLite should just expose PaymentClaimed, ie that we have received and claimed a payment, not PaymentReceived, which it should just handle by calling claim_payment.

tnull commented 1 year ago

Yes, this is also what I'm leaning towards, only issue being that I find PaymentReceived a much more intuitive name to use for this event in LDK Lite, which however would shadow the upstream event with different meaning, i.e., would be pretty confusing if someone is aware of LDK Events already. I'll have to think about whether there is a good alternative naming for it.

For the record, I'd find using something along the lines of PaymentIncoming / PaymentReceived also more intuitive than PaymentReceived/PaymentClaimed upstream, since PaymentReceived sounds pretty final by itself, even though it's actually not.

valentinewallace commented 1 year ago

For the record, I'd find using something along the lines of PaymentIncoming / PaymentReceived also more intuitive than PaymentReceived/PaymentClaimed upstream, since PaymentReceived sounds pretty final by itself, even though it's actually not.

+1, I've gotten similar feedback about PaymentReceived being a confusing name

tnull commented 1 year ago

+1, I've gotten similar feedback about PaymentReceived being a confusing name

Thanks, good to know, then maybe the solution could indeed be renaming the upstream event..

TheBlueMatt commented 1 year ago

Any suggestions for a better one? We should totally rename it upstream if it makes sense.

tnull commented 1 year ago

Any suggestions for a better one? We should totally rename it upstream if it makes sense.

As mentioned above (in an edit), maybe something along the lines of PaymentReceived -> PaymentIncoming / PaymentInbound PaymentClaimed -> PaymentReceived?

valentinewallace commented 1 year ago

PaymentClaimable? But I'm fine with all the above too^

TheBlueMatt commented 1 year ago

Any of those SGTM, feel free to PR :)

tnull commented 1 year ago

PaymentClaimable? But I'm fine with all the above too^

Ah, that's even better! Now the only thing is that the migration PaymentClaimed->PaymentReceived might be a pitfall for users as their code for the former PaymentReceived event might just keep on working for the wrong event without them noticing.

TheBlueMatt commented 1 year ago

Maybe after #1886 tho.

tnull commented 1 year ago

Any of those SGTM, feel free to PR :)

Alright, will open a PR tomorrow/in the next days!

tnull commented 1 year ago

Closing as clarified, will update after upstream event has been merged.