Skip to content

Minor code cleanups in chainmonitor.rs #3869

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

Merged
merged 2 commits into from
Jun 18, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 17, 2025

I've assigned @jkczyz 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.

@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz June 17, 2025 15:37
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-3847-followups branch from a8a5e6b to 6559164 Compare June 17, 2025 16:08
@valentinewallace
Copy link
Contributor

CI is sad

In c356070 we ran `rustfmt` on all
of `chainmonitor.rs`, but left a few expressions in
`test_async_ooo_offchain_updates` exceedingly vertical (with
redundant code across both `c_bindings` compile modes).

Here we reduce redundant code to simplify the expressions, reducing
verticality as a side effect.
In functional tests we're slowly moving towards using `node_*_id`
variables to access node ids rather than writing out
`nodes[*].node.get_our_node_id()` each time. This somewhat improves
readability by reducing cognitive load, so is generally nice to do
as we go through rustfmt-induced changes anyway.

Here we make the swap in `chainmonitor.rs` functional tests.
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-3847-followups branch from 6559164 to 3a862a3 Compare June 17, 2025 21:54
@TheBlueMatt
Copy link
Collaborator Author

Ugh, sorry, forgot to let the script run long enough locally:

$ git diff-tree -U2 6559164dbb 3a862a3f97
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index 326eaeb784..46ede6fd85 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -1317,5 +1317,5 @@ mod tests {
 		#[cfg(c_bindings)]
 		let pending_chan_updates =
-			pending_updates.iter().find(|(chan_id, _)| *chan_id == channel_id).unwrap().1;
+			&pending_updates.iter().find(|(chan_id, _)| *chan_id == channel_id).unwrap().1;
 		assert!(pending_chan_updates.contains(&next_update));

@@ -1328,5 +1328,5 @@ mod tests {
 		#[cfg(c_bindings)]
 		let pending_chan_updates =
-			pending_updates.iter().find(|(chan_id, _)| *chan_id == channel_id).unwrap().1;
+			&pending_updates.iter().find(|(chan_id, _)| *chan_id == channel_id).unwrap().1;
 		assert!(!pending_chan_updates.contains(&next_update));

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

Going ahead and landing this since the diff since Val's ACK is trivial.

@tnull tnull merged commit 1898755 into lightningdevkit:main Jun 18, 2025
28 checks passed
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