Support TokenKeg as a known visualizer (instead of Unknown test)#81
Open
prasanna-anchorage wants to merge 6 commits into
Open
Support TokenKeg as a known visualizer (instead of Unknown test)#81prasanna-anchorage wants to merge 6 commits into
prasanna-anchorage wants to merge 6 commits into
Conversation
prasanna-anchorage
changed the title
prasanna-anchorage
force-pushed
the
prasanna/support-tokenkeg-mint
branch
from
aee64db to
aea8b83
Compare
prasanna-anchorage
force-pushed
the
prasanna/support-tokenkeg-mint
branch
from
aea8b83 to
b36c0c1
Compare
prasanna-anchorage
force-pushed
the
prasanna/support-tokenkeg-mint
branch
from
b36c0c1 to
39e0880
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class SPL Token (TokenKeg) instruction visualization to the visualsign-solana parser so TokenKeg instructions render as meaningful, parsed actions instead of falling back to the unknown-program visualizer.
Changes:
- Introduces a new
spl_tokenpreset (config + visualizer + tests) that unpacks SPL Token instructions viaspl_token::instruction::TokenInstruction. - Updates Solana visualsign tests to assert TokenKeg is recognized as SPL Token and adds a regression test for the unknown-program fallback.
- Adds fixture-based testing documentation (Solana-specific + repo-wide) and ignores coverage artifacts in
.gitignore.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/chain_parsers/visualsign-solana/tests/fixtures/spl_token/mint_to_example.json | Adds a real-world MintTo fixture (base58 instruction data + expected fields). |
| src/chain_parsers/visualsign-solana/tests/fixtures/spl_token/README.md | Documents SPL Token fixture conventions and how to add/run fixtures. |
| src/chain_parsers/visualsign-solana/src/presets/swig_wallet/mod.rs | Skips SPL Token + unknown-program summaries for inner instructions to prefer Swig-specific formatting. |
| src/chain_parsers/visualsign-solana/src/presets/spl_token/tests.rs | Adds SPL Token unit tests and fixture test harness. |
| src/chain_parsers/visualsign-solana/src/presets/spl_token/mod.rs | Implements SplTokenVisualizer and preview layouts for several SPL Token instructions. |
| src/chain_parsers/visualsign-solana/src/presets/spl_token/config.rs | Registers TokenKeg program ID for preset dispatch. |
| src/chain_parsers/visualsign-solana/src/presets/mod.rs | Exposes the new spl_token preset module. |
| src/chain_parsers/visualsign-solana/src/core/visualsign.rs | Renames/updates TokenKeg test to expect SPL Token recognition; adds unknown fallback test. |
| src/chain_parsers/visualsign-solana/TESTING.md | Adds Solana fixture-based testing guide and coverage workflow notes. |
| src/chain_parsers/visualsign-solana/.gitignore | Ignores local coverage artifacts for the crate. |
| TESTING.md | Adds repo-level coverage/testing quickstart and links to chain-specific testing docs. |
| .gitignore | Ignores coverage artifacts at repo root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // SetAuthority accounts: [0] account whose authority is being set, [1] current authority | ||
| if let Some(account) = instruction.accounts.first() { | ||
| expanded_fields.push(create_text_field("Account", &account.pubkey.to_string())?); | ||
| } |
There was a problem hiding this comment.
The comment states SetAuthority accounts include [1] current authority, but the visualization only surfaces the target Account and omits the current authority signer. This loses important information for users reviewing the transaction. Consider adding an explicit "Current Authority" field sourced from instruction.accounts[1] when present.
Suggested change
| } | |
| } | |
| if let Some(current_authority) = instruction.accounts.get(1) { | |
| expanded_fields.push(create_text_field( | |
| "Current Authority", | |
| ¤t_authority.pubkey.to_string(), | |
| )?); | |
| } |
| "Destination", | ||
| &destination.pubkey.to_string(), | ||
| )?); | ||
| } |
There was a problem hiding this comment.
This Transfer visualization omits the authority/owner account (account index 2), even though the comment indicates it is part of the instruction. Without showing the owner, the UI loses a key security-relevant detail. Add an "Owner" (or "Authority") field from instruction.accounts[2] when available.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(2) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
| "Destination", | ||
| &destination.pubkey.to_string(), | ||
| )?); | ||
| } |
There was a problem hiding this comment.
TransferChecked visualization currently shows source/mint/destination but omits the owner/authority account at index 3. Since the owner is the signer/authority for the transfer, it should be displayed to make the instruction review meaningful. Add an "Owner" (or "Authority") field from instruction.accounts[3] when present.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(3) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
| } | ||
| if let Some(mint) = instruction.accounts.get(1) { | ||
| expanded_fields.push(create_text_field("Token Mint", &mint.pubkey.to_string())?); | ||
| } |
There was a problem hiding this comment.
Burn visualization omits the owner/authority account (index 2). For a burn, the authority is the critical signer that authorizes destroying tokens, so it should be surfaced. Add an "Owner"/"Authority" field from instruction.accounts[2] when available.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(2) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
| } | ||
| if let Some(mint) = instruction.accounts.get(1) { | ||
| expanded_fields.push(create_text_field("Token Mint", &mint.pubkey.to_string())?); | ||
| } |
There was a problem hiding this comment.
BurnChecked visualization omits the owner/authority account (index 2). This is the signer that authorizes the burn, so leaving it out reduces the value of the visualization. Add an "Owner"/"Authority" field from instruction.accounts[2] when present.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(2) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
| } | ||
| if let Some(delegate) = instruction.accounts.get(1) { | ||
| expanded_fields.push(create_text_field("Delegate", &delegate.pubkey.to_string())?); | ||
| } |
There was a problem hiding this comment.
Approve visualization omits the owner/authority account (index 2). Since approvals delegate spending authority, showing the approving owner is important for transaction review. Add an "Owner" (or "Authority") field from instruction.accounts[2] when present.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(2) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
| } | ||
| if let Some(delegate) = instruction.accounts.get(2) { | ||
| expanded_fields.push(create_text_field("Delegate", &delegate.pubkey.to_string())?); | ||
| } |
There was a problem hiding this comment.
ApproveChecked visualization shows source/mint/delegate but omits the owner/authority account at index 3 (the signer granting the approval). This is a key security detail and should be included. Add an "Owner"/"Authority" field from instruction.accounts[3] when available.
Suggested change
| } | |
| } | |
| if let Some(owner) = instruction.accounts.get(3) { | |
| expanded_fields.push(create_text_field("Owner", &owner.pubkey.to_string())?); | |
| } |
Comment on lines
+1144
to
+1176
| for (key, expected_value) in &fixture.expected_fields { | ||
| let expected_str = expected_value.as_str().unwrap(); | ||
|
|
||
| // Check in expanded fields | ||
| if let Some(expanded) = &preview_layout.expanded { | ||
| let found = expanded.fields.iter().any(|field| { | ||
| if let SignablePayloadField::TextV2 { common, text_v2 } = | ||
| &field.signable_payload_field | ||
| { | ||
| let label_matches = common.label.to_lowercase().replace(" ", "_") | ||
| == key.to_lowercase(); | ||
| let value_matches = text_v2.text == expected_str; | ||
| if label_matches { | ||
| if value_matches { | ||
| println!("✓ {key}: {expected_str} (matches)"); | ||
| } else { | ||
| println!( | ||
| "✗ {}: expected '{}', got '{}'", | ||
| key, expected_str, text_v2.text | ||
| ); | ||
| } | ||
| return value_matches; | ||
| } | ||
| false | ||
| } else { | ||
| false | ||
| } | ||
| }); | ||
|
|
||
| if !found { | ||
| println!("✗ {key}: field not found in output"); | ||
| } | ||
| } |
There was a problem hiding this comment.
In this fixture validation loop, mismatches only print messages; the test never fails when an expected field is missing or has the wrong value. This makes the fixture test ineffective (it can pass even when extraction is broken). Add assertions so each expected field must be found and match the expected value (e.g., assert on found, or collect failures and panic!/assert! at the end).
prasanna-anchorage
force-pushed
the
prasanna/support-tokenkeg-mint
branch
2 times, most recently
from
0ad2d67 to
10e41ae
Compare
Resolves #76. Adds a dedicated `SplTokenVisualizer` for the SPL Token program (`TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA`) so token transactions render with structured fields instead of falling back to `UnknownProgramVisualizer`. ## What it covers Full instruction coverage via `TokenInstruction::unpack()` from the `spl-token` crate, including: - MintTo / MintToChecked (amount + decimals + mint + destination) - Transfer / TransferChecked (amount + source + destination + owner) - Burn / BurnChecked (amount + account + mint + owner) - Approve / ApproveChecked (amount + source + delegate + owner) - InitializeMint / InitializeMintV1 - FreezeAccount / ThawAccount - SetAuthority (all authority types, including current authority signer) - Revoke, CloseAccount, default branch fallback For every instruction whose semantics depend on a signing owner / authority, the Expanded layout now surfaces that signer field (addressing the original Copilot review feedback on this PR): - Transfer / Burn / Approve: "Owner" from account index 2 - TransferChecked / ApproveChecked: "Owner" from account index 3 - BurnChecked: "Owner" from account index 2 - SetAuthority: "Current Authority" from account index 1 Without these the visualization would silently drop the security- relevant signer for the action. ## swig_wallet interaction `presets/swig_wallet/mod.rs::visualize_inner_instruction` now skips `SplToken` alongside `UnknownProgram`, so swig's inner-instruction summary keeps using its richer `format_token_instruction_summary` (From/To/Owner/Amount lines) instead of the new visualizer's terse title. ## Test coverage - 23 unit tests in `presets/spl_token/tests.rs` covering parsing (`unpack_instruction`) and visualization for every supported variant, including the default-branch path (Revoke, CloseAccount, InitializeMint). - Real-transaction fixture for MintTo at `tests/fixtures/spl_token/mint_to_example.json` with strict per-field assertions (the validation loop now fails on any divergence rather than just printing mismatches). - `core/visualsign.rs`: renamed `test_unknown_program_tokenkeg` -> `test_spl_token_tokenkeg_recognition` (now asserts TokenKeg is recognized as SPL Token) and added `test_unknown_program_fallback` with a clearly-fake program id to keep the unknown-program path exercised. Coverage on `presets/spl_token/mod.rs`: 82% lines / 86% regions, above the 70% target in TESTING.md. ## Build integration Auto-discovered by `presets/mod.rs` alphabetical ordering. Placed before `UnknownProgramVisualizer` in the visualizer chain so the generic catch-all only matches when no specific preset claims the program id. ## Rebase notes Rebased onto post-#288 main. `SplTokenConfig` was updated from `HashMap`/`HashSet` to `BTreeMap` to match the new `SolanaIntegrationConfigData` signature and to satisfy the workspace clippy `disallowed-types` rule introduced by #288. `cargo build -p visualsign-solana`, `cargo clippy -p visualsign-solana --all-targets -- -D warnings`, and `cargo test -p visualsign-solana` (165 unit tests including the 23 spl_token tests) all pass locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
prasanna-anchorage
force-pushed
the
prasanna/support-tokenkeg-mint
branch
from
10e41ae to
94d0a6f
Compare
… carries it Closes the gap flagged in review: the original PR only surfaced the mint for MintTo / MintToChecked / TransferChecked / Burn(Checked) / ApproveChecked, leaving readers unable to tell which mint was involved for the other ~half of the SPL Token instruction set. Adds dedicated visualization arms for every variant whose accounts contain a mint: - InitializeMint / InitializeMint2 -- surfaces Token Mint (acct[0]), Decimals, Mint Authority, and Freeze Authority (the last three come from the instruction data). - InitializeAccount / InitializeAccount2 / InitializeAccount3 -- surfaces Account (acct[0]), Token Mint (acct[1]), and Owner (from acct[2] for V1, from instruction data for V2/V3). - FreezeAccount / ThawAccount -- surfaces Account (acct[0]), Token Mint (acct[1]), and Freeze Authority (acct[2]). For variants whose accounts genuinely do not contain a mint, surfaces an explicit "Token Mint" notice so readers know why it is absent and what to do: - Transfer / Approve -- "Not in instruction -- use TransferChecked / ApproveChecked to surface and verify mint." - Revoke -- "Not in instruction -- derived from the source account's stored state." - CloseAccount -- "Not in instruction -- derived from the closed account's stored state." Also adds CloseAccount and Revoke as explicit arms (previously falling through to the catch-all default) with Account/Destination/Owner surfaced. ## Test updates - Renamed and strengthened `test_revoke_visualization_falls_through_to_default`, `test_close_account_visualization_falls_through_to_default`, and `test_initialize_mint_visualization_falls_through_to_default` to assert the new named fields instead of the old default-layout shape. - Strengthened `test_freeze_account_visualization` and `test_thaw_account_visualization` to verify the rendered "Token Mint" field matches the input mint pubkey (the prior assertion was only `!fields.is_empty()`, which would have silently accepted a regression). cargo build / cargo clippy -- -D warnings / cargo test presets::spl_token (23 tests) / cargo fmt --check: all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drives each new SPL Token code path through the public
`transaction_to_visual_sign` API end-to-end:
SolanaTransaction (built with spl_token::instruction::*)
-> transaction_to_visual_sign
-> SolanaVisualSignConverter
-> SplTokenVisualizer (dispatched by program id)
-> SignablePayload
Unlike the in-module unit tests, which exercise the visualizer in
isolation, these tests prove the dispatch + conversion chain works for
real Transaction inputs (the same path parser_cli takes after decoding
base64).
10 tests covering the mint-bearing variants and the mint-absent
variants:
- pipeline_initialize_mint_surfaces_mint_decimals_and_authorities
- pipeline_initialize_mint2_surfaces_mint_decimals_and_authorities
- pipeline_initialize_account_surfaces_account_mint_and_owner
- pipeline_initialize_account3_surfaces_mint_and_owner_from_instruction_data
- pipeline_freeze_account_surfaces_account_mint_and_authority
- pipeline_thaw_account_surfaces_account_mint_and_authority
- pipeline_close_account_explains_mint_absence_and_surfaces_destination
- pipeline_revoke_explains_mint_absence_and_surfaces_source_and_owner
- pipeline_unchecked_transfer_directs_user_to_transfer_checked
- pipeline_transfer_checked_surfaces_mint_explicitly
Each test asserts the rendered Expanded layout either contains the
literal mint pubkey (the surfaced cases) or contains a "Token Mint"
notice mentioning "Not in instruction" / "TransferChecked" (the
explained-absence cases).
Combined with the existing devnet MintTo fixture test
(`test_mint_to_real_transaction`), which has now been verified
end-to-end through `parser_cli` against the real on-chain transaction
35XirCzssnAVUB2FbLrf8vYUYmTq5omepqyR8tr5Y6eJ6yurs3LcRfzGxzn92wU3w5vBvM8BfodXsscz7nin8SbC,
this gives the SPL Token preset full pipeline coverage for every
documented variant.
cargo test -p visualsign-solana --test spl_token_e2e: 10/10 pass.
cargo clippy -p visualsign-solana --all-targets -- -D warnings: clean.
cargo fmt --check: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The integration test `parser_charset_validation_all_chains` asserts that parsed SignablePayload JSON is pure ASCII (`json_str.is_ascii()`). The em-dash (U+2014) I used in the new mint-absence notices broke that invariant when an SPL Token Transfer/Approve/Revoke/CloseAccount instruction appears in any parsed transaction (such as the inner instructions of the Jupiter swap fixture used by the test). Replace `—` with `--` in the four user-rendered notice strings: - Transfer: "Not in instruction -- use TransferChecked to ..." - Approve: "Not in instruction -- use ApproveChecked to ..." - CloseAccount: "Not in instruction -- derived from the closed ..." - Revoke: "Not in instruction -- derived from the source ..." Comments and println! debug output keep `—` since they never appear in parsed payload output. All existing unit/e2e tests still pass (assertions check for `Not in instruction` / `TransferChecked` substrings, which are unaffected). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eld to tracing
The explicit "Token Mint: Not in instruction -- ..." field on unchecked
Transfer / Approve / Revoke / CloseAccount was visible to wallet UIs
even when it was just educational footer text. That clutters the
hardware-wallet view for what is the most common SPL Token path
(unchecked Transfer is what most wallets emit).
Replaces each notice field with a `tracing::debug!` line that keeps the
operator-observability signal but stays out of the user-facing
SignablePayload. Wallet integrators see a clean Source/Destination/Owner
layout; operators tailing logs at debug level still see "mint omitted,
use TransferChecked" / etc.
Updates the affected tests:
- presets/spl_token/tests.rs: test_revoke_visualization and
test_close_account_visualization drop "Token Mint" from the expected
expanded-label list.
- tests/spl_token_e2e.rs: pipeline tests for CloseAccount / Revoke /
unchecked Transfer rename from `*_explains_mint_absence_*` to
describe the actual rendered output and assert
`find_text("Token Mint") == None`.
All 33 SPL Token tests (10 e2e + 23 unit) pass.
cargo clippy --all-targets -- -D warnings and cargo fmt --check are clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…64 examples
Adds an `#[ignore]`d integration test that prints base64-encoded
SolanaTransaction wire format for every SPL Token instruction variant
the preset handles. Each tx uses deterministic stand-in pubkeys so
output is reproducible across runs.
Run with:
cargo test -p visualsign-solana --test dump_spl_b64 \
-- --nocapture --include-ignored
Each captured base64 can be fed directly to `parser_cli`:
parser_cli --chain solana --network SOLANA_MAINNET \
--output json -t '<base64>'
Useful when adding fixtures, debugging visualizer output, or building
reference JSON outputs for documentation, without depending on mainnet
RPC (which rate-limits aggressively).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Support TokenKeg (SPL Token) program
Resolves #76
Summary
Adds dedicated visualizer for the TokenKeg program (
TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA) to display meaningful SPLtoken instruction details instead of showing as "unknown program". This enables SPL token minters to understand what
operations are being performed in their transactions.
Changes
New SPL Token Preset (
src/presets/spl_token/)TokenInstruction::unpack()fromspl-tokencrateTest Coverage
test_unknown_program_tokenkeg→test_spl_token_tokenkeg_recognitionto verify TokenKeg is properly recognizedas SPL Token
test_unknown_program_fallbackwith fake program ID pattern (FAKEPROGRAM!) to validate unknown program visualizerstill works
Build Integration
presets/mod.rsfor auto-discovery by build.rsUnknownProgramVisualizerin visualizer chainExample Output
Before (Unknown Program):
Program ID: TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA
Instruction Data: 0601014f42...
After (SPL Token):
Instruction: Mint To
Amount: 1000000
Decimals: 6
Program: SPL Token
Testing