visualsign: nothing guarantees the SignablePayload is derived from intermediate_output

[prasanna-anchorage]

Problem

intermediate_output is the machine-parseable description of a transaction that a downstream policy engine evaluates. It is borsh-appended into the signed digest (parser/app/src/routes/parse.rs:147), so a consumer that verifies the signature also commits to it.

That makes one property load-bearing:

The human-readable SignablePayload and the machine-readable intermediate_output must describe the same transaction.

Nothing enforces it. VisualSignConverter<T>::to_visual_sign_payload returns ConversionResult { payload, intermediate_output: Option<Vec<u8>> }, and the two fields are computed independently. A converter is free to derive them from different decoders, or to omit the intermediate entirely. Today the property is upheld by convention plus a single drift test on one Solana fixture.

If they diverge, a user approves one thing while policy evaluates another — for a signing control, that is the failure mode the control exists to prevent.

Evidence this is not hypothetical

• Solana shipped with two independent decoders over the same bytes. The payload came from instructions::decode_transfers → parse_transaction; the intermediate from extract_solana_intermediate_output → parse_transaction_with_idls. perf(solana): make intermediate_output a projection of one decode #472 closed this for transfer fields only, by threading one shared SolanaMetadata. Instruction-level fields still run their own decodes (core/instructions.rs:decode_instructions, core/txtypes/v0.rs:decode_v0_instructions).
• Five of six chains emit no intermediate at all. Ethereum, Sui, Tron, NEAR and Unspecified all return ConversionResult::new(payload). There is no per-chain IR type outside Solana.
• The onboarding path reproduces the gap. docs/adding-new-chain.mdx walks an author through Transaction and VisualSignConverter across six steps and never mentions intermediate_output. A new chain following the official guide inherits the hole by default.

Proposal

Split conversion into two phases at the trait level so rendering cannot observe anything but the intermediate representation:

// src/visualsign/src/vsptrait.rs
pub trait VisualSignConverter<T: Transaction> {
    /// Borsh-serializable decoded form. The single source of truth.
    type Intermediate: borsh::BorshSerialize;

    /// Phase 1 — the only place the raw transaction is read.
    fn to_intermediate(&self, tx: T, opts: &VisualSignOptions)
        -> Result<Self::Intermediate, VisualSignError>;

    /// Phase 2 — rendering. Receives ONLY the IR: no `T`, no raw bytes.
    fn render(&self, ir: &Self::Intermediate, opts: &VisualSignOptions)
        -> Result<SignablePayload, VisualSignError>;

    /// Provided; chains do not override.
    fn to_visual_sign_payload(&self, tx: T, opts: &VisualSignOptions)
        -> Result<ConversionResult, VisualSignError>
    {
        let ir = self.to_intermediate(tx, opts)?;
        let payload = self.render(&ir, opts)?;   // cannot see `tx`
        // ... serialize `ir` when opts.include_intermediate_output
    }
}

The guarantee becomes structural rather than conventional: render has no access to T, so a SignablePayload can only be built from Self::Intermediate, and the emitted bytes are to_vec of the same value render read. Divergence stops being expressible, and a new chain cannot reintroduce the gap by omission.

Type erasure is not a problem. VisualSignConverter<T> is never used as a trait object anywhere in the tree — the only dyn is VisualSignConverterAny (visualsign/src/registry.rs:64), which returns ConversionResult and stays unchanged. The associated type is erased inside ConverterWrapper, the one place that knows both C and C::Intermediate. HashMap<Chain, Box<dyn VisualSignConverterAny>> is untouched.

Emission stays opt-in. When include_intermediate_output is false the IR is still built and rendered from, but not serialized — the signed digest and the backward-compat contract at parse.rs:147 are unaffected.

What blocks full delivery

Five chains are straightforward: their renderers read scalars off the transaction, so a minimal-but-faithful IR is mostly mechanical. Two notes worth recording — Tron's renderer currently calls chrono::Utc::now() (visualsign-tron/src/lib.rs:145), i.e. non-determinism inside render that this refactor forces into to_intermediate; and for Ethereum and Sui the "decoded call/command" is produced by visualizer objects returning SignablePayloadField directly rather than data.

Solana is the hard case. Its 27 preset visualizers take raw instruction bytes and return finished fields — visualize_tx_commands(&self, ctx: &VisualizerContext) -> Result<AnnotatedPayloadField, _> where ctx.data() -> &[u8]. Decode and render are one step. Three information losses block rendering from parsed metadata:

	Loss	Status
L1	parsed_instruction is None for nearly every preset-covered program (solana_parser fills it only for ~13 built-in ProgramTypes plus caller IDLs, and extract_idl_mappings drops caller IDLs for trusted program ids)	Largely closed by #465's preset-IDL injection; remains open for the 8 non-IDL presets
L2	ALT compaction destroys positional account alignment — SolanaInstruction.accounts holds static accounts only, while the visualizer path preserves positions and renders unresolved(N) placeholders that tests assert on	Addressed upstream by anchorageoss/solana-parser#8; pending merge + pin bump
L3	No inner/CPI instructions — swig_wallet and squads_multisig synthesize them and re-enter the visualizer to depth 4	Closed by #465 for caller-simulated CPIs; open for statically synthesized ones

