Skip to content

refactor(solana): single definition for account and program display strings - #438

Open
shahan-khatchadourian-anchorage wants to merge 2 commits into
mainfrom
worktree-435-shared-account-resolution
Open

refactor(solana): single definition for account and program display strings#438
shahan-khatchadourian-anchorage wants to merge 2 commits into
mainfrom
worktree-435-shared-account-resolution

Conversation

@shahan-khatchadourian-anchorage

Copy link
Copy Markdown
Contributor

Summary

Closes #435. Stacked on #436 (worktree-434-fmt-preset-tree).

core::InstructionView owns the placeholder vocabulary for resolving account and program indices to display strings. Three presets hand-roll the same three-arm match instead of using it, and all three diverge on the out-of-bounds arm -- rendering unknown where the canonical form is unresolved(oob:N):

Location Was
presets/jupiter_swap/mod.rs:94 None => "unknown".to_string()
presets/system/mod.rs:123 None => "unknown".to_string()
presets/system/mod.rs:128 None => "unknown".to_string()
presets/unknown_program/mod.rs:67 None => "unknown".to_string()

unknown carries no index, so it is strictly less diagnosable than the canonical form, and a signer comparing two transactions sees different vocabulary for the same condition.

Approach

Extract the two resolution rules in core and build InstructionView on top of them, so there is one definition rather than one-per-caller:

pub fn resolve_program_display(context: &VisualizerContext) -> String
pub fn resolve_account_display(context: &VisualizerContext, position: usize) -> String

Then route each preset through the shape that fits how it reads accounts:

  • jupiter_swap needs every account, so it takes InstructionView::from_context(context).accounts -- the closure disappears entirely.
  • system names two fixed positions, so it calls resolve_account_display(context, 1) / (context, 0).
  • unknown_program drops both of its local copies (resolve_account_str and resolve_program_id_str -- the latter duplicated the program arm the same way) and calls the shared functions.

Behavior

No output changes for in-bounds indices, which is every index a well-formed instruction produces. The None arm is reachable only when a preset names a fixed position and the instruction carries fewer accounts than the layout expects; that case now renders unresolved(oob:N) instead of unknown.

Test plan

  • cargo test -p visualsign-solana -- 270 passed, 0 failed (269 before; +1 new)
  • make -C src lint -- clippy clean with -D warnings
  • New test test_resolve_account_display_matches_instruction_view asserts position-addressed resolution agrees with InstructionView for both resolved and unresolved indices, and covers the out-of-bounds arm that nothing exercised before
  • After this change, grep '=> "unknown"' across the crate leaves only the two swig_wallet enum-name helpers and spl_token (see below)

Scope

Two "unknown" sites are deliberately untouched:

  • presets/swig_wallet/mod.rs:2037,2047 -- program_scope_type_name / numeric_type_name map unrecognized discriminants to a name. Unrelated to account resolution.
  • presets/spl_token/mod.rs -- carries the same stale arm inside its own private InstructionView, which PR fix(solana/spl_token): migrate to shared core::InstructionView, drop data clone #381 removes. This branch does not touch that file, so the two do not conflict and can merge in either order. Once both land, core/mod.rs holds the only copy of the resolution logic in the crate.

Stacking

Based on #436 rather than main: #436 makes the CI fmt gate cover presets/*/, so the preset edits here are format-checked. There is no file overlap between the two branches.

🤖 Generated with Claude Code

`cargo fmt` stops at `presets/mod.rs`, which declares its submodules
through `include!(concat!(env!("OUT_DIR"), ...))` -- rustfmt does not
follow an `include!` of a generated path, so no preset source is
formatted or checked. The CI gate (`make generated` followed by a
clean-tree check) inherits that blind spot.

Enumerate the preset sources in the `fmt` target so they are covered by
the same gate as the rest of `src/`, and reformat the 11 files that the
gate never saw. The reformat is pure re-wrapping; no tokens change.

Add `src/rustfmt.toml` pinning edition 2024 so running `rustfmt <file>`
directly on a preset matches what `cargo fmt` and the Makefile produce
(bare rustfmt otherwise defaults to edition 2015).

Closes #434
…trings

Three presets hand-roll the same three-arm match that
`core::InstructionView` already performs, and each diverges on the
out-of-bounds arm: they render `unknown` where the canonical form is
`unresolved(oob:N)`. The duplicated logic is what let them drift.

Extract `resolve_program_display` and `resolve_account_display` in
`core`, have `InstructionView::from_context` build on them, and route the
three presets through them:

- `jupiter_swap` needs every account, so it takes
  `InstructionView::from_context(context).accounts`.
- `system` names two fixed positions, so it calls
  `resolve_account_display` directly.
- `unknown_program` drops both of its local copies
  (`resolve_account_str`, `resolve_program_id_str`).

`unknown` disappears from account resolution and the placeholder
vocabulary has one definition. No output changes for in-bounds indices,
which is every index a well-formed instruction produces.

Cover the previously-untested out-of-bounds arm, asserting that
position-addressed resolution agrees with `InstructionView`.

Closes #435
@shahan-khatchadourian-anchorage shahan-khatchadourian-anchorage added chain:solana test:proptest Run the Solana property-test suite on this PR test:surfpool Run the Solana surfpool mainnet-fork suite on this PR CI labels Aug 4, 2026

