Skip to content

Conversation

renan061
Copy link
Contributor

@renan061 renan061 commented Sep 26, 2025

Description

Backports #4213.


Note

Replaces the shared interfaces with per-chain clients (BTC/EVM/Solana/Sui/TON) and a Solana repo, updates observers/signers/orchestrator and mocks, and unifies outbound tracker retrieval.

  • Core/Interfaces:
    • Remove zetaclient/chains/interfaces; introduce per-chain clients (bitcoin, evm, solana, sui, ton) and interfaces/{tss,zetacore}.
    • Rename fields/methods (tsstssSigner; EVM client Signer() accessor).
    • Unify outbound tracker API to GetOutboundTrackers (remove ordered variant) and update callers.
  • Solana:
    • Add solana/repo abstraction; route reads (e.g., GetTransaction, signatures, healthcheck) through repo.
    • Update observer/signer to use SolanaClient + repo; adjust instruction/account types.
  • Bitcoin:
    • Replace RPC with BitcoinClient across common/observer/signer; adapt fee, mempool, and block queries.
  • EVM:
    • Update client types/aliases; observer/signer now depend on narrowed EVMClient interfaces; adjust tx signing and nonce fetching.
  • Sui & TON:
    • Introduce dedicated client interfaces; refactor observers/signers and tracker/reporting paths accordingly.
  • Orchestrator & Tools:
    • Rewire bootstrap to new clients; minor test/mocks updates; adjust mock generation paths.
    • Add changelog entry for preparing observer-signer client interfaces for dry mode.

Written by Cursor Bugbot for commit a2f0013. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Refactor

    • Standardized blockchain client interfaces across Bitcoin, EVM, Solana, Sui, and TON for observers and signers.
    • Simplified outbound tracker retrieval API.
    • Improved health-check paths via repository-backed clients (e.g., Solana).
    • Consistent naming for TSS signer across components.
  • Documentation

    • Changelog updated under Unreleased v36.0.0 with refactor notes.
  • Chores

    • Updated mocks, tests, and bootstrap wiring to new client interfaces.
    • Adjusted mock generation targets and constructor parameter orders to match the new APIs.

@renan061 renan061 self-assigned this Sep 26, 2025
@renan061 renan061 requested a review from a team as a code owner September 26, 2025 16:18
Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

📝 Walkthrough

Walkthrough

Refactors zetaclient to replace a monolithic interfaces layer with per-chain Client interfaces and repositories. Updates observers/signers across Bitcoin, EVM, Solana, Sui, and TON to depend on new clients, adjusts constructors and method signatures, migrates Solana logic to a repo wrapper, and replaces Zetacore GetAllOutboundTrackerByChain with GetOutboundTrackers. Updates mocks/tests and orchestrator wiring accordingly.

Changes

