[0.2-bindings] 0.2 bindings-specific changes #4264
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Various new structs in 0.2 were missing `Clone` and `Copy` derives, which we add here.
The new `OffersMessageFlow` stores yet another `Secp256k1` context internally, but to initialize requires passing one in explicitly. We call this in `ChannelManager` by `clone`ing the context we have before passing it in, allowing us to keep the randomization we do in `ChannelManager::new`. This isn't really worth the API annoyance of passing in a `Secp256k1` context, though, and luckily the contexts are eventually going to go away upstream eventually, so we just drop the argument from `OffersMessageFlow::new`, building an unrandomized `Secp256k1` context internally instead.
...as the bindings generation does not currently have the ability to map a reference to a `NodeId` inside a tuple.
Bindings can't handle references in return types, so reduce the visibility to pub(crate).
The bindings currently get confused by the implicit `Sign` type, so we temporarily remove it on the `impl` here.
Re-exports in Rust make `use` statements a little shorter, but for otherwise don't materially change a crate's API. Sadly, the C bindings generator currently can't figure out re-exports, but it also exports everything into one global namespace, so it doesn't matter much anyway.
Having struct fields with references to other structs is tough in our bindings logic, but even worse if the fields are in an enum. Its simplest to just take the clone penalty here.
The scorer currently relies on an associated type for the fee parameters. This isn't supportable at all in bindings, and for lack of a better option we simply hard-code the parameter for all scorers to `ProbabilisticScoringFeeParameters`.
These are not expressible in C/most languages, and thus must be hidden.
Rather than `ChannelMonitor` only being clonable when the signer is clonable, we require all signers to be clonable and then make all `ChannelMonitor`s clonable.
As we cannot express slices without inner references in bindings wrappers.
Bindings don't accept dyn traits, but instead map any traits to a single dynamic struct. Thus, we can always take a specific trait to accept any implementation, which we do here.
Mapping an `Hmac<Sha256>` would require somewhat custom logic as we'd have to behave differently based on generic parameters, so its simplest to just swap it to a `[u8; 32]` instead.
The bindings really should support this, but currently they don't and its late enough in the release cycle I don't want to try to fix that.
We don't expect anyone to access `Bolt11Bech32` directly in most use-cases, and the bindings don't support mapping an enum that cannot be constructed (not to mention there would be no point), so we mark it as no-export in bindings builds.
In 0.2 we added some places where `Clone` bounds on traits provided additional features. The bindings, of course, cannot capture such optionality as traits have to be concretized first. Thus, we add the `Clone` bounds explicitly on the traits themselves.
We use `MaybeSend` + `MaybeSync` in rust to only enable `Send`+`Sync` bounds if built with `std`, but there's no reason to bother with this in bindings, even for `no_std` builds. As the bindings generator doesn't currently have logic to identify traits like these as "synonyms" for `Send`+`Sync`, we just use `Send`+`Sync` explicitly.
Sadly its not currently possible to map `lightning-liquidity` to bindings as it requires additional (`Clone`) bounds on various trait objects passed in to the top-level manager constructor. Because we don't anticipate bindings users using it immediately, we simply drop it here.
This is used in bindings for the `Address` constructor, and needs to exist in the (public) source tree to ensure bindings derives it.
|
I've assigned @jkczyz as a reviewer! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sadly lightning-liquidity doesn't work in bindings anymore, but I don't think there's immediate users of it so its just dropped.