@prasanna-anchorage prasanna-anchorage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the shared resolve_program_display/resolve_account_display functions and traced every caller — the placeholder-string change ("unknown""unresolved(oob:N)") doesn't break anything downstream, and the two deliberately-untouched "unknown" sites (swig_wallet, spl_token) are correctly out of scope per the PR description.

One thing worth fixing before merge: create_system_preview_layout in src/chain_parsers/visualsign-solana/src/presets/system/mod.rs:50 still hand-rolls its own program_id match instead of calling resolve_program_display. That leaves a second definition of the program-placeholder vocabulary in the crate — exactly the drift this PR's stated goal was to eliminate. A future change to the unresolved-program placeholder format would update unknown_program and InstructionView but silently miss this local match, so System-program instructions with an unresolved program index would render differently from every other preset.

Non-blocking given the size, but please fold system/mod.rs:50 into resolve_program_display too — it's the same shape as the three presets already converted.

Base automatically changed from worktree-434-fmt-preset-tree to main August 25, 2026 19:35
@pepe-anchor
pepe-anchor requested a balanced review from Copilot August 28, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Centralizes Solana account/program display resolution to ensure consistent unresolved placeholders.

Changes:

  • Adds shared display-resolution helpers and tests.
  • Migrates Jupiter, System, and unknown-program presets.
  • Extends formatting coverage to Solana presets and reformats affected files.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/rustfmt.toml Sets Rust edition for direct rustfmt runs.
src/Makefile Formats all Solana preset sources.
src/chain_parsers/visualsign-solana/src/core/mod.rs Adds shared resolution helpers and tests.
.../presets/unknown_program/mod.rs Uses shared account/program resolution.
.../presets/system/mod.rs Uses shared fixed-position account resolution.
.../presets/jupiter_swap/mod.rs Uses the canonical instruction view.
.../presets/swig_wallet/mod.rs Applies rustfmt output.
.../presets/orca_whirlpool/mod.rs Applies rustfmt output.
.../presets/neutral_trade/mod.rs Applies rustfmt output.
.../presets/meteora_dlmm/mod.rs Applies rustfmt output.
.../presets/meteora_damm_v2/mod.rs Applies rustfmt output.
.../presets/kamino_borrow/mod.rs Applies rustfmt output.
.../presets/jupiter_perps/mod.rs Applies rustfmt output.
.../presets/jupiter_earn/mod.rs Applies rustfmt output.
.../presets/jupiter_borrow/mod.rs Applies rustfmt output.
.../presets/exponent_finance/mod.rs Applies rustfmt output.
.../presets/drift/mod.rs Applies rustfmt output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

use crate::core::{
AccountRef, InstructionVisualizer, ProgramRef, SolanaIntegrationConfig, VisualizerContext,
VisualizerKind,
InstructionVisualizer, ProgramRef, SolanaIntegrationConfig, VisualizerContext, VisualizerKind,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must: The program half of the vocabulary is not consolidated, which is why ProgramRef survives on this import line: system/mod.rs:50, jupiter_swap/mod.rs:441, and jupiter_swap/mod.rs:490 still inline the ProgramRef::Resolved/Unresolved match. prasanna-anchorage asked for system/mod.rs:50 in review and it is unchanged at HEAD, and the two jupiter_swap copies sit in a file this PR already edits and already imports crate::core from. Six more presets hold the same copy (compute_budget:69, spl_token:74, associated_token_account:48, token_2022:24, stakepool:43, swig_wallet:70), so the new doc comment at core/mod.rs:251, "This is the single definition of the program placeholder vocabulary", is false the moment this merges. A false documented invariant is worse than none: the next person changing the unresolved-program format will trust it, update resolve_program_display, and ship exactly the drift this PR exists to prevent. Minimal fix: three one-line substitutions to resolve_program_display(context) in the two files already open, or reword core/mod.rs:251 to stop asserting an invariant nothing enforces.

None => "unknown".to_string(),
};
let new_account = resolve_account_display(context, 1);
let payer = resolve_account_display(context, 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should: This is the only behavior change in the PR (unknown becomes unresolved(oob:N)) and nothing tests it here: system/mod.rs and unknown_program/mod.rs both contain zero #[test] functions, and the new core/mod.rs test exercises the helper in isolation, never through create_system_preview_layout. The arm is reachable, not theoretical: VisualizerContext::account at core/mod.rs:163 indexes compiled_instruction.accounts with no length guard, so a CreateAccount whose data bincode-decodes fine but whose account list carries fewer than two indices renders unresolved(oob:1) as New Account and unresolved(oob:0) as Payer in the signing preview. Given the 2ee2ee79 revert on these same files, whose rule was to keep user-visible fields identical across a refactor, the string a signer actually sees should be pinned by a test that builds that instruction and asserts the rendered New Account and Payer fields.

let view = InstructionView::from_context(&ctx);
assert_eq!(resolve_account_display(&ctx, 0), view.accounts[0]);
assert_eq!(resolve_account_display(&ctx, 1), view.accounts[1]);
assert_eq!(resolve_program_display(&ctx), view.program_id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: InstructionView::from_context now delegates straight to resolve_account_display and resolve_program_display, so these three assertions compare each function against itself and cannot fail. They guard the one definition that structurally cannot drift and none of the nine hand-rolled copies that can. Line 500's unresolved(oob:2) assertion is the only line in this test carrying signal; the drift the test name claims to cover would be caught by asserting against literal expected strings, or by testing a preset that reads a fixed position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chain:solana CI test:proptest Run the Solana property-test suite on this PR test:surfpool Run the Solana surfpool mainnet-fork suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants