Skip to content

Do not auto-select the lightning std feature from tx-sync crate #2069

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

We have some downstream folks who are using LDK in wasm compiled via the normal rust wasm path. To ensure nothing breaks they want to use no-std on the lightning crate, disabling time calls as those panic. However, the HTTP logic in
lightning-transaction-sync gets automatically stubbed out by the HTTP client crates when targeting wasm via wasm_bindgen, so it works fine despite the std restrictions.

In order to make both work, lightning-transaction-sync can remain std, but needs to not automatically enable the std flag on the lightning crate, ie by setting default-features = false. We do so here.

@TheBlueMatt TheBlueMatt added this to the 0.0.114 milestone Mar 3, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.09 ⚠️

Comparison is base (6ddf69c) 87.19% compared to head (b15dfdc) 87.10%.

❗ Current head b15dfdc differs from pull request most recent head 1a18b88. Consider uploading reports for the commit 1a18b88 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2069      +/-   ##
==========================================
- Coverage   87.19%   87.10%   -0.09%     
==========================================
  Files         100      100              
  Lines       44560    45009     +449     
  Branches    44560    45009     +449     
==========================================
+ Hits        38853    39206     +353     
- Misses       5707     5803      +96     
Impacted Files Coverage Δ
lightning-invoice/src/utils.rs 96.14% <0.00%> (+0.64%) ⬆️
lightning/src/ln/peer_handler.rs 64.19% <0.00%> (+5.78%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM mod outstanding comments.


# Obviously lightning-transaction-sync doesn't support no-std, but it should build
# even if lightning is built with no-std.
lightning-transaction-sync = { path = "../lightning-transaction-sync", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that currently you'd either need to enable esplora-blocking or esplora-async to have the acutal code built.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, I don't actually want the code built, I want to make sure lightning builds :)

Copy link
Contributor

@tnull tnull Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, just wanted to mention since the comment is technically slightly off. But really doesn't matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it does build, there's just no code built :p

We have some downstream folks who are using LDK in wasm compiled
via the normal rust wasm path. To ensure nothing breaks they want
to use `no-std` on the lightning crate, disabling time calls as
those panic. However, the HTTP logic in
`lightning-transaction-sync` gets automatically stubbed out by the
HTTP client crates when targeting wasm via `wasm_bindgen`, so it
works fine despite the std restrictions.

In order to make both work, `lightning-transaction-sync` can remain
`std`, but needs to not automatically enable the `std` flag on the
`lightning` crate, ie by setting `default-features = false`. We do
so here.
@TheBlueMatt TheBlueMatt force-pushed the 2023-03-no-tx-sync-auto-std branch from b15dfdc to 1a18b88 Compare March 3, 2023 19:26
@TheBlueMatt TheBlueMatt merged commit 852cf2b into lightningdevkit:main Mar 3, 2023
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.

5 participants