✨ (signer-solana) [DSDK-1519]: Lock in substructure framing + warn on pinned skips - #1888
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Descriptor-warning paths, framing wording, and duplicate changeset cleanup remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR locks Solana substructure framing and improves diagnostics for unresolved clear-sign descriptors.
Changes:
- Adds regression coverage for one substructure per exchange.
- Adds warnings for missing descriptor resolutions.
- Clarifies framing and ALT-resolution comments and adds changesets.
File summaries
| File | Reviewed changes |
|---|---|
packages/signer/signer-solana/src/internal/app-binder/task/ProvideGenericClearSignContextTask.ts |
Adds descriptor-skip warnings and ALT handling. |
packages/signer/signer-solana/src/internal/app-binder/task/context-providers/provideInstructionInfoContext.test.ts |
Tests individual substructure exchanges. |
packages/signer/signer-solana/src/internal/app-binder/command/ProvideInstructionSubstructureCommand.ts |
Clarifies substructure framing behavior. |
packages/signer/signer-solana/src/internal/app-binder/clear-sign/requirements/rules/altResolutionRule.ts |
Documents read-only ALT exclusions. |
.changeset/real-taxis-kiss.md |
Adds a package patch changeset. |
.changeset/dull-baths-poke.md |
Adds an overlapping package patch changeset. |
Review details
Suppressed comments (4)
packages/signer/signer-solana/src/internal/app-binder/command/ProvideInstructionSubstructureCommand.ts:54
- Only the length field is absent from the wire: the implementation sends
substructure.kindfollowed by the TLV. Saying the entireSUBSTRUCT_TYPE ‖ uint32be length ‖ TLVframing is absent contradicts the preceding sentence and can mislead protocol changes; clarify that onlyuint32be lengthis omitted from the wire.
* `SUBSTRUCT_TYPE ‖ uint32be length ‖ TLV` framing is absent on the wire (the
* device recovers the total length from the chunk flags) but the device still
* folds that framing into the running `SUBSTRUCTURES_HASH`, so each call must
packages/signer/signer-solana/src/internal/app-binder/task/ProvideGenericClearSignContextTask.ts:316
provideChallengeBoundDescriptorAndReturnfilters outERRORcontexts and returns an empty array when the lookup has no result, so this loop is skipped entirely for the missing/failing ALT_RESOLUTION cases this warning is intended to report. Emit the warning whenaltContexts.length === 0before entering the loop; otherwise the new log is silent on the actual failure path.
) {
// Unresolved ALT_RESOLUTION for a TOKEN_AMOUNT.TOKEN ref pins the
// instruction (G-051): the amount's token cannot be displayed and
// the merge cannot compact it away.
this.logger.warn(
packages/signer/signer-solana/src/internal/app-binder/task/ProvideGenericClearSignContextTask.ts:423
provideChallengeBoundDescriptorAndReturnfilters outERRORcontexts and returns an empty array when the lookup has no result, so this loop is skipped entirely for the missing/failing ALT_RESOLUTION cases this warning is intended to report. Emit the warning whenaltContexts.length === 0before entering the loop; otherwise the new log is silent on the actual failure path.
) {
// Unresolved ALT_RESOLUTION for an owner/mint-map target pins the
// instruction (G-051): neither the IS_SIGNER hide nor the mint
// display can be established for it.
this.logger.warn(
packages/signer/signer-solana/src/internal/app-binder/task/ProvideGenericClearSignContextTask.ts:283
- The generic
altResolutionsloop above usesprovideChallengeBoundDescriptor, which discards the helper's returned contexts. Since challenge-bound failures are represented astype: ERRORand successful ALT payloads may omitresolvedAddress, that path still produces no warning for either unresolved case. If the PR is to make every ALT skip visible, propagate an outcome from the helper and log the empty/error or no-address result in this caller (or centralize that logging).
this.logger.warn(
"[run] ALT_RESOLUTION fetch failed for a MINT_ASSOC ref; instruction may be pinned",
{ data: { altAddress, entryIndex } },
);
continue;
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9445d6e to
2a906df
Compare
2a906df to
8f664b8
Compare
b9352eb to
a35230c
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
a35230c to
ef4285c
Compare
|



📝 Description
Substructure framing:
SUBSTRUCTURES_HASHnow commits to each substructure framed asSUBSTRUCT_TYPE ‖ uint32be length ‖ TLV, so the device rejects two substructures packed into onePROVIDE INSTRUCTION SUBSTRUCTURE (0x25)exchange. DMK’s implementation was already correct, oneSendCommandInChunksTaskper substructure. This PR locks that behavior in with a regression test and fixes a misleading comment inProvideInstructionSubstructureCommand.tsabout why the framing matters.Pinned-descriptor logging: A missing best-effort descriptor (
TOKEN_INFO, TOKEN_ACCOUNT_STATE, ALT_RESOLUTION) previously degraded a predicate to false, it now leaves the port unresolved, which pins the instruction no merge, drop, or hide, instead of costing only a ticker. Addedlogger.warnat every best-effort skip site inProvideGenericClearSignContextTask.tsso these cases are visible in logs rather than silent. Also updated the read-only ALT exclusion comment inaltResolutionRule.tsto explain why it remains safe under the new three-valued predicate rule.❓ Context
✅ Checklist
Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.
🧐 Checklist for the PR Reviewers