-
Notifications
You must be signed in to change notification settings - Fork 411
Trivial #3752 followups #3848
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
base: main
Are you sure you want to change the base?
Trivial #3752 followups #3848
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
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.
Interesting direct implementation of Deref
. Will carry this forward for async kv store.
Perhaps you want to do this one too #3752 (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
Double-checked this works with LDK Node.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3848 +/- ##
==========================================
- Coverage 89.93% 89.91% -0.02%
==========================================
Files 163 163
Lines 131276 131278 +2
Branches 131276 131278 +2
==========================================
- Hits 118062 118043 -19
- Misses 10529 10545 +16
- Partials 2685 2690 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Did it in #3851 |
Because LDK generally relies on traits passed via `Deref`, its tempting to just wrap implementations in `Arc` even when we don't otherwise need access to the implementation later. This does result in an unnecessary allocation, which is one-time and not a huge deal but irritates my OCD substantially. Here we drop the unnecessary `Arc` indirections in favor of simply implementing `Deref` directly on the sync wrappers used in `bump_transaction`.
5f7a934
to
2fc13d1
Compare
Oops, squash-pushed a rustfmt fix. |
Pretty thorough work |
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
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.
Ah, actually, rustfmt
is unhappy here, too.
Just remove some unnecessary clones