Revert "Fix deserialization of squads txs with lookup tables"

[ngundotra]

Reverts #500

---

Important

Reverts handling of squads transactions with lookup tables, affecting message deserialization and related tests.

• Revert Changes:
  • Reverts handling of MessageV0 to Message in InspectorPage.tsx.
  • Removes address table lookup handling in convertVaultTransactionToMessage() in InspectorPage.tsx.
  • Adjusts InspectorInstructionCard in InstructionsSection.tsx to use staticAccountKeys.
• Tests:
  • Removes test case for squads transaction with lookup table in InspectorPage.spec.tsx.
  • Adjusts existing tests to reflect reverted behavior.
• Utilities:
  • Reverts changes in utils.ts related to address table lookups and transaction instruction creation.

^{This description was created by for bf017de. It will automatically update as commits are pushed.}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 20, 2025 0:10am

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to bf017de in 2 minutes and 4 seconds

More details

• Looked at 227 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. app/components/inspector/InstructionsSection.tsx:38

• Draft comment:
  Using message.staticAccountKeys[ix.programIdIndex] now bypasses any lookup table logic. If squads transactions use lookup tables, this might be problematic.
• 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%
  The comment raises a potentially valid technical concern about address lookup tables. However, it's speculative ("might be problematic") and doesn't provide clear evidence that this is actually an issue. The code still calls intoTransactionInstructionFromVersionedMessage() later, which presumably handles lookup tables correctly. Without seeing the implementation of that function or evidence of actual problems, this seems like an unnecessary speculation.
  I could be wrong about how lookup tables work in Solana. Maybe this is a real issue that could cause bugs.
  Even if lookup tables are a concern, the comment is speculative and doesn't provide clear evidence or a specific fix. It violates our rule about speculative "if X then Y" comments.
  Delete this comment as it's speculative and lacks clear evidence of an actual problem. If lookup tables are a real issue, it should be demonstrated with concrete examples.

2. app/components/inspector/__tests__/InspectorPage.spec.tsx:50

• Draft comment:
  The removal of test cases related to squads transactions with lookup tables reduces coverage for this feature. Ensure that necessary edge cases are tested upstream.
• 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. app/components/inspector/utils.ts:87

• Draft comment:
  Reverting the logic here: using originalMessage.staticAccountKeys.at(compiledInstruction.programIdIndex) ignores dynamic lookup table indices. This reversion may break squads transactions that previously relied on lookup table resolution.
• Reason this comment was not posted:
  Marked as duplicate.

4. app/components/inspector/InspectorPage.tsx:11

• Draft comment:
  Reverting from MessageV0 to Message removes the built‐in lookup table handling. Ensure that the computed header fields and accountKeys cover all cases for squads transactions.
• 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%
  The comment points out a real technical concern - MessageV0 has built-in lookup table support while Message is an older format. However, looking at the code, this appears to be an intentional change to simplify the conversion for Squads transactions. The author likely knows what they're doing. The comment is speculative ("ensure that...") and asks for verification rather than pointing out a clear issue.
  I could be wrong about the author's intentions - maybe they didn't realize they were removing lookup table support. The change could cause real issues if Squads transactions rely on lookup tables.
  While the concern is valid technically, the comment violates our rules by being speculative and asking for verification rather than pointing out a clear issue. If lookup tables were critical, this would likely break builds/tests.
  Delete the comment. While it raises a valid technical point, it's speculative and asks for verification rather than identifying a clear issue that needs fixing.

5. app/components/inspector/InspectorPage.tsx:221

• Draft comment:
  Instruction data is now being encoded with bs58. Confirm that downstream consumers expect base58 strings instead of raw Buffers.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to confirm that downstream consumers expect base58 strings instead of raw Buffers. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior is intended.

6. app/components/inspector/InspectorPage.tsx:226

• Draft comment:
  A static recentBlockhash (all zeros) is used. Verify this placeholder is acceptable and won’t affect transaction validation in production.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify if using a static recentBlockhash is acceptable. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue.

7. app/components/inspector/InstructionsSection.tsx:38

• Draft comment:
  The programId is now directly extracted from message.staticAccountKeys instead of using the converted instruction. Ensure this change aligns with handling lookup table entries.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  The comment is asking the author to ensure that the change aligns with handling lookup table entries. This is a request for confirmation, which violates the rules. However, it does point out a specific change in the code, which could be useful if rephrased to suggest checking or testing a specific aspect.

8. app/components/inspector/InstructionsSection.tsx:53

• Draft comment:
  Fallback in UnknownDetailsCard now uses 'Anchor Program' as programName. Confirm that this label is appropriate when the anchor program is not found.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This comment violates the rule about not asking authors to confirm their intentions. The comment is asking "Confirm that this label is appropriate..." which is exactly the kind of speculative verification we should avoid. Additionally, this seems like a straightforward UI label change that the author deliberately made.
  Perhaps this label change could cause confusion for users if an anchor program fails to load. Maybe there's a technical reason why "Unknown Program" was more accurate.
  The author clearly made this change intentionally, and it's in an error fallback case. Even if it's not perfect, it's a UI label that the author chose and we should trust their judgment on UI changes per the rules.
  Delete this comment as it violates the rule about asking authors to confirm their intentions and it's about a UI label change which we should trust the author on.

9. app/components/inspector/__tests__/InspectorPage.spec.tsx:14

• Draft comment:
  The test for 'renders Squads transaction with lookup table without crashing' has been removed. Ensure overall test coverage still validates squads transactions thoroughly.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that test coverage is still thorough after a test removal. This falls under asking the author to ensure something is tested, which is against the rules.

10. app/components/inspector/utils.ts:87

• Draft comment:
  The outer programId lookup logic (handling indices beyond staticAccountKeys) was removed. This may break squads vault transactions that rely on addressTableLookups for programId determination.
• 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%
  The comment appears technically correct - functionality was removed that handled a special case. However, the removed code was specifically noted as only being needed for "Squads vault transactions". This seems like a very specific edge case. Without more context about how common/important Squads vault transactions are, or whether this is actually used in practice, it's hard to be certain this is a real issue vs theoretical.
  I may be underestimating the importance of Squads vault transactions. This could be a critical feature that many users depend on.
  While that's possible, we should err on the side of not keeping speculative comments. If this is truly critical functionality, it should be documented somewhere or there should be tests that would catch the regression.
  The comment points out a real change but is too speculative about its impact. We don't have enough evidence that this will actually cause problems in practice.

Workflow ID: wflow_vrh5ttroiZ6JroaB

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Reverting from MessageV0 to Message removes the special handling for lookup tables. This change may break squads transactions that rely on address table lookups.

[ngundotra]

True!