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

Re-export common libs #49

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented May 28, 2024

This PR re-exports common libs bitcoin, elements and lightning_invoice.

TLDR: This allows callers, if necessary, to use structs from these libs without having to worry about having an incompatible lib version.

For example, without this PR, a caller would have access to lightning_invoice::Bolt11Invoice as boltz_client::Bolt11Invoice, but not to lightning_invoice::Bolt11InvoiceDescription since this is not re-exported at the moment. If the caller would import the lightning_invoice dependency to get access to this, they may run into compatibility issues if they use e.g. Bolt11Invoice::from_str(..)?.description() with the import from boltz_client and Bolt11InvoiceDescription from their own dependency.

With the new re-exports, callers can access all structs from boltz_client's specific versions of bitcoin, elements and lightning_invoice.

@michael1011 michael1011 requested a review from i5hi May 28, 2024 13:50
@i5hi
Copy link
Collaborator

i5hi commented May 28, 2024

It maybe an overkill to re-export the entire library. Instead we should be looking to only re-export those structs that we use as inputs and outputs in our code.

@i5hi
Copy link
Collaborator

i5hi commented May 28, 2024

Bolt11Invoice for example is already re-exported so you can already use:

boltz_client::Bolt11Invoice

@ok300
Copy link
Contributor Author

ok300 commented May 28, 2024

It maybe an overkill to re-export the entire library

Well, it doesn't hurt boltz-client to do it, but could help the projects integrating it.

Bolt11Invoice for example is already re-exported

Yes, but for anything more advanced, for example matching on the description, which is of type Bolt11InvoiceDescription, the caller now has to import the precise correct version of lightning_invoice crate. If its a different version, best case the compiler accepts it and the project now carries 2 dependency trees of 2 versions of lightning_invoice, worst case the compiler says the structs are incompatible. If they use any other dependency, which also uses e.g. lightning_invoice but with a different version, it becomes exponentially more difficult trying to bring the different modules to integrate with each other. Being able to use a dependency's specific e.g. lightning_invoice version massively simplifies that.

Especially for utility libraries, IMO it makes sense to export them, as projects using the boltz crate will likely need to use those utility libraries as well, so they will likely run in the above conundrum.

In the end its your call, but re-exporting them would be greatly appreciated.

@i5hi
Copy link
Collaborator

i5hi commented May 28, 2024

makes sense.

@i5hi
Copy link
Collaborator

i5hi commented May 28, 2024

Thanks for the PR @ok300 !

@i5hi i5hi merged commit 6e1b2cc into SatoshiPortal:trunk May 28, 2024
3 of 4 checks passed
@ok300 ok300 deleted the ok300-re-export-common-libs branch August 15, 2024 11:44
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 this pull request may close these issues.

2 participants