-
Notifications
You must be signed in to change notification settings - Fork 377
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
Cut 0.0.119 #2794
Cut 0.0.119 #2794
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2794 +/- ##
==========================================
- Coverage 88.43% 88.42% -0.01%
==========================================
Files 114 114
Lines 91806 91718 -88
Branches 91806 91718 -88
==========================================
- Hits 81187 81101 -86
- Misses 8128 8134 +6
+ Partials 2491 2483 -8 ☔ 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.
The CHANGELOG looks great!
There are a few PRs that were not mentioned in CHANGELOG. These PRs were either refactoring to internal functions, updates to log, or improvements to test, and hence no need to be mentioned in CHANGELOG.
List of unmentioned PRs
(not tagging them to prevent unnecessary referencing)
2613, 2669, 2693, 2668, 2686, 2682, 2641, 2558, 2702, 2732, 2726, 2746, 2741, 2512, 2755, 2739, 2766, 2769, 2771, 2642, 2764, 2773, 2762, 2765, 2691, 2776, 2774, 2782, 2786, 2637, 2787, 2703, 2790, 2792
I think out of these PRs, there are two that can require a CHANGELOG.
2693 Trait requirements for a pub fn are altered here. And I believe it can use a CHANGELOG.
2741 Fixure of the double-back routing bug here can use a CHANGELOG.
Fair, though the trait requirements are strictly relaxed. All existing code should compile just fine.
I'm not convinced. LDK will never generate such a route, and while users can, those routes might actually work (or might not, which in my case was true). We should reject them, but in general users should never see this rejection, and its really just a helpful notice to catch bugs in user code (which presumably they would have already fixed, if they had them). |
96abdd8
to
bef425a
Compare
CHANGELOG.md
Outdated
* Since 0.0.116, sending payments which required data in the onion for the | ||
recipient which was too large for the onion may have caused corruption which | ||
resulted in payment failure (#2752). |
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.
Which "which" do we want to keep 😂
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.
Neither: "that ... and ..."
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.
Went with "requiring...which" which seemed easier since
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.
Drive-by nits
CHANGELOG.md
Outdated
time-fetch function, and `ScoreUpDate` methods now take the current time as a | ||
`Duration` argument. This avoids fetching time during pathfinding (#2656). | ||
* Receiving payments to multi-hop blinded paths is now supported (#2688). | ||
* `MessageRouter` and `Router` now feature methods to blinded paths back to the |
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.
s/to/to create
CHANGELOG.md
Outdated
* Since 0.0.116, sending payments which required data in the onion for the | ||
recipient which was too large for the onion may have caused corruption which | ||
resulted in payment failure (#2752). |
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.
Neither: "that ... and ..."
bef425a
to
bee5b68
Compare
Should be good to go now 🎉 |
bee5b68
to
c6e4deb
Compare
Oops, sorry, missed @jkczyz's review. Addressed. |
CI Passed, multiple folks looked at it. |
Still need to finalize some comments, plus any new PRs, and bump crate versions.