-
Notifications
You must be signed in to change notification settings - Fork 407
ChannelManager+Router++ Logger Arc --> Deref #622
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
ChannelManager+Router++ Logger Arc --> Deref #622
Conversation
Oop.. I'll rebase tmrw |
920f09b
to
e7eb342
Compare
Rebased! |
Codecov Report
@@ Coverage Diff @@
## master #622 +/- ##
==========================================
- Coverage 91.26% 91.22% -0.05%
==========================================
Files 35 35
Lines 20787 20675 -112
==========================================
- Hits 18972 18861 -111
+ Misses 1815 1814 -1
Continue to review full report at Codecov.
|
@@ -1144,7 +1137,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> { | |||
/// | |||
/// Note that it is still possible to hit these assertions in case we find a preimage on-chain | |||
/// but then have a reorg which settles on an HTLC-failure on chain. | |||
fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>), ChannelError> { | |||
fn get_update_fulfill_htlc<L: Deref>(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>), ChannelError> where L::Target: Logger { |
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.
This (and a thousand other places) probably doesn't need to be a Deref - we pass in a reference to the thing, so might as well just dereference it at the callsite and pass in L: Logger.
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.
Hm, it looks like that causes this error: https://i.imgur.com/2wdqfaX.png And requiring Sized
would seemingly be an issue for e.g. the fuzz crate's TestLogger, since it contains a pointer to a trait object?
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, alright. Funny that rust can't figure out how to do it without the "&*", but, totally fine to leave it as-is if it doesnt work.
Oh, can you do the same for PeerManager's Arc as well? |
4eca9ba
to
fc897de
Compare
This caused a bunch of cascading changes, including passing loggers down to Channels in function calls rather than having each Channel have a pointer to the ChannelManager's Logger (which was a circular reference). Other structs that the Channel had passed its Logger to also had their loggers removed. Other newly unused Loggers were also removed, especially when keeping them would've caused a bunch of extra test changes to be necessary, e.g. with the ChainWatchInterfaceUtil's Logger.
fc897de
to
2f2ef85
Compare
2f2ef85
to
222f0cb
Compare
Done! (build failing because the code coverage upload website seems to be down) |
This caused a bunch of cascading changes, including
passing loggers down to Channels in function calls
rather than having each Channel have a pointer to the
ChannelManager's Logger (which was a circular reference).
Other structs that the Channel had passed its Logger to also
had their loggers removed. Other newly unused Loggers were
also removed, especially when keeping them would've caused
a bunch of extra test changes to be necessary, e.g. with
the ChainWatchInterfaceUtil's Logger.
Sorry, as always these PRs are massive and hard to break up. But, the changes are almost all really mechanical.