[0.0.123-bindings] Bindings changes for 0.0.123 #3062
Merged
Conversation
This file contains 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
Now that `ChannelId` has useful constructors and methods we need to start exposing it to bindings users rather than relying on using `[u8; 32]`.
This avoids the bindings trying to figure out what a `lightning::prelude::Vec` is rather than matching it as a `Vec`.
`Amount` is less than two pointers long, so there's no reason it shouldn't be `Copy`. Further, because its an enum, bindings can't map a reference to it in an `Option`. Thus, here, we simply make it `Copy` and return it in `Option`s rather than a reference to it.
TheBlueMatt
force-pushed
the
2024-04-123-bindings
branch
from
62fce5a to
7ac5b46
Compare
This is required for bindings as passing types from Rust to GC'd languages can't map the concept of a type that has a lifetime of the called function but instead needs to clone for safety.
This is required for GC'd languages which need to be able to clone in order to pass owned objects to Rust.
TheBlueMatt
force-pushed
the
2024-04-123-bindings
branch
from
7ac5b46 to
3ef8211
Compare
This matches our normal API semantics and allows, for example, `Arc`s to `EntropySource`s.
TheBlueMatt
force-pushed
the
2024-04-123-bindings
branch
from
3ef8211 to
e7e551c
Compare
jkczyz
reviewed
May 14, 2024
| @@ -34,6 +34,8 @@ use core::ops::Deref; | |||
|
|
|||
| use crate::prelude::*; | |||
|
|
|||
| use core::ops::Deref; | |||
There was a problem hiding this comment.
Little confused about 4cab9cb. The message doesn't seem to match the change and then it is reverted in the next commit.
There was a problem hiding this comment.
Oh oops its just an old commit that didn't get dropped during the rebase, dropped.
| /// | ||
| /// Thus, this method should be polled regularly to detect messages await such a direct | ||
| /// connection. | ||
| fn get_and_clear_connections_needed(&self) -> Vec<(PublicKey, Vec<SocketAddress>)>; |
There was a problem hiding this comment.
Should we have this return Vec<Event> given #2973 expands the type of events returned?
There was a problem hiding this comment.
We could, indeed, but we've done this for a release already so I'd kinda rather we just do it in the next one?
| pub fn as_array(&self) -> &[u8; PUBLIC_KEY_SIZE] { | ||
| pub fn as_array(&self) -> &[u8; 33] { |
There was a problem hiding this comment.
Is this needed? Seems unrelated to the rest of the commit.
There was a problem hiding this comment.
Yes, the bindings doesn't know how to parse constants out of CONSTANTS :/
While there's not really much harm in requiring a `Clone`able reference (they almost always are), it does make our bindings struggle a bit as they don't support multi-trait bounds (as it would require synthesizing a new C trait, which the bindings don't do automatically). Luckily, there's really no reason for it, and we can just call the `DefaultMessageRouter` directly when we want to route a message.
...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.
`OnionMessageHandler`s are expected to regularly release a set of nodes which we need to directly connect to to deliver onion messages. In order to do so, they currently extend `EventsProvider`, returning that set as `Event::ConnectionNeeded`s. While this works fine in Rust, the `EventsProvider` interface doesn't map well in bindings due to it taking a flexible trait impl as a method argument. Instead, here, we convert `OnionMessageHandler` to include a single function which returns nodes in the form of a `node_id` and `Vec<SocketAddress>`. This is a bit simpler, if less flexible, API, and while largely equivalent, is easier to map in bindings.
As we cannot express slices without inner references in bindings wrappers.
...as we currently struggle with `AChannelManager` on it
TheBlueMatt
force-pushed
the
2024-04-123-bindings
branch
from
e7e551c to
4f8f9b4
Compare
|
Dropped commits that totaled empty, no diff. |
jkczyz
approved these changes
May 14, 2024
TheBlueMatt
merged commit 9b523d9
into
lightningdevkit:0.0.123-bindings
2 of 16 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Bindings specific changes, including #3061.