Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

Just remove some unnecessary clones

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 12, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tnull tnull self-requested a review June 12, 2025 13:22
joostjager
joostjager previously approved these changes Jun 12, 2025
Copy link
Contributor

@joostjager joostjager left a 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.

@joostjager
Copy link
Contributor

Perhaps you want to do this one too #3752 (comment)

tnull
tnull previously approved these changes Jun 12, 2025
Copy link
Contributor

@tnull tnull left a 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.

Copy link

codecov bot commented Jun 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.91%. Comparing base (217a5b0) to head (5f7a934).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt
Copy link
Collaborator Author

Perhaps you want to do this one too #3752 (comment)

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`.
@TheBlueMatt TheBlueMatt dismissed stale reviews from tnull and joostjager via 2fc13d1 June 12, 2025 20:05
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-3752-clones branch from 5f7a934 to 2fc13d1 Compare June 12, 2025 20:05
@TheBlueMatt
Copy link
Collaborator Author

Oops, squash-pushed a rustfmt fix.

@TheBlueMatt TheBlueMatt requested review from joostjager and tnull June 12, 2025 20:05
@joostjager
Copy link
Contributor

Perhaps you want to do this one too #3752 (comment)

Did it in #3851

Pretty thorough work

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@tnull tnull left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants