From 5ed51a4bb630953598c88a5a96f4dfc48f8358a6 Mon Sep 17 00:00:00 2001 From: "pepe.figueira" Date: Wed, 13 May 2026 20:37:38 +0000 Subject: [PATCH] chore(ethereum): forbid HashMap/HashSet via clippy disallowed-types Mirrors the in-flight Solana branch (b723208) for visualsign-ethereum. Adds `src/chain_parsers/visualsign-ethereum/clippy.toml` enforcing `disallowed-types` for `std::collections::HashMap` and `HashSet`, and migrates all internal usage to BTreeMap/BTreeSet. Rationale: iteration order over any map that ends up in rendered SignablePayload output must be stable across runs. HashMap's randomized hasher silently breaks this. Forbidding HashMap crate-wide prevents the pattern from being reintroduced. Boundary conversions (2 sites): abi_metadata.rs test helper and lib_test.rs each return BTreeMap and collect into the proto field's HashMap at the call site (`into_iter().collect()`), keeping BTreeMap internally and only converting at the FFI boundary. Verified: cargo clippy -p visualsign-ethereum --all-targets -D warnings clean, cargo fmt clean, 203 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../visualsign-ethereum/clippy.toml | 25 +++++++++++++++++ .../visualsign-ethereum/src/abi_metadata.rs | 24 ++++++++++++---- .../visualsign-ethereum/src/abi_registry.rs | 10 +++---- .../visualsign-ethereum/src/networks.rs | 6 ++-- .../visualsign-ethereum/src/registry.rs | 28 +++++++++---------- .../visualsign-ethereum/src/token_metadata.rs | 6 ++-- .../visualsign-ethereum/src/visualizer.rs | 8 +++--- .../visualsign-ethereum/tests/lib_test.rs | 9 ++++-- 8 files changed, 78 insertions(+), 38 deletions(-) create mode 100644 src/chain_parsers/visualsign-ethereum/clippy.toml diff --git a/src/chain_parsers/visualsign-ethereum/clippy.toml b/src/chain_parsers/visualsign-ethereum/clippy.toml new file mode 100644 index 000000000..d0f4b1c88 --- /dev/null +++ b/src/chain_parsers/visualsign-ethereum/clippy.toml @@ -0,0 +1,25 @@ +# Determinism enforcement for visualsign-ethereum. +# +# The rendered SignablePayload JSON is consumed downstream for hashing and +# wallet display, so iteration order over any map that ends up in the output +# MUST be stable across runs. HashMap/HashSet's randomized hasher silently +# breaks this. +# +# Use BTreeMap/BTreeSet instead. The performance delta is negligible at the +# sizes we use these maps (typically <100 entries) and the determinism +# guarantee is worth the trade. +# +# This rule is intentionally crate-wide rather than scoped to one module -- +# even maps that look "lookup-only" today (e.g. the AbiRegistry's internal +# maps, the ContractRegistry's lookup tables) can leak iteration order via +# debug formatting, error messages, or future code paths that walk them. +# Forcing BTreeMap everywhere keeps the determinism story simple and +# review-friendly: no `#[allow(clippy::disallowed_types)]` escape hatches in +# the codebase. +# +# Mirror of visualsign-solana/clippy.toml (commit b723208). + +disallowed-types = [ + { path = "std::collections::HashMap", reason = "use BTreeMap -- iteration order affects rendered SignablePayload output and breaks deterministic hashing. See clippy.toml comment." }, + { path = "std::collections::HashSet", reason = "use BTreeSet for the same reason as HashMap." }, +] diff --git a/src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs b/src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs index eccfabf37..7acdd477f 100644 --- a/src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs +++ b/src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs @@ -372,7 +372,9 @@ mod tests { const TEST_ADDRESS: &str = "0xdAC17F958D2ee523a2206206994597C13D831ec7"; - fn make_abi_mappings(entries: Vec<(&str, Abi)>) -> std::collections::HashMap { + /// Builds the test fixture as a `BTreeMap` (crate determinism rule) and lets each + /// call site `.into_iter().collect()` into the proto field's `HashMap`. + fn make_abi_mappings(entries: Vec<(&str, Abi)>) -> std::collections::BTreeMap { entries .into_iter() .map(|(addr, abi)| (addr.to_string(), abi)) @@ -418,7 +420,9 @@ mod tests { value: VALID_ABI.to_string(), signature: None, }, - )]), + )]) + .into_iter() + .collect(), })), }; let registry = @@ -475,7 +479,9 @@ mod tests { value: VALID_ABI.to_string(), signature: Some(proto_sig), }, - )]), + )]) + .into_iter() + .collect(), })), }; let registry = @@ -509,7 +515,9 @@ mod tests { value: VALID_ABI.to_string(), signature: None, }, - )]), + )]) + .into_iter() + .collect(), })), }; // Invalid entries are skipped; with no valid entries left, result is None @@ -527,7 +535,9 @@ mod tests { value: "not valid json".to_string(), signature: None, }, - )]), + )]) + .into_iter() + .collect(), })), }; // Invalid ABI JSON is skipped; with no valid entries left, result is None @@ -555,7 +565,9 @@ mod tests { signature: None, }, ), - ]), + ]) + .into_iter() + .collect(), })), }; // The valid entry should be registered; the invalid one skipped diff --git a/src/chain_parsers/visualsign-ethereum/src/abi_registry.rs b/src/chain_parsers/visualsign-ethereum/src/abi_registry.rs index b7c73c5e7..535afdefa 100644 --- a/src/chain_parsers/visualsign-ethereum/src/abi_registry.rs +++ b/src/chain_parsers/visualsign-ethereum/src/abi_registry.rs @@ -8,7 +8,7 @@ //! - Performance: No file I/O or JSON parsing overhead //! - Determinism: Same binary always uses same ABIs -use std::collections::HashMap; +use std::collections::BTreeMap; use std::sync::Arc; use alloy_json_abi::JsonAbi; @@ -35,17 +35,17 @@ pub type ChainId = u64; #[derive(Clone)] pub struct AbiRegistry { /// Maps ABI name -> parsed JsonAbi - abis: Arc>>, + abis: Arc>>, /// Maps (chain_id, contract_address) -> ABI name - address_mappings: Arc>, + address_mappings: Arc>, } impl AbiRegistry { /// Creates a new empty ABI registry pub fn new() -> Self { Self { - abis: Arc::new(HashMap::new()), - address_mappings: Arc::new(HashMap::new()), + abis: Arc::new(BTreeMap::new()), + address_mappings: Arc::new(BTreeMap::new()), } } diff --git a/src/chain_parsers/visualsign-ethereum/src/networks.rs b/src/chain_parsers/visualsign-ethereum/src/networks.rs index c5b81d52d..d96367721 100644 --- a/src/chain_parsers/visualsign-ethereum/src/networks.rs +++ b/src/chain_parsers/visualsign-ethereum/src/networks.rs @@ -497,17 +497,17 @@ mod tests { #[test] fn test_constants_are_unique() { - let mut seen_ids = std::collections::HashSet::new(); + let mut seen_ids = std::collections::BTreeSet::new(); for &(chain_id, _, _, _) in ALL_NETWORKS { assert!(seen_ids.insert(chain_id)); } - let mut seen_names = std::collections::HashSet::new(); + let mut seen_names = std::collections::BTreeSet::new(); for &(_, network_id, _, _) in ALL_NETWORKS { assert!(seen_names.insert(network_id)); } - let mut seen_displays = std::collections::HashSet::new(); + let mut seen_displays = std::collections::BTreeSet::new(); for &(_, _, display, _) in ALL_NETWORKS { assert!(seen_displays.insert(display)); } diff --git a/src/chain_parsers/visualsign-ethereum/src/registry.rs b/src/chain_parsers/visualsign-ethereum/src/registry.rs index e7d384d1e..d52d71c73 100644 --- a/src/chain_parsers/visualsign-ethereum/src/registry.rs +++ b/src/chain_parsers/visualsign-ethereum/src/registry.rs @@ -1,6 +1,6 @@ use crate::token_metadata::{ChainMetadata, TokenMetadata, parse_network_id}; use alloy_primitives::{Address, utils::format_units}; -use std::collections::HashMap; +use std::collections::BTreeMap; /// Type alias for chain ID to avoid depending on external chain types pub type ChainId = u64; @@ -9,7 +9,7 @@ pub type ChainId = u64; /// /// This enum provides type safety for well-known contract addresses, /// preventing typos and enabling compile-time checks. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum WellKnownAddress { /// Permit2 contract - universal across all chains Permit2, @@ -78,24 +78,24 @@ pub trait ContractType: 'static { #[derive(Clone)] pub struct ContractRegistry { /// Maps (chain_id, address) to contract type - address_to_type: HashMap<(ChainId, Address), String>, + address_to_type: BTreeMap<(ChainId, Address), String>, /// Maps (chain_id, contract_type) to list of addresses - type_to_addresses: HashMap<(ChainId, String), Vec
>, + type_to_addresses: BTreeMap<(ChainId, String), Vec
>, /// Maps (chain_id, token_address) to token metadata - token_metadata: HashMap<(ChainId, Address), TokenMetadata>, + token_metadata: BTreeMap<(ChainId, Address), TokenMetadata>, /// Maps (well_known_address, optional_chain_id) to address /// For chain-specific addresses, use Some(chain_id); for universal addresses, use None - well_known_addresses: HashMap<(WellKnownAddress, Option), Address>, + well_known_addresses: BTreeMap<(WellKnownAddress, Option), Address>, } impl ContractRegistry { /// Creates a new empty registry pub fn new() -> Self { Self { - address_to_type: HashMap::new(), - type_to_addresses: HashMap::new(), - token_metadata: HashMap::new(), - well_known_addresses: HashMap::new(), + address_to_type: BTreeMap::new(), + type_to_addresses: BTreeMap::new(), + token_metadata: BTreeMap::new(), + well_known_addresses: BTreeMap::new(), } } @@ -550,7 +550,7 @@ mod tests { fn test_load_chain_metadata() { let mut registry = ContractRegistry::new(); - let mut assets = HashMap::new(); + let mut assets = BTreeMap::new(); assets.insert( "USDC".to_string(), create_token_metadata( @@ -715,7 +715,7 @@ mod tests { fn test_load_chain_metadata_with_invalid_addresses() { let mut registry = ContractRegistry::new(); - let mut assets = HashMap::new(); + let mut assets = BTreeMap::new(); // Valid token assets.insert( "USDC".to_string(), @@ -779,7 +779,7 @@ mod tests { let metadata = ChainMetadata { network_id: "UNKNOWN_NETWORK".to_string(), - assets: HashMap::new(), + assets: BTreeMap::new(), }; let result = registry.load_chain_metadata(&metadata); @@ -925,7 +925,7 @@ mod tests { } WellKnownAddress::Weth => { // WETH should be chain-specific - different addresses per chain - let mut addresses = std::collections::HashSet::new(); + let mut addresses = std::collections::BTreeSet::new(); for &chain_id in &test_chains { let addr = registry diff --git a/src/chain_parsers/visualsign-ethereum/src/token_metadata.rs b/src/chain_parsers/visualsign-ethereum/src/token_metadata.rs index baabd2f73..b9138fca2 100644 --- a/src/chain_parsers/visualsign-ethereum/src/token_metadata.rs +++ b/src/chain_parsers/visualsign-ethereum/src/token_metadata.rs @@ -1,6 +1,6 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; -use std::collections::HashMap; +use std::collections::BTreeMap; /// Standard for ERC token types #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)] @@ -56,7 +56,7 @@ pub struct ChainMetadata { /// Network identifier as string (e.g., "ETHEREUM_MAINNET") pub network_id: String, /// Map of token symbol to token metadata - pub assets: HashMap, + pub assets: BTreeMap, } /// Error type for token metadata operations @@ -248,7 +248,7 @@ mod tests { fn test_chain_metadata_serialization() { let mut metadata = ChainMetadata { network_id: "ETHEREUM_MAINNET".to_string(), - assets: HashMap::new(), + assets: BTreeMap::new(), }; let usdc = TokenMetadata { diff --git a/src/chain_parsers/visualsign-ethereum/src/visualizer.rs b/src/chain_parsers/visualsign-ethereum/src/visualizer.rs index 49d4ba146..fc9896161 100644 --- a/src/chain_parsers/visualsign-ethereum/src/visualizer.rs +++ b/src/chain_parsers/visualsign-ethereum/src/visualizer.rs @@ -1,5 +1,5 @@ use crate::context::VisualizerContext; -use std::collections::HashMap; +use std::collections::BTreeMap; use visualsign::AnnotatedPayloadField; use visualsign::vsptrait::VisualSignError; @@ -62,7 +62,7 @@ pub trait CalldataVisualizer: Send + Sync { /// This registry is designed to be built once and shared immutably (e.g., in an Arc). /// Use `EthereumVisualizerRegistryBuilder` to construct a registry. pub struct EthereumVisualizerRegistry { - visualizers: HashMap>, + visualizers: BTreeMap>, } impl EthereumVisualizerRegistry { @@ -92,14 +92,14 @@ impl crate::context::VisualizerRegistry for EthereumVisualizerRegistry { /// Once all visualizers are registered, call `build()` to create an immutable registry. #[derive(Default)] pub struct EthereumVisualizerRegistryBuilder { - visualizers: HashMap>, + visualizers: BTreeMap>, } impl EthereumVisualizerRegistryBuilder { /// Creates a new empty builder pub fn new() -> Self { Self { - visualizers: HashMap::new(), + visualizers: BTreeMap::new(), } } diff --git a/src/chain_parsers/visualsign-ethereum/tests/lib_test.rs b/src/chain_parsers/visualsign-ethereum/tests/lib_test.rs index cf5f784df..5e8f3b3f5 100644 --- a/src/chain_parsers/visualsign-ethereum/tests/lib_test.rs +++ b/src/chain_parsers/visualsign-ethereum/tests/lib_test.rs @@ -4,7 +4,7 @@ use alloy_primitives::U256; use alloy_rlp::Encodable; use alloy_sol_types::{SolCall, sol}; use generated::parser::{Abi, ChainMetadata, EthereumMetadata, chain_metadata::Metadata}; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::fs; use std::path::PathBuf; use visualsign::vsptrait::{VisualSignConverterFromString, VisualSignOptions}; @@ -221,7 +221,7 @@ fn test_abi_from_metadata_decodes_function() { "stateMutability": "nonpayable" }]"#; - let mut abi_mappings = HashMap::new(); + let mut abi_mappings = BTreeMap::new(); abi_mappings.insert( unknown_contract.to_string(), Abi { @@ -230,13 +230,16 @@ fn test_abi_from_metadata_decodes_function() { }, ); + // Boundary conversion: proto `EthereumMetadata.abi_mappings` is still + // `HashMap` in generated code. The crate-wide rule (clippy.toml) keeps us + // on `BTreeMap` internally and we collect at the FFI point. let options = VisualSignOptions { decode_transfers: true, transaction_name: None, metadata: Some(ChainMetadata { metadata: Some(Metadata::Ethereum(EthereumMetadata { network_id: Some("ETHEREUM_MAINNET".to_string()), - abi_mappings, + abi_mappings: abi_mappings.into_iter().collect(), })), }), developer_config: None,