feat: (PRO-345) Fixed inner instructions

[dev-jodee]

• Inner instructions weren't properly being handled
• Added parsing of Parsed / PartiallyDecoded instructions coming back from the RPC
• Added tests with inner instructions to make sure the inner instructions are properly used

---

Important

Enhanced inner instruction handling by adding parsing for Parsed and PartiallyDecoded instructions, with comprehensive tests and updated dependencies.

• Behavior:
  • Added parsing for Parsed and PartiallyDecoded instructions in instruction_util.rs.
  • Updated fetch_inner_instructions() in versioned_transaction.rs to handle new instruction types.
  • Added tests for inner instruction parsing in instruction_util.rs and versioned_transaction.rs.
• Dependencies:
  • Added solana-transaction-status to Cargo.toml and Cargo.lock.
  • Updated spl-associated-token-account version in Cargo.lock.
• Tests:
  • Added comprehensive tests in fee_estimation.rs for transaction fee estimation with various scenarios.
  • Updated test runner and transaction builder in test_runner.rs and transaction.rs to support new test cases.

^{This description was created by for ce2478a. You can customize this summary. It will automatically update as commits are pushed.}

---

📊 Unit Test Coverage

Unit Test Coverage: 85.4%

View Detailed Coverage Report

[linear]

PRO-345 More tests on fee estimate (double counting, etc)

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to dd3d84a in 3 minutes and 3 seconds. Click for details.

• Reviewed 2480 lines of code in 10 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. tests/rpc/fee_estimation.rs:90

• Draft comment:
  Consider adding dynamic checks/ranges for fee conversion values instead of relying solely on hardcoded expected numbers; this will ensure robustness if pricing or conversion logic changes.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. tests/src/bin/test_runner.rs:112

• Draft comment:
  Consider using structured logging instead of println! for production-like test reporting to enhance debuggability and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. tests/src/common/fixtures/kora-test.toml:29

• Draft comment:
  Review the allowed programs and tokens to ensure they stay up-to-date with current test requirements.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. tests/src/test_runner/kora.rs:7

• Draft comment:
  Consider using an async-aware mutex (e.g., tokio::sync::Mutex) for the global port tracker instead of std::sync::Mutex to avoid potential blocking in async contexts.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. Cargo.lock:7193

• Draft comment:
  Typo: The dependency "Inflector" is capitalized. Typically, the crate name is in lowercase ("inflector"). Please verify if this is intentional or if it should be corrected.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment points out something unusual, there are several reasons to delete it: 1) It's about a dependency in Cargo.lock which is auto-generated and not meant for manual editing 2) The comment is speculative, asking to "verify if this is intentional" rather than pointing out a clear issue 3) The capitalization likely comes from the upstream package and isn't something the PR author can control 4) The comment violates the rule about not commenting on dependency changes The capitalization is indeed unusual and could potentially indicate a mistake somewhere. Maybe there's a deeper issue that should be investigated? While unusual, the capitalization of a third-party dependency name in an auto-generated lockfile is not something that requires investigation or comment. The rules explicitly state not to comment on dependency changes or anything in auto-generated files. This comment should be deleted as it violates multiple rules - it's about dependencies, it's in an auto-generated file, and it's speculative rather than pointing out a clear issue.

Workflow ID: wflow_J2tSGoaf72klRgbq

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 803de5c in 1 minute and 1 seconds. Click for details.

• Reviewed 34 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/versioned_transaction.rs:152

• Draft comment:
  Ensure the flag change (replace_recent_blockhash: false) is intentional; consider adding a brief comment explaining why false is needed for inner instruction handling.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. tests/src/common/transaction.rs:314

• Draft comment:
  Good use of spl_token::state::Account::LEN instead of a magic number; this avoids hard-coded sizes.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_u9Jm3s5p9p6OOWaW

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed aeb91ca in 2 minutes and 45 seconds. Click for details.

• Reviewed 1105 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/instruction_util.rs:265

• Draft comment:
  Performance: Consider constructing the account keys hashmap just once rather than rebuilding it in multiple branches (e.g. lines 265, 272, 278). This can avoid redundant work if the same keys are passed in.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. crates/lib/src/transaction/instruction_util.rs:292

