`lightning-liquidity::lsps2::event::LSPS2ServiceEvent::OpenChannel` is incredibly unclear

[TheBlueMatt]

• What is amt_to_forward_msat, is that what I should use as push_msat (its not, but it really reads like it, in fact its not something the user has to deal with at all!)?
• The name opening_fee_msat sounds like what I should use for the tx fee, but should really be opening_fee_earned_msat
• Why is LSPS2 involved in user_channel_id at all? It should have no problem tracking everything by intercept_scid. We shouldn't even be relying on user_channel_id being globally unique cause it isn't required to be.

[tnull]

• What is amt_to_forward_msat, is that what I should use as push_msat (its not, but it really reads like it, in fact its not something the user has to deal with at all!)?

I agree the docs here could be improved, but it's def. something the user has to deal with as it describes the payment size that need to be forwarded (read: need to be available) after the channel open. Basically, the channel size needs to be amt_to_forward_msat + some overprovisioning factor.

• The name opening_fee_msat sounds like what I should use for the tx fee, but should really be opening_fee_earned_msat

Again, the docs could be better, the naming however corresponds directly to the opening_fee parameter used throughout the LSPS2 spec. So naming it something different could be misleading/less clear, IMO.

• Why is LSPS2 involved in user_channel_id at all? It should have no problem tracking everything by intercept_scid.

Yes, by now we could track the buy flow by intercept SCID in the LSPS2 service side, however, this is for example not true for the LSPS1 service. We generally wanted to allow a relatively homogeneous interface in which the LSP can bring their own ID semantics to keep track of opened channels, independently of which spec they are using, and they'll see the IDs reflected in the ChannelManager state.

FWIW, we also hoped to eventually get rid of the additional internal id-to-id maps, but that will require to include user_channel_id on all the related LDK event types, which, while not always super straightforward, we still intend to do.

[TheBlueMatt]

I agree the docs here could be improved, but it's def. something the user has to deal with as it describes the payment size that need to be forwarded (read: need to be available) after the channel open. Basically, the channel size needs to be amt_to_forward_msat + some overprovisioning factor.

Ah, right. The variable probably needs renaming, not just docs improved.

Again, the docs could be better, the naming however corresponds directly to the opening_fee parameter used throughout the LSPS2 spec. So naming it something different could be misleading/less clear, IMO.

In general we shouldn't name things just because the spec calls them that. Relying on documentation to fix confusing variable names is a losing battle - documentation is rarely read, and when it is its an afterthought. If the code that uses the variable isn't clear from the variable name, the variable name needs changed. In this case, the variable might make sense in the context of the spec, because there's not much else it could refer to (tho I kinda disagree), but that doesn't mean its right in the context of our event, where it could very reasonably refer to something very different. Someone who isn't farmiliar with the spec should be able to use the event without being confused (as I was!).

however, this is for example not true for the LSPS1 service.

I'm quite confused by this. Looking at the lsps1 module, I only see two references to u128, both related to PeerState::request_to_cid. However, the only use of request_to_cid is in insert_request, which inserts the channel id into that map, but its never used anywhere. No u128/user_channel_id appears anywhere in the public API of LSPS1.

[tnull]

I'm quite confused by this. Looking at the lsps1 module, I only see two references to u128, both related to PeerState::request_to_cid. However, the only use of request_to_cid is in insert_request, which inserts the channel id into that map, but its never used anywhere. No u128/user_channel_id appears anywhere in the public API of LSPS1.

Note that LSPS1ServiceHandler is still in a dysfunctional state and ~pending a rewrite. It's event variants should roughly be in order, but the actual API will be reworked.

[TheBlueMatt]

Okay but the events also don't currently rely on/expose a user_channel_id, and its very unclear to me why that would be a requirement.