Making the guarantee real for Solana still requires splitting all 27 presets into decode and render halves — but for a different reason than L1/L2/L3. Those three are about whether the IR carries enough data to render from. The split is required by something more basic: visualize_tx_commands takes &[u8] and returns a finished AnnotatedPayloadField, so decode and render are fused in one call. No amount of enriching the IR changes that; the presets have to be factored before rendering can read from an IR at all.

That work is larger than the trait change itself and should be tracked separately. Note the tempting shortcut — an IR that carries raw bytes so render can re-decode them — satisfies the compiler while giving back exactly the guarantee we are trying to win.

Suggested sequencing

1. Hoist the duplicated preset renderers. build_named_accounts (16 copies) and build_parsed_fields (12 copies) are copy-pasted across presets and already are the parsed→fields renderer. Sharing them creates the seam along which presets later split. Depends on nothing. Coordinate with feat(solana): core::arg_rendering + Marginfi preset; bootstrap-only solana-add-idl skill #375 and Migrate existing IDL presets off local format_arg_value (charset-safety gap for array/struct args) #417, which cover the third member of that trio (format_arg_value, 18 copies) — do not duplicate that work. See also solana: four presets hand-roll account resolution and render "unknown" instead of the canonical unresolved placeholder #435.
2. Delete decode_v0_transfers in favour of decode_transfers_from_metadata. Not the pure deletion it appears to be: it serializes the full versioned transaction rather than the message, and injects a refreshed Jupiter v6 IDL override so route_v2 doesn't make the whole-transaction decode bail. Since perf(solana): make intermediate_output a projection of one decode #472 it also serves as the fallback when the shared decode fails. Safe only once the shared decode carries preset IDLs — i.e. after both perf(solana): make intermediate_output a projection of one decode #472 and Flag unregistered programs in Solana intermediate output #465 land.
3. The trait change plus the five simpler chains, with Solana satisfying the new trait via its existing IR. Update docs/adding-new-chain.mdx in the same change — that is what stops the next chain from reintroducing the gap.
4. The 27-preset split, tracked separately. Gated on the fused decode/render shape of visualize_tx_commands, and on L1/L3 for the presets whose data the IR still cannot carry — no longer on L2.

Correction: L2 was not a fundamental limitation

An earlier revision of this issue described L2 (positional account alignment) as "the hard blocker" and implied it could not be worked around without changing what solana_parser returns. That was wrong in an important way, and the correction lowers the cost estimate.

The positional data was already being computed inside the parser. all_instructions_and_transfers builds all_transaction_addresses: Vec<AccountAddress> in declaration order, interleaving static accounts and lookup-table entries, uses it for transfer and IDL decoding — and then drops it at the end of each loop iteration. accounts and address_table_lookups are that same data partitioned by kind, which is what loses the interleaving.

anchorageoss/solana-parser#8 surfaces it as SolanaInstruction::all_accounts and exports AccountAddress. Six lines of library change; the existing fields are untouched. Once merged and the pin bumped, an InstructionView::from_metadata_instruction constructor can reproduce the unresolved(N) placeholders that tests assert on.

This does not remove the preset split — see above for why it is required regardless — but it does remove the argument that rendering from the IR is structurally impossible for ALT-backed transactions.

Alternatives considered

Keep the current shape, add drift tests per chain. What exists today (one Solana fixture cross-checking transfer amounts). It scales linearly with chains and fixtures, catches divergence only where a test happens to look, and does nothing about a chain that emits no IR at all. Rejected: the property is worth a compile-time guarantee, not a sampling one.

Box<dyn Any> at the erased boundary. Would let a caller downstream of the registry recover the typed IR. But Any requires 'static, every consumer must downcast_ref to a concrete per-chain type anyway, and the wire format (ParsedTransactionPayload::intermediate_output) is already Vec<u8> of borsh — so it adds a downcast without removing a serialize. Rejected.

An object-safe IntermediateOutput supertrait carrying a fn to_borsh(&self) -> Result<Vec<u8>, io::Error>, via a blanket impl over BorshSerialize (needed because dyn BorshSerialize is rejected — serialize<W: Write> is generic over W). Useful only if the IR itself must be dyn-able; unnecessary for this goal, where erasure to Vec<u8> inside ConverterWrapper suffices. Kept in reserve.

Allow type Intermediate = () as a migration escape hatch. () implements BorshSerialize, so chains could satisfy the trait without deriving anything from an IR — and render would need the raw T back to do any work. That reinstates precisely the hole this issue is about. Rejected.

Render Solana directly from SolanaMetadata (the existing parsed structure), avoiding a new IR type. Blocked by L1 and L3 above: the metadata is not rich enough for the eight non-IDL presets, and carries no counterpart for statically synthesized inner instructions. (L2 was also on this list; it turned out to be a missing field rather than a missing capability — see the correction below.)

Do Solana's internal unification first, defer the cross-chain trait. Reasonable, and it would prove the render-from-IR shape on the hardest chain first. Rejected for sequencing: it leaves the five other chains and every future chain unguarded for longer, and the trait change is what makes the preset split verifiable rather than merely intended.