Type Renames for lightning-liquidity bindings
#3574
Conversation
since it is
In general it kinda sucks to export multiple enums/structs as the same name, event across multiple modules/crates as it could cause downstream users to have to use overly verbose fully qualified types. More importantly, however, we cannot express multiple things under the same name in bindings (as we ultimately export to C which has a single, global, namespace). Here we rename `lightning-liquidity`'s `Event` `LSPSEvent` to avoid clashing with `lightning::events::Event`.
There's no reason to require a `clone`able `ChannelManager` reference (even though they often are).
In general it kinda sucks to export multiple enums/structs as the same name, event across multiple modules/crates as it could cause downstream users to have to use overly verbose fully qualified types. More importantly, however, we cannot express multiple things under the same name in bindings (as we ultimately export to C which has a single, global, namespace). Here we rename several structs in `lightning-liquidity`'s LSPS1 module to avoid clashing with `lightning-liqudity`'s LSPS2 structs.
In general it kinda sucks to export multiple enums/structs as the same name, event across multiple modules/crates as it could cause downstream users to have to use overly verbose fully qualified types. More importantly, however, we cannot express multiple things under the same name in bindings (as we ultimately export to C which has a single, global, namespace). Here we rename several structs in `lightning-liquidity`'s LSPS2 similar to what we did with LSPS1 structs in the previous commit to avoid having unprefixed and prefixed structs with the same name in `lightning-liquidity`.
There was a problem hiding this comment.
I guess this is necessary. But if we do this, can we please make sure to rename all the types to keep them following a clear pattern?
Also, since we're already renaming the world and breaking API, it would be a good point in time to bite the bullet and drop all the LSPSX prefixes, module names, etc in favor of BLIP50/BLIP51/BLIP52. Let me know if you'd prefer I'd pick that up though.
There was a problem hiding this comment.
I guess it's fine for now, but given that we eventually might want to get away from LSPS nomenclature, we could consider naming it differently (LiquidityEvent?). Then again, moving away from LSPS will be a larger refactor at some point anyways, so might as well rename it accordingly again at that point.
There was a problem hiding this comment.
No, these bounds are (going) to be necessary as we're cloing the ChannelManager reference for the LSPS1ServiceHandler. It's just not surfaced as it lives behind cfg(lsps1_service) for now. Assuming we don't want to macroize the bounds, I'd propose to drop this commit for now.
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| use super::event::LSPS1ClientEvent; | |||
| use super::msgs::{ | |||
| CreateOrderRequest, CreateOrderResponse, GetInfoRequest, GetInfoResponse, GetOrderRequest, | |||
| CreateOrderRequest, CreateOrderResponse, LSPS1GetInfoRequest, LSPS1GetInfoResponse, GetOrderRequest, | |||
There was a problem hiding this comment.
If we have to do so, can we at least rename all LSPS1 and LSPS2 message types accordingly? I.e., please prefix everything, not just the types that might clash.
Also note to adjust all the testcases accordingly.
| @@ -26,7 +26,7 @@ use core::default::Default; | |||
| use core::ops::Deref; | |||
|
|
|||
| use crate::lsps2::msgs::{ | |||
| BuyRequest, BuyResponse, GetInfoRequest, GetInfoResponse, LSPS2Message, LSPS2Request, | |||
| BuyRequest, BuyResponse, LSPS2GetInfoRequest, LSPS2GetInfoResponse, LSPS2Message, LSPS2Request, | |||
There was a problem hiding this comment.
See above: let's rename the world.
|
Now opened #3583 to supersede this. |
lightning-liquidityviolated this in a few places, which we fix here. To get it to compile for bindings a few additional no-exports (or String conversions) are needed, but this is the bulk of the upstream work.