Skip to content

Rustfmt tests that touch persistence #3882

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 7 commits into
base: main
Choose a base branch
from

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Jun 23, 2025

Rustfmt preparatory PR for #3778.

My view on the changes is again that refactors, however trivial, are risky. Especially with large quantities of similar edits. In this PR I didn't do any touch ups for that reason.

Let me know what pressing, non-subjective formatting problems are really unacceptable. I can add a commit for those where I either change the formatting, or simpliy rustfmt::skip the section.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 23, 2025

👋 Thanks for assigning @TheBlueMatt 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.

@joostjager joostjager marked this pull request as ready for review June 23, 2025 11:20
@joostjager joostjager requested a review from TheBlueMatt June 23, 2025 11:20
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

A few patterns worth cleaning up imo across the diff.


{
let msgs = nodes[0].node.get_and_clear_pending_msg_events();
assert!(msgs.is_empty(), "Expected no message events; got {:?}", msgs);
}

nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &channel_id_0, SignerOp::GetPerCommitmentPoint);
nodes[0].enable_channel_signer_op(
&nodes[1].node.get_our_node_id(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to go ahead and do the global search+replace for nodes[n].node.get_our_node_id -> node_n_id (and (\t*)(let nodes = create_network.*) -> \1\2\n\1let node_n_id = nodes[n].node.get_our_node_id)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added commit with script in commit message

nodes[0].node.handle_accept_channel(nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
nodes[0].node.handle_accept_channel(
nodes[1].node.get_our_node_id(),
&get_event_msg!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we pull the get_event_msg calls out into variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the node ids extracted, this wrapping doesn't seem to be happening anymore.

src.node
.peer_connected(
dst.node.get_our_node_id(),
&msgs::Init {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you pull the Init message out into a variable (and reuse it in the second connect below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Second one isn't the same.

check_closed_event(
&nodes[1],
1,
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we break this out into a reason? Also this is gonna be annoying to rebase #3881, can we do this after that lands?

Copy link
Contributor Author

@joostjager joostjager Jun 24, 2025

Choose a reason for hiding this comment

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

The delta in that PR is tiny. Seems the lowest level of annoying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted this reason. Not very necessary in my opinion.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Script used:

git reset --hard

for file in lightning/src/ln/async_signer_tests.rs lightning/src/ln/functional_test_utils.rs lightning/src/ln/priv_short_conf_tests.rs lightning/src/ln/shutdown_tests.rs; do
    sed -i '' -E \
    -e 's/nodes\[0\]\.node.get_our_node_id\(\)/node_a_id/g' \
    -e 's/nodes\[1\]\.node.get_our_node_id\(\)/node_b_id/g' \
    -e 's/nodes\[2\]\.node.get_our_node_id\(\)/node_c_id/g' \
    -e 's/nodes\[3\]\.node.get_our_node_id\(\)/node_d_id/g' \
    -e 's/nodes\[4\]\.node.get_our_node_id\(\)/node_e_id/g' \
    -e 's/nodes\[5\]\.node.get_our_node_id\(\)/node_f_id/g' \
    -e 's/(^.*create_network\(2, .*\);)/\1\nlet node_a_id = nodes\[0\].node.get_our_node_id\(\);\nlet node_b_id = nodes\[1\].node.get_our_node_id\(\);/g' \
    -e 's/(^.*create_network\(3, .*\);)/\1\nlet node_a_id = nodes\[0\].node.get_our_node_id\(\);\nlet node_b_id = nodes\[1\].node.get_our_node_id\(\);\nlet node_c_id = nodes\[2\].node.get_our_node_id\(\);/g' \
    -e 's/(^.*create_network\(4, .*\);)/\1\nlet node_a_id = nodes\[0\].node.get_our_node_id\(\);\nlet node_b_id = nodes\[1\].node.get_our_node_id\(\);\nlet node_c_id = nodes\[2\].node.get_our_node_id\(\);\nlet node_d_id = nodes\[3\].node.get_our_node_id\(\);/g' \
    -e 's/(^.*create_network\(5, .*\);)/\1\nlet node_a_id = nodes\[0\].node.get_our_node_id\(\);\nlet node_b_id = nodes\[1\].node.get_our_node_id\(\);\nlet node_c_id = nodes\[2\].node.get_our_node_id\(\);\nlet node_d_id = nodes\[3\].node.get_our_node_id\(\);\nlet node_e_id = nodes\[4\].node.get_our_node_id\(\);/g' \
    -e 's/(^.*create_network\(6, .*\);)/\1\nlet node_a_id = nodes\[0\].node.get_our_node_id\(\);\nlet node_b_id = nodes\[1\].node.get_our_node_id\(\);\nlet node_c_id = nodes\[2\].node.get_our_node_id\(\);\nlet node_d_id = nodes\[3\].node.get_our_node_id\(\);\nlet node_e_id = nodes\[4\].node.get_our_node_id\(\);\nlet node_f_id = nodes\[5\].node.get_our_node_id\(\);/g' \
    "$file"
done

cargo +1.63.0 fmt
@joostjager joostjager force-pushed the rustfmt-async-kv-store branch 2 times, most recently from e82e183 to 2d084d8 Compare June 24, 2025 11:39
@joostjager joostjager force-pushed the rustfmt-async-kv-store branch from 2d084d8 to 2c2415e Compare June 24, 2025 11:44
@joostjager joostjager requested a review from TheBlueMatt June 24, 2025 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants