Skip to content

fix: Improve Steel IDL detection for 1-byte discriminator arrays #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adiman9 merged 1 commit into from
Mar 19, 2026
+16 −3
Merged

fix: Improve Steel IDL detection for 1-byte discriminator arrays #62

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions hyperstack-idl/src/snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,28 @@ impl<'de> Deserialize<'de> for IdlSnapshot {
return false;
}
instrs.iter().all(|ix| {
// Steel-style: has discriminant field, no discriminator or empty discriminator
let has_discriminant = ix.get("discriminant").is_some();
let discriminator = ix.get("discriminator");
let disc_len = discriminator
.and_then(|d| d.as_array())
.map(|a| a.len())
.unwrap_or(0);

// Steel-style variant 1: has explicit discriminant field, no discriminator
let has_discriminant = ix.get("discriminant").is_some();
let has_discriminator = discriminator
.map(|d| {
!d.is_null() && d.as_array().map(|a| !a.is_empty()).unwrap_or(true)
})
.unwrap_or(false);
has_discriminant && !has_discriminator
let is_steel_discriminant = has_discriminant && !has_discriminator;

// Steel-style variant 2: discriminator is stored as a 1-byte array (no
// discriminant field). This happens when the AST was generated from a
// Steel IDL that serialised its u8 discriminant directly into the
// discriminator field.
let is_steel_short_discriminator = !has_discriminant && disc_len == 1;
Copy link

@greptile-apps greptile-apps bot Mar 19, 2026

Choose a reason for hiding this comment

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

P2 Heuristic may conflict with get_discriminator() for 1-byte arrays

When is_steel_short_discriminator is true, discriminant_size is set to 1. Inside get_discriminator() (line 187), if self.discriminator is non-empty (e.g., [42]), it is returned as-is — so the returned Vec<u8> has length 1.

However, if any downstream consumer validates get_discriminator().len() == discriminant_size or slices transaction bytes using discriminant_size, the 1-byte vec returned for variant 2 is consistent with discriminant_size == 1. This is fine as long as all consumers are aligned.

The subtle risk is: if a program has a mix of instructions — some satisfying variant 1 (is_steel_discriminant) and others variant 2 (is_steel_short_discriminator) — the .all() still resolves to true and discriminant_size becomes 1. In that case, variant-1 instructions (which have no discriminator array at all, so self.discriminator is empty) fall through to get_discriminator()'s discriminant branch and return a 1-byte vec, which remains consistent. Mixed programs appear safe, but the interaction is non-obvious and worth a comment.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: hyperstack-idl/src/snapshot.rs
Line: 60

Comment:
**Heuristic may conflict with `get_discriminator()` for 1-byte arrays**

When `is_steel_short_discriminator` is true, `discriminant_size` is set to `1`. Inside `get_discriminator()` (line 187), if `self.discriminator` is non-empty (e.g., `[42]`), it is returned as-is — so the returned `Vec<u8>` has length 1.

However, if any downstream consumer validates `get_discriminator().len() == discriminant_size` or slices transaction bytes using `discriminant_size`, the 1-byte vec returned for variant 2 is consistent with `discriminant_size == 1`. This is fine as long as all consumers are aligned.

The subtle risk is: if a program has a mix of instructions — some satisfying variant 1 (`is_steel_discriminant`) and others variant 2 (`is_steel_short_discriminator`) — the `.all()` still resolves to `true` and `discriminant_size` becomes `1`. In that case, variant-1 instructions (which have no `discriminator` array at all, so `self.discriminator` is empty) fall through to `get_discriminator()`'s `discriminant` branch and return a 1-byte vec, which remains consistent. Mixed programs appear safe, but the interaction is non-obvious and worth a comment.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code


is_steel_discriminant || is_steel_short_discriminator
Comment on lines +56 to +62
Copy link

@greptile-apps greptile-apps bot Mar 19, 2026

Choose a reason for hiding this comment

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

P2 No tests added for new detection variant

The new is_steel_short_discriminator branch is the only untested code path in this file. The existing test in test_snapshot_serde uses an 8-byte discriminator and a null discriminant, which exercises neither variant 1 nor variant 2 of the Steel detection through the custom Deserialize impl (since the round-trip test bypasses raw JSON parsing of this heuristic).

A dedicated test like the following should be added to verify that the detection works end-to-end:

#[test]
fn test_steel_short_discriminator_detection() {
    let json = r#"{
        "name": "test_program",
        "version": "0.1.0",
        "accounts": [],
        "instructions": [
            {
                "name": "my_ix",
                "discriminator": [42],
                "accounts": [],
                "args": []
            }
        ],
        "types": [],
        "events": [],
        "errors": []
    }"#;
    let snapshot: IdlSnapshot = serde_json::from_str(json).expect("deserialize");
    assert_eq!(snapshot.discriminant_size, 1);
}

Without this, a regression that breaks variant 2 detection would go unnoticed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: hyperstack-idl/src/snapshot.rs
Line: 56-62

Comment:
**No tests added for new detection variant**

The new `is_steel_short_discriminator` branch is the only untested code path in this file. The existing test in `test_snapshot_serde` uses an 8-byte discriminator and a `null` discriminant, which exercises neither variant 1 nor variant 2 of the Steel detection through the custom `Deserialize` impl (since the round-trip test bypasses raw JSON parsing of this heuristic).

A dedicated test like the following should be added to verify that the detection works end-to-end:

```rust
#[test]
fn test_steel_short_discriminator_detection() {
    let json = r#"{
        "name": "test_program",
        "version": "0.1.0",
        "accounts": [],
        "instructions": [
            {
                "name": "my_ix",
                "discriminator": [42],
                "accounts": [],
                "args": []
            }
        ],
        "types": [],
        "events": [],
        "errors": []
    }"#;
    let snapshot: IdlSnapshot = serde_json::from_str(json).expect("deserialize");
    assert_eq!(snapshot.discriminant_size, 1);
}
```

Without this, a regression that breaks variant 2 detection would go unnoticed.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

})
})
.map(|is_steel| if is_steel { 1 } else { 8 })
Expand Down
Loading