Uh oh!
There was an error while loading. Please reload this page.
Rustfmt chainmonitor.rs - #3847
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
TheBlueMatt
commented
Jun 12, 2025
Presumably we should just format all of |
joostjager
commented
Jun 12, 2025
Happy to do that. I just wanted to be conservative and only format the parts that I touch. |
df28592 to
d0e501aComparejoostjager
commented
Jun 12, 2025
Pushed full rustfmt'ed chainmonitor |
valentinewallace
left a comment
There was a problem hiding this comment.
I'm fine to do these in follow-up if you want to Just Land
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.monitors | ||
| .read() | ||
| .unwrap() | ||
| .iter() |
There was a problem hiding this comment.
nit: extract into var
There was a problem hiding this comment.
Based on CI failures, looks like you might have to extract only self.monitors.read().unwrap()
| match super::channelmonitor::process_events_body!( | ||
| self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor), self.logger, ev, handler(ev).await) { | ||
| self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor), | ||
| self.logger, | ||
| ev, | ||
| handler(ev).await | ||
| ) { |
There was a problem hiding this comment.
I'd prefer:
for channel_id in mons_to_process {letmut ev;let monitor = self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor);let res = process_events_body!(monitor,self.logger, ev, handler(ev).await);match res {Ok(()) => {},Err(ReplayEvent()) => {self.event_notifier.notify();},}}with importing process_events_body
There was a problem hiding this comment.
This starts to complain about borrow from a temporary value. Left as is, this is formatting only?
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Jun 12, 2025
Certainly if you don't want to bother cleaning things up we can just land a subset that doesn't need it and come back to it later, I was just suggesting it probably wasn't much effort to clean up as we went given the file's size. |
joostjager
commented
Jun 12, 2025
Right. Yes, the clean up is kind of subjective, so wanted to avoid that. But will now go with @valentinewallace's suggestions above. |
ab6d893 to
449591aComparejoostjager
commented
Jun 13, 2025
Rebased |
449591a to
3242b93Compareldk-reviews-bot
commented
Jun 14, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Gonna go ahead and land but will clean things up a bit more in a followup.
| #[cfg(not(c_bindings))] | ||
| assert!(nodes[1].chain_monitor.chain_monitor.list_pending_monitor_updates().get(&channel_id) | ||
| .unwrap().contains(&next_update)); | ||
| assert!(nodes[1] |
There was a problem hiding this comment.
Probably
worth
making
this
less
vertical
:)
| check_closed_event!(&nodes[2], 1, ClosureReason::CommitmentTxConfirmed, false, | ||
| [nodes[0].node.get_our_node_id()], 1000000); | ||
| let closure_reason = ClosureReason::CommitmentTxConfirmed; | ||
| let node_a_id = nodes[0].node.get_our_node_id(); |
There was a problem hiding this comment.
Probably worth making this change across the two functional tests, given it somewhat reduces cognitive load when reading tests.
I badly miss rustfmt for my changes in #3778.
The larger strategy for rustfmt is still being discussed (#3749, #3809), but getting this one with immediate benefit through would be great.