• Draft comment:
  In the PartiallyDecoded instruction branch, the use of filter_map (lines 294–302) silently drops account keys that fail to parse. Consider handling these errors explicitly to avoid unexpected omissions.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. crates/lib/src/transaction/instruction_util.rs:609

• Draft comment:
  For the SPL token burn (non-checked) case, using unwrap_or(0) when extracting the 'amount' (line 610) may silently default a missing field to 0. It might be more robust to propagate an error instead of defaulting.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern about error handling. Silently defaulting to 0 could mask issues and lead to unexpected behavior. However, looking at the context, this appears to be reconstruction logic for parsing instructions, not the actual burn execution. The code is trying to be lenient in parsing historical transactions. The unwrap_or(0) pattern is used deliberately here as a fallback. The comment raises a good point about robust error handling. Silent defaults can be dangerous. But am I missing some reason why lenient parsing would be preferable here? Yes - this is instruction reconstruction code that needs to handle historical transactions that may have varying formats. Being strict about parsing could break compatibility. The unwrap_or(0) is an intentional choice for compatibility. The comment should be deleted. While it raises a valid general concern about error handling, the lenient parsing with unwrap_or(0) is appropriate in this specific context of reconstructing historical transaction instructions.

4. crates/lib/src/transaction/instruction_util.rs:256

• Draft comment:
  Consider handling errors from bs58 decoding explicitly rather than unwrapping to default (e.g. in uncompile_instructions and in reconstruct_instruction_from_ui). Explicit error logging or propagation could aid debugging if decoding fails.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. crates/lib/src/transaction/instruction_util.rs:197

• Draft comment:
  Using a u8 for account index is likely fine given Solana’s constraints, but consider documenting the potential for overflow if the account keys exceed 256 elements.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

6. crates/lib/src/transaction/instruction_util.rs:248

• Draft comment:
  Overall, the new inner-instruction handling and reconstruction logic is comprehensive and well-tested. The helper methods improve code readability. Ensure that the error handling paths (returning None on errors) are aligned with how downstream callers are expected to behave.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

7. crates/lib/src/transaction/instruction_util.rs:245

• Draft comment:
  Typographical error: "force the RPC to not parsed the instructions" should be changed to "force the RPC to not parse the instructions" for grammatical correctness.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is technically correct about the grammar, it's a very minor issue in a docstring comment. The meaning is still clear even with the grammatical error. The rules state not to make comments that are obvious or unimportant. A small grammatical fix in documentation is not critical enough to warrant a comment. Could incorrect grammar in documentation lead to confusion for non-native English speakers? Should we maintain high standards for documentation quality? While documentation quality is important, this particular error is minor and doesn't impact understanding. The meaning is still clear even with the grammatical error. Delete the comment. The grammatical fix is too minor to warrant a PR comment.

Workflow ID: wflow_cu58a1cEAIACRGX5

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed ce2478a in 2 minutes and 1 seconds. Click for details.

• Reviewed 72 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/instruction_util.rs:262

• Draft comment:
  Avoid using unwrap_or_default() when decoding the bs58 data. Silently defaulting may hide decoding errors.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. crates/lib/src/transaction/instruction_util.rs:290

• Draft comment:
  Parsing account strings in partially decoded instructions silently drops failures via filter_map. Consider handling parse errors explicitly.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. crates/lib/src/transaction/instruction_util.rs:277

• Draft comment:
  For unsupported instruction types, a default stub is returned. Consider adding more documentation or even error‐handling so that unexpected instruction types aren’t silently ignored.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. crates/lib/src/transaction/instruction_util.rs:267

• Draft comment:
  The account_keys_hashmap is built separately in multiple branches. If performance becomes critical, consider caching it instead of recomputing.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

5. crates/lib/src/transaction/instruction_util.rs:213

• Draft comment:
  The helper function build_default_compiled_instruction returns a stub with empty accounts and data. A brief comment explaining its use (especially for inner instructions) would improve clarity.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

Workflow ID: wflow_dNFsVMsGbY5RVTtQ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}