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

Use ln-types crate? #1167

Closed
Kixunil opened this issue Nov 12, 2021 · 6 comments
Closed

Use ln-types crate? #1167

Kixunil opened this issue Nov 12, 2021 · 6 comments

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Nov 12, 2021

I created an ln-types crate for my WIP and upcoming projects and was thinking it should be useful here as well. Is it something you'd like to use? I'm willing to move the crate to rust-bitcoin org and lower MSRV to 1.36 (didn't want to do extra work until confirmed) if yes. I only want it to keep the main goal of being Rust-idiomatic and allow relevant integrations.

In particular, NodeId could be replaced but perhaps other things as well. TBH I'm not familiar with rust-lightning codebase that much.
If you have other suggestions or want to do a code review, I'd be happy to re receive those!

@TheBlueMatt
Copy link
Collaborator

We already have a number of those types internally (and where we don't we probably eventually should). Without some downstream user need to use types from such a crate I dunno what value it brings us integrating them (aside from it being harder to to update them), and presumably if a user needed better types support they could just depend on rust-lightning?

@Kixunil
Copy link
Contributor Author

Kixunil commented Nov 12, 2021

Some things I'm working on don't require a full lightning crate - e.g. when they use RPC, so avoiding long compile times is preferred. Having a shared crate can improve quality of that part of the code - more reviewers and contributors. Being small and simple should hopefully mitigate the possible update slowness.

@TheBlueMatt
Copy link
Collaborator

Some things I'm working on don't require a full lightning crate

Right, so there's no requirement for them to have the same type :)

Having a shared crate can improve quality of that part of the code - more reviewers and contributors.

True, but for newtypes there's only so much code.

Being small and simple should hopefully mitigate the possible update slowness.

Maybe, but you can't just make a change in a single PR, you still have to go through making a change, getting a release cut, and then making the downstream change. It ends up being a lot of work for even simple things.

@Kixunil
Copy link
Contributor Author

Kixunil commented Nov 12, 2021

there's no requirement for them to have the same type

Was thinking about this too and I'm not convinced it has zero value.

Take for example loptos. It currently communicates with LND using GRPC but I could imagine adding support for custom backends and eventually someone implementing it for rust-lightning. In that case the types would have to be same or converted. To be clear I'm not claiming it's a huge improvement, just non-zero.

Anyway, thanks for your response! If you believe it's not valuable enough feel free to close, no hard feelings, was worth asking anyway, I think. :)

@TheBlueMatt
Copy link
Collaborator

Yea, I'd say for now lets not bother - there are cases where users may want it, and if that becomes common we can reconsider, but there's an immediate maintenance burden that we'd have to pay even if no users ever care, which seems like a pain not worth taking on IMO.

@Kixunil
Copy link
Contributor Author

Kixunil commented Nov 12, 2021

Yeah, good point. Not now, maybe later.

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

No branches or pull requests

2 participants