-
Notifications
You must be signed in to change notification settings - Fork 374
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
Remove next_funding_txid
tlv from Channel
read/write
#3417
Conversation
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. |
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. |
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.