Skip to content

Conversation

@ws4charlie
Copy link
Contributor

@ws4charlie ws4charlie commented Sep 26, 2025

This PR is a PoC to support Solana Address Lookup Table. See the corresponding PRs

  • In node: 4427
  • In solana-protocol-contracts: 128

Summary by CodeRabbit

  • Bug Fixes

    • More reliable Solana transaction processing with automatic fallback between message formats.
    • Improved handling of address lookup tables and writable indexes to reduce transaction failures.
    • Clearer, concise errors when decoding cannot proceed.
  • Refactor

    • Centralized construction, signing, and submission of SOL and SPL token transfers.
    • Message-format-aware transaction building with consistent post-transaction logging.
    • Reduced duplication by consolidating shared logic.
  • Chores

    • Public API and inputs remain unchanged.

@ws4charlie ws4charlie requested review from a team, lumtis and skosito as code owners September 26, 2025 02:55
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

📝 Walkthrough

Walkthrough

Rewrote Solana execution flow in src/chains/solana/execute.ts: added two-step message decoding (non-ALT then ALT), introduced centralized helpers for creating/signing/submitting SOL and SPL transactions, and added ALT-aware transaction construction and submission while preserving public API and error semantics.

Changes

Cohort / File(s) Change summary
Primary implementation
src/chains/solana/execute.ts
Added internal helpers decodeMessage and decodeMessageALT (two-step decode). Introduced execute, executeSplToken, and executeTransaction helpers to centralize instruction creation, signing, v0 (ALT) vs legacy transaction building, submission, and confirmation. Refactored main flow to use these helpers and preserved existing public API surface and logging/error behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as solanaExecute
    participant Decoder as decodeMessage
    participant DecoderALT as decodeMessageALT
    participant Executor as execute / executeSplToken
    participant TxBuilder as executeTransaction
    participant RPC as RPC/Network

    Caller->>Decoder: attempt non-ALT decode
    alt non-ALT decode succeeds
        Decoder-->>Caller: decoded message (non-ALT)
    else non-ALT decode fails
        Caller->>DecoderALT: attempt ALT decode
        alt ALT decode succeeds
            DecoderALT-->>Caller: decoded message (ALT)
        else both fail
            DecoderALT-->>Caller: throw decode error
        end
    end

    Caller->>Executor: create instructions & signers (SOL or SPL)
    Executor->>TxBuilder: build transaction (v0 if ALT else legacy)
    TxBuilder->>RPC: sendRawTransaction
    RPC-->>TxBuilder: tx signature
    TxBuilder->>RPC: confirm transaction
    RPC-->>Caller: final confirmation / result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “feat: add support for Solana ALT” directly summarizes the primary change of the pull request, which is introducing Address Lookup Table support for Solana, and it does so in concise, clear language without extraneous detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-solana-ALT

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ws4charlie ws4charlie requested a review from fadeev September 26, 2025 02:55
@ws4charlie ws4charlie changed the title Add support for Solana ALT feat: add support for Solana ALT Sep 26, 2025
@github-actions github-actions bot added the feat label Sep 26, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (4)
src/chains/solana/execute.ts (4)

391-394: Use resolved tokenDecimals in logging

Ensure the log reflects the actual decimals used.

-      decimals
+      tokenDecimals

114-117: Log the error object, not string interpolation

Interpolating err risks “[object Object]”. Pass the error as structured metadata to retain stack/message.

-    logger.error(`Error executing Gateway execute: ${err}`, {
-      chain: NetworkID.Solana,
-    });
+    logger.error("Error executing Gateway execute", {
+      chain: NetworkID.Solana,
+      err,
+    });

56-74: Preserve decode failure context for troubleshooting

Add debug logs for both decoding attempts to aid diagnosis, without changing user-facing errors.

   try {
     ({ remainingAccounts, data } = decodeMessage(message));
   } catch (nonAltError) {
+    logger.debug(`Non-ALT decode failed: ${(nonAltError as Error).message}`, {
+      chain: NetworkID.Solana,
+    });
     try {
       ({ altAccount, remainingAccounts, data } = await decodeMessageALT(
         message,
         connection
       ));
     } catch (altError) {
+      logger.debug(`ALT decode failed: ${(altError as Error).message}`, {
+        chain: NetworkID.Solana,
+      });
       throw new Error(
         `Failed to decode message as neither non-ALT nor ALT format`
       );
     }
   }

402-434: Unify confirmation strategy for consistency

For non-ALT, confirmTransaction(signature, "confirmed") does not bind to the blockhash used for send. Prefer the same latest-blockhash pattern as ALT for deterministic confirmation.

   } else {
     // build transaction without ALT
-    const transaction = new anchor.web3.Transaction().add(executeIx);
-    signature = await connection.sendTransaction(transaction, [payer]);
-    await connection.confirmTransaction(signature, "confirmed");
+    const latestBh = await connection.getLatestBlockhash();
+    const transaction = new anchor.web3.Transaction({
+      feePayer: payer.publicKey,
+      recentBlockhash: latestBh.blockhash,
+    }).add(executeIx);
+    signature = await connection.sendTransaction(transaction, [payer]);
+    await connection.confirmTransaction({ signature, ...latestBh }, "confirmed");
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between aa34e31 and c993283.

📒 Files selected for processing (1)
  • src/chains/solana/execute.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/chains/solana/execute.ts (3)
src/chains/solana/constants.ts (1)
  • payer (24-26)
src/logger.ts (1)
  • logger (47-47)
src/constants.ts (1)
  • NetworkID (13-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: cli-integration
🔇 Additional comments (1)
src/chains/solana/execute.ts (1)

120-145: Non-ALT decoding looks correct

Decodes accounts and data and maps to AccountMeta with correct writability handling.

@fadeev
Copy link
Member

fadeev commented Nov 4, 2025

@ws4charlie what's the status of this PR? Is it compatible with protocol contracts solana v6? Should we review and merge it?

@ws4charlie
Copy link
Contributor Author

@ws4charlie what's the status of this PR? Is it compatible with protocol contracts solana v6? Should we review and merge it?

This one was created according to node PR , which was recreated as 4226. Can @skosito confirm there is no significant gap between the 4226 and latest develop ALT implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants