search

lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.14k stars 358 forks source link

Move `lightning::ln::types::*` to `lightning-types` #3251

Open tnull opened 1 month ago

tnull commented 1 month ago

Follow-up to https://github.com/lightningdevkit/rust-lightning/pull/3234

IMO, it's confusing that we now have both lightning::ln::types::* and lightning-types.

E.g., ChannelId is currently still in lightning::ln::types, should we move it to lightning-types?

TheBlueMatt commented 1 month ago

I do kinda prefer we re-export somewhere, and given our current paths are lightning::ln::types I don't see much reason to jump to remove that, but we could definitely move it to lightning::types (maybe after deprecating first - does rust let us mark re-exports deprecated?)

tnull commented 1 month ago

Right, we can keep the re-export, although longer term I'd prefer a single one via lightning::types. Just stumbled across the inconsistency while upgrading lightning-liquidity, so might be good to move ChannelId and make sure we won't keep scattering type definitions over multiple places.

TheBlueMatt commented 1 month ago

So maybe we re-export it as lightning::types now and deprecate (if possible) the ln::types exports?

tnull commented 1 month ago

I don't think you can currently deprecate a re-export? https://github.com/rust-lang/rust/issues/30827

But still, switching to lightning::types and possibly refactoring a good chunk of modules now would be a good idea?

TheBlueMatt commented 1 month ago

IMO we already have a good bit of API breakage in this release, it'd be kinda nice to put off yet more until the next one.

tnull commented 1 month ago

IMO we already have a good bit of API breakage in this release, it'd be kinda nice to put off yet more until the next one.

Right, I meant we can still keep the double-reexport even though we can't deprecate one of them? But yeah, also not too important to do it right away. IMO we should fix the inconsistency mentioned above before the release though.

TheBlueMatt commented 1 month ago

I don't see much reason to move ChannelId to lightning-types, and sadly doing so is pretty irritating cause it has several in-crate dependencies.

TheBlueMatt commented 1 month ago

3253

Kixunil commented 1 month ago

Maybe worth moving some things to ln-types? ChannelId definitely sounds like one of them.

TheBlueMatt commented 1 month ago

Sadly ChannelId depends on things in lightning so I don't think that's super practical. I did take a look through ln-types and it doesn't look like there's really much there we could migrate to, sadly, for various reasons. We generally have our own versions of all those types already, anyway, and our design tends to differ for various reasons.

Kixunil commented 1 month ago

I don't see any such dependency - the type contains just [u8; 32]. If you are referring to some methods having lightning types in their signatures that is solvable using extension traits (though I understand if you don't want to use them) or moving those things too. Especially your version of OutPoint looks interesting. EntropySource has frankly too weird design to my taste though. (what's the Deref thing about? Why harm performance by forcing shared references even when not needed?)

Anyway, no pressure, just feel free to ping me any time you want the crate. :)

TheBlueMatt commented 1 month ago

Yea, I was mostly referring to not wanting to deal with extension traits, which make docs super confusing for people :/.

TheBlueMatt commented 1 month ago

Untagging from 0.0.124 post-#3253, but moving to 0.1 to resolve the deprecateds by removing the old re-exports.