chore(ethereum): forbid HashMap/HashSet via clippy disallowed-types#298
Draft
pepe-anchor wants to merge 1 commit into
Draft
chore(ethereum): forbid HashMap/HashSet via clippy disallowed-types#298pepe-anchor wants to merge 1 commit into
pepe-anchor wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enforces deterministic collection iteration within the visualsign-ethereum crate by disallowing std::collections::HashMap/HashSet via Clippy and migrating internal usage to BTreeMap/BTreeSet, reducing the risk of non-deterministic rendered SignablePayload JSON.
Changes:
- Add crate-local
clippy.tomlwithdisallowed-typesrules banningHashMap/HashSet. - Migrate production code and tests from
HashMap/HashSettoBTreeMap/BTreeSet, including addingOrdderives where required. - Update test fixtures to perform boundary conversions into generated-proto
HashMapfields via.into_iter().collect().
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/chain_parsers/visualsign-ethereum/clippy.toml | Adds Clippy disallowed-types rules to forbid HashMap/HashSet crate-wide for determinism. |
| src/chain_parsers/visualsign-ethereum/src/abi_registry.rs | Migrates ABI registry storage to BTreeMap under Arc for deterministic ordering. |
| src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs | Updates test fixtures to build with BTreeMap and collect into proto HashMap at the boundary. |
| src/chain_parsers/visualsign-ethereum/src/visualizer.rs | Migrates visualizer registries from HashMap to BTreeMap. |
| src/chain_parsers/visualsign-ethereum/src/token_metadata.rs | Switches ChainMetadata.assets to BTreeMap and updates tests accordingly. |
| src/chain_parsers/visualsign-ethereum/src/registry.rs | Migrates multiple registries to BTreeMap and derives Ord for WellKnownAddress; updates tests/sets. |
| src/chain_parsers/visualsign-ethereum/src/networks.rs | Migrates uniqueness tests from HashSet to BTreeSet. |
| src/chain_parsers/visualsign-ethereum/tests/lib_test.rs | Updates integration test metadata construction to use BTreeMap and collect into proto HashMap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+25
| 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." }, | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why am I making this PR?
The
visualsign-ethereumcrate usedHashMap/HashSetin production and test code. Iteration order over these maps is non-deterministic across runs, which is a silent correctness risk: the renderedSignablePayloadJSON feeds downstream hashing and wallet display, so any map that leaks iteration order into output breaks determinism. This mirrors the same fix already applied tovisualsign-solanain commit b723208.What am I changing?
src/chain_parsers/visualsign-ethereum/clippy.tomlwithdisallowed-typesrules forstd::collections::HashMapandstd::collections::HashSet. The rule is crate-wide intentionally (see comment in the file).visualsign-ethereum/src/fromHashMap/HashSettoBTreeMap/BTreeSet:abi_registry.rs,registry.rs,token_metadata.rs,visualizer.rs,networks.rs.PartialOrd/Ordderives toWellKnownAddress(required byBTreeMapkey constraint).abi_metadata.rsandtests/lib_test.rs. Boundary conversions at the proto FFI point (where generated code still usesHashMap) use.into_iter().collect()with the destination type inferred.Also included in this branch (already merged as #294):
ci(stagex)versioned TVC summary + surfpool gitignore.What is the Linear ticket?
N/A (internal chore, mirrors b723208 on
prasanna/clippy-disallow-hashmap-in-solana-presets)What are the rollback steps?
Delete
src/chain_parsers/visualsign-ethereum/clippy.tomland revert theBTreeMap/BTreeSetrenames back toHashMap/HashSetin the 7 changed source files. No data migrations, no config changes, no deployed state to unwind.Is this change backwards compatible?
Yes.
BTreeMapandBTreeSetsatisfy the same public interfaces as theirHash*counterparts at every call site. No exported types changed shape. TheChainMetadata.assetsfield type changed fromHashMaptoBTreeMapin the Rust struct, but that struct is internal to this crate and not part of any serialized wire format.Does this require cross-team/service coordination?
No.
How do I know it works as designed? Which tests exercise this code?
203 tests pass (199 unit + 4 lib integration):
Clippy clean (no
disallowed_typesviolations remain):Format clean:
Co-authored with Claude.
Generated with Claude Code