-
Notifications
You must be signed in to change notification settings - Fork 421
Remove next_funding_txid tlv from Channel read/write
#3417
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
Remove next_funding_txid tlv from Channel read/write
#3417
Conversation
optout21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor comment regarding the commas
5f02ff5 to
b78ec80
Compare
Thanks. Done! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3417 +/- ##
==========================================
+ Coverage 89.22% 89.24% +0.02%
==========================================
Files 130 130
Lines 106965 106960 -5
Branches 106965 106960 -5
==========================================
+ Hits 95438 95458 +20
+ Misses 8734 8709 -25
Partials 2793 2793 ☔ View full report in Codecov by Sentry. |
jkczyz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could you please include the rationale in the commit message? Better to not rely on github for this information.
We want to remove this before release so that we can work on a way to not persist this but rather get it from other persisted data and just free up the TLV. Note that the "added in 0.0.124" comment was incorrect as it was actually added in lightningdevkit#3137 but the comment was stale so it's safe to remove.
b78ec80 to
7177acb
Compare
Yeah true, thanks! Done. |
|
Can we actually just remove the field entirely and use |
Yeah that's likely what we'll do. I just wanted to remove the persistence here to keep this small. I can address removing the field entirely in #3423. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, definitely need to figure it out before 0.1, though.
Yeah, I'm adding it to #3423 which needs to go in before 0.1. |
Want to remove this before release so that we can work on a way to not persist this but rather get it from other persisted data and just free up the TLV.
Note that the "added in 0.0.124" comment was incorrect as it was actually added in #3137 but the comment was stale. So it's safe to remove.
See #3137 (comment) for context.