refactor: qualify free-function calls and generalize public API params - #668
refactor: qualify free-function calls and generalize public API params#668varex83agent wants to merge 3 commits into
Conversation
#605) Part A — qualify free-function calls: import parent modules/types instead of importing free (standalone) functions bare, and call them qualified (module::func()). Traits, types, structs, enums and constants stay imported bare. Touches ~30 production use statements across app, cli, cluster, consensus, core, dkg, eth2api, eth2util, p2p, priority, relay-server. Part B — generalize public API parameter types for consistency within modules: - eth2util::network: valid_network / network_to_genesis_time now take impl AsRef<str> (matching sibling network_to_fork_version). - cluster::version: support_pregen_registrations / support_node_signatures now take impl AsRef<str>. - k1util::load / save now take impl AsRef<Path> (matching the rest of the workspace). - build-proto::compile_protos now takes impl AsRef<Path> instead of &str. Part C — document both conventions in the rust-style skill: import modules/types not free functions; public APIs accept impl AsRef<str>/impl AsRef<Path>/impl Into<String>/impl IntoIterator where only the borrowed/converted form is needed. No behavior change. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
There was a problem hiding this comment.
this is not consistent with above change
There was a problem hiding this comment.
Good catch — imported the serde_utils module and call serde_utils::strip_0x_prefix qualified to match the top of the file. Fixed.
There was a problem hiding this comment.
do the same thing for internal_subscriber and threshold_subscriber?
There was a problem hiding this comment.
Done — both internal_subscriber and threshold_subscriber are now called via memory:: (imported the module with self).
| name, | ||
| p2p_context::P2PContext, | ||
| utils::{ | ||
| filter_direct_quic_addrs, has_direct_quic_conn, has_direct_tcp_conn, is_quic_addr, |
There was a problem hiding this comment.
Done — imported the utils module and qualified all six helpers (has_direct_quic_conn, is_tcp_addr, is_relay_addr, etc.).
| gater::ConnGater, | ||
| p2p::{Node, NodeType}, | ||
| p2p_context::P2PContext, | ||
| peer::{MutablePeer, Peer, peer_id_from_key, verify_p2p_key}, |
There was a problem hiding this comment.
peer_id_from_key, verify_p2p_key
There was a problem hiding this comment.
Done — qualified peer_id_from_key/verify_p2p_key via the peer module here and across the other production call sites (bootnode, priority/component, dkg/node), plus peer_id_to_public_key/addr_infos_from_p2p_addrs for within-module consistency. Test-only imports left as-is.
Resolve conn_logger.rs conflict keeping the qualified name::peer_name call. Address review feedback: qualify remaining free-function calls (strip_0x_prefix, internal_subscriber/threshold_subscriber, quic_upgrade utils helpers, peer_id_from_key/verify_p2p_key/peer_id_to_public_key/ addr_infos_from_p2p_addrs) via their parent module. Co-Authored-By: varex83 <bohdan.ohorodnii@nethermind.io>
Co-Authored-By: varex83 <bohdan.ohorodnii@nethermind.io>
2 participants
Summary
Mechanical, no-behavior-change style sweep addressing the two convention gaps in #605.
Part A — Qualify free-function calls
Free (standalone) functions are now imported via their parent module and called qualified (
module::func()) instead of being imported bare. Types, structs, enums, constants, and traits remain imported bare (traits must stay bare for method resolution). Touched ~30 productionusestatements acrossapp,cli,cluster,consensus,core,dkg,eth2api,eth2util,p2p,priority, andrelay-server(e.g.peer_name,hash_proto,sig_to_eth2,to_0x_hex,validate_addresses,hash_typed_data,fork_version_to_chain_id,create_dist_validators, theagg_*helpers, etc.). Test-only (#[cfg(test)]) imports and publicpub usere-exports were intentionally left untouched.Part B — Generalize public API parameter types
Applied the established generic idiom consistently within each module:
eth2util::network:valid_network/network_to_genesis_timenow takeimpl AsRef<str>(matching siblingnetwork_to_fork_version).cluster::version:support_pregen_registrations/support_node_signaturesnow takeimpl AsRef<str>.k1util::load/savenow takeimpl AsRef<Path>(matching the rest of the workspace); redundant borrows at call sites cleaned up.build-proto::compile_protosnow takesimpl AsRef<Path>instead of&str.Part C — Conventions doc
Updated
.claude/skills/rust-style/SKILL.mdwith a new Imports section (import modules/types, not free functions) and expanded the Generalized Parameter Types guidance (public APIs acceptimpl AsRef<str>/impl AsRef<Path>/impl Into<String>/impl IntoIteratorwhere only the borrowed/converted form is needed; full uniformity is not the goal).Scope notes
Per the issue's Note, this stays scoped to public APIs and mixed conventions within the same module — private helpers and hot internal paths keep concrete types. Widely-established cross-crate bare imports that would cause large call-site churn without fixing an in-module inconsistency (e.g.
peer_id_from_key/verify_p2p_keyused bare in ~10 files) were left as-is to avoid over-reach.Quality gates
cargo +nightly fmt --all— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace --all-features— passingCloses #605
🤖 Generated with Claude Code