refactor(ethereum): collapse duplicate network_id->chain_id mapping#370
Merged
Conversation
token_metadata::parse_network_id hardcoded 5 mainnets (case-sensitive) while networks::network_id_to_chain_id is macro-generated from the networks! table (28 networks, case-insensitive). Two sources of truth for the same mapping is a latent drift / foot-gun: if wallet-token loading is re-enabled, a wallet declaring a valid network handled by networks! would be silently rejected by parse_network_id. Delete parse_network_id and its now-dead TokenMetadataError enum, and route the registry plus both CLI ABI-signing sites through network_id_to_chain_id. The registry caller is gated behind a commented-out TODO so its behavior is unchanged. The CLI ABI-signing paths were live callers (not anticipated by the original task): they now resolve chain ids for all 28 networks instead of only the 5 mainnets, so signing ABIs for valid non-mainnet networks works instead of erroring. This is strictly more permissive and the no-ABI path was already unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the visualsign-ethereum crate to use a single canonical network_id -> chain_id mapping (networks::network_id_to_chain_id) by removing the duplicated, narrower mapping in token_metadata, then routing registry + CLI ABI-signing call sites through the canonical mapping and updating documentation accordingly.
Changes:
- Removed
token_metadata::parse_network_idand itsTokenMetadataError+ associated unit tests. - Updated
ContractRegistry::load_chain_metadatato usenetworks::network_id_to_chain_idand mapNoneinto the registry’s existingStringerror. - Updated CLI ABI signing (ABI mappings + proxy ABI mappings) and docs to reference the canonical
networks.rstable/mapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/chain_parsers/visualsign-ethereum/src/token_metadata.rs | Removes duplicate network-id parsing API/tests and updates docs to point at the canonical mapping. |
| src/chain_parsers/visualsign-ethereum/src/registry.rs | Uses network_id_to_chain_id for wallet ChainMetadata loading. |
| src/chain_parsers/visualsign-ethereum/src/cli_plugin.rs | Uses network_id_to_chain_id for ABI-signing chain id resolution. |
| src/chain_parsers/visualsign-ethereum/CLAUDE.md | Updates documentation to reference networks.rs as the canonical mapping source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
prasanna-anchorage
approved these changes
Jun 15, 2026
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 had two separatenetwork_id -> chain_idmappings that could silently drift apart.networks::network_id_to_chain_idis macro-generated from thenetworks!table: 28 networks, case-insensitive.token_metadata::parse_network_idwas a hand-written match on 5 mainnets (ETHEREUM / POLYGON / ARBITRUM / OPTIMISM / BASE), case-sensitive.Two sources of truth for the same mapping is the smell. No live divergence today (the registry caller is gated behind a commented-out TODO), but if wallet-token loading is ever re-enabled, a wallet declaring a valid network handled by
networks!would be silently rejected byparse_network_id. The case-sensitivity gap is its own foot-gun.What am I changing?
parse_network_idfromtoken_metadata.rs, plus the now-deadTokenMetadataErrorenum (its only consumer, including the unusedHashErrorvariant) and the 7 tests that only covered the 5 hardcoded networks.networks.rsalready has full coverage of all 28.registry::ContractRegistry::load_chain_metadatathroughnetworks::network_id_to_chain_id, mapping theOption<u64>into the registry's existingStringerror.cli_plugin.rsABI-signing call sites (ABI mappings + proxy ABI mappings) tonetwork_id_to_chain_id, and rewrite the stale comment that defended the old narrow 5-mainnet behavior.CLAUDE.mddoc references to point at the canonicalnetworks.rstable.Files:
token_metadata.rs,registry.rs,cli_plugin.rs,CLAUDE.md(all undersrc/chain_parsers/visualsign-ethereum/).What is the Linear ticket?
N/A. Small internal refactor (single source of truth for the network mapping).
What are the rollback steps?
Revert the single commit on this branch. No migrations, no data changes, no config changes, no new dependencies.
Is this change backwards compatible?
Production gRPC path: yes, unchanged. The only registry caller (
load_chain_metadata) is reached from a commented-out TODO block inlib.rs, so it is effectively dead and its behavior does not change in production.One intentional behavior change in the CLI (called out deliberately, since the original task scope did not anticipate the
cli_plugin.rscallers): ABI signing now resolves chain ids for all 28 networks instead of only the 5 mainnets. So--abi-json-mappings/--abi-proxy-mappingswith a valid non-mainnet network (e.g. a testnet) now succeeds instead of erroring out. This is strictly more permissive; the no-ABI CLI path was already unaffected.The chain_id <-> ABI-signature binding is preserved (the security audit confirmed the resolved chain_id is identical for the 5 prior mainnets and that cross-network signature replay remains impossible).
Does this require cross-team/service coordination?
No. No proto changes, no shared API changes, no deployment config touched. The deleted symbols (
parse_network_id,TokenMetadataError) have no references outside this crate.How do I know it works as designed? Which tests exercise this code?
cargo +1.88.0 fmt --check(fromsrc/): clean.cargo +1.88.0 clippy -p visualsign-ethereum --all-targets -- -D warnings: clean.cargo +1.88.0 test -p visualsign-ethereum: green. 265 unit + 9 integration + 2 doc-tests, 0 failed.grep -rn "parse_network_id\|TokenMetadataError"acrosssrc/: no references remain.network_id_to_chain_idtests innetworks.rs.