Fix Solana execute crash on Relay's raw-instruction bridge quotes - #513
Fix Solana execute crash on Relay's raw-instruction bridge quotes#513kome12 wants to merge 3 commits into
Conversation
Relay's Solana-source bridge quotes return uncompiled
{instructions, addressLookupTableAddresses} instead of a ready-to-sign
transaction, unlike Jupiter (base64) and OKX ({data: base58}). This shape
fell through the existing normalization and crashed on Buffer.from(object,
'base64'), so `trade execute` failed for any Solana bridge where Relay won
the aggregator price race.
Compile the raw instructions into a static-keys VersionedTransaction
(address lookup tables are a size optimization, not a correctness
requirement, so skipping them is valid as long as the transaction fits
Solana's packet limit — this throws instead of silently building an
oversized one).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pr-reviewer Summary for #e11c234✅ No issues found The code review completed successfully with no findings. Review effort: 3/5 (Moderate) SummaryThis PR correctly fixes a crash in Overall assessment: The fix is targeted and safe. Token usage: 1,917 input, 2,882 output, 382,716 cache read, 38,457 cache write | Usage Guide New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment |
…structions Address review feedback: compileRawSolanaTransaction picked its fee-payer by trusting whichever account the quote itself marked isSigner:true, with no check against the wallet actually about to sign, and didn't reject a raw instruction set requiring more than one signature — either would silently sign the wrong account (or leave a signature slot empty) and fail on-chain with an opaque error instead of a clear pre-flight one. Also: validate the signer/signature-count before spending a network round trip on a real blockhash (recentBlockhash is fixed-size regardless of value, so a placeholder is exact for that preflight), decode instruction data with an explicit optional-0x-prefix strip instead of raw Buffer.from(hex) (bare, non-prefixed hex — the real shape Relay returns), and fail with an actionable message instead of a raw TypeError when an instruction is missing its accounts list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
decodeInstructionData called .startsWith unconditionally, so a Relay instruction omitting `data` (valid at the protocol level — some instructions legitimately carry none) threw a raw TypeError instead of compiling correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
I think we're still missing one important safety check here.
This now compiles and signs whatever program IDs, accounts, and instruction data Relay returns. We verify that the connected wallet is the signer and that the transaction fits, but we don't verify what the transaction actually does. If the Relay/trading response is compromised, we could still sign an arbitrary SOL/SPL transfer, authority change, or very high fee.
Can we validate the compiled transaction against the original trade intent before signing — expected token, amount, recipient and programs; no unrelated authority changes; and capped fees? Simulation would be a useful extra guard, but I don't think it should be the only check.
The focused tests and CI are green. Reviewed at e11c234ee2711e963667e688d045812861d6fcf4.
|
To help move this forward, here is a specific remediation plan for the Solana raw-instruction validation:
This would bring the Solana raw-instruction path up to the same security standard we have for EVM trades in |
Ah, sorry, I should have replied here. I have PRs to work on hardening the Solana side so I'll look at this PR again once those are merged. |
There will probably be another PR in addition to the above PRs. I'll ping here again once this is ready for review. In the meantime, let me switch this back to draft. |
Summary
trade executenormalized Solana quote transactions into two known shapes — Jupiter (base64 string) and OKX ({data: base58}) — but Relay's Solana-source bridge quotes return a third, uncompiled shape ({instructions, addressLookupTableAddresses}). That shape fell through and crashed onBuffer.from(object, 'base64'), so any Solana→EVM bridge failed whenever Relay won the aggregator price race, even thoughtrade quotereported success.VersionedTransaction, keeping all accounts static (address lookup tables are a size optimization, not a correctness requirement — skipped for now, with a clear error if a future quote is ever too large to fit without them).Test plan
npm test— all existing tests pass, plus new unit tests covering: Jupiter/OKX passthrough (no RPC call), compiling a real captured Relay-shaped quote into a valid, signable transaction (byte-level assertions), an oversized-transaction error path, and a missing-signer error path.npm run lint--aggregator relay). Confirmed the exact previously-crashing transaction shape, and it signed + broadcast successfully. Verified the destination balance increased by the quoted amount (bridge completed).🤖 Generated with Claude Code