feat(near): add a parser_cli token-metadata flag for NEAR - #437
Conversation
b9b8c2d to
40c3f20
Compare
40c3f20 to
485a961
Compare
There was a problem hiding this comment.
Peer Review Summary — NEAR stack #437 (COMMENT)\n\n5 findings: 5 LOW. Detailed inline comments below.\n\nKey concern: @ in file paths breaks mapping parsing. Dev-signing edge cases.
AI Review on behalf of @pepe-anchor. Please flag any inaccuracies.
| return Err(format!( | ||
| "Invalid mapping format (expected Name@FilePath@AssetId): {mapping_str}" | ||
| )); | ||
| }; |
There was a problem hiding this comment.
[LOW] @ in file paths breaks Name@FilePath@AssetId parsing because splitn(3, '@') consumes the path component greedily, and the error message doesn't surface which component is the problem
parse_near_mapping uses splitn(3, '@') which splits on the first two @ characters regardless of position. A file path containing @ (valid on Linux/macOS) causes mis-parsing: /tmp/my@dir/token.json yields name='/tmp/my', path='dir/token.json', asset_id='<rest>' instead of a clear error. The format is documented as Name@FilePath@AssetId and the help text expects @/path/to/file.json@AssetId (implying absolute paths with a leading @ that disambiguates), but a relative path with an embedded @ may still confuse users — consider adding a validation step that rejects a path component containing @ and points to the third @ as the intended separator.
Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.
There was a problem hiding this comment.
Good catch, fixed in f21237c -- and the mis-parse was silent, which made it worse than a bad error message.
parse_near_mapping now splits on every @ and requires exactly three components. A NEAR Intents asset id never contains @, so a fourth component can only mean the path did, and the error says so: it names the component count and states that a path containing @ can't be used in this format.
Before this, MyToken@/tmp/my@dir/token.json@nep141:wrap.near parsed as path /tmp/my and asset id dir/token.json@nep141:wrap.near with no complaint at all -- it would then fail as a missing file, pointing at the wrong thing. Test parse_near_mapping_rejects_an_at_sign_in_the_path pins that exact string.
On the leading-@ reading of the help text: the @ there is the separator, not part of the path, so absolute and relative paths are both fine -- it's only an embedded @ that's unrepresentable.
| asset_id, | ||
| value, | ||
| &DEV_NEAR_SIGNING_KEY_SEED, | ||
| visualsign::signing::near_token_metadata_prehash, |
There was a problem hiding this comment.
[LOW] DEV_NEAR_SIGNING_KEY_SEED is a deterministic all-0x51 seed — consistent with DEV_ETHEREUM_SIGNING_KEY_SEED (0x52) and DEV_SOLANA_SIGNING_KEY_SEED (0x53) but worth re-confirming these are intentionally non-random for dev-only use and would never reach a production enclave binary
The seed [0x51u8; 32] is hardcoded, gated behind #[cfg(any(test, feature = "dev-signing"))], and allowlisted only under the same cfg. The split-build Makefile strategy prevents unification, so parser_app never links the key. However, if anyone changes the build to a single cargo build --workspace invocation (e.g. for a CI optimization), the key and its allowlist entry would silently leak into the production enclave — there's no compile-time assertion or build-script check that dev-signing is absent from the parser_app/grpc-server dependency graph. Consider adding a compile-time guard (e.g. a compile_error! in a non-dev-signing parser_app module that depends on visualsign-near but is #[cfg(not(dev_signing))]) or a CI-only check in narrow-build-check.
Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.
There was a problem hiding this comment.
Confirmed non-random and dev-only, and you're right that nothing enforced it -- added a CI gate in f21237c.
narrow-build-check now runs dev-signing-absent-check, which fails if any --workspace recipe in src/Makefile lacks --exclude parser_cli. That's the actual invariant: Cargo unifies features within a single invocation, so the only thing keeping dev-signing off parser_app/grpc-server is that build compiles parser_cli separately. A future cargo build --workspace covering both now fails the build rather than quietly linking a dev key into an attested binary.
Worth recording why it isn't the graph probe you suggested, since I tried that first: cargo tree -p parser_app -e features does not print enabled features that carry no dependency edges, and dev-signing = [] is exactly that. The probe matched nothing even against parser_cli, which does enable it -- so it would have passed unconditionally and looked like coverage. I verified the replacement fires by injecting a violating recipe.
It also covers visualsign-ethereum/dev-signing, which has the same exposure and predates NEAR.
| } | ||
| }; | ||
| if NearNetwork::from_network_id(&network).is_none() { | ||
| return Err(format!( |
There was a problem hiding this comment.
[LOW] create_chain_metadata returns Ok(None) when all mappings fail to load, but test_cli_near_token_metadata_invalid_file_still_parses only covers the file-not-found case — add a test for a file that exists but contains invalid JSON (e.g. {) to ensure the serde_json parse failure path in load_json_file is exercised
load_json_file returns Err(...) when serde_json::from_str::<Value> fails (invalid JSON). The CLI test test_cli_near_token_metadata_invalid_file_still_parses uses /nonexistent/token.json which exercises the File::open error path but not the serde_json parse error path. A separate test with a real but syntactically invalid JSON file would close this coverage gap — important because the error message shape (Invalid JSON in file {path}: ...) is user-visible and worth snapshotting.
Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.
There was a problem hiding this comment.
Agreed, added in f21237c. build_token_mappings_skips_a_file_whose_json_is_invalid writes a real file containing {, so File::open succeeds and the failure comes from serde_json in load_json_file -- the branch the existing /nonexistent/token.json test can't reach.
Asserts both halves of the outcome: the returned map is empty and valid_count is 0, so a future change that logged the error but still registered the entry would fail here.
| "near", | ||
| "--output", | ||
| "json", | ||
| "--near-token-metadata-mappings", |
There was a problem hiding this comment.
[LOW] CLI integration tests exercise the happy path and file-not-found error, but not the dev-signing-disabled error path — add a test with an intentionally disabled feature to catch the regression where sign_token_metadata_for_cli returns Err and the entry loads unsigned then gets silently dropped
The two new CLI tests (test_cli_near_token_metadata_mappings and test_cli_near_token_metadata_invalid_file_still_parses) cover happy-path resolution and file-not-found. But neither exercises what happens when dev-signing is absent: sign_token_metadata_for_cli returns Err(...), build_token_mappings_from_files inserts the entry unsigned, and RequireAllowlistedSigner drops it. A #[cfg(not(feature = "dev-signing"))] test (or a dedicated test binary without the feature) that asserts the entry is dropped (symbol stays unresolved) would prevent a future regression where the feature gate is accidentally removed or broken.
Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.
Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.
There was a problem hiding this comment.
Added in 2834460 -- and my first read of this was wrong, so worth spelling out why it is testable.
You're right that #[cfg(not(feature = "dev-signing"))] doesn't work from the crate's own test module: the gate is cfg(any(test, feature = "dev-signing")), so under cargo test the signing variant is always linked and its Err arm is unreachable. But an integration test compiles the library without cfg(test), so with dev-signing off the error-returning twin at token_signature.rs:646 is what links -- no separate test binary needed.
tests/unsigned_without_dev_signing.rs drives the real public path (NearPlugin::create_metadata with a mapping file) and asserts the entry registers with signature: None and its value carried verbatim -- i.e. unsigned rather than dropped, which is the half nothing else covered. I checked it isn't vacuous two ways: it reports 1 passed rather than being silently cfg-ed out, and with the gate temporarily removed and --features dev-signing on it fails against a real ed25519 signature.
make test already runs it in the right configuration -- the workspace pass excludes parser_cli, the only crate that enables dev-signing, so unification can't switch it back on.
Scoped deliberately to the unsigned-registration half. Asserting the subsequent drop would need #439's TokenMetadataExtraction API, which doesn't exist on this PR -- and that behaviour is already covered by token_signature.rs's tests, which don't depend on this feature.
485a961 to
2834460
Compare
2834460 to
1d5dfb5
Compare
prasanna-anchorage
left a comment
There was a problem hiding this comment.
Reviewed the parser_cli NEAR token-metadata flag, the @-delimited mapping parser, and the dev-key signing/allowlist wiring (dev-signing feature gating for cli_plugin/parser_cli, parser_app carrying neither dev-signing nor diagnostics). Checked the colon-in-asset-id regression test and the --network composition logic. All correct.
One non-blocking finding: build_token_mappings_from_files (with parse_near_mapping) in src/chain_parsers/visualsign-near/src/cli_plugin.rs:232 hand-reimplements the generic load/parse/sign/dedupe/count/log loop that parser_cli_core::mapping_parser::load_mappings already provides for Ethereum's build_abi_mappings_from_files, instead of generalizing that shared helper to accept a pluggable parse function/delimiter. Not a bug today, but a future fix to load_mappings's dedup/logging behavior won't propagate to this copy, and there's now no signal to a maintainer that a second copy exists.
Approving — worth a follow-up to generalize load_mappings, doesn't need to block this PR.
554918f to
69baa9f
Compare
parser_cli has no way to supply NEAR token metadata, unlike Ethereum (--abi-json-mappings, signed with the CLI dev key) or Solana (--idl-json-mappings, unsigned only). Assets outside the compiled-in seed table therefore render as raw base units tagged `unresolved <asset id>` with no local override available. - --near-token-metadata-mappings takes `Name@/path/to/token.json@AssetId`. `@` is the field separator, not `:` (the convention the other two mapping flags use), because NEAR Intents asset ids embed their own colons (nep141:wrap.near), which would make the identifier ambiguous under a colon-delimited format. - Each loaded entry is signed with the CLI's local dev key (NEAR-origin ed25519). The CLI installs the strict RequireAllowlistedSigner posture, so an unsigned entry is dropped; signing is what makes the flag do anything. This follows Ethereum's fuller template rather than Solana's unsigned-only one. - authorized_token_metadata_signers enrolls that dev key under `dev-signing`/`cfg(test)`, matching visualsign-ethereum's authorized_abi_signers. parser_cli's `near` feature enables visualsign-near/dev-signing, as its `ethereum` feature already does for visualsign-ethereum. parser_app enables neither, so the enclave binary still carries no key material. - sign_token_metadata_for_cli is decoupled from the `dev-signing` feature the same way sign_abi_for_cli is, so cli_plugin compiles in a cli-plugin-without-dev-signing build. - CLI-signed entries are always NEAR-origin (origin_chain unset); Ethereum/Solana-origin CLI signing is not wired up. The flag composes with --network rather than replacing it: an invalid network still errors before any mapping file is read, and metadata is emitted when either input yields something. Tests: 11 plugin-level cases (composition with --network, the colon-in-asset-id regression, partial-failure handling) plus an end-to-end case proving a CLI-signed entry resolves through the posture `register` installs -- the gate that fails if the dev key leaves the allowlist or the domain tag drifts. Two parser_cli tests drive the real binary, so clap exposure and the dev-signing feature wiring are covered too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…enclave parse_near_mapping used splitn(3, '@'), so a path containing '@' -- legal on Linux/macOS -- was absorbed into the asset id: "MyToken@/tmp/a@b/t.json@ID" parsed as path "/tmp/a" and asset id "b/t.json@ID" with no complaint. It now splits on every '@' and reports which component is at fault, since an asset id never contains one. narrow-build-check gains dev-signing-absent-check. dev-signing carries hardcoded signing-key seeds and allowlists them; only parser_cli enables it, and the sole thing keeping it off parser_app/grpc-server is that `build` compiles parser_cli in a separate cargo invocation. Nothing enforced that, so a --workspace recipe covering both now fails the build. Asserted against the recipes, not the feature graph: `cargo tree -e features` omits enabled features carrying no dependency edges, and `dev-signing = []` is one, so a graph probe matches nothing and passes regardless. Adds the coverage gap for a file that exists but holds invalid JSON, which exercises load_json_file's serde_json branch rather than File::open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sign_token_metadata_for_cli is gated on cfg(any(test, feature = "dev-signing")), so the crate's own tests always link the signing variant and its Err arm is unreachable there. An integration test compiles the library without cfg(test), so with dev-signing off the error-returning twin links instead -- the configuration a shipped binary has. Asserts the half nothing else covered: an entry registers unsigned, value carried verbatim, rather than being dropped when signing is unavailable. What happens to an unsigned entry afterwards is already covered by token_signature.rs's tests, which don't depend on this feature. make test runs it in the right shape: the workspace pass excludes parser_cli, the only crate enabling dev-signing, so feature unification cannot switch it back on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
69baa9f to
20ad19b
Compare
prasanna-anchorage
left a comment
There was a problem hiding this comment.
Re-approving. My 2026-08-25 approval was dismissed by the stale-review rule, but unlike the rebase-only cases in this stack this PR gained two real commits since, so I re-read the whole thing rather than re-issuing.
The two post-approval commits
b4dfbcce — the @ fix is correct, and it's a real bug. splitn(3, '@') did absorb a path's @ into the asset id silently: MyToken@/tmp/my@dir/token.json@nep141:wrap.near parsed as path /tmp/my and asset id dir/token.json@nep141:wrap.near. Splitting on every @ and rejecting a fourth component is the right call, the error message names the actual fault ("found N components, expected 3 ... A file path containing '@' cannot be used here"), and parse_near_mapping_rejects_an_at_sign_in_the_path pins it against exactly the string that used to pass.
I checked the load-bearing assumption rather than taking it on faith: "an asset id never contains one" holds for everything this crate resolves — grep nep245\|nep171 across visualsign-near/src returns nothing, and tokens.rs is entirely nep141:. Worth keeping in mind if multi-token ids ever reach token_mappings, since a NEP-245 token id is contract-defined and #460 already names mt_withdraw intents; not a concern today.
b4dfbcce — the dev-signing gate. The reasoning in the commit message is right, including the part that's easy to get wrong: cargo tree -e features really does omit enabled features that carry no dependency edges, and dev-signing = [] is exactly that, so a graph probe would have passed vacuously. Asserting on the recipes instead is a reasonable call.
I verified the surface it's guarding:
- The guard passes clean on the current Makefile, and the three real
--workspacerecipes (lines 35, 44, 72) all carry--exclude parser_cli. narrow-build-checkis in CI (main.yml:64), so this actually runs.- No
cargo ... --workspaceinvocation exists anywhere outsidesrc/Makefile— I grepped the workflows, shell scripts and Containerfiles. - The shipping binary was never at risk regardless:
images/parser_app/Containerfile:32builds from/src/parser/appwith--no-default-features --features "vsock ${CHAIN_FEATURES}", andparser_app'snear = ["dep:visualsign-near"]doesn't pulldev-signingthe wayparser_cli's does. - The
insert_near_dev_signercfg pair is the right shape — a real fn undercfg(any(test, feature = "dev-signing"))and a no-op undercfg(not(...))— so with the feature off the dev key is genuinely never enrolled, not merely unused.
20ad19b3 — the integration test earns its place. The header explains precisely why it can't live in the crate's test module: sign_token_metadata_for_cli is gated on cfg(any(test, feature = "dev-signing")), so under unit tests the signing variant always links and its Err arm is unreachable. An integration test compiles the library without cfg(test), so with dev-signing off the error-returning variant is what links — the configuration a shipped binary has. That's the distinction most people miss, and the #![cfg(all(feature = "cli-plugin", not(feature = "dev-signing")))] gate makes it self-disabling rather than silently vacuous when the feature is on.
make lint and make test green locally (40 suites, 1689 tests, both exit 0). I also confirmed the gate doesn't make it vacuous — without_dev_signing_the_cli_registers_the_entry_unsigned ... ok appears in the run, so it really does execute under make test rather than being cfg'd out, along with parse_near_mapping_rejects_an_at_sign_in_the_path, build_token_mappings_skips_a_file_whose_json_is_invalid and test_cli_near_token_metadata_mappings.
Non-blocking
1. The guard is anchored to lines that literally start with cargo. ^[[:space:]]*cargo (build|test|check|clippy).*--workspace misses any recipe with a prefix — RUSTFLAGS=... cargo build --workspace, time cargo build --workspace, cd foo && cargo build --workspace. It also false-positives on a line-continued recipe, which I confirmed:
cargo build --workspace \
--exclude parser_cli
flags as bad even though it's correct. That direction errs safe, so neither is urgent.
If you want the version that can't be routed around, assert on the resolved feature rather than on recipe text — it then holds for the Containerfile and for a hand-run cargo too:
// visualsign-near/src/lib.rs
pub const DEV_SIGNING_ENABLED: bool = cfg!(feature = "dev-signing");
// parser/app/src/main.rs
#[cfg(feature = "near")]
const _: () = assert!(
!visualsign_near::DEV_SIGNING_ENABLED,
"dev-signing must never resolve in parser_app"
);That's a compile error at the moment unification happens, rather than a lint on one file's text. Happy either way — the grep is a fine stopgap and the shipped path is already safe.
2. The two new tests hand-roll fixed-name temp dirs, while this same PR uses the collision-safe helper elsewhere. cli_test.rs (added here) calls write_temp_json("vsp_cli_tests", ...), whose doc comment says filenames "include the PID and a nanosecond timestamp to avoid collisions when tests run in parallel" and which also checks the resolved path stays inside the temp dir. But cli_plugin.rs:462 uses env::temp_dir().join("vsp_near_cli_plugin_invalid_json") and unsigned_without_dev_signing.rs:32 uses .join("vsp_near_no_dev_signing"), both ending in remove_dir_all.
Within one make test they don't collide — the five cargo invocations are sequential and the two names differ. The exposure is a concurrent run on the same machine (a developer running cargo test -p visualsign-near while make test is going): one test's remove_dir_all deletes the other's file mid-read. parser_cli_core::test_utils::write_temp_json is pub and already reachable from both call sites, so this is a two-line change if you want it.
3. The original non-blocking finding stands, and this round is mild evidence for it: parse_near_mapping still reimplements parser_cli_core::mapping_parser::load_mappings rather than generalizing it over the delimiter. The @-in-path bug is exactly the class of thing a single shared splitter gets right once instead of twice.
Adds the NEAR token-metadata flag
parser_cliwas missing, closing the CLIparity gap with Ethereum (
--abi-json-mappings) and Solana(
--idl-json-mappings).Depends on #432 (
near-d-token-metadata), which adds theChainMetadata.neartoken-metadata plumbing this flag feeds.Based on #463 rather than #432 directly. That is merge sequencing, not a
dependency: #463 only adds seed entries and touches no file this PR does, and it
sits lower because it is ready to merge while this one is still in review.
The gap
NearPlugin::create_metadataaccepts only--network. An asset outside thecompiled-in seed table renders its raw base-unit amount tagged
unresolved <asset id>, with no way to supply a symbol and decimals locally:The flag
Two things differ from the sibling flags:
@separates the fields, not:. NEAR Intents asset ids embed their owncolons (
nep141:wrap.near), so the shared colon-delimitedmapping_parser::parse_mappingwould truncate the id at its first embeddedcolon.
parse_near_mappingsplits on@into exactly three parts and takesthe asset id verbatim. A regression test pins this.
Each entry is signed with the CLI dev key. The NEAR plugin installs the
strict
MetadataTrustPolicy::RequireAllowlistedSignerposture, carrying theenv-configured curator allowlist the decode path checks against, so an entry
the parser cannot attribute to a curator is dropped —
signing is what makes the flag do anything at all. This follows Ethereum's
fuller template (mappings + signing) rather than Solana's unsigned-only one.
Three pieces make that work:
sign_token_metadata_for_cliis decoupled from thedev-signingcargofeature the same way
sign_abi_for_cliis, socli_pluginstill compilesin a
cli-plugin-without-dev-signingbuild (verified explicitly).authorized_token_metadata_signersenrolls that dev key underdev-signing/cfg(test), matchingvisualsign-ethereum'sauthorized_abi_signers. Without it the CLI would sign entries its owndecode path then rejects as an untrusted signer.
parser_cli'snearfeature enablesvisualsign-near/dev-signing, as itsethereumfeature already does forvisualsign-ethereum.parser_appenables neitherdev-signingnordiagnostics, so the enclavebinary carries no key material and no allowlist entry trusting the dev key.
make buildsplitsparser_cliout of the workspace build to keep Cargofeature unification from crossing that line, and the release image builds from
parser/appalone; this PR extends the Makefile comment enumerating thosefeatures, which no longer listed all of them.
CLI-signed entries are always NEAR-origin (
origin_chainunset).Ethereum/Solana-origin CLI signing is not wired up.
Composition with
--networkThe flag composes with
--networkrather than replacing it. An invalidnetwork still errors before any mapping file is read, so a bad
--networkcan't be masked by a successful mapping load, and metadata is emitted when
either input yields something.
Noneis returned only when neither does.--networkalso decides what the signatures are valid for. A token-metadatasignature is scoped to one NEAR network, so entries are signed for the network
the parser will resolve for the same request: the flag's value when given,
otherwise the network the converter defaults to.
--network NEAR_TESTNETtherefore produces testnet-scoped entries, and the same mappings signed for one
network do not verify against the other.
Coverage
--networkin both directions, thecolon-in-asset-id regression, duplicate asset ids, and partial failure
(a malformed mapping and a missing file alongside a good one).
registerinstalls. This is the gate that fails if the dev key leaves theallowlist or the signing domain tag drifts — assertions on
signature.is_some()and on unsigned-entry refusal both pass in that case.parser_clitests drive the real binary, so clap exposure and thedev-signingfeature wiring are covered, not just directNearArgsconstruction.
docs/parser-cli.mdx(whose--chainrow also didnot list
near) and a worked example indocs/chains/near.mdx, both runagainst the built binary before being written down.
Known follow-up, raised in review and not addressed here:
build_token_mappings_from_filesreimplements the load/parse/sign/dedupe/count/log loop that
parser_cli_core::mapping_parser::load_mappingsprovides forEthereum, rather than generalizing that helper to take a pluggable delimiter. A
fix to
load_mappings's dedup or logging will not reach this copy.make lintandmake testare green across the workspace.🤖 Generated with Claude Code