Skip to content

Rename Event::PaymentClaimable::inbound_channel_ids to via_... #3764

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

In c62aa23 we merged via_channel_id and via_user_channel_id and added support for MPP to the fields, but created a new inbound_channel_ids field to replace them. The existing fields were labeled via_ to differentiate between inbound channels (i.e. channels which were to us) and channels over which we received the payment.

Here we restore the original naming by renaming the new inbound_channel_ids field to via_channel_ids.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented May 2, 2025

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

In c62aa23 we merged
`via_channel_id` and `via_user_channel_id` and added support for
MPP to the fields, but created a new `inbound_channel_ids` field to
replace them. The existing fields were labeled `via_` to
differentiate between inbound channels (i.e. channels which were to
us) and channels over which we received the payment.

Here we restore the original naming by renaming the new
`inbound_channel_ids` field to `via_channel_ids`.
@TheBlueMatt TheBlueMatt force-pushed the 2025-05-via-not-inbound branch from 824a703 to 60540f7 Compare May 2, 2025 14:36
@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@TheBlueMatt TheBlueMatt merged commit 89f5217 into lightningdevkit:main May 2, 2025
27 of 28 checks passed
@joostjager
Copy link
Contributor

I thought that in the context of a payment, inbound channel isn't ambiguous? I can understand that there might be some confusion if inbound channel is a global term within LDK for remote-initiated channels.

Via is also non-ambiguous for an incoming payment and avoids that confusion, so better indeed.

But is there a general term for "channel that something came in through"? Because in other cases, it might be necessary to distinguish between in and out, and via doesn't do that.

@TheBlueMatt
Copy link
Collaborator Author

Yea, in context its not quite as ambiguous, but via is at least not used elsewhere for anything else, which seemed worth it (I know it was discussed when the fields were first added here). I'm definitely open to something better than "via", because indeed with a list now I can see someone thinking they're getting a list of all channels the payment went through. Maybe last_hop_channels?

@joostjager
Copy link
Contributor

That is a question of whether one can assume that users understand that it is impossible to know all channels?

Perhaps a less ambiguous name than 'inbound channel' for the channel open direction would have been better, such as 'remote-initiated channel'. That would free up the term 'inbound channel' to indicate htlc direction.

Other alternatives are:

  • Incoming channel - although it is quite similar to inbound channel
  • Receiving channel

To me, receiving channel is the clearest.

Probably something to revisit the next time we touch this code.

@TheBlueMatt
Copy link
Collaborator Author

That is a question of whether one can assume that users understand that it is impossible to know all channels?

Yea, I think its fair to say we shouldn't assume that.

Probably something to revisit the next time we touch this code.

For that reason, I think I'd prefer to just rename this again (sorry lol).

To me, receiving channel is the clearest.

Hmm, yea, that's not bad. Is receiving or last_hop clearer?

@joostjager
Copy link
Contributor

joostjager commented May 5, 2025

Hop is a bit strange because this node is the last hop? I'd almost think we'd be dealing with some other hop. My preference would be receiving, staying close to the payment direction.

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.

5 participants