Bindings doc updates for 0.2 #4203
Conversation
|
I've assigned @joostjager as a reviewer! |
The details of these errors aren't particularly relevant, and its not worth the effort of exporting them, so we just mark them no-export here.
As move semantics do not map to bindings, the BOLT 12 object builders are marked no-export. In the new `OffersMesageFlow`, here, we also mark the methods which return these builders as no-export.
Methods like `StaticInvoice::sign` have their own signing-function traits, so there's no actual need to use `SignFn` directly, its just a useful generic wrapper. Sadly, because it uses `AsRef` bounds, which aren't really practical to map in bindings, we can't really expose it directly in bindings. Because there's an alternative and its tricky to expose, we simply mark it no-export here.
We do not currently support async traits or methods in our home-grown bindings logic, so here mark them no-export.
Rather than importing `ChannelTransactionParameters` via the `use` in `lightning::sign`, import it via its real path in `lightning::sign::ecdsa`. This makes reading the code (incredibly marginally) simpler, but also makes the bindings generator happy.
In 0.2 we added new `LengthLimitedRead` and `LengthReadable` traits, but forgot to copy the no-export tags from the existing `Read` and `Readable` traits. Here we add the missing tags.
This avoids confusing the bindings generator.
TheBlueMatt
force-pushed
the
2025-11-0.2-doc-tweaks
branch
from
32fce5f to
60a7713
Compare
When we added the async traits and wrapper structs for the transaction-bumping logic, we didn't bother copying the documentation to the sync wrappers as we figured the links sufficed. Sadly, however, this means that our bindings logic will have no docs but a broken link to an async object that doesn't exist. Instead, here, we copy the docs from the async objects to the sync ones, at least leaving behind a comment noting that both need updating whenever one gets updated. We also fix one bogus link in `Wallet`'s docs.
When we added the async trait for `KVStore`, we didn't bother copying the documentation to the sync wrappers as we figured the links sufficed. Sadly, however, this means that our bindings logic will have no docs but a broken link to an async object that doesn't exist. Instead, here, we copy the docs from the async objects to the sync ones, at least leaving behind a comment noting that both need updating whenever one gets updated.
When we added the async wrapper for `OutputSweeper`, we didn't bother copying the documentation to the sync wrappers as we figured the links sufficed. Sadly, however, this means that our bindings logic will have no docs but a broken link to an async object that doesn't exist. Instead, here, we copy the docs from the async objects to the sync ones, at least leaving behind a comment noting that both need updating whenever one gets updated.
TheBlueMatt
force-pushed
the
2025-11-0.2-doc-tweaks
branch
from
60a7713 to
4bc993c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4203 +/- ##
==========================================
+ Coverage 88.86% 89.28% +0.41%
==========================================
Files 180 180
Lines 137913 137913
Branches 137913 137913
==========================================
+ Hits 122560 123130 +570
+ Misses 12540 12172 -368
+ Partials 2813 2611 -202
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Mostly basic questions for my own understanding
| #[cfg(not(feature = "std"))] | ||
| /// Marker trait to optionally implement `Sync` under std. | ||
| /// | ||
| /// This is not exported to bindings users as async is only supported in Rust. |
There was a problem hiding this comment.
Out of scope: some of the Maybe* markers were needed for rust 1.63. With the MSRV bump, perhaps some clean up is possible.
There was a problem hiding this comment.
Nice, yea, would be good to go back and remove what we can now.
There was a problem hiding this comment.
MIssing the hypothetical rust 'unnecessary bound' warning.
|
|
||
| /// A synchronous version of the [`WalletSource`] trait. | ||
| /// An alternative to [`CoinSelectionSourceSync`] that can be implemented and used along | ||
| /// [`WalletSync`] to provide a default implementation to [`CoinSelectionSourceSync`]. |
There was a problem hiding this comment.
Would it be an option to link in the reverse direction to not duplicate?
There was a problem hiding this comment.
We could do that, but previously we wanted to avoid that because it it felt like it implied that sync was the "main" way of doing things in Rust, and that async was just a special mode, which isn't what we wanted. I don't feel super strongly, but it did seem simple enough to just copy.
There was a problem hiding this comment.
Yes, that was the intention indeed. Hopefully the duplication comment is sufficient.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
This is all doc changes so I'm gonna go ahead and land this. |
3 participants
No-export tags and copy the docs from async to sync traits and structs so that bindings users actually have real docs.