Small API tweaks to make bindings easier#4186
Small API tweaks to make bindings easier#4186TheBlueMatt merged 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
TheBlueMatt
force-pushed
the
2025-10-first-bindings-pass
branch
from
7e9c8ff to
d9426a3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4186 +/- ##
==========================================
- Coverage 88.86% 88.83% -0.04%
==========================================
Files 180 180
Lines 137869 137873 +4
Branches 137869 137873 +4
==========================================
- Hits 122520 122480 -40
- Misses 12539 12581 +42
- Partials 2810 2812 +2
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:
|
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
I don't think we want to do the first commit. No strong opinions on the others, though generally not sure if it's worth the last-minute API churn.
| /// - `"x-lsps5-timestamp"`: with the timestamp in RFC3339 format (`"YYYY-MM-DDThh:mm:ss.uuuZ"`). | ||
| /// - `"x-lsps5-signature"`: with the signature of the notification payload, signed using the LSP's node ID. | ||
| /// Other custom headers may also be included as needed. | ||
| headers: HashMap<String, String>, |
There was a problem hiding this comment.
Using a HashMap here is more convenient as the go-to reqwest::HeaderMap type offers a simply TryFrom implementation. I guess the change doesn't hurt too much.
There was a problem hiding this comment.
This is a lightning::util::hash_tables::HashMap anyway, not an std::collection::HashMap :(
|
👋 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. |
While HTTP headers should be a unique K->V mapping, returning three headers to a user in an event via a `HashMap` is substantially overkill (and also not trivial to do in bindings). Instead, we expose them as a `Vec`.
`WebhookNotification` already has all fields `pub`, making its `new` constructor somewhat redundant, but also conflicting with the bindings-auto-generated `new` constructor. Thus we just drop it.
If we're already passing `AChannelManagerRef` and `ChainMonitorRef` to `can_support_additional_anchor_channel` there's no need to take them by reference.
TheBlueMatt
force-pushed
the
2025-10-first-bindings-pass
branch
from
d9426a3 to
e4512d1
Compare
|
Went ahead and dropped the integer size change for errors. |
|
Backported to 0.2 in #4193 |
3 participants
No description provided.