Skip to content

Minor Tweaks to lightning-invoice for C bindings #908

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

This + lightningdevkit/ldk-c-bindings#19 correctly builds the new invoice construction utility method for C (+Java).

The C bindings generator now looks to default generic types as the
way to map a struct or enum parameter. Because SignOrCreationError
is only used directly with an error type of `()`, we set that to
the default and assume no other error types are needed.
... due to A and A aliasing each other.
@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #908 (54c97ee) into main (e26c3df) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 54c97ee differs from pull request most recent head 83ab933. Consider uploading reports for the commit 83ab933 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
+ Coverage   90.50%   90.51%   +0.01%     
==========================================
  Files          59       59              
  Lines       29624    29624              
==========================================
+ Hits        26810    26813       +3     
+ Misses       2814     2811       -3     
Impacted Files Coverage Δ
lightning-invoice/src/de.rs 80.99% <ø> (ø)
lightning-invoice/src/lib.rs 87.59% <ø> (ø)
lightning-invoice/src/ser.rs 92.13% <ø> (ø)
lightning/src/ln/functional_tests.rs 96.86% <0.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e26c3df...83ab933. Read the comment docs.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Code Review ACK 54c97ee

While this is less readable, I spent way too long trying to adapt
the bindings generation code to handle glob imports and concluded
it would take refactoring almost the entire import-resolution
logic. While this may be a good refactor to do eventually, its
probably not worth it today.
RawHrp is already not-exported, so implementations for it should be
as well.
@TheBlueMatt TheBlueMatt force-pushed the 2021-04-invoice-real-bindings branch from 54c97ee to 83ab933 Compare May 1, 2021 00:37
@TheBlueMatt
Copy link
Collaborator Author

Squashed one fixup commit without diff. Will merge after CI

$ git diff-tree -U3 54c97ee3 83ab933f
$

@TheBlueMatt TheBlueMatt merged commit d4d3225 into lightningdevkit:main May 1, 2021
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.

4 participants