Skip to content

0.0.116 Bindings Changes - #2434

Merged
TheBlueMatt merged 10 commits into
lightningdevkit:mainfrom
TheBlueMatt:2023-07-116-bindings-part-2
Jul 21, 2023
Merged

0.0.116 Bindings Changes#2434
TheBlueMatt merged 10 commits into
lightningdevkit:mainfrom
TheBlueMatt:2023-07-116-bindings-part-2

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

This is the remaining bindings-specific/-induced changes for 116, built on #2430.

@TheBlueMattTheBlueMatt added this to the 0.0.116 milestone Jul 20, 2023
@codecov-commenter

codecov-commenter commented Jul 20, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 63.63% and project coverage change: -0.02⚠️

Comparison is base (e4c44f3) 90.24% compared to head (27bf1d0) 90.23%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@ Coverage Diff @@## main #2434 +/- ##
==========================================
- Coverage 90.24% 90.23% -0.02% 
==========================================
Files 106 106 Lines 55774 55784 +10 Branches 55774 55784 +10 ==========================================
Hits 50334 50334 - Misses 5440 5450 +10 
Impacted FilesCoverage Δ
lightning/src/chain/mod.rs82.60% <ø> (ø)
lightning/src/ln/peer_handler.rs61.60% <ø> (ø)
lightning/src/offers/invoice.rs90.24% <ø> (ø)
lightning/src/onion_message/offers.rs8.33% <0.00%> (ø)
lightning/src/util/ser.rs84.80% <0.00%> (-0.28%)⬇️
lightning/src/events/bump_transaction.rs77.55% <33.33%> (-1.84%)⬇️
lightning/src/ln/features.rs97.12% <100.00%> (ø)
lightning/src/offers/parse.rs100.00% <100.00%> (ø)
lightning/src/sign/mod.rs83.80% <100.00%> (+0.02%)⬆️

... and 4 files with indirect coverage changes

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

@jkczyz
jkczyz self-requested a review July 20, 2023 17:08

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the commits exclusive to this PR, LGTM.

@wpaulinowpaulino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadlightning/src/sign/mod.rs
@jkczyz
jkczyz removed their request for review July 20, 2023 19:46
@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-116-bindings-part-2 branch from 33bda73 to f23dbd1CompareJuly 20, 2023 20:12
In bindings we can't practically pass a mutable PSBT, and instead
need to pass an owned transaction and have the sign method return a
signed copy. We do this here for all build modes as its not a
material API change for Rust users.
This matches what we've done for other `[u8; 32]` newtypes.
`APeerManager` will simply be mapped as `PeerManager` in bindings,
as structs are all concretized into a single form.
We missed one method that now cannot be bindings exported - the
`payment_paths` method, as it returns a slice of objects, which
cannot be supported in bindings.
The bindings are being updated to consider all traits even if the
trait itself is no-export, which causes issues generating code
around the `Duration` impl here.
While bindings should probably be able to figure out that this is
the same type as `Self`, for now we simply swap the type to make
the bindings generator have an easier go of it.
Notably including `Hash`/`Eq`/`Ord` for structs which may be used
in maps.
@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-116-bindings-part-2 branch from f23dbd1 to 27bf1d0CompareJuly 20, 2023 21:43
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Merged the dependent PR so went ahead and rebased to remove the dependent commits.

@TheBlueMatt
TheBlueMatt merged commit a617462 into lightningdevkit:mainJul 21, 2023
Sign up for freeto 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.

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@valentinewallace