Make AttributionData actually pub since its used in the public API#4268
Conversation
|
I've assigned @joostjager as a reviewer! |
`AttributionData` is a part of the public `UpdateFulfillHTLC` and `UpdateFailHTLC` messages, but its not actually `pub`. Yet again re-exports bite us and leave us with a broken public API - we ended up accidentally sealing `AttributionData`. Instead, here, we just make `onion_utils` `pub` so that we avoid making the same mistake in the future. Note that this still leaves us with arather useless public `AttributionData` API - it can't be created, updated, or decoded, it can only be serialized and deserialized, but at least it exists.
TheBlueMatt
force-pushed
the
2025-12-dont-seal-structs
branch
from
70be158 to
bd57823
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4268 +/- ##
=======================================
Coverage 89.33% 89.33%
=======================================
Files 180 180
Lines 139302 139300 -2
Branches 139302 139300 -2
=======================================
+ Hits 124439 124450 +11
+ Misses 12241 12219 -22
- Partials 2622 2631 +9
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:
|
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Why not make the AttributionData API more useful by making more public?
General comment is that there are a few changes bundled in one commit that would be easier to review if separated. Nothing complicated happening of course here.
| pub(crate) mod channel; | ||
|
|
||
| pub(crate) mod onion_utils; | ||
| pub mod onion_utils; |
There was a problem hiding this comment.
One commit where just this and making everything inside onion_utils pub(crate) would have been a nice intermediate step.
There was a problem hiding this comment.
Yea, it grew organically 😂
|
👋 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. |
I didn't think too hard about it, but looked through the existing API and it all really seemed to be internal utilities that don't make sense public. I'd love to expose a useful public API but changing the interface seemed out of scope for a PR just trying to fix our public API which is currently broken. |
|
Backported in #4344 |
3 participants
AttributionDatais a part of the publicUpdateFulfillHTLCandUpdateFailHTLCmessages, but its not actuallypub. Yet again re-exports bite us and leave us with a broken public API - we ended up accidentally sealingAttributionData.Instead, here, we just make
onion_utilspubso that we avoid making the same mistake in the future.Note that this still leaves us with arather useless public
AttributionDataAPI - it can't be created, updated, or decoded, it can only be serialized and deserialized, but at least it exists.Best reviewed with
--color-moved --color-moved-ws=ignore-space-change.