Skip to content

Conversation

@TheBlueMatt
Copy link
Collaborator

Our Trusted* wrappers in chan_utils expose additional inner
fields by reference. However, because they were not explicitly
marked as returning a reference with the wrapped struct's
lifetimes, rustc was considering them to return a reference with
the wrapper struct's lifetime.

This is unnecessarily restrictive, and resulted in the addition of
a clone in 9850c58 which we remove
here.

Our `Trusted*` wrappers in `chan_utils` expose additional inner
fields by reference. However, because they were not explicitly
marked as returning a reference with the wrapped struct's
lifetimes, rustc was considering them to return a reference with
the wrapper struct's lifetime.

This is unnecessarily restrictive, and resulted in the addition of
a clone in 9850c58 which we remove
here.
/// The transaction ID of the built Bitcoin transaction
pub fn txid(&self) -> Txid {
self.inner.built.txid
}
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this apply to Txid?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its not returned by reference, its returned by copy, so there's no need to add the lifetime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I meant its equivalent to clone right ? 😅
But yeah since its already like that, nbd i guess.

@G8XSU
Copy link
Contributor

G8XSU commented Sep 15, 2023

Otherwise, LGTM.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 91.66% and project coverage change: -0.01% ⚠️

Comparison is base (89fb5a3) 90.63% compared to head (53c8f89) 90.63%.
Report is 10 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2582      +/-   ##
==========================================
- Coverage   90.63%   90.63%   -0.01%     
==========================================
  Files         113      113              
  Lines       59054    59057       +3     
  Branches    59054    59057       +3     
==========================================
+ Hits        53522    53524       +2     
- Misses       5532     5533       +1     
Files Changed Coverage Δ
lightning/src/ln/chan_utils.rs 95.03% <85.71%> (ø)
lightning/src/chain/channelmonitor.rs 94.59% <100.00%> (-0.06%) ⬇️
lightning/src/chain/onchaintx.rs 90.95% <100.00%> (+0.04%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

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

lgtm

/// The transaction ID of the built Bitcoin transaction
pub fn txid(&self) -> Txid {
self.inner.built.txid
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I meant its equivalent to clone right ? 😅
But yeah since its already like that, nbd i guess.

@TheBlueMatt TheBlueMatt merged commit f97d520 into lightningdevkit:main Sep 18, 2023
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.

5 participants