You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The original CompiledInstruction struct is missing the field stackHeight
Without this field, parse inner instruction would miss part of the stack height data.
I checked the upstream Rust types, and stack_height does not belong on the core CompiledInstruction.
In Solanas Rust code, the message-level CompiledInstruction only contains:
program_id_index
accounts
data
stack_height is carried separately in higher-level status / UI types, such as:
UiCompiledInstruction
InnerInstruction
That distinction matters here because solana-gos CompiledInstruction is not only used as an RPC DTO, it is also part of binary encoding paths. Adding StackHeight directly to this struct changes the serialized instruction layout, which is why TestCompiledInstructions starts failing: the encoded bytes gain two extra trailing bytes.
I dont think this PR should be merged in its current form. If the goal is RPC compatibility, the safer fix would be to keep stackHeight in the RPC/UI layer only, or explicitly exclude it from binary encoding.
Thanks again for your feedback! I've just updated the PR to address the concern by changing the StackHeight field type to *uint16 (a pointer to uint16) — this adjustment perfectly resolves the issue we discussed:
Key Improvements with *uint16
Preserve RPC Data Compatibility: RPC responses that include stack_height can still be properly unmarshaled into the struct (the pointer type accepts valid stack_height values from RPC).
Align with Core Instruction Format: For the core CompiledInstruction (used in transaction encoding/decoding, where stack_height is not part of the on-chain format), the pointer type allows the field to be nil — this avoids extraneous values in the core type and eliminates encoding/decoding mismatches.
No Breaking Changes: Existing code that relies on stack_height from RPC data will continue to work (we only changed the field type to a pointer, not removed the field entirely for RPC handling).
This change strikes a balance between:
Keeping stack_height for RPC data parsing (as it’s present in RPC responses)
Ensuring the core CompiledInstruction adheres to Solana’s on-chain instruction structure (no unnecessary non-nil fields)
Could we proceed to merge this updated version? Let me know if you have any further questions or need additional adjustments!
Thanks,
xiaobaiskill
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
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.
The original
CompiledInstructionstruct is missing the fieldstackHeightWithout this field, parse inner instruction would miss part of the stack height data.