refactor: unify 0x/0X hex-prefix handling across chains#365
refactor: unify 0x/0X hex-prefix handling across chains#365shahan-khatchadourian-anchorage wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes 0x/0X hex-prefix handling in visualsign::encodings and updates chain parsers (Ethereum/Solana/Sui/Tron) to route hex decoding through the shared helpers, ensuring consistent prefix acceptance and fixing SupportedEncodings::detect() to classify 0x…-prefixed inputs as hex.
Changes:
- Add shared helpers in
visualsign::encodingsfor optional/required hex prefix handling plus adecode_hexconvenience function, with unit tests. - Update chain-specific hex decoding paths to use
visualsign::encodings::decode_hex(andsplit_hex_prefixwhere prefix is required). - Promote
hexto a non-dev dependency ofvisualsignand remove now-unneeded per-cratehexdependency usage (e.g., Sui), with corresponding docs updates.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/visualsign/src/encodings.rs | Adds shared prefix helpers, updates detect() to account for 0x/0X, and adds unit tests. |
| src/visualsign/Cargo.toml | Promotes hex to a normal dependency (workspace-managed). |
| src/chain_parsers/visualsign-tron/src/lib.rs | Uses visualsign::encodings::decode_hex for hex transaction decoding. |
| src/chain_parsers/visualsign-sui/src/core/transaction/decoder.rs | Uses shared hex decoding to accept optional 0x/0X prefixes. |
| src/chain_parsers/visualsign-sui/Cargo.toml | Drops direct hex dependency (now inherited via visualsign). |
| src/chain_parsers/visualsign-solana/src/idl/signature.rs | Delegates fixed-size hex decode to visualsign::encodings::decode_hex. |
| src/chain_parsers/visualsign-solana/src/core/visualsign.rs | Uses shared hex decode for Solana transaction wrapper hex path. |
| src/chain_parsers/visualsign-ethereum/src/lib.rs | Uses shared hex decode for raw transaction hex decoding. |
| src/chain_parsers/visualsign-ethereum/src/eth_json.rs | Uses split_hex_prefix for mandatory-prefix JSON-RPC hex fields. |
| src/chain_parsers/visualsign-ethereum/src/cli_plugin.rs | Uses shared prefix helpers for address validation/normalization. |
| src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs | Uses shared hex decode for signature/public-key and canonical pubkey decoding. |
| src/chain_parsers/visualsign-ethereum/CLAUDE.md | Documents the unified hex/prefix convention for Ethereum module. |
| src/Cargo.lock | Reflects dependency graph changes (e.g., Sui no longer directly depends on hex). |
| CLAUDE.md | Documents repo-wide unified hex/0x handling guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add `decode_hex`, `strip_hex_prefix`, and `split_hex_prefix` to `visualsign::encodings` as the single definition of how the parser accepts a hex prefix, and route every chain's hex inputs through them instead of hand-rolling prefix stripping per crate. - visualsign::encodings: add the three helpers; `detect()` now treats a leading `0x`/`0X` as hex (the `x` is stripped before the digit test). - ethereum: ABI signature/public-key decode, address validate/normalize, `require_hex_prefix`, and raw-transaction decode. - solana: IDL signature decode and `SolanaTransactionWrapper::from_string` hex arm, so prefixed Solana hex transactions decode consistently with the other chains. - sui/tron: raw-transaction decode; drop the now-unused per-crate `hex` dep. - docs: record the unified hex/`0x` convention in the CLAUDE.md files. Closes #364 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shahan-khatchadourian-anchorage
force-pushed
the
shahankhatch/364-unify-0x-hex-prefix
branch
from
3a90c33 to
9c36529
Compare
prasanna-anchorage
left a comment
prasanna-anchorage
left a comment
There was a problem hiding this comment.
Clean, well-scoped de-duplication — collapsing the copy-pasted 0x/0X stripping into three tested helpers is the right move, and it incidentally fixes Solana's prefixed-hex path and the 0X gap in Ethereum/Tron from_string. Approving. A few nits inline, plus two non-code notes:
- PR description nit: it says "sui / tron — drop the now-unused per-crate
hexdependency," but tron correctly keepshex(still useshex::encodeatlib.rs:164/169/424). Only sui dropped it. Code is right; just fix the description. - Consistency nit:
visualsign-tron/Cargo.toml:17still pinshex = "0.4.3"while solana was switched to{ workspace = true }— worth aligning tron too while touching deps.
| /// the test). | ||
| pub fn detect(data: &str) -> Self { | ||
| if data.chars().all(|c| c.is_ascii_hexdigit()) { | ||
| if strip_hex_prefix(data) |
There was a problem hiding this comment.
nit: this PR makes detect() the sole router (the old Ethereum/Tron code force-routed any 0x prefix to Hex unconditionally), which slightly widens the heuristic's ambiguity: a Base64 string that starts with 0x/0X and whose remainder is all hex digits of even length now classifies as Hex. It's a pre-existing property (an all-hex Base64 string was already mis-detected), negligible for real ~1KB txs, and the consequence is a downstream decode error rather than a silent mis-parse. Worth a one-line note in the doc comment that classification is best-effort, since this is now the single decision point.
| SupportedEncodings::Hex => { | ||
| hex::decode(data).map_err(|e| TransactionParseError::DecodeError(e.to_string()))? | ||
| } | ||
| SupportedEncodings::Hex => visualsign::encodings::decode_hex(data) |
There was a problem hiding this comment.
nit: the headline behavioral win here is chain-level — a 0x-prefixed Solana tx now decoding where it previously failed — but the new tests live only in encodings.rs at the helper level. Consider one end-to-end assertion (SolanaTransactionWrapper::from_string with a 0x prefix) so a future detect() refactor can't silently re-break this path.
Summary
Centralizes
0x/0Xhex-prefix acceptance intovisualsign::encodingsand routes every chain's hex inputs through it, replacing per-crate hand-rolled prefix stripping. Closes #364.New helpers in
visualsign::encodings(the single definition of how the parser accepts a hex prefix):strip_hex_prefix(&str) -> &str— optional0x/0X, case-insensitivesplit_hex_prefix(&str) -> Option<&str>— for sites where the prefix is mandatory (returnsNoneto turn into a caller error)decode_hex(&str) -> Result<Vec<u8>, hex::FromHexError>— strip + decodeSupportedEncodings::detect()now treats a leading0x/0Xas hex (thexis stripped before the ASCII-hex-digit test).Per-chain changes
require_hex_prefix, and raw-transaction decode.SolanaTransactionWrapper::from_stringhex arm so0x-prefixed Solana hex transactions decode consistently with the other chains (previously the only chain whose raw-tx path couldn't accept a prefix).hexdependency.0xconvention in the root and ethereumCLAUDE.md.Behavior note
detect()previously classified0x…-prefixed input as Base64 (thexisn't an ASCII hex digit); it now correctly classifies it as Hex. This makes prefixed hex transactions decode rather than fail.Test plan
cargo fmt— no changescargo clippy --all-targets -- -D warnings— clean (also enforced by pre-commit across all workspaces)cargo testfor visualsign + all four chain crates — pass (648 tests across unit + integration suites)encodings.rscover prefix presence/absence/case, mandatory-prefix, decode, and thedetect()classification.🤖 Generated with Claude Code