Skip to content
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

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

Open
tnull opened this issue Aug 19, 2024 · 14 comments · Fixed by #3359
Open

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

tnull opened this issue Aug 19, 2024 · 14 comments · Fixed by #3359
Milestone

Comments

@tnull
Copy link
Contributor

tnull commented Aug 19, 2024

Follow-up to #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?

@tnull tnull added this to the 0.0.124 milestone Aug 19, 2024
@TheBlueMatt
Copy link
Collaborator

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
Copy link
Contributor Author

tnull commented Aug 19, 2024

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
Copy link
Collaborator

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

@tnull
Copy link
Contributor Author

tnull commented Aug 19, 2024

I don't think you can currently deprecate a re-export? rust-lang/rust#30827

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

@TheBlueMatt
Copy link
Collaborator

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
Copy link
Contributor Author

tnull commented Aug 19, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

#3253

@Kixunil
Copy link
Contributor

Kixunil commented Aug 19, 2024

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

@TheBlueMatt
Copy link
Collaborator

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
Copy link
Contributor

Kixunil commented Aug 20, 2024

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
Copy link
Collaborator

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

@TheBlueMatt
Copy link
Collaborator

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

@TheBlueMatt
Copy link
Collaborator

Oops, github was a bit eager. We made more progress, but sadly #3359 deprecated one more set of exports which we'll want to remove in 0.2...

@TheBlueMatt TheBlueMatt reopened this Oct 18, 2024
@TheBlueMatt TheBlueMatt modified the milestones: 0.1, 0.2 Oct 18, 2024
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 a pull request may close this issue.

3 participants