Merged
Conversation
jpthor
reviewed
Feb 26, 2026
Contributor
jpthor
left a comment
jpthor
left a comment
There was a problem hiding this comment.
Good addition overall — clean structure, follows existing patterns, solid test coverage with SDK compat verification. A few things to address:
RaghavSood
reviewed
Feb 26, 2026
4others
force-pushed
the
feat/solana-send
branch
from
1c7511d to
b9fc729
Compare
WalkthroughAdds Solana support: new Solana client, balance and transaction-builder tools (native SOL and SPL), Solana RPC configuration, tool registration wiring, and chain-address resolution update to select EdDSA vs ECDSA keys. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/config.go (1)
3-43:⚠️ Potential issue | 🟠 MajorUse
envconfigwith struct tags instead of manualos.Getenv()calls.Configuration currently uses manual environment variable reads; must switch to
github.com/kelseyhightower/envconfigwith struct field tags per coding guidelines.🔧 Proposed refactor
-import "os" +import "github.com/kelseyhightower/envconfig" type Config struct { - ETHRPCURL string - CoinGeckoAPIKey string - BlockchairURL string - ThorchainURL string - SolanaRPCURL string + ETHRPCURL string `envconfig:"ETH_RPC_URL" default:"https://ethereum-rpc.publicnode.com"` + CoinGeckoAPIKey string `envconfig:"COINGECKO_API_KEY"` + BlockchairURL string `envconfig:"BLOCKCHAIR_API_URL" default:"https://api.vultisig.com/blockchair"` + ThorchainURL string `envconfig:"THORCHAIN_URL" default:"https://thornode.ninerealms.com"` + SolanaRPCURL string `envconfig:"SOLANA_RPC_URL" default:"https://api.mainnet-beta.solana.com"` } func Load() Config { - rpcURL := os.Getenv("ETH_RPC_URL") - if rpcURL == "" { - rpcURL = defaultETHRPCURL - } - blockchairURL := os.Getenv("BLOCKCHAIR_API_URL") - if blockchairURL == "" { - blockchairURL = defaultBlockchairURL - } - thorchainURL := os.Getenv("THORCHAIN_URL") - if thorchainURL == "" { - thorchainURL = defaultThorchainURL - } - solanaRPCURL := os.Getenv("SOLANA_RPC_URL") - if solanaRPCURL == "" { - solanaRPCURL = defaultSolanaRPCURL - } - return Config{ - ETHRPCURL: rpcURL, - CoinGeckoAPIKey: os.Getenv("COINGECKO_API_KEY"), - BlockchairURL: blockchairURL, - ThorchainURL: thorchainURL, - SolanaRPCURL: solanaRPCURL, - } + cfg := Config{} + _ = envconfig.Process("", &cfg) + return cfg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 3 - 43, Replace manual os.Getenv usage in the Load function by switching to github.com/kelseyhightower/envconfig: add env tags to the Config struct fields (e.g., ETHRPCURL, CoinGeckoAPIKey, BlockchairURL, ThorchainURL, SolanaRPCURL) mapping to ETH_RPC_URL, COINGECKO_API_KEY, BLOCKCHAIR_API_URL, THORCHAIN_URL, SOLANA_RPC_URL and provide default values via tags referencing the existing constants (defaultETHRPCURL, defaultBlockchairURL, defaultThorchainURL, defaultSolanaRPCURL); then update Load to call envconfig.Process into a Config instance and return it, handling and logging any envconfig errors instead of manual getenv fallback logic.
♻️ Duplicate comments (3)
internal/solana/client.go (2)
102-106:⚠️ Potential issue | 🟠 MajorUse strict integer parsing for token account amounts.
fmt.Sscanf("%d", ...)is permissive and can accept trailing garbage; parse this RPC field with strictstrconv.ParseUint.🔧 Proposed fix
import ( "context" "encoding/binary" "errors" "fmt" + "strconv" "strings" @@ - var amount uint64 - _, err = fmt.Sscanf(balance.Value.Amount, "%d", &amount) + amount, err := strconv.ParseUint(balance.Value.Amount, 10, 64) if err != nil { return 0, fmt.Errorf("parse token amount: %w", err) }In Go, does fmt.Sscanf with "%d" accept trailing characters (e.g., "123abc") without returning an error? What is the strict alternative for parsing uint64?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/solana/client.go` around lines 102 - 106, The current code uses fmt.Sscanf(balance.Value.Amount, "%d", &amount) which can accept trailing characters and isn't strict; replace this with strconv.ParseUint(balance.Value.Amount, 10, 64) to strictly parse the RPC amount into a uint64 (handle and wrap the returned error similarly), and assign the parsed value to the variable amount; update the error message in the same block to reflect "parse token amount" when wrapping the strconv error.
204-207:⚠️ Potential issue | 🟠 MajorUse idempotent ATA create to avoid TOCTOU transaction failures.
After the destination existence check, the ATA can be created before execution. With discriminator
0(non-idempotent create), the transfer tx can fail unnecessarily.🔧 Proposed fix
func buildCreateATAInstruction(payer, owner, mint, ataAddress, tokenProgram solana.PublicKey) solana.Instruction { @@ - []byte{0}, + []byte{1}, // CreateIdempotent ) }For the SPL Associated Token Account program, which discriminator value corresponds to CreateIdempotent, and is it intended for safe use when the ATA may already exist?Also applies to: 248-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/solana/client.go` around lines 204 - 207, The ATA creation uses the non-idempotent constructor which uses discriminator 0 and can cause TOCTOU failures; update the ATA creation to use the CreateIdempotent variant (discriminator 1) instead. Concretely, change the call that constructs the instruction in buildCreateATAInstruction (or replace it with a buildCreateIdempotentATAInstruction) so the associated-token instruction uses the idempotent discriminator (1) and keep the same args (fromOwner, toOwner, mint, destATA, tokenProgram); apply the same change to the other ATA creation site referenced around lines 248-260 so both places use the idempotent instruction. Ensure the program id and account ordering remain identical to the existing buildCreateATAInstruction API.internal/tools/build_spl_transfer_tx.go (1)
41-44:⚠️ Potential issue | 🟠 MajorRemove user-supplied
decimals; use mint decimals returned on-chain.
GetTokenProgramalready returns mint decimals, but they’re discarded. Requiringdecimalsfrom the caller introduces mismatch risk and can makeTransferCheckedfail for valid mints.🔧 Proposed fix
- mcp.WithNumber("decimals", - mcp.Description("Token decimals (required for transfer_checked instruction)."), - mcp.Required(), - ), ) } @@ - decimalsVal := req.GetFloat("decimals", -1) - if decimalsVal < 0 || decimalsVal > 255 || decimalsVal != float64(uint8(decimalsVal)) { - return mcp.NewToolResultError("missing or invalid decimals parameter"), nil - } - decimals := uint8(decimalsVal) - @@ - tokenProgram, _, err := solClient.GetTokenProgram(ctx, mintPubkey) + tokenProgram, decimals, err := solClient.GetTokenProgram(ctx, mintPubkey) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to detect token program: %v", err)), nil }Also applies to: 75-80, 96-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/build_spl_transfer_tx.go` around lines 41 - 44, Remove the user-supplied "decimals" parameter and instead use the on-chain mint decimals returned by GetTokenProgram; delete mcp.WithNumber("decimals", ...) from the command flags and remove any function parameters or option parsing that expect caller-provided decimals, then propagate the mint.Decimals value returned by GetTokenProgram (or the mint info) into the TransferChecked call (replace uses of the parsed "decimals" variable with the mint decimals). Apply the same changes to the other occurrences referenced (the additional mcp.WithNumber blocks and any code that reads or passes that parsed decimals at the other sites) so all TransferChecked invocations consistently use the on-chain mint decimals.
🧹 Nitpick comments (4)
internal/tools/solana_tx_test.go (2)
199-230: Avoid hardcoded mainnet RPC URL in integration tests.Using a hardcoded
https://api.mainnet-beta.solana.comURL makes the test dependent on external infrastructure and could cause flaky CI runs. Consider using an environment variable with a fallback.♻️ Suggested fix
func TestBuildSolanaTx_Integration(t *testing.T) { skipUnlessSolanaTest(t) from := solana.MustPublicKeyFromBase58("11111111111111111111111111111111") to := solana.MustPublicKeyFromBase58("11111111111111111111111111111111") - client := solanaclient.NewClient("https://api.mainnet-beta.solana.com") + rpcURL := os.Getenv("SOLANA_RPC_URL") + if rpcURL == "" { + rpcURL = "https://api.mainnet-beta.solana.com" + } + client := solanaclient.NewClient(rpcURL)Also add the import:
import "os"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/solana_tx_test.go` around lines 199 - 230, Replace the hardcoded mainnet RPC URL in TestBuildSolanaTx_Integration by reading a configurable environment variable (e.g., SOLANA_RPC_URL) with a sensible fallback to "https://api.mainnet-beta.solana.com"; update the call to solanaclient.NewClient to pass that variable instead of the literal string and add the "os" import so the test reads os.Getenv("SOLANA_RPC_URL") and uses the fallback when empty to avoid hardcoded external dependency in the test.
150-197: Consider simplifying the VaultDerived test logic.The test has complex conditional logic to handle both success and error paths. Since the RPC will always fail with
localhost:0, the success branch (lines 173-184) is effectively dead code. Consider restructuring to explicitly test the expected behavior.💡 Suggested simplification
func TestBuildSolanaTx_VaultDerived(t *testing.T) { store := vault.NewStore() store.Set("default", vault.Info{ ECDSAPublicKey: testECDSAPubKey, EdDSAPublicKey: testEdDSAPubKey, ChainCode: testChainCode, }) handler := handleBuildSolanaTx(store, solanaclient.NewClient("https://localhost:0")) ctx := context.Background() req := callToolReq("build_solana_tx", map[string]any{ "to": "11111111111111111111111111111111", "amount": "1000000", }) res, err := handler(ctx, req) if err != nil { t.Fatalf("unexpected Go error: %v", err) } - // The RPC call will fail (localhost:0), but address derivation should succeed first. - // We expect an RPC error, not an address derivation error. - if !res.IsError { - // If somehow it didn't error (unexpected), verify the result structure - text := resultText(t, res) - var result types.TransactionResult - err = json.Unmarshal([]byte(text), &result) - if err != nil { - t.Fatalf("unmarshal result: %v", err) - } - if result.Transactions[0].SigningMode != types.SigningModeEdDSA { - t.Errorf("signing mode = %q, want %q", result.Transactions[0].SigningMode, types.SigningModeEdDSA) - } - return - } - - // Verify the error is from RPC, not from address derivation - tc, ok := res.Content[0].(interface{ MarshalJSON() ([]byte, error) }) - if !ok { - return - } - data, _ := json.Marshal(tc) - errStr := string(data) - if strings.Contains(errStr, "vault info") || strings.Contains(errStr, "derive") { - t.Fatalf("expected RPC error, got address error: %s", errStr) - } + // Expected: RPC error (not address derivation error) since localhost:0 is unreachable + if !res.IsError { + t.Fatal("expected tool error from unreachable RPC") + } + + // Verify the error is from RPC, not from address derivation + errText := extractErrorText(res) + if strings.Contains(errText, "vault info") || strings.Contains(errText, "derive") { + t.Fatalf("expected RPC error, got address derivation error: %s", errText) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/solana_tx_test.go` around lines 150 - 197, The TestBuildSolanaTx_VaultDerived test contains a dead success branch because the RPC client to "https://localhost:0" will always fail; simplify the test by removing the success-path block that unmarshals TransactionResult and checks SigningMode, and instead assert res.IsError is true immediately after calling handler(ctx, req), then extract the error JSON (as the existing tc/MarshalJSON logic does) and assert the error string does not contain vault/address-derivation markers (e.g., "vault info" or "derive"); keep the existing handler invocation, ctx/req setup and the final RPC-vs-derivation error assertions but delete the unreachable verification of result.Transactions[0].SigningMode to make the test explicit and simpler.internal/tools/tools.go (1)
21-21: Consider using an options struct for RegisterAll parameters.The function signature now has 11 parameters, which is becoming unwieldy and error-prone. While not blocking, consider refactoring to use a configuration struct in a future iteration.
💡 Example refactor approach
type RegisterOptions struct { Store *vault.Store EthClient *ethereum.Client EvmSDK *evm.SDK ChainID *big.Int CGClient *coingecko.Client BCClient *blockchair.Client SwapSvc *swap.Service UTXOBuilder *btcsdk.Builder TCClient *thorchain.Client SolClient *solanaclient.Client } func RegisterAll(s *server.MCPServer, opts RegisterOptions) { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/tools.go` at line 21, The RegisterAll function has too many parameters; refactor it to accept a single options/config struct (e.g., RegisterOptions) that contains fields for Store, EthClient, EvmSDK, ChainID, CGClient, BCClient, SwapSvc, UTXOBuilder, TCClient, and SolClient, then change the signature from RegisterAll(s *server.MCPServer, store *vault.Store, ...) to RegisterAll(s *server.MCPServer, opts RegisterOptions) and update all call sites to construct and pass the RegisterOptions value accordingly; ensure internal uses inside RegisterAll access opts.Store, opts.EthClient, etc., and keep the original field names to minimize churn.internal/solana/client_test.go (1)
55-77: Consider adding assertion for expected ATA value.The test verifies determinism and non-zero result, but doesn't validate the actual derived ATA address. Adding an expected value assertion would catch regressions if the derivation logic changes.
💡 Suggested enhancement
func TestFindAssociatedTokenAddress(t *testing.T) { wallet := solana.MustPublicKeyFromBase58("7nYhDeFWriouc5PhCH98WCxocNPKfXjJqeFJo59DMKSA") mint := solana.MustPublicKeyFromBase58("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v") // USDC tokenProgram := solana.TokenProgramID + // Pre-computed expected ATA for this wallet/mint/program combination + expectedATA := solana.MustPublicKeyFromBase58("<expected_ata_address>") ata1, _, err := FindAssociatedTokenAddress(wallet, mint, tokenProgram) if err != nil { t.Fatalf("unexpected error: %v", err) } + if ata1 != expectedATA { + t.Errorf("ATA mismatch: got %s, want %s", ata1, expectedATA) + } + ata2, _, err := FindAssociatedTokenAddress(wallet, mint, tokenProgram)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/solana/client_test.go` around lines 55 - 77, Update TestFindAssociatedTokenAddress to assert the derived ATA equals the known expected address: call FindAssociatedTokenAddress (as already done) and compare ata1 (or ata2) against the canonical expected ATA string for the given wallet and USDC mint; fail the test if they differ. Reference the TestFindAssociatedTokenAddress test and the FindAssociatedTokenAddress function when adding the assertion so the test will catch regressions in ATA derivation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tools/solana_tx_test.go`:
- Around line 150-156: The test references shared test fixtures
(testECDSAPubKey, testEdDSAPubKey, testChainCode) and helpers (callToolReq,
resultText) that live in another test file; factor these into a single shared
test utility or duplicate them locally: either create a new test utility file
(e.g., testutil_test.go in the same package) and move the constant definitions
and helper functions there so both TestBuildSolanaTx_VaultDerived and the EVM
tests can import them, or copy the constants and helpers into solana_tx_test.go
directly; ensure the symbols (testECDSAPubKey, testEdDSAPubKey, testChainCode,
callToolReq, resultText) remain unexported but visible to tests in the package
and update any references accordingly.
---
Outside diff comments:
In `@internal/config/config.go`:
- Around line 3-43: Replace manual os.Getenv usage in the Load function by
switching to github.com/kelseyhightower/envconfig: add env tags to the Config
struct fields (e.g., ETHRPCURL, CoinGeckoAPIKey, BlockchairURL, ThorchainURL,
SolanaRPCURL) mapping to ETH_RPC_URL, COINGECKO_API_KEY, BLOCKCHAIR_API_URL,
THORCHAIN_URL, SOLANA_RPC_URL and provide default values via tags referencing
the existing constants (defaultETHRPCURL, defaultBlockchairURL,
defaultThorchainURL, defaultSolanaRPCURL); then update Load to call
envconfig.Process into a Config instance and return it, handling and logging any
envconfig errors instead of manual getenv fallback logic.
---
Duplicate comments:
In `@internal/solana/client.go`:
- Around line 102-106: The current code uses fmt.Sscanf(balance.Value.Amount,
"%d", &amount) which can accept trailing characters and isn't strict; replace
this with strconv.ParseUint(balance.Value.Amount, 10, 64) to strictly parse the
RPC amount into a uint64 (handle and wrap the returned error similarly), and
assign the parsed value to the variable amount; update the error message in the
same block to reflect "parse token amount" when wrapping the strconv error.
- Around line 204-207: The ATA creation uses the non-idempotent constructor
which uses discriminator 0 and can cause TOCTOU failures; update the ATA
creation to use the CreateIdempotent variant (discriminator 1) instead.
Concretely, change the call that constructs the instruction in
buildCreateATAInstruction (or replace it with a
buildCreateIdempotentATAInstruction) so the associated-token instruction uses
the idempotent discriminator (1) and keep the same args (fromOwner, toOwner,
mint, destATA, tokenProgram); apply the same change to the other ATA creation
site referenced around lines 248-260 so both places use the idempotent
instruction. Ensure the program id and account ordering remain identical to the
existing buildCreateATAInstruction API.
In `@internal/tools/build_spl_transfer_tx.go`:
- Around line 41-44: Remove the user-supplied "decimals" parameter and instead
use the on-chain mint decimals returned by GetTokenProgram; delete
mcp.WithNumber("decimals", ...) from the command flags and remove any function
parameters or option parsing that expect caller-provided decimals, then
propagate the mint.Decimals value returned by GetTokenProgram (or the mint info)
into the TransferChecked call (replace uses of the parsed "decimals" variable
with the mint decimals). Apply the same changes to the other occurrences
referenced (the additional mcp.WithNumber blocks and any code that reads or
passes that parsed decimals at the other sites) so all TransferChecked
invocations consistently use the on-chain mint decimals.
---
Nitpick comments:
In `@internal/solana/client_test.go`:
- Around line 55-77: Update TestFindAssociatedTokenAddress to assert the derived
ATA equals the known expected address: call FindAssociatedTokenAddress (as
already done) and compare ata1 (or ata2) against the canonical expected ATA
string for the given wallet and USDC mint; fail the test if they differ.
Reference the TestFindAssociatedTokenAddress test and the
FindAssociatedTokenAddress function when adding the assertion so the test will
catch regressions in ATA derivation.
In `@internal/tools/solana_tx_test.go`:
- Around line 199-230: Replace the hardcoded mainnet RPC URL in
TestBuildSolanaTx_Integration by reading a configurable environment variable
(e.g., SOLANA_RPC_URL) with a sensible fallback to
"https://api.mainnet-beta.solana.com"; update the call to solanaclient.NewClient
to pass that variable instead of the literal string and add the "os" import so
the test reads os.Getenv("SOLANA_RPC_URL") and uses the fallback when empty to
avoid hardcoded external dependency in the test.
- Around line 150-197: The TestBuildSolanaTx_VaultDerived test contains a dead
success branch because the RPC client to "https://localhost:0" will always fail;
simplify the test by removing the success-path block that unmarshals
TransactionResult and checks SigningMode, and instead assert res.IsError is true
immediately after calling handler(ctx, req), then extract the error JSON (as the
existing tc/MarshalJSON logic does) and assert the error string does not contain
vault/address-derivation markers (e.g., "vault info" or "derive"); keep the
existing handler invocation, ctx/req setup and the final RPC-vs-derivation error
assertions but delete the unreachable verification of
result.Transactions[0].SigningMode to make the test explicit and simpler.
In `@internal/tools/tools.go`:
- Line 21: The RegisterAll function has too many parameters; refactor it to
accept a single options/config struct (e.g., RegisterOptions) that contains
fields for Store, EthClient, EvmSDK, ChainID, CGClient, BCClient, SwapSvc,
UTXOBuilder, TCClient, and SolClient, then change the signature from
RegisterAll(s *server.MCPServer, store *vault.Store, ...) to RegisterAll(s
*server.MCPServer, opts RegisterOptions) and update all call sites to construct
and pass the RegisterOptions value accordingly; ensure internal uses inside
RegisterAll access opts.Store, opts.EthClient, etc., and keep the original field
names to minimize churn.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
CLAUDE.mdcmd/mcp-server/main.gogo.modinternal/config/config.gointernal/resolve/resolve.gointernal/solana/client.gointernal/solana/client_test.gointernal/tools/build_solana_tx.gointernal/tools/build_spl_transfer_tx.gointernal/tools/get_sol_balance.gointernal/tools/get_spl_token_balance.gointernal/tools/solana_tx_test.gointernal/tools/tools.go
4others
force-pushed
the
feat/solana-send
branch
from
b9fc729 to
73d7ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/solana/client.go (1)
62-84: Consider adding special handling or clearer error messaging for native SOL mint.The
GetTokenProgramfunction will fail when queried with the native SOL mint address (So11111111111111111111111111111111111111112), since native SOL is not owned by the token program. While current callers don't pass native SOL to this function, adding a guard clause or returning a sensible default (such assolana.TokenProgramID) would improve API robustness and provide clearer semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/solana/client.go` around lines 62 - 84, GetTokenProgram should special-case the native SOL mint address (So11111111111111111111111111111111111111112) to avoid the "not owned by token program" error; update GetTokenProgram to check if the provided mint equals the native SOL mint (use the SDK constant if available or the literal) and return a sensible default like (solana.TokenProgramID, 9, nil) or an explicit documented sentinel result instead of an error, with a short comment explaining why; keep existing logic for non-native mints (references: GetTokenProgram, mint parameter, owner, mintData.Decimals).internal/tools/solana_tx_test.go (1)
165-172: Gate live-RPC integration tests behind explicit opt-in.This test uses a real mainnet RPC URL. Relying only on
testing.Short()can still make non-short CI or local runs flaky/network-dependent.Proposed refactor
import ( "bytes" "context" "crypto/sha256" "encoding/binary" "encoding/hex" "encoding/json" + "os" "strings" "testing" @@ func skipUnlessSolanaTest(t *testing.T) { t.Helper() if testing.Short() { t.Skip("skipping Solana integration test in short mode") } + if os.Getenv("RUN_SOLANA_INTEGRATION") != "1" { + t.Skip("set RUN_SOLANA_INTEGRATION=1 to run Solana integration tests") + } }Also applies to: 335-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/solana_tx_test.go` around lines 165 - 172, Tests like TestBuildSolanaTx_Integration are hitting a real mainnet RPC and should be opt-in only; change skipUnlessSolanaTest to require an explicit environment flag (e.g., CHECK that os.Getenv("RUN_LIVE_SOLANA_TESTS") == "1" or similar) rather than relying on testing.Short(), then update TestBuildSolanaTx_Integration (and the other live Solana integration test referenced around lines 335-340) to call the revised skip helper so the tests are only run when the env flag is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 11-12: The go.mod currently pins github.com/gagliardetto/solana-go
v1.13.0 and github.com/gagliardetto/binary v0.8.0 which are unstable/introduce
breaking API changes; update or pin to a vetted stable version (consider v1.14.0
for solana-go) after a security review, run `go get`/mod tidy, and run full test
coverage; search for usages of EncodeCompactU16Length (or any binary package
calls) and update call sites to handle the new error return (check functions
that previously ignored a return value and add error handling/propagation), and
audit any solana-go API changes by locating uses of solana-go types/functions
and adjusting code to match the new signatures or lock to a safe version if
changes are unacceptable.
In `@internal/tools/solana_tx_test.go`:
- Around line 139-162: The test currently indexes res.Content[0] without
checking length and returns silently on a failed type assertion, allowing false
positives; update the error-path in the test that follows the res.IsError branch
to first assert len(res.Content) > 0 and call t.Fatalf if empty, then perform
the type assertion on res.Content[0] and call t.Fatalf with a clear message if
the assertion fails (don’t return), and also check the json.Marshal call for an
error before converting to string so any marshal failure fails the test; this
affects the variables/responses referenced as res.IsError, res.Content, tc (the
interface{ MarshalJSON() ... } assertion), and the json.Marshal result used to
build errStr.
---
Nitpick comments:
In `@internal/solana/client.go`:
- Around line 62-84: GetTokenProgram should special-case the native SOL mint
address (So11111111111111111111111111111111111111112) to avoid the "not owned by
token program" error; update GetTokenProgram to check if the provided mint
equals the native SOL mint (use the SDK constant if available or the literal)
and return a sensible default like (solana.TokenProgramID, 9, nil) or an
explicit documented sentinel result instead of an error, with a short comment
explaining why; keep existing logic for non-native mints (references:
GetTokenProgram, mint parameter, owner, mintData.Decimals).
In `@internal/tools/solana_tx_test.go`:
- Around line 165-172: Tests like TestBuildSolanaTx_Integration are hitting a
real mainnet RPC and should be opt-in only; change skipUnlessSolanaTest to
require an explicit environment flag (e.g., CHECK that
os.Getenv("RUN_LIVE_SOLANA_TESTS") == "1" or similar) rather than relying on
testing.Short(), then update TestBuildSolanaTx_Integration (and the other live
Solana integration test referenced around lines 335-340) to call the revised
skip helper so the tests are only run when the env flag is set.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
CLAUDE.mdcmd/mcp-server/main.gogo.modinternal/config/config.gointernal/resolve/resolve.gointernal/solana/client.gointernal/solana/client_test.gointernal/tools/build_solana_tx.gointernal/tools/build_spl_transfer_tx.gointernal/tools/get_sol_balance.gointernal/tools/get_spl_token_balance.gointernal/tools/solana_tx_test.gointernal/tools/tools.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/mcp-server/main.go
- internal/config/config.go
- internal/tools/tools.go
- internal/solana/client_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/tools/solana_tx_test.go (1)
140-151:⚠️ Potential issue | 🟡 MinorAdd length guard before accessing
result.Transactions[0].If
result.Transactionsis empty, line 148 will panic with an index out of range error.Proposed fix
err = json.Unmarshal([]byte(text), &result) if err != nil { t.Fatalf("unmarshal result: %v", err) } + if len(result.Transactions) == 0 { + t.Fatal("expected at least one transaction in result") + } if result.Transactions[0].SigningMode != types.SigningModeEdDSA { t.Errorf("signing mode = %q, want %q", result.Transactions[0].SigningMode, types.SigningModeEdDSA) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tools/solana_tx_test.go` around lines 140 - 151, Add a length check before indexing result.Transactions[0] in the test block that calls resultText(t, res) and unmarshals into types.TransactionResult: after unmarshalling into result (types.TransactionResult) assert len(result.Transactions) > 0 and fail the test with t.Fatalf or t.Errorf if it's zero to avoid an index out of range panic; only then check result.Transactions[0].SigningMode against types.SigningModeEdDSA. Ensure this change is applied inside the same test function that uses resultText and the local variable result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tools/solana_tx_test.go`:
- Around line 352-358: Before indexing res.Content, add a length check to avoid
a panic: verify len(res.Content) > 0 and call t.Fatalf (or t.Fatal) with a clear
message if it's zero, then continue to assert res.Content[0] is an
mcp.TextContent; update the block around the res and tc checks (the code dealing
with res.IsError, res.Content and the mcp.TextContent cast) so the test first
guards on content length before performing the type assertion.
---
Duplicate comments:
In `@internal/tools/solana_tx_test.go`:
- Around line 140-151: Add a length check before indexing result.Transactions[0]
in the test block that calls resultText(t, res) and unmarshals into
types.TransactionResult: after unmarshalling into result
(types.TransactionResult) assert len(result.Transactions) > 0 and fail the test
with t.Fatalf or t.Errorf if it's zero to avoid an index out of range panic;
only then check result.Transactions[0].SigningMode against
types.SigningModeEdDSA. Ensure this change is applied inside the same test
function that uses resultText and the local variable result.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modinternal/solana/client.gointernal/solana/client_test.gointernal/tools/build_spl_transfer_tx.gointernal/tools/solana_tx_test.go
webpiratt
approved these changes
Feb 27, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes vultisig/recipes#535
Summary by CodeRabbit
New Features
Configuration
Tools
Tests