Cohort / File(s) Summary
Docs
changelog.md
Adds Unreleased v36.0.0 refactor entry for observer-signers dry-mode prep.
CLI cctx (Solana repo adoption)
cmd/zetatool/cctx/inbound.go, cmd/zetatool/cctx/outbound.go
Switch Solana tx fetch from solanarpc to solana/repo wrapper.
Base TSS field rename
zetaclient/chains/base/observer.go, .../observer_test.go, .../signer.go
Rename private field tss → tssSigner; update constructor param name; TSS() accessor adjusted.
Bitcoin API refactor
zetaclient/chains/bitcoin/client.go, .../common/fee.go, .../observer/*, .../signer/*
Introduce aggregated bitcoin.Client; rename RPC → BitcoinClient across fee calc, observer, signer; constructor param reorders; method calls updated; tests aligned.
EVM client/interface refactor
zetaclient/chains/evm/client.go, .../client/client.go, .../observer/*, .../signer/*, .../evm.go, .../observer/inbound_test.go, .../observer/outbound.go, tests
Add composite evm.Client; define observer.EVMClient and signer EVMClient; replace ethtypes with eth; expose Signer() accessor; swap tracker fetch API; mocks/tests updated.
Solana repo and client refactor
zetaclient/chains/solana/client.go, .../observer/*, .../repo/*, .../signer/*, tests
Add composite solana.Client; introduce repo.SolanaRepo and SolanaClient; migrate inbound/outbound/health to repo; update signer/observer to sol alias types and new client; adjust instruction types; tests updated.
Sui client and interfaces refactor
zetaclient/chains/sui/client.go, .../client/client.go, .../observer/*, .../signer/*, tests
Add composite sui.Client; collapse constructors to client.New(endpoint); define observer/signer SuiClient interfaces; replace generic RPC with suiClient; update orchestrations and tests.
TON client refactor
zetaclient/chains/ton/client.go, .../observer/*, .../signer/*, tests
Add composite ton.Client; replace RPC with TONClient in observer/signer; constructors updated; tracker and tx queries use tonClient; tests aligned.
Zetacore client API change
zetaclient/zetacore/client_crosschain.go, .../client_test.go
Rename and simplify: GetAllOutboundTrackerByChain(...) → GetOutboundTrackers(ctx, chainID); always ascending sort; remove interfaces.Order.
Interfaces package split
zetaclient/chains/interfaces/interfaces.go (deleted), .../tss.go, .../zetacore.go
Remove monolithic interfaces; reintroduce focused interfaces for TSSSigner and ZetacoreClient/Writer.
Orchestrator wiring
zetaclient/orchestrator/bootstrap.go, .../bootstrap_test.go, .../orchestrator_test.go, .../preflight_metrics_reporter.go
Instantiate standard per-chain clients; pass new client interfaces to observers/signers; use Solana repo HealthCheck; switch Sui constructor; update tracker API usage.
Mocks updates
zetaclient/chains/testutils/mocks/evm_client.go, zetaclient/testutils/mocks/evm_client.go, .../zetacore_client.go
Add new EVMClient mock; rename ObserverEvmClient; update ZetacoreClient mock signatures/types and tracker API name.
Scripts
scripts/mocks-generate.sh
Update mock generation path to evm/observer.
TON RPC test tweak
zetaclient/chains/ton/rpc/client_test.go
Loop changed to range-based form.
Misc tests rename/reorder
Multiple *_test.go across bitcoin/evm/solana/sui/ton
Align with new constructor argument orders, client types, and renamed fields (tssSigner).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Orch as Orchestrator
  participant BTC as bitcoin.Client
  participant EVM as evm.Client
  participant SOL as solana.Client
  participant SUI as sui.Client
  participant TON as ton.Client
  participant ZC as ZetacoreClient

  Orch->>BTC: create standardBitcoinClient
  Orch->>EVM: create standardEvmClient
  Orch->>SOL: create standardSolanaClient
  Orch->>SUI: create standardSuiClient
  Orch->>TON: create standardTonClient

  note over Orch,SOL: Solana health via repo
  Orch->>SOL: repo.New(solanaClient).HealthCheck(ctx)

  Orch->>ZC: GetOutboundTrackers(ctx, chainID)
  ZC-->>Orch: []OutboundTracker (ascending)
Loading
sequenceDiagram
  autonumber
  participant Obs as Solana Observer
  participant Repo as SolanaRepo
  participant RPC as SolanaClient

  Obs->>Repo: GetFirstSignatureForAddress(ctx, addr, limit)
  Repo->>RPC: GetSignaturesForAddressWithOpts(...)
  RPC-->>Repo: []*TransactionSignature
  Repo-->>Obs: first Signature

  Obs->>Repo: GetTransaction(ctx, sig)
  Repo->>RPC: GetTransaction(sig, opts)
  RPC-->>Repo: GetTransactionResult
  Repo-->>Obs: GetTransactionResult
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested labels

zetaclient, refactor, SOLANA_TESTS, TON_TESTS, SUI_TESTS

Suggested reviewers

  • lumtis
  • ws4charlie
  • brewmaster012
  • swift1337

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The pull request title currently reads as a truncated ellipsis and does not fully describe the primary change, leaving the intended scope ambiguous and failing to concisely summarize the key refactor for observer-signer client interfaces. It references “observe…” without completing the phrase, which undermines clarity for someone scanning the history. As such, the title does not meet the repository’s guidelines for a clear, self-contained summary. Please remove the ellipsis and revise the title to fully describe the change in a single clear sentence, for example: “refactor(backport): prepare per-chain client interfaces for observer-signers dry mode.”
Description Check ⚠️ Warning The pull request description includes a summary section but does not follow the repository’s template by omitting the “How Has This Been Tested?” heading and checklist, leaving out any information on unit tests, integration tests, localnet validation, or CI results. This missing section prevents reviewers from understanding how the changes were verified and reproducing the test steps. Without these details, the description does not comply with the required structure. Add a “How Has This Been Tested?” section with the required test descriptions and completed checkboxes (e.g., localnet CCTX tests, development environment, Go unit and integration tests, and any GitHub Actions results) so that reviewers can see how the change was verified.
Docstring Coverage ⚠️ Warning Docstring coverage is 48.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/backport-refactor-dry-mode

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.

Copy link

codecov bot commented Sep 26, 2025

Codecov Report

❌ Patch coverage is 18.50789% with 568 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.25%. Comparing base (d260236) to head (a2f0013).

Files with missing lines Patch % Lines
zetaclient/chains/testutils/mocks/evm_client.go 0.00% 394 Missing ⚠️
zetaclient/chains/solana/repo/repo.go 0.00% 22 Missing ⚠️
zetaclient/chains/solana/signer/execute_spl.go 0.00% 20 Missing ⚠️
zetaclient/chains/solana/signer/withdraw_spl.go 0.00% 18 Missing ⚠️
zetaclient/chains/solana/signer/execute.go 0.00% 12 Missing ⚠️
zetaclient/chains/solana/signer/signer.go 50.00% 10 Missing ⚠️
zetaclient/chains/solana/signer/whitelist.go 0.00% 10 Missing ⚠️
zetaclient/chains/ton/observer/observer.go 35.71% 7 Missing and 2 partials ⚠️
zetaclient/chains/solana/observer/inbound.go 0.00% 8 Missing ⚠️
zetaclient/chains/evm/client/client.go 0.00% 7 Missing ⚠️
... and 24 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4271      +/-   ##
===========================================
- Coverage    66.01%   65.25%   -0.76%     
===========================================
  Files          454      455       +1     
  Lines        33571    33978     +407     
===========================================
+ Hits         22161    22172      +11     
- Misses       10450    10845     +395     
- Partials       960      961       +1     
Files with missing lines Coverage Δ
zetaclient/chains/bitcoin/observer/db.go 93.61% <100.00%> (ø)
zetaclient/chains/bitcoin/observer/mempool.go 100.00% <100.00%> (ø)
zetaclient/chains/bitcoin/observer/utxos.go 80.31% <100.00%> (ø)
zetaclient/chains/bitcoin/observer/witness.go 81.63% <100.00%> (ø)
zetaclient/chains/bitcoin/signer/fee_bumper.go 97.22% <100.00%> (ø)
zetaclient/chains/bitcoin/signer/sign_rbf.go 88.67% <100.00%> (ø)
zetaclient/chains/evm/evm.go 56.70% <100.00%> (ø)
zetaclient/chains/evm/observer/observer.go 70.54% <100.00%> (ø)
zetaclient/chains/evm/signer/signer.go 45.91% <100.00%> (ø)
zetaclient/chains/sui/observer/inbound.go 64.47% <100.00%> (ø)
... and 43 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@renan061 renan061 force-pushed the feature/backport-logging-3 branch from 265c3c9 to 4234313 Compare September 26, 2025 16:34
@renan061 renan061 force-pushed the feature/backport-refactor-dry-mode branch from eef74eb to 8f824b5 Compare September 26, 2025 16:34
@renan061 renan061 force-pushed the feature/backport-logging-3 branch from 4234313 to e9998df Compare September 26, 2025 23:13
@renan061 renan061 force-pushed the feature/backport-refactor-dry-mode branch from 8f824b5 to c027ffd Compare September 26, 2025 23:13
@renan061 renan061 force-pushed the feature/backport-logging-3 branch from e9998df to 61fea73 Compare September 27, 2025 01:08
Base automatically changed from feature/backport-logging-3 to develop September 27, 2025 03:15
…r-signers for dry-mode (#4213)

refactor: prepare the client interfaces of the observer-signers for dry-mode
@renan061 renan061 force-pushed the feature/backport-refactor-dry-mode branch from c027ffd to a2f0013 Compare September 28, 2025 04:09
@renan061 renan061 added SOLANA_TESTS Run make start-solana-test TON_TESTS Runs TON E2E tests SUI_TESTS Run make start-sui-tests labels Sep 28, 2025
@renan061 renan061 enabled auto-merge September 28, 2025 04:12
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: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (10)
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1)

145-163: Fix P2WPKH scriptCode, hashType usage, and error handling in getTSSTX

  • Build the P2PKH scriptCode (OP_DUP OP_HASH160 OP_EQUALVERIFY OP_CHECKSIG) from tssSigner.PubKey().Bytes(true) and pass it to txscript.CalcWitnessSigHash instead of the P2WPKH output script.
  • Use the hashType parameter (not a hard-coded txscript.SigHashAll) when calling CalcWitnessSigHash.
  • Check err immediately after tssSigner.Sign—before slicing sig65B—to avoid nil-slice panics.
  • Optional: normalize S to low-S if the TSS doesn’t guarantee it.
zetaclient/chains/solana/signer/outbound_tracker_reporter.go (1)

61-67: Potential nil dereference on tx.Meta

rpc.GetTransaction may return a result with nil Meta; accessing tx.Meta.Err would panic. Guard both tx and tx.Meta before use.

-    tx, err := signer.solanaClient.GetTransaction(ctx, txSig, &rpc.GetTransactionOpts{
+    tx, err := signer.solanaClient.GetTransaction(ctx, txSig, &rpc.GetTransactionOpts{
       Commitment: rpc.CommitmentConfirmed,
     })
     if err != nil {
       continue
     }
 
     // exit goroutine if tx failed.
-    if tx.Meta.Err != nil {
+    if tx == nil || tx.Meta == nil {
+      continue
+    }
+    if tx.Meta.Err != nil {
       ...
     }

Also applies to: 71-82

zetaclient/zetacore/client_crosschain.go (1)

27-49: Missing pagination risks dropping recent trackers

We cap this query at 2 000 results with Reverse hardcoded to false, yet the comment advertises “returns all”. Once a chain accumulates more than 2 000 outbound trackers, this will only ever return the earliest nonces. Callers such as EVM.scheduleCCTX depend on recent tracker membership to tune scheduling intervals; without the latest entries, that logic will quietly degrade. Previously the order parameter let us flip the store iterator to grab the newest slice—this refactor removes that escape hatch. Please either restore a way to fetch the most recent window (e.g., expose the reverse flag) or paginate until we really do cover the full set; otherwise we’re shipping with a latent data-loss bug.

zetaclient/chains/evm/client/client.go (1)

57-66: Validate tx hash instead of silently coercing with HexToHash

Avoid accepting malformed hashes; parse with error handling to prevent querying with zero/invalid hashes.

Apply this diff:

-	hash := common.HexToHash(txHash)
+	var hash common.Hash
+	if err := hash.UnmarshalText([]byte(txHash)); err != nil {
+		return false, errors.Wrapf(err, "invalid tx hash %q", txHash)
+	}
zetaclient/chains/solana/observer/inbound.go (1)

60-63: Clarify log message

“got wrong amount of signatures” reads like an error. Prefer a neutral message (e.g., “found signatures”).

Apply this diff:

-			Msg("got wrong amount of signatures")
+			Msg("found new signatures")
zetaclient/orchestrator/bootstrap.go (1)

172-178: Don’t fail Solana bootstrap when the relayer key is absent (optional key).

The code returns an error if the relayer key is not found, despite the comment stating it is optional. Treat “not found” as non-fatal and proceed with a nil key.

Apply this diff:

-  password := chain.RelayerKeyPassword()
-  relayerKey, err := keys.LoadRelayerKey(app.Config().GetRelayerKeyPath(), chain.RawChain().Network, password)
-  if err != nil {
-    return nil, errors.Wrap(err, "unable to load relayer key")
-  }
+  var relayerKey *keys.RelayerKey
+  password := chain.RelayerKeyPassword()
+  rk, err := keys.LoadRelayerKey(app.Config().GetRelayerKeyPath(), chain.RawChain().Network, password)
+  if err != nil {
+    if errors.Is(err, keys.ErrRelayerKeyNotFound) || os.IsNotExist(err) {
+      // optional: proceed without relayer key
+    } else {
+      return nil, errors.Wrap(err, "unable to load relayer key")
+    }
+  } else {
+    relayerKey = rk
+  }

Additionally add the missing import:

// add to imports
import "os"
zetaclient/chains/solana/signer/execute_spl.go (1)

143-154: Avoid panics: replace MustPublicKeyFromBase58 with safe parse and propagate error

sol.MustPublicKeyFromBase58 will panic on malformed input; this is in the hot path of instruction creation. Parse and return an error instead.

Apply:

-		serializedInst, err := borsh.Serialize(contracts.ExecuteSPLRevertInstructionParams{
+		revertSender, err := sol.PublicKeyFromBase58(msg.Sender())
+		if err != nil {
+			return nil, errors.Wrapf(err, "invalid Solana sender %s", msg.Sender())
+		}
+		serializedInst, err := borsh.Serialize(contracts.ExecuteSPLRevertInstructionParams{
 			Discriminator: contracts.DiscriminatorExecuteSPLRevert,
 			Decimals:      msg.Decimals(),
 			Amount:        msg.Amount(),
-			Sender:        sol.MustPublicKeyFromBase58(msg.Sender()),
+			Sender:        revertSender,
 			Data:          msg.Data(),
 			Signature:     msg.SigRS(),
 			RecoveryID:    msg.SigV(),
 			MessageHash:   msg.Hash(),
 			Nonce:         msg.Nonce(),
 		})
zetaclient/chains/solana/signer/execute.go (1)

59-64: Validate amount fits in uint64 to avoid silent truncation

params.Amount.Uint64() will truncate if the underlying value exceeds uint64, producing incorrect lamports. Guard explicitly.

- amount := params.Amount.Uint64()
+ if !params.Amount.IsUint64() {
+   return nil, nil, errors.Errorf("amount does not fit into uint64: %s", params.Amount.String())
+ }
+ amount := params.Amount.Uint64()
zetaclient/chains/solana/repo/repo.go (1)

176-190: Nil-check blockTime to avoid potential panic

GetBlockTime can return a nil pointer when the cluster returns null. Calling Time() on nil will panic.

- blockTime, err := repo.solanaClient.GetBlockTime(ctx, slot)
+ blockTime, err := repo.solanaClient.GetBlockTime(ctx, slot)
  if err != nil {
    return time.Time{}, errors.Wrap(err, "unable to get latest block time")
  }
+ if blockTime == nil {
+   return time.Time{}, errors.New("latest block time is nil")
+ }
  return blockTime.Time(), nil
zetaclient/chains/evm/observer/observer.go (1)

176-186: Compile-time bug: slice index must be int, not uint

receipt.TransactionIndex is uint; indexing a slice with it fails to compile. Cast once and reuse.

-	if receipt.TransactionIndex >= uint(len(block.Transactions)) {
-		return fmt.Errorf("transaction index %d out of range [0, %d), txHash %s nonce %d block %d",
-			receipt.TransactionIndex, len(block.Transactions), tx.Hash(), tx.Nonce(), receipt.BlockNumber.Uint64())
-	}
-
-	txAtIndex := block.Transactions[receipt.TransactionIndex]
+	idx := int(receipt.TransactionIndex)
+	if idx >= len(block.Transactions) {
+		return fmt.Errorf("transaction index %d out of range [0, %d), txHash %s nonce %d block %d",
+			idx, len(block.Transactions), tx.Hash(), tx.Nonce(), receipt.BlockNumber.Uint64())
+	}
+
+	txAtIndex := block.Transactions[idx]
🧹 Nitpick comments (43)
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1)

59-63: Strengthen the test by verifying the TSS-signed tx.

After fixing the sighash, validate the witness using the script engine (StandardVerifyFlags) rather than only logging, to assert correctness independent of deterministic k/equality with wallet signature.

zetaclient/chains/ton/observer/outbound.go (1)

55-58: Switch to GetOutboundTrackers: confirm ordering semantics.

The prior call accepted an explicit Ascending order. Please confirm the new API returns a deterministic order (or sort by nonce locally) to avoid reprocessing/skips on restarts.

zetaclient/chains/solana/client.go (1)

12-16: No signature conflicts detected.

  • Verified that GetTransaction, GetAccountInfo, GetSlot, and GetSignaturesForAddressWithOpts share identical signatures across observer.SolanaClient, signer.SolanaClient, and repo.SolanaClient.
  • Optional: Consolidate the Client interface to embed only repo.SolanaClient (per TODO, see issue #4224) to eliminate redundancy.
zetaclient/chains/bitcoin/client.go (1)

9-13: Unified Bitcoin client interface: LGTM; add doc/mocks to cement the contract

The composition is clean and expresses intent well.

Consider adding a short package doc for the exported type and a mock generator directive to ease testing:

+// Client aggregates Bitcoin capabilities required by common, signer, and observer components.
 type Client interface {
   common.BitcoinClient
   signer.BitcoinClient
   observer.BitcoinClient
 }
zetaclient/chains/sui/observer/inbound.go (1)

126-134: Preserve original RPC error while keeping the sentinel for flow control

Wrap errTxNotFound with contextual details so the underlying RPC error isn't lost.

-    txFresh, err := ob.suiClient.SuiGetTransactionBlock(ctx, txReq)
+    txFresh, err := ob.suiClient.SuiGetTransactionBlock(ctx, txReq)
     if err != nil {
-      return errors.Wrap(errTxNotFound, err.Error())
+      return errors.Wrapf(errTxNotFound, "get tx %s: %v", event.TxHash, err)
     }
zetaclient/orchestrator/preflight_metrics_reporter.go (4)

61-66: Per-chain elapsed time measurement is off; start timer inside the loop

time.Since(start) currently measures from before the loop, not per chain.

-  // perform preflight check
-  start := time.Now()
-  for _, chain := range unsupportedChains {
+  // perform preflight check
+  for _, chain := range unsupportedChains {
+    start := time.Now()
     switch {
       ...
     }
     if err != nil {
       logger.Error().
         Err(err).
         Int64(logs.FieldChain, chain.ChainId).
-        Float64("time_taken", time.Since(start).Seconds()).
+        Float64("time_taken", time.Since(start).Seconds()).
         Msg("unable to report preflight metrics")
     }
   }

Also applies to: 79-85


147-150: Inject solana repo/client via dependencies; minor TODO typo

Prefer DI over inline construction to improve testability, and fix the comment typo.

-  // TODO: The Solana repositorty should be injected as a dependency into this function. We
-  // should not have to instantiate the Solana client here.
+  // TODO: The Solana repository should be injected as a dependency into this function.
+  // We should not have to instantiate the Solana client here.

136-156: Bound Solana preflight RPC with a per-call timeout

Avoid indefinite waits on flaky endpoints.

 func reportPreflightMetricsSolana(ctx context.Context, app *zctx.AppContext, chain chains.Chain) error {
   ...
-  blockTime, err := solrepo.New(rpcClient).HealthCheck(ctx)
+  localCtx, cancel := context.WithTimeout(ctx, 15*time.Second)
+  defer cancel()
+  blockTime, err := solrepo.New(rpcClient).HealthCheck(localCtx)
   if err != nil {
     return errors.Wrap(err, "unable to get solana last block time")
   }

176-191: Likewise, bound Sui preflight RPC with a timeout

Mirror the same defensive timeout for Sui.

 func reportPreflightMetricsSui(ctx context.Context, app *zctx.AppContext, chain chains.Chain) error {
   ...
-  suiClient := suiclient.New(cfg.Endpoint)
+  suiClient := suiclient.New(cfg.Endpoint)
+  localCtx, cancel := context.WithTimeout(ctx, 15*time.Second)
+  defer cancel()
-  blockTime, err := suiClient.HealthCheck(ctx)
+  blockTime, err := suiClient.HealthCheck(localCtx)
   if err != nil {
     return errors.Wrap(err, "unable to get sui last block time")
   }
zetaclient/chains/solana/signer/outbound_tracker_reporter.go (1)

50-60: Honor context cancellation inside the polling loop

Exit promptly when ctx is canceled to avoid orphan goroutines.

   start := time.Now()
   for {
+    select {
+    case <-ctx.Done():
+      logger.Info().Msg("context canceled; stopping tracker reporter")
+      return nil
+    default:
+    }
     // Solana block time is 0.4~0.8 seconds; wait 5 seconds between each check
     time.Sleep(5 * time.Second)
zetaclient/chains/solana/signer/increment_nonce.go (1)

95-123: Instruction construction with sol alias: LGTM; consider returning the interface type

Current return type is *sol.GenericInstruction. If signTx accepts the sol.Instruction interface, prefer returning sol.Instruction to decouple from the concrete type. If signTx requires *sol.GenericInstruction, keep as-is.

Would you like me to draft a follow-up that updates signTx and call sites to accept sol.Instruction if feasible?

zetaclient/chains/solana/signer/signer_test.go (1)

54-88: Explicitly set the chain in every test case

The second, third, and fourth table entries leave chain at its zero value, so base.NewSigner receives a chains.Chain{} with ChainId == 0. That currently works because the constructor only decorates log fields, but it obscures intent and will break if base.NewSigner starts enforcing a non-empty chain (or if metrics rely on the chain metadata). Please initialize the chain field for those cases to match the first entry so the wiring remains explicit and future-proof.

 		{
 			name:         "should create solana signer successfully without relayer key",
+			chain:        chain,
 			chainParams:  *chainParams,
 			solanaClient: nil,
 			tssSigner:    nil,
 			relayerKey:   nil,
 			ts:           nil,
 			logger:       base.DefaultLogger(),
 		},
 		{
 			name: "should fail to create solana signer with invalid gateway address",
+			chain: chain,
 			chainParams: func() observertypes.ChainParams {
 				cp := *chainParams
 				cp.GatewayAddress = "invalid"
 				return cp
 			}(),
@@
 		{
 			name:         "should fail to create solana signer with invalid relayer key",
+			chain:        chain,
 			chainParams:  *chainParams,
 			solanaClient: nil,
 			tssSigner:    nil,
 			relayerKey: &keys.RelayerKey{
 				PrivateKey: "3EMjCcCJg53fMEGVj13", // too short
zetaclient/chains/evm/client/client.go (1)

29-35: Dial options: HTTP client is ignored for ws:// endpoints

rpc.WithHTTPClient applies to http(s) only. If any caller passes ws:// or wss://, dialing may fail or ignore the custom client. Either restrict endpoints to http(s) or branch on scheme.

zetaclient/chains/solana/observer/inbound.go (2)

32-38: Persist initial last scanned signature

You set LastTxScanned in memory only. Consider persisting to DB so a restart doesn’t rescan from genesis for this address.


116-117: Update last scanned block even when processing signatures

You only call WithLastBlockScanned when no new signatures are found. Consider updating it after the loop too (when errSlot==nil) to keep metrics fresh.

Additional snippet outside the hunk:

// after the for-loop, before return nil
if errSlot == nil {
	ob.WithLastBlockScanned(lastSlot)
}
zetaclient/chains/solana/signer/withdraw_spl.go (2)

121-124: Reuse recipient ATA from message; avoid recomputation

MsgWithdrawSPL already embeds recipient ATA; recomputing risks divergence and adds an RPC/CPU hit if helpers evolve.

Apply this diff:

-	recipientAta, _, err := sol.FindAssociatedTokenAddress(msg.To(), msg.MintAccount())
-	if err != nil {
-		return nil, errors.Wrapf(err, "cannot find ATA for %s and mint account %s", msg.To(), msg.MintAccount())
-	}
+	recipientAta := msg.RecipientAta()

37-51: Fallback path coverage

prepareWithdrawSPLTx uses signMsgWithFallback and createOutboundWithFallback. Consider adding a unit test that exercises the fallback broadcasting path for SPL withdraws. Based on learnings.

zetaclient/orchestrator/bootstrap.go (2)

167-168: Minor: rename alias for clarity.

Consider renaming solbserver to solobserver (or simply solobserver) to avoid confusion at call sites. Behavior is unchanged.


161-166: Instrument Solana RPC client with timeouts/metrics via jsonrpc.NewClientWithOpts and rpc.NewWithCustomRPCClient

  httpClient, err := metrics.GetInstrumentedHTTPClient(cfg.Endpoint)
  if err != nil {
    return nil, errors.Wrap(err, "unable to create instrumented HTTP client")
  }
  // create a jsonrpc client with your custom HTTP client
  jsonrpcClient, err := jsonrpc.NewClientWithOpts(
    cfg.Endpoint,
    jsonrpc.RPCClientOpts{HTTPClient: httpClient},
  )
  if err != nil {
    return nil, errors.Wrap(err, "unable to initialize JSON-RPC client")
  }
  // pass the instrumented jsonrpc client into the Solana RPC wrapper
  standardSolanaClient := rpc.NewWithCustomRPCClient(jsonrpcClient)
  if standardSolanaClient == nil {
    return nil, errors.New("unable to create Solana RPC client")
  }
zetaclient/chains/bitcoin/observer/observer_test.go (1)

110-116: Fix typo in test name.

“env var us invalid” → “env var is invalid”.

-      name:           "should fail if env var us invalid",
+      name:           "should fail if env var is invalid",
zetaclient/chains/bitcoin/signer/signer.go (1)

67-71: Lower log level for raw transaction payload.

Hex-encoding entire tx at Info may be noisy; Debug is sufficient while preserving observability.

-  signer.Logger().Std.Info().
+  signer.Logger().Std.Debug().
     Stringer(logs.FieldTx, signedTx.TxHash()).
     Str("signer_tx_payload", hex.EncodeToString(outBuff.Bytes())).
     Msg("broadcasting transaction")
zetaclient/chains/solana/signer/execute_spl.go (1)

111-117: Prefer sol.Meta builder for consistency and clarity when mapping remaining accounts

Constructing *sol.AccountMeta manually is correct but inconsistent with the rest of the file and easy to get flags wrong. Use sol.Meta(pk) and chain .WRITE() when needed.

-remainingAccounts := []*sol.AccountMeta{}
-for _, a := range msg.Accounts {
-	remainingAccounts = append(remainingAccounts, &sol.AccountMeta{
-		PublicKey:  sol.PublicKey(a.PublicKey),
-		IsWritable: a.IsWritable,
-	})
-}
+remainingAccounts := make([]*sol.AccountMeta, 0, len(msg.Accounts))
+for _, a := range msg.Accounts {
+	meta := sol.Meta(sol.PublicKey(a.PublicKey))
+	if a.IsWritable {
+		meta = meta.WRITE()
+	}
+	remainingAccounts = append(remainingAccounts, meta)
+}
zetaclient/chains/solana/observer/observer_test.go (2)

19-27: Add t.Helper() to improve test failure reporting

Mark the factory as a helper to attribute assertion failures to the calling test.

 func MockSolanaObserver(
 	t *testing.T,
 	chain chains.Chain,
 	solanaClient observer.SolanaClient,
 	chainParams observertypes.ChainParams,
 	zetacoreClient interfaces.ZetacoreClient,
 	tssSigner interfaces.TSSSigner,
 ) *observer.Observer {
+	t.Helper()

41-51: Resource hygiene: close in-memory DB

Even for in-memory DBs, close handles to avoid descriptor leaks when tests scale.

 	database, err := db.NewFromSqliteInMemory(true)
 	require.NoError(t, err)
+	t.Cleanup(func() { _ = database.Close() })
zetaclient/chains/bitcoin/common/fee.go (2)

60-66: Avoid interface duplication across packages

This BitcoinClient partially overlaps with observer.BitcoinClient (broader surface). Divergent interfaces hinder reuse and mocks.

  • Extract a shared zetaclient/chains/bitcoin/client package with cohesive sub-interfaces (Core, Mempool, Blocks, Tx) and consume those here and in observers.
  • Alternatively, import the existing broader interface where feasible to avoid a second name for the same concept.

264-309: Protect against underflow near chain genesis when looking back blocks

On very short chains (test/dev), blockNumber-i can go negative. Clamp the start height.

-	highestRate := int64(0)
-	for i := int64(0); i < feeRateCountBackBlocks; i++ {
-		// get the block
-		hash, err := bitcoinClient.GetBlockHash(ctx, blockNumber-i)
+	highestRate := int64(0)
+	start := blockNumber - feeRateCountBackBlocks + 1
+	if start < 0 {
+		start = 0
+	}
+	for h := blockNumber; h >= start; h-- {
+		hash, err := bitcoinClient.GetBlockHash(ctx, h)
zetaclient/chains/solana/signer/execute.go (1)

112-119: Avoid MustPublicKeyFromBase58; handle parse errors instead

Using MustPublicKeyFromBase58 can panic on unexpected inputs (e.g., future call sites). You already validate the sender earlier; still, prefer explicit error handling here.

- serializedInst, err := borsh.Serialize(contracts.ExecuteRevertInstructionParams{
-   Discriminator: discriminator,
-   Amount:        msg.Amount(),
-   Sender:        sol.MustPublicKeyFromBase58(msg.Sender()),
+ senderPK, err := sol.PublicKeyFromBase58(msg.Sender())
+ if err != nil {
+   return nil, errors.Wrap(err, "invalid sender pubkey for revert execute")
+ }
+ serializedInst, err := borsh.Serialize(contracts.ExecuteRevertInstructionParams{
+   Discriminator: discriminator,
+   Amount:        msg.Amount(),
+   Sender:        senderPK,
zetaclient/chains/solana/repo/repo_test.go (2)

45-48: Avoid shadowing the imported repo package with a local variable

repo := repo.New(client) shadows the imported package name and hurts readability. Prefer solRepo or r.

- repo := repo.New(client)
+ solRepo := repo.New(client)

Apply similarly in all helper functions.

Also applies to: 65-68, 83-86, 108-111, 126-129


76-79: Reduce flakiness from hardcoded first signature

Devnet history can reset; asserting the exact first signature is brittle. Assert non-emptiness or properties instead.

- actualSig := "2tUQtcrXxtNFtV9kZ4kQsmY7snnEoEEArmu9pUptr4UCy8UdbtjPD6UtfEtPJ2qk5CTzZTmLwsbmZdLymcwSUcHu"
- require.Equal(t, actualSig, sig.String())
+ require.NotEmpty(t, sig.String())
zetaclient/chains/solana/repo/repo.go (3)

155-173: Use typed RPCError instead of string matching

Relying on strings.Contains for "-32015" is brittle. Prefer a type assertion to *solrpc.RPCError and check Code.

- switch {
- case err != nil && strings.Contains(err.Error(), errorCodeUnsupportedTransactionVersion):
-   return nil, ErrUnsupportedTxVersion
- case err != nil:
-   return nil, err
- default:
-   return txResult, nil
- }
+ if err != nil {
+   var rpcErr *solrpc.RPCError
+   if errors.As(err, &rpcErr) && rpcErr.Code == -32015 {
+     return nil, ErrUnsupportedTxVersion
+   }
+   return nil, err
+ }
+ return txResult, nil

60-101: Default pageLimit when unset and bound it

Guard against callers passing 0/negative pageLimit; also cap to a sane upper bound if desired.

 func (repo SolanaRepo) GetFirstSignatureForAddress(ctx context.Context,
   address sol.PublicKey,
   pageLimit int,
 ) (sol.Signature, error) {
+ if pageLimit <= 0 {
+   pageLimit = DefaultPageLimit
+ }

Apply similar handling in GetSignaturesForAddressUntil.


21-26: Unexport DefaultPageLimit or use it internally

The TODO notes this constant need not be exported. Either unexport it (defaultPageLimit) or start using it when pageLimit ≤ 0 as suggested above.

zetaclient/chains/sui/observer/observer.go (1)

34-45: Consider centralizing the Sui client contract

We now have an observer-scoped SuiClient alongside the signer variant, each carrying a slightly different method set. Keeping these interfaces in-package risks drift the next time we add or rename a method. Please consider moving a single shared definition into the chains/sui/client package (or another common location) and importing it here to keep the contract authoritative.

zetaclient/chains/sui/signer/signer.go (1)

32-68: Unify the Sui client interface definition

The signer now owns its own SuiClient interface while the observer keeps a parallel one; both describe the same concrete client but expose different subsets of methods. To avoid divergent contracts and redundant mock maintenance, please consolidate these into a single shared interface (for example in chains/sui/client) and have both signer/observer depend on that canonical type.

zetaclient/chains/evm/signer/signer.go (4)

71-86: Constructor should validate evmClient is non-nil

Prevent nil deref later by validating the dependency at construction time.

 func New(
 	baseSigner *base.Signer,
-	evmClient EVMClient,
+	evmClient EVMClient,
 	zetaConnectorAddress ethcommon.Address,
 	erc20CustodyAddress ethcommon.Address,
 	gatewayAddress ethcommon.Address,
 ) (*Signer, error) {
-	return &Signer{
+	if evmClient == nil {
+		return nil, errors.New("evm client is nil")
+	}
+	return &Signer{
 		Signer:               baseSigner,
 		evmClient:            evmClient,
 		zetaConnectorAddress: zetaConnectorAddress,
 		er20CustodyAddress:   erc20CustodyAddress,
 		gatewayAddress:       gatewayAddress,
 	}, nil
 }

355-365: Use SplitN to avoid truncating params containing ':'

Ensures admin command params can include colons.

-		msg := strings.Split(cctx.RelayedMessage, ":")
-		if len(msg) != 2 {
+		msg := strings.SplitN(cctx.RelayedMessage, ":", 2)
+		if len(msg) < 2 {
 			return nil, fmt.Errorf("invalid message %s", msg)
 		}

447-454: Add timeout to NonceAt to avoid hanging retries

Wrap the nonce query with the same broadcast timeout to bound each attempt.

-		latestNonce, err := signer.evmClient.NonceAt(ctx, signer.TSS().PubKey().AddressEVM(), nil)
+		ctxNonce, cancelNonce := context.WithTimeout(ctx, broadcastTimeout)
+		defer cancelNonce()
+		latestNonce, err := signer.evmClient.NonceAt(ctxNonce, signer.TSS().PubKey().AddressEVM(), nil)

193-201: Parameter ‘chainID’ is unused in newTx

Either use it (re-enable EIP‑1559 path) or drop it to reduce confusion.

Would you like me to provide a minimal diff to remove the param from newTx and its callers, or to restore the dynamic-fee branch behind gas.isLegacy?

zetaclient/chains/ton/observer/observer.go (1)

125-135: Consistent error context in CheckRPCStatus

Message says “TON client health” elsewhere; consider aligning wording for log/search consistency.

-	return errors.Wrap(err, "unable to check TON client health")
+	return errors.Wrap(err, "unable to check TON client health")

Note: If you prefer “RPC health”, update related logs accordingly for consistency across chains.

zetaclient/chains/evm/observer/observer.go (2)

85-102: Constructor should validate evmClient is non-nil

Guard against nil dependencies to avoid runtime panics.

-func New(baseObserver *base.Observer, evmClient EVMClient) (*Observer, error) {
+func New(baseObserver *base.Observer, evmClient EVMClient) (*Observer, error) {
+	if evmClient == nil {
+		return nil, errors.New("evm client is nil")
+	}
 	// create evm observer
 	ob := &Observer{
 		Observer:                      baseObserver,
 		evmClient:                     evmClient,
 		outboundConfirmedReceipts:     make(map[string]*eth.Receipt),
 		outboundConfirmedTransactions: make(map[string]*eth.Transaction),
 		priorityFeeConfig:             priorityFeeConfig{},
 	}

141-147: Minor: avoid double OutboundID computation under lock

Compute once to reduce work while holding the mutex.

 func (ob *Observer) setTxNReceipt(nonce uint64, receipt *eth.Receipt, transaction *eth.Transaction) {
 	ob.Mu().Lock()
 	defer ob.Mu().Unlock()
-	ob.outboundConfirmedReceipts[ob.OutboundID(nonce)] = receipt
-	ob.outboundConfirmedTransactions[ob.OutboundID(nonce)] = transaction
+	id := ob.OutboundID(nonce)
+	ob.outboundConfirmedReceipts[id] = receipt
+	ob.outboundConfirmedTransactions[id] = transaction
 }
zetaclient/chains/solana/observer/observer.go (1)

17-67: Unify the SolanaClient interface in a single package

We now have exported SolanaClient interfaces in observer, signer, and repo, each with a slightly different method set. This duplication is brittle—every signature tweak has to be synchronized across packages, and it only takes one missed update for the concrete client to stop compiling or to hide capability mismatches at runtime. Please move the interface to a shared location (for example, a dedicated chains/solana/interfaces package) and have all consumers import that single definition. It keeps the contract authoritative and avoids future drift.

zetaclient/chains/solana/signer/signer.go (1)

51-79: Reuse the SolanaClient interface defined for the observer/repo

The signer declares yet another exported SolanaClient interface, overlapping—but not identical—with the versions in observer and repo. This fragmentation guarantees drift as soon as we add or rename a method. Please factor the interface into a common package (or reuse the one introduced for the observer) so every Solana component depends on the same contract. It simplifies mocking and keeps the API surface coherent.

Comment on lines 29 to 33
chainID := ob.Chain().ChainId
trackers, err := ob.ZetacoreClient().GetAllOutboundTrackerByChain(ctx, chainID, interfaces.Ascending)
trackers, err := ob.ZetacoreClient().GetOutboundTrackers(ctx, chainID)
if err != nil {
return errors.Wrap(err, "unable to get all outbound trackers")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Restore deterministic ordering when processing outbound trackers.

Previously we explicitly requested interfaces.Ascending, guaranteeing we walk trackers in nonce order. The new GetOutboundTrackers API no longer exposes an order parameter, so unless the server happens to return them sorted we can hit a higher nonce first, fail the GetCctxByNonce lookup, and break—starving lower nonces that are actually ready. Please sort the slice locally (or otherwise guarantee ordering) before iterating.

 func (ob *Observer) ProcessOutboundTrackers(ctx context.Context) error {
 	chainID := ob.Chain().ChainId
 	trackers, err := ob.ZetacoreClient().GetOutboundTrackers(ctx, chainID)
 	if err != nil {
 		return errors.Wrap(err, "unable to get all outbound trackers")
 	}
+
+	sort.Slice(trackers, func(i, j int) bool {
+		return trackers[i].Nonce < trackers[j].Nonce
+	})

Remember to add the sort import at the top of the file.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chainID := ob.Chain().ChainId
trackers, err := ob.ZetacoreClient().GetAllOutboundTrackerByChain(ctx, chainID, interfaces.Ascending)
trackers, err := ob.ZetacoreClient().GetOutboundTrackers(ctx, chainID)
if err != nil {
return errors.Wrap(err, "unable to get all outbound trackers")
}
func (ob *Observer) ProcessOutboundTrackers(ctx context.Context) error {
chainID := ob.Chain().ChainId
trackers, err := ob.ZetacoreClient().GetOutboundTrackers(ctx, chainID)
if err != nil {
return errors.Wrap(err, "unable to get all outbound trackers")
}
// Ensure deterministic, ascending-nonce order before processing:
sort.Slice(trackers, func(i, j int) bool {
return trackers[i].Nonce < trackers[j].Nonce
})
// ... existing processing logic ...
}
🤖 Prompt for AI Agents
In zetaclient/chains/bitcoin/observer/outbound.go around lines 29 to 33, the
call to GetOutboundTrackers no longer enforces deterministic ordering and may
return trackers out of nonce order; before iterating you must sort the returned
trackers slice by nonce (ascending) so you always process lowest nonces first,
handle nil/zero nonce cases if applicable, and preserve existing error handling;
also add the "sort" import to the top of the file.

Comment on lines +109 to +112
// Overrides c.Client ChainID method.
func (c *Client) Signer() eth.Signer {
return c.signer
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix misleading comment on Signer accessor

The comment says it overrides ChainID, but the method returns the signer. Update the comment to avoid confusion.

Apply this diff:

-// Overrides c.Client ChainID method.
+// Signer returns the EVM signer for the connected chain.
 func (c *Client) Signer() eth.Signer {
 	return c.signer
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Overrides c.Client ChainID method.
func (c *Client) Signer() eth.Signer {
return c.signer
}
// Signer returns the EVM signer for the connected chain.
func (c *Client) Signer() eth.Signer {
return c.signer
}
🤖 Prompt for AI Agents
In zetaclient/chains/evm/client/client.go around lines 109 to 112, the comment
above the Signer() method incorrectly says it overrides ChainID while the method
actually returns the signer; update the comment to accurately describe the
accessor (e.g., "Signer returns the eth.Signer for this client" or "Accessor for
the client's signer") so it matches the method behavior and avoids confusion.

Comment on lines +53 to 54
ob, err := observer.New(baseObserver, solanaClient, chainParams.GatewayAddress)
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify constructor tolerates nil solanaClient

observer.New(baseObserver, solanaClient, ...) is passed a nil client in tests. Ensure observer.New and internals (e.g., repo.New) guard against nil before use.


🏁 Script executed:

#!/bin/bash
# Inspect constructor and usages for nil-guards
rg -n -C2 'func New\(' zetaclient/chains/solana/observer/observer.go
rg -n -C3 'solanaClient\.' zetaclient/chains/solana/observer/observer.go

Length of output: 133


🏁 Script executed:

#!/bin/bash
sed -n '80,140p' zetaclient/chains/solana/observer/observer.go
ls -1 zetaclient/chains/solana/observer

Length of output: 1828


🏁 Script executed:

#!/bin/bash
# List repository root to locate potential 'repo' package paths
ls -1

echo "=== solana directory tree ==="
ls -R zetaclient/chains/solana

echo "=== Locate SolanaClient type ==="
rg -n "type SolanaClient" --glob "*.go"

echo "=== Find package repo declarations ==="
rg -n "package repo" --glob "*.go"

echo "=== Find all repo.New references ==="
rg -n "repo.New" --glob "*.go"

echo "=== Inspect func New inside package repo ==="
rg -n -C3 "package repo" --glob "*.go" | rg -C3 "func New"

Length of output: 1977


🏁 Script executed:

#!/bin/bash
# Display the implementation of repo.New to check nil handling
sed -n '1,200p' zetaclient/chains/solana/repo/repo.go
rg -n -C3 'func New' zetaclient/chains/solana/repo/repo.go

Length of output: 6029


Enforce non-nil SolanaClient in observer.New
Add a nil check at the top of observer.New that returns an error if solanaClient is nil, and update the test in observer_test.go to supply a minimal stub SolanaClient instead of passing nil.

🤖 Prompt for AI Agents
In zetaclient/chains/solana/observer/observer_test.go around lines 53-54 and in
the observer.New constructor, add a nil check for the solanaClient parameter: if
solanaClient == nil return a descriptive error (e.g., fmt.Errorf("solanaClient
is nil")). Then update the test at lines 53-54 to stop passing nil and instead
instantiate a minimal stub type that implements the SolanaClient interface used
by observer (implement only the methods observer calls, returning simple
deterministic values or nil errors) and pass that stub into observer.New so the
test exercises the constructor without relying on a real client.

Comment on lines +189 to 200
predefinedAccounts := []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(pdaAta).WRITE(),
sol.Meta(msg.MintAccount()),
sol.Meta(msg.To()),
sol.Meta(destinationProgramPda).WRITE(),
sol.Meta(msg.RecipientAta()).WRITE(),
sol.Meta(sol.TokenProgramID),
sol.Meta(sol.SPLAssociatedTokenAccountProgramID),
sol.Meta(sol.SystemProgramID),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

❓ Verification inconclusive

Nil-deref risk: relayerKey is documented optional but dereferenced unconditionally

signer.relayerKey.PublicKey() will panic if relayerKey is not configured. Guard and return a clear error, or conditionally omit the relayer meta (if program does not require it).

Minimal guard:

 func (signer *Signer) createExecuteSPLInstruction(msg contracts.MsgExecuteSPL) (*sol.GenericInstruction, error) {
   // ...
-	pdaAta, _, err := sol.FindAssociatedTokenAddress(signer.pda, msg.MintAccount())
+	if signer.relayerKey == nil {
+		return nil, errors.New("relayer private key not configured")
+	}
+	pdaAta, _, err := sol.FindAssociatedTokenAddress(signer.pda, msg.MintAccount())
   if err != nil {
     return nil, errors.Wrapf(err, "cannot find ATA for %s and mint account %s", signer.pda, msg.MintAccount())
   }

If the relayer account is optional at runtime, gate the first element of predefinedAccounts behind the same check instead of failing hard. Please confirm program account constraints.


Guard relayerKey before dereference: add if signer.relayerKey == nil { return nil, errors.New("relayer private key not configured") } before calling signer.relayerKey.PublicKey(), or—if a nil relayer is allowed—wrap the first entry in predefinedAccounts to only include the relayer meta when signer.relayerKey != nil.

🤖 Prompt for AI Agents
In zetaclient/chains/solana/signer/execute_spl.go around lines 189 to 200, the
code dereferences signer.relayerKey without guarding for nil; add a nil-check
before building predefinedAccounts — e.g., if signer.relayerKey == nil { return
nil, errors.New("relayer private key not configured") } — or, if a missing
relayer is acceptable, construct predefinedAccounts conditionally by only
appending the relayer Meta when signer.relayerKey != nil; ensure imports include
errors if adding the error return.

Comment on lines +152 to +160
predefinedAccounts := []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(msg.To()).WRITE(),
sol.Meta(destinationProgramPda).WRITE(),
}
allAccounts := append(predefinedAccounts, msg.RemainingAccounts()...)

inst := &solana.GenericInstruction{
inst := &sol.GenericInstruction{
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Guard against nil relayerKey to prevent panic

createExecuteInstruction dereferences signer.relayerKey without a nil check. relayerKey is documented as optional; in dry/non-relay modes this will panic. Fail fast with a clear error or adjust account metas when no relayer is configured.

Apply this diff near the start of createExecuteInstruction, before building predefinedAccounts:

 func (signer *Signer) createExecuteInstruction(msg contracts.MsgExecute) (*sol.GenericInstruction, error) {
+  if signer.relayerKey == nil {
+    return nil, errors.New("relayer key is not configured; cannot build execute instruction")
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
predefinedAccounts := []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(msg.To()).WRITE(),
sol.Meta(destinationProgramPda).WRITE(),
}
allAccounts := append(predefinedAccounts, msg.RemainingAccounts()...)
inst := &solana.GenericInstruction{
inst := &sol.GenericInstruction{
func (signer *Signer) createExecuteInstruction(msg contracts.MsgExecute) (*sol.GenericInstruction, error) {
if signer.relayerKey == nil {
return nil, errors.New("relayer key is not configured; cannot build execute instruction")
}
predefinedAccounts := []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(msg.To()).WRITE(),
sol.Meta(destinationProgramPda).WRITE(),
}
allAccounts := append(predefinedAccounts, msg.RemainingAccounts()...)
inst := &sol.GenericInstruction{
// ...
}
// rest of function...
}
🤖 Prompt for AI Agents
In zetaclient/chains/solana/signer/execute.go around lines 152 to 160,
createExecuteInstruction currently dereferences signer.relayerKey when building
predefinedAccounts which will panic if relayerKey is nil; add an explicit nil
check before constructing predefinedAccounts and either return a clear error
immediately (e.g., "relayer key not configured for execute instruction") or
conditionally omit the relayer account meta and adjust account ordering when
running in dry/non-relay mode; ensure any downstream logic that expects the
relayer account handles the alternative path and update error messages to be
descriptive.

Comment on lines +126 to 139
inst := &sol.GenericInstruction{
ProgID: signer.gatewayID,
DataBytes: dataBytes,
AccountValues: []*solana.AccountMeta{
solana.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
solana.Meta(signer.pda).WRITE(),
solana.Meta(pdaAta).WRITE(),
solana.Meta(msg.MintAccount()),
solana.Meta(msg.To()),
solana.Meta(recipientAta).WRITE(),
solana.Meta(solana.TokenProgramID),
solana.Meta(solana.SPLAssociatedTokenAccountProgramID),
solana.Meta(solana.SystemProgramID),
AccountValues: []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(pdaAta).WRITE(),
sol.Meta(msg.MintAccount()),
sol.Meta(msg.To()),
sol.Meta(recipientAta).WRITE(),
sol.Meta(sol.TokenProgramID),
sol.Meta(sol.SPLAssociatedTokenAccountProgramID),
sol.Meta(sol.SystemProgramID),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Nil-pointer risk: relayerKey may be nil

relayerKey is optional per struct docs, but you unconditionally access relayerKey.PublicKey() and mark it SIGNER. This will panic when relayerKey is unset.

Apply this diff:

 func (signer *Signer) createWithdrawSPLInstruction(msg contracts.MsgWithdrawSPL) (*sol.GenericInstruction, error) {
+	if signer.relayerKey == nil {
+		return nil, fmt.Errorf("relayer key is not configured")
+	}

And add fmt to imports:

 import (
 	"context"
 
 	"cosmossdk.io/errors"
+	"fmt"
 	sol "github.com/gagliardetto/solana-go"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
inst := &sol.GenericInstruction{
ProgID: signer.gatewayID,
DataBytes: dataBytes,
AccountValues: []*solana.AccountMeta{
solana.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
solana.Meta(signer.pda).WRITE(),
solana.Meta(pdaAta).WRITE(),
solana.Meta(msg.MintAccount()),
solana.Meta(msg.To()),
solana.Meta(recipientAta).WRITE(),
solana.Meta(solana.TokenProgramID),
solana.Meta(solana.SPLAssociatedTokenAccountProgramID),
solana.Meta(solana.SystemProgramID),
AccountValues: []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(pdaAta).WRITE(),
sol.Meta(msg.MintAccount()),
sol.Meta(msg.To()),
sol.Meta(recipientAta).WRITE(),
sol.Meta(sol.TokenProgramID),
sol.Meta(sol.SPLAssociatedTokenAccountProgramID),
sol.Meta(sol.SystemProgramID),
},
import (
"context"
"fmt"
"cosmossdk.io/errors"
sol "github.com/gagliardetto/solana-go"
)
func (signer *Signer) createWithdrawSPLInstruction(msg contracts.MsgWithdrawSPL) (*sol.GenericInstruction, error) {
if signer.relayerKey == nil {
return nil, fmt.Errorf("relayer key is not configured")
}
inst := &sol.GenericInstruction{
ProgID: signer.gatewayID,
DataBytes: dataBytes,
AccountValues: []*sol.AccountMeta{
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER(),
sol.Meta(signer.pda).WRITE(),
sol.Meta(pdaAta).WRITE(),
sol.Meta(msg.MintAccount()),
sol.Meta(msg.To()),
sol.Meta(recipientAta).WRITE(),
sol.Meta(sol.TokenProgramID),
sol.Meta(sol.SPLAssociatedTokenAccountProgramID),
sol.Meta(sol.SystemProgramID),
},
// ...
}
// ...
}
🤖 Prompt for AI Agents
In zetaclient/chains/solana/signer/withdraw_spl.go around lines 126-139,
relayerKey is optional but code unconditionally calls relayerKey.PublicKey() and
marks it SIGNER, which can panic if nil; guard this by checking if
signer.relayerKey != nil before creating and appending the
sol.Meta(signer.relayerKey.PublicKey()).WRITE().SIGNER() entry and otherwise
append an alternative AccountMeta (or omit the signer entry) appropriate for the
instruction, ensuring account ordering remains correct; also add fmt to the file
imports as requested.

Comment on lines +22 to +25
// EVMClient is an autogenerated mock type for the EVMClient type
type EVMClient struct {
mock.Mock
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Mock does not implement signer interface

Line [22]: EVMClient is missing the NonceAt, IsTxConfirmed, and Signer methods required by signer.EVMClient (see zetaclient/chains/evm/signer/signer.go). Any test wiring this mock into signer code will now fail to compile because the composite client interface cannot be satisfied. Please regenerate the mock (or add the missing methods) so that it fully implements the new client surface.

+// IsTxConfirmed provides a mock function with given fields: ctx, txHash, confirmations
+func (_m *EVMClient) IsTxConfirmed(ctx context.Context, txHash string, confirmations uint64) (bool, error) {
+	ret := _m.Called(ctx, txHash, confirmations)
+
+	if len(ret) == 0 {
+		panic("no return value specified for IsTxConfirmed")
+	}
+
+	var r0 bool
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, string, uint64) (bool, error)); ok {
+		return rf(ctx, txHash, confirmations)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, string, uint64) bool); ok {
+		r0 = rf(ctx, txHash, confirmations)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, string, uint64) error); ok {
+		r1 = rf(ctx, txHash, confirmations)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// NonceAt provides a mock function with given fields: ctx, account, blockNumber
+func (_m *EVMClient) NonceAt(ctx context.Context, account common.Address, blockNumber *big.Int) (uint64, error) {
+	ret := _m.Called(ctx, account, blockNumber)
+
+	if len(ret) == 0 {
+		panic("no return value specified for NonceAt")
+	}
+
+	var r0 uint64
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, common.Address, *big.Int) (uint64, error)); ok {
+		return rf(ctx, account, blockNumber)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, common.Address, *big.Int) uint64); ok {
+		r0 = rf(ctx, account, blockNumber)
+	} else {
+		r0 = ret.Get(0).(uint64)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, common.Address, *big.Int) error); ok {
+		r1 = rf(ctx, account, blockNumber)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// Signer provides a mock function with no fields
+func (_m *EVMClient) Signer() types.Signer {
+	ret := _m.Called()
+
+	if len(ret) == 0 {
+		panic("no return value specified for Signer")
+	}
+
+	var r0 types.Signer
+	if rf, ok := ret.Get(0).(func() types.Signer); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(types.Signer)
+		}
+	}
+
+	return r0
+}
🤖 Prompt for AI Agents
In zetaclient/chains/testutils/mocks/evm_client.go around lines 22–25, the
autogenerated EVMClient mock is missing the NonceAt, IsTxConfirmed, and Signer
methods required to satisfy signer.EVMClient; regenerate the mock or add
implementations for these three methods with the exact signatures from
zetaclient/chains/evm/signer/signer.go, forwarding calls to the underlying mock
(mock.MethodCalled) and returning the expected result types and errors so the
mock compiles where signer.EVMClient is expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli SOLANA_TESTS Run make start-solana-test SUI_TESTS Run make start-sui-tests TON_TESTS Runs TON E2E tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants