From 0ea5199fbd29f0dd2d3707227831a6b7641e9071 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 12:09:11 -0600 Subject: [PATCH 01/23] feat: add WithExecutePayers ctx function for setting solana accounts as signers in proposal transformation Signed-off-by: Pablo --- .../solana/timelock_bypass_payer_collision.go | 241 ++++++++++++++++++ sdk/solana/context.go | 32 +++ sdk/solana/timelock_converter.go | 26 +- sdk/solana/timelock_converter_test.go | 117 +++++++++ 4 files changed, 415 insertions(+), 1 deletion(-) create mode 100644 e2e/tests/solana/timelock_bypass_payer_collision.go create mode 100644 sdk/solana/context.go diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go new file mode 100644 index 00000000..055cba58 --- /dev/null +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -0,0 +1,241 @@ +//go:build e2e + +package solanae2e + +import ( + "context" + "encoding/json" + "time" + + "github.com/ethereum/go-ethereum/common" + + "github.com/gagliardetto/solana-go" + "github.com/gagliardetto/solana-go/programs/system" + "github.com/gagliardetto/solana-go/rpc" + + "github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/v0_1_1/timelock" + + "github.com/smartcontractkit/mcms" + e2eutils "github.com/smartcontractkit/mcms/e2e/utils/solana" + "github.com/smartcontractkit/mcms/sdk" + solanasdk "github.com/smartcontractkit/mcms/sdk/solana" + "github.com/smartcontractkit/mcms/types" +) + +var ( + testPDASeedBypassPayerWith = [32]byte{'t', 'e', 's', 't', '-', 'b', 'y', 'p', 'a', 's', 's', '-', 'p', 'a', 'y', 'e', 'r', '-', 'w'} + testPDASeedBypassPayerWithout = [32]byte{'t', 'e', 's', 't', '-', 'b', 'y', 'p', 'a', 's', 's', '-', 'p', 'a', 'y', 'e', 'r', '-', 'n'} +) + +const bypassPayerTransferLamports = 1_000_000 // 0.001 SOL + +// TestBypassExecutePayerInRemainingAccounts covers the Solana bypass failure +// where the execute payer (the deployer key) also appears in the +// BypasserExecuteBatch op's remaining_accounts. +// +// Real-world shape: a BPF-loader `upgrade` instruction lists the deployer as the +// spill/close recipient — a writable, non-signer account. Off-chain the Solana +// converter forces every remaining account to IsSigner=false before computing +// the Merkle root, so the deployer is hashed with IsSigner=false. At execution +// time the same deployer key is the outer transaction fee payer, so the Solana +// runtime presents it to the MCM program as IsSigner=true. The MCM program +// rebuilds the Merkle leaf from the runtime account infos, hashes IsSigner=true, +// and the one-bit mismatch invalidates the proof -> ProofCannotBeVerified. +// +// This test uses a system.Transfer whose recipient is the deployer/executor +// wallet to reproduce the identical one-bit collision without deploying an +// upgradeable program + buffer. +// +// - "with execute payers": the proposal is converted with +// solanasdk.WithExecutePayers, so the converter marks the executor account +// IsSigner=true before the root is computed and the bypass executes cleanly. +// - "without execute payers": the same proposal converted without the context +// override still fails with ProofCannotBeVerified, documenting the bug and +// guarding against the fix silently becoming a no-op. +func (s *TestSuite) TestBypassExecutePayerInRemainingAccounts() { + s.Run("with execute payers in context: bypass succeeds", func() { + s.runBypassPayerCollision(testPDASeedBypassPayerWith, true) + }) + s.Run("without execute payers in context: proof fails", func() { + s.runBypassPayerCollision(testPDASeedBypassPayerWithout, false) + }) +} + +// runBypassPayerCollision drives the full bypass flow (convert -> set config -> +// sign -> set root -> execute) for a batch whose inner instruction sends +// lamports to the executor wallet. When injectExecutePayer is true, the executor +// is threaded through context so the converter marks it as a signer and the +// bypass execute succeeds; otherwise the final op fails with +// ProofCannotBeVerified. +func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bool) { + // --- arrange --- + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + s.T().Cleanup(cancel) + + // wallet is the deployer key: MCM executor / outer transaction fee payer. + wallet, err := solana.PrivateKeyFromBase58(privateKey) + s.Require().NoError(err) + + s.SetupMCM(seed) + s.SetupTimelock(seed, 1*time.Second) + + mcmSignerPDA, err := solanasdk.FindSignerPDA(s.MCMProgramID, seed) + s.Require().NoError(err) + // The MCM signer PDA drives the bypass instructions, so it must hold the + // bypasser role. + s.AssignRoleToAccounts(ctx, seed, wallet, []solana.PublicKey{mcmSignerPDA}, timelock.Bypasser_Role) + + timelockSignerPDA, err := solanasdk.FindTimelockSignerPDA(s.TimelockProgramID, seed) + s.Require().NoError(err) + + // Fund the timelock signer PDA (transfer source, signs via CPI) and the mcm + // signer PDA. + e2eutils.FundAccounts(s.T(), []solana.PublicKey{mcmSignerPDA, timelockSignerPDA}, 1, s.SolanaClient) + + mcmAddress := solanasdk.ContractAddress(s.MCMProgramID, seed) + timelockAddress := solanasdk.ContractAddress(s.TimelockProgramID, seed) + + // --- inner "spill-like" instruction --- + // Transfer lamports from the timelock signer PDA to the deployer wallet. + // The recipient (wallet) is a writable, non-signer account: exactly the + // role a BPF upgrade spill account plays in production. + transferIx, err := system.NewTransferInstruction(bypassPayerTransferLamports, timelockSignerPDA, wallet.PublicKey()). + ValidateAndBuild() + s.Require().NoError(err) + + transferTx, err := solanasdk.NewTransactionFromInstruction(transferIx, "System", + []string{"bypass-payer-collision"}) + s.Require().NoError(err) + + batchOp := types.BatchOperation{ + ChainSelector: s.ChainSelector, + Transactions: []types.Transaction{transferTx}, + } + + // --- chain metadata --- + opCount, err := solanasdk.NewInspector(s.SolanaClient).GetOpCount(ctx, mcmAddress) + s.Require().NoError(err) + metadata, err := solanasdk.NewChainMetadata(opCount, s.MCMProgramID, seed, + s.Roles[timelock.Proposer_Role].AccessController.PublicKey(), + s.Roles[timelock.Canceller_Role].AccessController.PublicKey(), + s.Roles[timelock.Bypasser_Role].AccessController.PublicKey()) + s.Require().NoError(err) + + // --- bypass proposal --- + timelockProposal, err := mcms.NewTimelockProposalBuilder(). + SetVersion("v1"). + SetValidUntil(2051222400). // 2035-01-01T12:00:00 UTC + SetDescription("bypass proposal: executor payer appears in remaining_accounts"). + SetOverridePreviousRoot(true). + SetDelay(types.NewDuration(1*time.Second)). + SetAction(types.TimelockActionBypass). + AddTimelockAddress(s.ChainSelector, timelockAddress). + AddChainMetadata(s.ChainSelector, metadata). + AddOperation(batchOp). + Build() //nolint:contextcheck //OPT-400 + s.Require().NoError(err) + + converters := map[types.ChainSelector]sdk.TimelockConverter{ + s.ChainSelector: solanasdk.TimelockConverter{}, + } + + // The fix: when the expected execute payer is threaded through context, the + // converter marks that account IsSigner=true so the off-chain Merkle root + // matches what the runtime presents at execution time. + if injectExecutePayer { + ctx = solanasdk.WithExecutePayers(ctx, solanasdk.ExecutePayers{ + s.ChainSelector: wallet.PublicKey(), + }) + } + + mcmsProposal, _, err := timelockProposal.Convert(ctx, converters) + s.Require().NoError(err) + + // The executor wallet lands in the final BypasserExecuteBatch op as a + // writable remaining account. Its IsSigner flag must reflect whether the + // execute-payer override was applied. + s.assertExecutorSignerBit(mcmsProposal, wallet.PublicKey(), injectExecutePayer) + + // --- set config + sign + set root --- + signerEVMAccount := NewEVMTestAccount(s.T()) + mcmConfig := types.Config{Quorum: 1, Signers: []common.Address{signerEVMAccount.Address}} + configurer := solanasdk.NewConfigurer(s.SolanaClient, wallet, s.ChainSelector) + _, err = configurer.SetConfig(ctx, mcmAddress, &mcmConfig, true) + s.Require().NoError(err) + + inspectors := map[types.ChainSelector]sdk.Inspector{s.ChainSelector: solanasdk.NewInspector(s.SolanaClient)} + signable, err := mcms.NewSignable(&mcmsProposal, inspectors) //nolint:contextcheck //OPT-400 + s.Require().NoError(err) + _, err = signable.SignAndAppend(mcms.NewPrivateKeySigner(signerEVMAccount.PrivateKey)) //nolint:contextcheck //OPT-400 + s.Require().NoError(err) + + encoders, err := mcmsProposal.GetEncoders() //nolint:contextcheck //OPT-400 + s.Require().NoError(err) + encoder := encoders[s.ChainSelector].(*solanasdk.Encoder) + executors := map[types.ChainSelector]sdk.Executor{ + s.ChainSelector: solanasdk.NewExecutor(encoder, s.SolanaClient, wallet), + } + executable, err := mcms.NewExecutable(&mcmsProposal, executors) //nolint:contextcheck //OPT-400 + s.Require().NoError(err) + + _, err = executable.SetRoot(ctx, s.ChainSelector) + s.Require().NoError(err) + + // --- act + assert --- + // The set-up ops (init/append/finalize bypasser operation) never include the + // executor key, so their proofs verify regardless. Only the final + // BypasserExecuteBatch op carries the executor key in its remaining accounts. + lastOp := len(mcmsProposal.Operations) - 1 + s.Require().Positive(lastOp, "expected multiple bypass ops") + + balanceBefore := s.lamports(ctx, timelockSignerPDA) + + for i := range mcmsProposal.Operations { + _, execErr := executable.Execute(ctx, i) + switch { + case i < lastOp: + s.Require().NoError(execErr, "unexpected failure on op %d before BypasserExecuteBatch", i) + case injectExecutePayer: + s.Require().NoError(execErr, "BypasserExecuteBatch should succeed once the execute payer is a signer") + default: + s.Require().Error(execErr, "expected BypasserExecuteBatch to fail due to execute-payer signer collision") + s.Require().ErrorContains(execErr, "ProofCannotBeVerified") + } + } + + if injectExecutePayer { + // The inner transfer actually moved lamports out of the timelock signer PDA. + balanceAfter := s.lamports(ctx, timelockSignerPDA) + s.Require().Equal(balanceBefore-bypassPayerTransferLamports, balanceAfter, + "timelock signer PDA should have sent exactly the transfer amount") + } +} + +// assertExecutorSignerBit checks the executor key appears in the last converted +// op (BypasserExecuteBatch) as a writable remaining account with the expected +// IsSigner flag. +func (s *TestSuite) assertExecutorSignerBit(proposal mcms.Proposal, executor solana.PublicKey, wantSigner bool) { + s.Require().NotEmpty(proposal.Operations) + lastOp := proposal.Operations[len(proposal.Operations)-1] + + var fields solanasdk.AdditionalFields + s.Require().NoError(json.Unmarshal(lastOp.Transaction.AdditionalFields, &fields)) + + found := false + for _, acc := range fields.Accounts { + if acc.PublicKey.Equals(executor) { + found = true + s.Require().Equal(wantSigner, acc.IsSigner, "executor IsSigner flag mismatch in converted bypass op") + s.Require().True(acc.IsWritable, "executor (transfer recipient) should be writable") + } + } + s.Require().True(found, "executor key must appear in the BypasserExecuteBatch remaining accounts") +} + +// lamports returns the current lamport balance of the given account. +func (s *TestSuite) lamports(ctx context.Context, account solana.PublicKey) uint64 { + res, err := s.SolanaClient.GetBalance(ctx, account, rpc.CommitmentConfirmed) + s.Require().NoError(err) + + return res.Value +} diff --git a/sdk/solana/context.go b/sdk/solana/context.go new file mode 100644 index 00000000..c2d441c9 --- /dev/null +++ b/sdk/solana/context.go @@ -0,0 +1,32 @@ +package solana + +import ( + "context" + + "github.com/gagliardetto/solana-go" + + "github.com/smartcontractkit/mcms/types" +) + +// executePayersKey is the unexported context key for the expected execute payers. +type executePayersKey struct{} + +// ExecutePayers maps a chain selector to the public key that will pay for (and +// therefore sign) the MCM execute transaction on that chain. +type ExecutePayers map[types.ChainSelector]solana.PublicKey + +// WithExecutePayers returns a copy of ctx carrying the expected execute payer +// per chain. This is consumed by the Solana TimelockConverter when converting +// bypass operations: if the payer also appears in an operation's remaining +// accounts, it must be hashed as a signer in the off-chain Merkle root so that +// on-chain proof verification (which always sees the fee payer as a signer) +// succeeds. See ConvertBatchToChainOperations. +func WithExecutePayers(ctx context.Context, payers ExecutePayers) context.Context { + return context.WithValue(ctx, executePayersKey{}, payers) +} + +// ExecutePayersFrom returns the ExecutePayers stored in ctx, if any. +func ExecutePayersFrom(ctx context.Context) (ExecutePayers, bool) { + payers, ok := ctx.Value(executePayersKey{}).(ExecutePayers) + return payers, ok +} diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index f9c8cf06..1eeaa540 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -103,7 +103,18 @@ func (t TimelockConverter) ConvertBatchToChainOperations( case types.TimelockActionBypass: accounts, rerr := getAccountsFromBatchOperation(batchOp) if rerr != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", err) + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) + } + // If an expected execute payer is supplied via context, mark that account + // as a signer in the bypass remaining accounts. At execution time the payer + // is the outer transaction fee payer, which the Solana runtime always + // presents as a signer; without this override the off-chain Merkle leaf + // would hash it as IsSigner=false and on-chain proof verification would + // fail with ProofCannotBeVerified. + if payers, ok := ExecutePayersFrom(ctx); ok { + if payer, ok := payers[batchOp.ChainSelector]; ok && !payer.IsZero() { + applyExecutePayerSignerOverride(accounts, payer) + } } instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec @@ -253,6 +264,19 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco return uniqueAccounts, nil } +// applyExecutePayerSignerOverride marks the payer account as a signer in the +// given account metas (in place). Used for Solana bypass operations where the +// execute payer also appears in the instruction's remaining accounts: the +// Solana runtime always presents the fee payer as a signer, so the off-chain +// Merkle leaf must hash it the same way for proof verification to succeed. +func applyExecutePayerSignerOverride(accounts []*solana.AccountMeta, payer solana.PublicKey) { + for _, acc := range accounts { + if acc.PublicKey.Equals(payer) { + acc.IsSigner = true + } + } +} + func syncWritableAttribute(accounts []*solana.AccountMeta) []*solana.AccountMeta { writableAttrMap := map[solana.PublicKey]bool{} for _, account := range accounts { diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 32e2864b..0eef1d54 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -501,6 +501,123 @@ func TestTimelockConverter_ConvertBatchToChainOperations(t *testing.T) { } } +func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { + t.Parallel() + + timelockAddress := ContractAddress(testTimelockProgramID, testPDASeed) + mcmAddress := ContractAddress(testMCMProgramID, testPDASeed) + + proposerAC, err := solana.NewRandomPrivateKey() + require.NoError(t, err) + cancellerAC, err := solana.NewRandomPrivateKey() + require.NoError(t, err) + bypasserAC, err := solana.NewRandomPrivateKey() + require.NoError(t, err) + + metaBytes, err := json.Marshal(AdditionalFieldsMetadata{ + ProposerRoleAccessController: proposerAC.PublicKey(), + CancellerRoleAccessController: cancellerAC.PublicKey(), + BypasserRoleAccessController: bypasserAC.PublicKey(), + }) + require.NoError(t, err) + metadata := types.ChainMetadata{MCMAddress: mcmAddress, AdditionalFields: metaBytes} + + payer, err := solana.NewRandomPrivateKey() + require.NoError(t, err) + + // A batch op whose remaining accounts include the payer as a writable, + // non-signer account (mirroring a BPF spill / transfer recipient). + batchOp := func() types.BatchOperation { + return types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "11111111111111111111111111111111", + Data: []byte{1, 2, 3, 4}, + AdditionalFields: toJSON(t, AdditionalFields{Accounts: []*solana.AccountMeta{ + {PublicKey: payer.PublicKey(), IsWritable: true}, + }}), + OperationMetadata: types.OperationMetadata{ContractType: "System", Tags: []string{"t"}}, + }}, + } + } + + convert := func(t *testing.T, ctx context.Context, action types.TimelockAction) []types.Operation { + t.Helper() + ops, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(ctx, metadata, batchOp(), + timelockAddress, mcmAddress, types.NewDuration(time.Second), action, common.Hash{}, + common.HexToHash("0x01")) + require.NoError(t, cerr) + require.NotEmpty(t, ops) + + return ops + } + + // payerIsSigner reports the IsSigner flag of the payer account in the last + // converted op (BypasserExecuteBatch for a bypass), and whether it was found. + payerIsSigner := func(t *testing.T, ops []types.Operation) (isSigner, found bool) { + t.Helper() + last := ops[len(ops)-1] + var fields AdditionalFields + require.NoError(t, json.Unmarshal(last.Transaction.AdditionalFields, &fields)) + for _, acc := range fields.Accounts { + if acc.PublicKey.Equals(payer.PublicKey()) { + return acc.IsSigner, true + } + } + + return false, false + } + + t.Run("bypass, no ctx: payer stays non-signer", func(t *testing.T) { + t.Parallel() + isSigner, found := payerIsSigner(t, convert(t, context.Background(), types.TimelockActionBypass)) + require.True(t, found, "payer must appear in bypass remaining accounts") + require.False(t, isSigner) + }) + + t.Run("bypass, ctx with payer: payer becomes signer", func(t *testing.T) { + t.Parallel() + ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: payer.PublicKey()}) + isSigner, found := payerIsSigner(t, convert(t, ctx, types.TimelockActionBypass)) + require.True(t, found) + require.True(t, isSigner) + }) + + t.Run("bypass, ctx payer not in accounts: no-op", func(t *testing.T) { + t.Parallel() + other, oerr := solana.NewRandomPrivateKey() + require.NoError(t, oerr) + ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: other.PublicKey()}) + isSigner, found := payerIsSigner(t, convert(t, ctx, types.TimelockActionBypass)) + require.True(t, found) + require.False(t, isSigner, "override must not touch accounts other than the configured payer") + }) + + t.Run("schedule, ctx with payer: unchanged", func(t *testing.T) { + t.Parallel() + ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: payer.PublicKey()}) + withCtx := convert(t, ctx, types.TimelockActionSchedule) + noCtx := convert(t, context.Background(), types.TimelockActionSchedule) + require.Empty(t, cmp.Diff(noCtx, withCtx), "schedule conversion must ignore execute payers") + }) +} + +func TestApplyExecutePayerSignerOverride(t *testing.T) { + t.Parallel() + + a := solana.NewWallet().PublicKey() + b := solana.NewWallet().PublicKey() + accounts := []*solana.AccountMeta{ + {PublicKey: a, IsWritable: true}, + {PublicKey: b, IsWritable: true}, + } + + applyExecutePayerSignerOverride(accounts, b) + + require.False(t, accounts[0].IsSigner, "non-payer account must be untouched") + require.True(t, accounts[1].IsSigner, "payer account must be marked signer") +} + func TestAppendIxDataChunkSize(t *testing.T) { tests := []struct { name string From 7d3ab9fa4a6329409f405dc2e0862d91da0a5bb8 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 13:52:30 -0600 Subject: [PATCH 02/23] fix: linting errors Signed-off-by: Pablo --- .../solana/timelock_bypass_payer_collision.go | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 055cba58..3d7c455a 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -190,17 +190,21 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo balanceBefore := s.lamports(ctx, timelockSignerPDA) - for i := range mcmsProposal.Operations { - _, execErr := executable.Execute(ctx, i) - switch { - case i < lastOp: - s.Require().NoError(execErr, "unexpected failure on op %d before BypasserExecuteBatch", i) - case injectExecutePayer: - s.Require().NoError(execErr, "BypasserExecuteBatch should succeed once the execute payer is a signer") - default: - s.Require().Error(execErr, "expected BypasserExecuteBatch to fail due to execute-payer signer collision") - s.Require().ErrorContains(execErr, "ProofCannotBeVerified") - } + // Execute setup ops (init/append/finalize); they don't carry the executor key + // in their accounts so their proofs verify regardless of injectExecutePayer. + for i := range lastOp { + _, err = executable.Execute(ctx, i) + s.Require().NoError(err, "unexpected failure on setup op %d", i) + } + + // Execute the final BypasserExecuteBatch op — the one whose remaining_accounts + // include the executor wallet, causing the signer-bit collision. + _, execErr := executable.Execute(ctx, lastOp) + if injectExecutePayer { + s.Require().NoError(execErr, "BypasserExecuteBatch should succeed once the execute payer is a signer") + } else { + s.Require().Error(execErr, "expected BypasserExecuteBatch to fail due to execute-payer signer collision") + s.Require().ErrorContains(execErr, "ProofCannotBeVerified") } if injectExecutePayer { From 7d2f1026a79154f90a3077b1e2742ac105399fb3 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 14:07:49 -0600 Subject: [PATCH 03/23] fix: linting errors Signed-off-by: Pablo --- e2e/tests/solana/timelock_bypass_payer_collision.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 3d7c455a..37dae7a0 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -132,7 +132,7 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo AddTimelockAddress(s.ChainSelector, timelockAddress). AddChainMetadata(s.ChainSelector, metadata). AddOperation(batchOp). - Build() //nolint:contextcheck //OPT-400 + Build() s.Require().NoError(err) converters := map[types.ChainSelector]sdk.TimelockConverter{ @@ -164,9 +164,9 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo s.Require().NoError(err) inspectors := map[types.ChainSelector]sdk.Inspector{s.ChainSelector: solanasdk.NewInspector(s.SolanaClient)} - signable, err := mcms.NewSignable(&mcmsProposal, inspectors) //nolint:contextcheck //OPT-400 + signable, err := mcms.NewSignable(&mcmsProposal, inspectors) s.Require().NoError(err) - _, err = signable.SignAndAppend(mcms.NewPrivateKeySigner(signerEVMAccount.PrivateKey)) //nolint:contextcheck //OPT-400 + _, err = signable.SignAndAppend(mcms.NewPrivateKeySigner(signerEVMAccount.PrivateKey)) s.Require().NoError(err) encoders, err := mcmsProposal.GetEncoders() //nolint:contextcheck //OPT-400 From d9103a63ce24c9298768182d4bbe69a003611044 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 14:57:24 -0600 Subject: [PATCH 04/23] fix: linting errors Signed-off-by: Pablo --- sdk/solana/context.go | 7 ++++++- sdk/solana/timelock_converter.go | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/sdk/solana/context.go b/sdk/solana/context.go index c2d441c9..101ab2ac 100644 --- a/sdk/solana/context.go +++ b/sdk/solana/context.go @@ -22,7 +22,12 @@ type ExecutePayers map[types.ChainSelector]solana.PublicKey // on-chain proof verification (which always sees the fee payer as a signer) // succeeds. See ConvertBatchToChainOperations. func WithExecutePayers(ctx context.Context, payers ExecutePayers) context.Context { - return context.WithValue(ctx, executePayersKey{}, payers) + copied := make(ExecutePayers, len(payers)) + for chain, payer := range payers { + copied[chain] = payer + } + + return context.WithValue(ctx, executePayersKey{}, copied) } // ExecutePayersFrom returns the ExecutePayers stored in ctx, if any. diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index 1eeaa540..471c6906 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -273,6 +273,7 @@ func applyExecutePayerSignerOverride(accounts []*solana.AccountMeta, payer solan for _, acc := range accounts { if acc.PublicKey.Equals(payer) { acc.IsSigner = true + return } } } From 8e3e1ab4c5afa7cc61e297db2e486078476f6516 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 15:07:12 -0600 Subject: [PATCH 05/23] fix: linting errors Signed-off-by: Pablo --- e2e/tests/solana/timelock_bypass_payer_collision.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 37dae7a0..688e145c 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -169,13 +169,13 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo _, err = signable.SignAndAppend(mcms.NewPrivateKeySigner(signerEVMAccount.PrivateKey)) s.Require().NoError(err) - encoders, err := mcmsProposal.GetEncoders() //nolint:contextcheck //OPT-400 + encoders, err := mcmsProposal.GetEncoders() //nolint:contextcheck,nolintlint //OPT-400 s.Require().NoError(err) encoder := encoders[s.ChainSelector].(*solanasdk.Encoder) executors := map[types.ChainSelector]sdk.Executor{ s.ChainSelector: solanasdk.NewExecutor(encoder, s.SolanaClient, wallet), } - executable, err := mcms.NewExecutable(&mcmsProposal, executors) //nolint:contextcheck //OPT-400 + executable, err := mcms.NewExecutable(&mcmsProposal, executors) //nolint:contextcheck,nolintlint //OPT-400 s.Require().NoError(err) _, err = executable.SetRoot(ctx, s.ChainSelector) From 8d11e703309154f7d1cce3fbddbf10024ab05b6f Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:08:51 -0600 Subject: [PATCH 06/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- e2e/tests/solana/timelock_bypass_payer_collision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 688e145c..780eb67a 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -124,7 +124,7 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo // --- bypass proposal --- timelockProposal, err := mcms.NewTimelockProposalBuilder(). SetVersion("v1"). - SetValidUntil(2051222400). // 2035-01-01T12:00:00 UTC + SetValidUntil(2051222400). // 2035-01-01T00:00:00 UTC SetDescription("bypass proposal: executor payer appears in remaining_accounts"). SetOverridePreviousRoot(true). SetDelay(types.NewDuration(1*time.Second)). From 0f0010124b1c3cbd9c44bdf64ba2506822f7ac7f Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:26:06 -0600 Subject: [PATCH 07/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- e2e/tests/solana/timelock_bypass_payer_collision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 780eb67a..8638eeba 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -192,7 +192,7 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo // Execute setup ops (init/append/finalize); they don't carry the executor key // in their accounts so their proofs verify regardless of injectExecutePayer. - for i := range lastOp { + for i := 0; i < lastOp; i++ { _, err = executable.Execute(ctx, i) s.Require().NoError(err, "unexpected failure on setup op %d", i) } From af05ac5a0f3d4c9071680967d8309336f82e412a Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 16:11:50 -0600 Subject: [PATCH 08/23] fix: linting errors Signed-off-by: Pablo --- e2e/tests/solana/timelock_bypass_payer_collision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 8638eeba..780eb67a 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -192,7 +192,7 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo // Execute setup ops (init/append/finalize); they don't carry the executor key // in their accounts so their proofs verify regardless of injectExecutePayer. - for i := 0; i < lastOp; i++ { + for i := range lastOp { _, err = executable.Execute(ctx, i) s.Require().NoError(err, "unexpected failure on setup op %d", i) } From 316e5c02860a30d33923710a5bc1ab4cf0aea738 Mon Sep 17 00:00:00 2001 From: Pablo Date: Fri, 10 Jul 2026 23:22:25 -0600 Subject: [PATCH 09/23] feat: update approach to get execute payers from chain metadata instead of ctx Signed-off-by: Pablo --- .../solana/timelock_bypass_payer_collision.go | 52 +++++++++---------- sdk/solana/chain_metadata.go | 12 +++++ sdk/solana/context.go | 37 ------------- sdk/solana/timelock_converter.go | 18 +++---- sdk/solana/timelock_converter_test.go | 46 +++++++++------- 5 files changed, 71 insertions(+), 94 deletions(-) delete mode 100644 sdk/solana/context.go diff --git a/e2e/tests/solana/timelock_bypass_payer_collision.go b/e2e/tests/solana/timelock_bypass_payer_collision.go index 780eb67a..1d6e3670 100644 --- a/e2e/tests/solana/timelock_bypass_payer_collision.go +++ b/e2e/tests/solana/timelock_bypass_payer_collision.go @@ -46,28 +46,28 @@ const bypassPayerTransferLamports = 1_000_000 // 0.001 SOL // wallet to reproduce the identical one-bit collision without deploying an // upgradeable program + buffer. // -// - "with execute payers": the proposal is converted with -// solanasdk.WithExecutePayers, so the converter marks the executor account -// IsSigner=true before the root is computed and the bypass executes cleanly. -// - "without execute payers": the same proposal converted without the context -// override still fails with ProofCannotBeVerified, documenting the bug and -// guarding against the fix silently becoming a no-op. +// - "with execute payer in metadata": the proposal's Solana chain metadata records +// the executor as executePayer, so the converter marks that account IsSigner=true +// before the root is computed and the bypass executes cleanly. +// - "without execute payer in metadata": the same proposal converted without the +// field still fails with ProofCannotBeVerified, documenting the bug and guarding +// against the fix silently becoming a no-op. func (s *TestSuite) TestBypassExecutePayerInRemainingAccounts() { - s.Run("with execute payers in context: bypass succeeds", func() { + s.Run("with execute payer in metadata: bypass succeeds", func() { s.runBypassPayerCollision(testPDASeedBypassPayerWith, true) }) - s.Run("without execute payers in context: proof fails", func() { + s.Run("without execute payer in metadata: proof fails", func() { s.runBypassPayerCollision(testPDASeedBypassPayerWithout, false) }) } // runBypassPayerCollision drives the full bypass flow (convert -> set config -> // sign -> set root -> execute) for a batch whose inner instruction sends -// lamports to the executor wallet. When injectExecutePayer is true, the executor -// is threaded through context so the converter marks it as a signer and the -// bypass execute succeeds; otherwise the final op fails with -// ProofCannotBeVerified. -func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bool) { +// lamports to the executor wallet. When setExecutePayerInMetadata is true, the +// executor is recorded in the proposal's Solana chain metadata so the converter +// marks it as a signer and the bypass execute succeeds; otherwise the final op +// fails with ProofCannotBeVerified. +func (s *TestSuite) runBypassPayerCollision(seed [32]byte, setExecutePayerInMetadata bool) { // --- arrange --- ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) s.T().Cleanup(cancel) @@ -120,6 +120,13 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo s.Roles[timelock.Canceller_Role].AccessController.PublicKey(), s.Roles[timelock.Bypasser_Role].AccessController.PublicKey()) s.Require().NoError(err) + if setExecutePayerInMetadata { + var additionalFields solanasdk.AdditionalFieldsMetadata + s.Require().NoError(json.Unmarshal(metadata.AdditionalFields, &additionalFields)) + additionalFields = additionalFields.WithExecutePayer(wallet.PublicKey()) + metadata.AdditionalFields, err = json.Marshal(additionalFields) + s.Require().NoError(err) + } // --- bypass proposal --- timelockProposal, err := mcms.NewTimelockProposalBuilder(). @@ -139,22 +146,13 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo s.ChainSelector: solanasdk.TimelockConverter{}, } - // The fix: when the expected execute payer is threaded through context, the - // converter marks that account IsSigner=true so the off-chain Merkle root - // matches what the runtime presents at execution time. - if injectExecutePayer { - ctx = solanasdk.WithExecutePayers(ctx, solanasdk.ExecutePayers{ - s.ChainSelector: wallet.PublicKey(), - }) - } - mcmsProposal, _, err := timelockProposal.Convert(ctx, converters) s.Require().NoError(err) // The executor wallet lands in the final BypasserExecuteBatch op as a // writable remaining account. Its IsSigner flag must reflect whether the - // execute-payer override was applied. - s.assertExecutorSignerBit(mcmsProposal, wallet.PublicKey(), injectExecutePayer) + // execute payer was recorded in chain metadata. + s.assertExecutorSignerBit(mcmsProposal, wallet.PublicKey(), setExecutePayerInMetadata) // --- set config + sign + set root --- signerEVMAccount := NewEVMTestAccount(s.T()) @@ -191,7 +189,7 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo balanceBefore := s.lamports(ctx, timelockSignerPDA) // Execute setup ops (init/append/finalize); they don't carry the executor key - // in their accounts so their proofs verify regardless of injectExecutePayer. + // in their accounts so their proofs verify regardless of execute payer metadata. for i := range lastOp { _, err = executable.Execute(ctx, i) s.Require().NoError(err, "unexpected failure on setup op %d", i) @@ -200,14 +198,14 @@ func (s *TestSuite) runBypassPayerCollision(seed [32]byte, injectExecutePayer bo // Execute the final BypasserExecuteBatch op — the one whose remaining_accounts // include the executor wallet, causing the signer-bit collision. _, execErr := executable.Execute(ctx, lastOp) - if injectExecutePayer { + if setExecutePayerInMetadata { s.Require().NoError(execErr, "BypasserExecuteBatch should succeed once the execute payer is a signer") } else { s.Require().Error(execErr, "expected BypasserExecuteBatch to fail due to execute-payer signer collision") s.Require().ErrorContains(execErr, "ProofCannotBeVerified") } - if injectExecutePayer { + if setExecutePayerInMetadata { // The inner transfer actually moved lamports out of the timelock signer PDA. balanceAfter := s.lamports(ctx, timelockSignerPDA) s.Require().Equal(balanceBefore-bypassPayerTransferLamports, balanceAfter, diff --git a/sdk/solana/chain_metadata.go b/sdk/solana/chain_metadata.go index 6a29075e..3be0e52b 100644 --- a/sdk/solana/chain_metadata.go +++ b/sdk/solana/chain_metadata.go @@ -17,6 +17,18 @@ type AdditionalFieldsMetadata struct { ProposerRoleAccessController solana.PublicKey `json:"proposerRoleAccessController" validate:"required"` CancellerRoleAccessController solana.PublicKey `json:"cancellerRoleAccessController" validate:"required"` BypasserRoleAccessController solana.PublicKey `json:"bypasserRoleAccessController" validate:"required"` + // ExecutePayer, when set, is the account that will pay for (and therefore sign) + // the MCM execute transaction on this chain. If it also appears in a bypass op's + // remaining accounts, the converter marks it as a signer so the off-chain Merkle + // leaf matches on-chain proof verification (runtime always presents the fee payer + // as a signer). Optional; ignored for schedule/cancel. + ExecutePayer *solana.PublicKey `json:"executePayer,omitempty"` +} + +// WithExecutePayer returns a copy of f with ExecutePayer set to pk. +func (f AdditionalFieldsMetadata) WithExecutePayer(pk solana.PublicKey) AdditionalFieldsMetadata { + f.ExecutePayer = &pk + return f } func (f AdditionalFieldsMetadata) Validate() error { diff --git a/sdk/solana/context.go b/sdk/solana/context.go deleted file mode 100644 index 101ab2ac..00000000 --- a/sdk/solana/context.go +++ /dev/null @@ -1,37 +0,0 @@ -package solana - -import ( - "context" - - "github.com/gagliardetto/solana-go" - - "github.com/smartcontractkit/mcms/types" -) - -// executePayersKey is the unexported context key for the expected execute payers. -type executePayersKey struct{} - -// ExecutePayers maps a chain selector to the public key that will pay for (and -// therefore sign) the MCM execute transaction on that chain. -type ExecutePayers map[types.ChainSelector]solana.PublicKey - -// WithExecutePayers returns a copy of ctx carrying the expected execute payer -// per chain. This is consumed by the Solana TimelockConverter when converting -// bypass operations: if the payer also appears in an operation's remaining -// accounts, it must be hashed as a signer in the off-chain Merkle root so that -// on-chain proof verification (which always sees the fee payer as a signer) -// succeeds. See ConvertBatchToChainOperations. -func WithExecutePayers(ctx context.Context, payers ExecutePayers) context.Context { - copied := make(ExecutePayers, len(payers)) - for chain, payer := range payers { - copied[chain] = payer - } - - return context.WithValue(ctx, executePayersKey{}, copied) -} - -// ExecutePayersFrom returns the ExecutePayers stored in ctx, if any. -func ExecutePayersFrom(ctx context.Context) (ExecutePayers, bool) { - payers, ok := ctx.Value(executePayersKey{}).(ExecutePayers) - return payers, ok -} diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index 471c6906..e75d0d59 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -105,16 +105,14 @@ func (t TimelockConverter) ConvertBatchToChainOperations( if rerr != nil { return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) } - // If an expected execute payer is supplied via context, mark that account - // as a signer in the bypass remaining accounts. At execution time the payer - // is the outer transaction fee payer, which the Solana runtime always - // presents as a signer; without this override the off-chain Merkle leaf - // would hash it as IsSigner=false and on-chain proof verification would - // fail with ProofCannotBeVerified. - if payers, ok := ExecutePayersFrom(ctx); ok { - if payer, ok := payers[batchOp.ChainSelector]; ok && !payer.IsZero() { - applyExecutePayerSignerOverride(accounts, payer) - } + // If the expected execute payer is recorded in chain metadata, mark that + // account as a signer in the bypass remaining accounts. At execution time + // the payer is the outer transaction fee payer, which the Solana runtime + // always presents as a signer; without this override the off-chain Merkle + // leaf would hash it as IsSigner=false and on-chain proof verification + // would fail with ProofCannotBeVerified. + if additionalFields.ExecutePayer != nil && !additionalFields.ExecutePayer.IsZero() { + applyExecutePayerSignerOverride(accounts, *additionalFields.ExecutePayer) } instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 0eef1d54..03fb0796 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -514,16 +514,22 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { bypasserAC, err := solana.NewRandomPrivateKey() require.NoError(t, err) - metaBytes, err := json.Marshal(AdditionalFieldsMetadata{ + payer, err := solana.NewRandomPrivateKey() + require.NoError(t, err) + + baseMetadata := AdditionalFieldsMetadata{ ProposerRoleAccessController: proposerAC.PublicKey(), CancellerRoleAccessController: cancellerAC.PublicKey(), BypasserRoleAccessController: bypasserAC.PublicKey(), - }) + } + metadataWithoutPayer := types.ChainMetadata{MCMAddress: mcmAddress} + metadataWithPayer := types.ChainMetadata{MCMAddress: mcmAddress} + metaBytes, err := json.Marshal(baseMetadata) require.NoError(t, err) - metadata := types.ChainMetadata{MCMAddress: mcmAddress, AdditionalFields: metaBytes} - - payer, err := solana.NewRandomPrivateKey() + metadataWithoutPayer.AdditionalFields = metaBytes + metaBytesWithPayer, err := json.Marshal(baseMetadata.WithExecutePayer(payer.PublicKey())) require.NoError(t, err) + metadataWithPayer.AdditionalFields = metaBytesWithPayer // A batch op whose remaining accounts include the payer as a writable, // non-signer account (mirroring a BPF spill / transfer recipient). @@ -541,9 +547,9 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { } } - convert := func(t *testing.T, ctx context.Context, action types.TimelockAction) []types.Operation { + convert := func(t *testing.T, metadata types.ChainMetadata, action types.TimelockAction) []types.Operation { t.Helper() - ops, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(ctx, metadata, batchOp(), + ops, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(context.Background(), metadata, batchOp(), timelockAddress, mcmAddress, types.NewDuration(time.Second), action, common.Hash{}, common.HexToHash("0x01")) require.NoError(t, cerr) @@ -568,37 +574,37 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { return false, false } - t.Run("bypass, no ctx: payer stays non-signer", func(t *testing.T) { + t.Run("bypass, no execute payer in metadata: payer stays non-signer", func(t *testing.T) { t.Parallel() - isSigner, found := payerIsSigner(t, convert(t, context.Background(), types.TimelockActionBypass)) + isSigner, found := payerIsSigner(t, convert(t, metadataWithoutPayer, types.TimelockActionBypass)) require.True(t, found, "payer must appear in bypass remaining accounts") require.False(t, isSigner) }) - t.Run("bypass, ctx with payer: payer becomes signer", func(t *testing.T) { + t.Run("bypass, metadata with execute payer: payer becomes signer", func(t *testing.T) { t.Parallel() - ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: payer.PublicKey()}) - isSigner, found := payerIsSigner(t, convert(t, ctx, types.TimelockActionBypass)) + isSigner, found := payerIsSigner(t, convert(t, metadataWithPayer, types.TimelockActionBypass)) require.True(t, found) require.True(t, isSigner) }) - t.Run("bypass, ctx payer not in accounts: no-op", func(t *testing.T) { + t.Run("bypass, metadata payer not in accounts: no-op", func(t *testing.T) { t.Parallel() other, oerr := solana.NewRandomPrivateKey() require.NoError(t, oerr) - ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: other.PublicKey()}) - isSigner, found := payerIsSigner(t, convert(t, ctx, types.TimelockActionBypass)) + metaBytesOtherPayer, merr := json.Marshal(baseMetadata.WithExecutePayer(other.PublicKey())) + require.NoError(t, merr) + metadataOtherPayer := types.ChainMetadata{MCMAddress: mcmAddress, AdditionalFields: metaBytesOtherPayer} + isSigner, found := payerIsSigner(t, convert(t, metadataOtherPayer, types.TimelockActionBypass)) require.True(t, found) require.False(t, isSigner, "override must not touch accounts other than the configured payer") }) - t.Run("schedule, ctx with payer: unchanged", func(t *testing.T) { + t.Run("schedule, metadata with execute payer: unchanged", func(t *testing.T) { t.Parallel() - ctx := WithExecutePayers(context.Background(), ExecutePayers{chaintest.Chain4Selector: payer.PublicKey()}) - withCtx := convert(t, ctx, types.TimelockActionSchedule) - noCtx := convert(t, context.Background(), types.TimelockActionSchedule) - require.Empty(t, cmp.Diff(noCtx, withCtx), "schedule conversion must ignore execute payers") + withPayer := convert(t, metadataWithPayer, types.TimelockActionSchedule) + withoutPayer := convert(t, metadataWithoutPayer, types.TimelockActionSchedule) + require.Empty(t, cmp.Diff(withoutPayer, withPayer), "schedule conversion must ignore execute payers") }) } From b4223f90e79216290aecb68c659e1fbc4d71b5ca Mon Sep 17 00:00:00 2001 From: Pablo Date: Mon, 13 Jul 2026 12:49:42 -0600 Subject: [PATCH 10/23] feat: update docs Signed-off-by: Pablo --- CHANGELOG.md | 6 +++ docs/docs/key-concepts/chain-metadata.md | 41 +++++++++++++++ docs/docs/key-concepts/timelock-proposal.md | 2 +- docs/docs/usage/building-proposals.md | 3 ++ sdk/solana/chain_metadata_test.go | 57 +++++++++++++++++++++ 5 files changed, 108 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e6f3327..e851999a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # @smartcontractkit/mcms +## Unreleased + +### Features + +* **solana:** optional `executePayer` on Timelock chain metadata so bypass Merkle leaves match Solana fee-payer signer bits + ## [0.51.0](https://github.com/smartcontractkit/mcms/compare/v0.50.1...v0.51.0) (2026-07-07) diff --git a/docs/docs/key-concepts/chain-metadata.md b/docs/docs/key-concepts/chain-metadata.md index 0e488af8..0925ba4b 100644 --- a/docs/docs/key-concepts/chain-metadata.md +++ b/docs/docs/key-concepts/chain-metadata.md @@ -27,3 +27,44 @@ The starting operation count, typically used for parallel signing processes. **mcmAddress** string
The MCM contract address that will process this proposal on the respective chain. + +--- + +**additionalFields** object _optional_
+Chain-family-specific fields encoded as JSON. Structure depends on the chain family (see below). + +### Solana Additional Fields + +Solana chain metadata uses `additionalFields` for the Timelock role access-controller accounts and, for bypass proposals, the execute fee payer. + +| Field | Required | When used | +| --- | --- | --- | +| `proposerRoleAccessController` | yes | schedule conversion | +| `cancellerRoleAccessController` | yes | cancel conversion | +| `bypasserRoleAccessController` | yes | bypass conversion | +| `executePayer` | no | bypass only — account that pays (and therefore signs) the outer MCM execute transaction | + +Example Solana `chainMetadata` entry: + +```json +"5013781088424303360": { + "startingOpCount": 0, + "mcmAddress": ".", + "additionalFields": { + "proposerRoleAccessController": "...", + "cancellerRoleAccessController": "...", + "bypasserRoleAccessController": "...", + "executePayer": "" + } +} +``` + +#### `executePayer` + +When the execute payer also appears in a bypass operation's `remaining_accounts` (for example as a BPF upgrade spill / close recipient), the Solana runtime always presents the fee payer as `IsSigner=true` at execution time. Off-chain conversion otherwise defaults remaining accounts to non-signer. Without recording `executePayer` in chain metadata, the Merkle leaf hashed off-chain does not match on-chain proof verification and execution fails with `ProofCannotBeVerified`. + +**When to set it:** Solana **bypass** proposals where the fee-payer pubkey is listed as a writable remaining account. Omit for schedule/cancel; the converter ignores `executePayer` for non-bypass actions. + +**Go helper:** `AdditionalFieldsMetadata.WithExecutePayer(pk)` in [`sdk/solana/chain_metadata.go`](https://github.com/smartcontractkit/mcms/blob/main/sdk/solana/chain_metadata.go). + +**Reference scenario:** [`e2e/tests/solana/timelock_bypass_payer_collision.go`](https://github.com/smartcontractkit/mcms/blob/main/e2e/tests/solana/timelock_bypass_payer_collision.go). diff --git a/docs/docs/key-concepts/timelock-proposal.md b/docs/docs/key-concepts/timelock-proposal.md index eeec6df1..5b0624b5 100644 --- a/docs/docs/key-concepts/timelock-proposal.md +++ b/docs/docs/key-concepts/timelock-proposal.md @@ -114,7 +114,7 @@ A Unix timestamp that specifies the proposal's expiration. If the proposal is no Specifies the high-level action for the proposal. Can be one of: - `schedule`: Sets up transactions to execute after a delay. - `cancel`: Cancels previously scheduled transactions. -- `bypass`: Directly executes transactions, skipping the timelock. +- `bypass`: Directly executes transactions, skipping the timelock. For Solana bypass proposals, if the execute fee payer also appears in a batch op's remaining accounts, set `executePayer` in that chain's `additionalFields` so Merkle proof verification succeeds. See [Chain Metadata — Solana Additional Fields](./chain-metadata.md#solana-additional-fields). --- diff --git a/docs/docs/usage/building-proposals.md b/docs/docs/usage/building-proposals.md index 957c794e..eb69d4d2 100644 --- a/docs/docs/usage/building-proposals.md +++ b/docs/docs/usage/building-proposals.md @@ -349,6 +349,9 @@ builder.AddOperation(types.Operation{ChainSelector: selector, Transaction: tx}) ``` +When building Solana **timelock bypass** proposals programmatically, if the execute fee payer appears in remaining accounts, set `executePayer` on that chain's metadata (for example via `AdditionalFieldsMetadata.WithExecutePayer`) so conversion hashes the Merkle leaf with `IsSigner=true`. See [Chain Metadata — Solana Additional Fields](../key-concepts/chain-metadata.md#solana-additional-fields). + + ### Aptos Operations Use the `aptos.NewTransaction` helper to build an Aptos specific transaction. diff --git a/sdk/solana/chain_metadata_test.go b/sdk/solana/chain_metadata_test.go index 819e4fb2..de88bb7f 100644 --- a/sdk/solana/chain_metadata_test.go +++ b/sdk/solana/chain_metadata_test.go @@ -109,6 +109,54 @@ func TestNewChainMetadataFromTimelock(t *testing.T) { } } +func TestAdditionalFieldsMetadata_ExecutePayerJSON(t *testing.T) { + t.Parallel() + + proposer := solana.NewWallet().PublicKey() + canceller := solana.NewWallet().PublicKey() + bypasser := solana.NewWallet().PublicKey() + payer := solana.NewWallet().PublicKey() + + t.Run("omits executePayer when nil", func(t *testing.T) { + t.Parallel() + + fields := AdditionalFieldsMetadata{ + ProposerRoleAccessController: proposer, + CancellerRoleAccessController: canceller, + BypasserRoleAccessController: bypasser, + } + raw, err := json.Marshal(fields) + require.NoError(t, err) + require.NotContains(t, string(raw), "executePayer") + + var roundTrip AdditionalFieldsMetadata + require.NoError(t, json.Unmarshal(raw, &roundTrip)) + require.Nil(t, roundTrip.ExecutePayer) + require.True(t, roundTrip.ProposerRoleAccessController.Equals(proposer)) + }) + + t.Run("serializes executePayer as base58 when set", func(t *testing.T) { + t.Parallel() + + fields := AdditionalFieldsMetadata{ + ProposerRoleAccessController: proposer, + CancellerRoleAccessController: canceller, + BypasserRoleAccessController: bypasser, + }.WithExecutePayer(payer) + raw, err := json.Marshal(fields) + require.NoError(t, err) + + var asMap map[string]any + require.NoError(t, json.Unmarshal(raw, &asMap)) + require.Equal(t, payer.String(), asMap["executePayer"]) + + var roundTrip AdditionalFieldsMetadata + require.NoError(t, json.Unmarshal(raw, &roundTrip)) + require.NotNil(t, roundTrip.ExecutePayer) + require.True(t, roundTrip.ExecutePayer.Equals(payer)) + }) +} + func TestAdditionalFieldsMetadata_Validate(t *testing.T) { t.Parallel() @@ -135,6 +183,15 @@ func TestAdditionalFieldsMetadata_Validate(t *testing.T) { }, expectedErr: nil, }, + { + name: "valid keys with optional execute payer", + fields: AdditionalFieldsMetadata{ + ProposerRoleAccessController: validPK1.PublicKey(), + CancellerRoleAccessController: validPK2.PublicKey(), + BypasserRoleAccessController: validPK3.PublicKey(), + }.WithExecutePayer(validPK1.PublicKey()), + expectedErr: nil, + }, { name: "zero proposer key", fields: AdditionalFieldsMetadata{ From f1d61e2a883a36f6784fe2db361ea7a3366ca9a6 Mon Sep 17 00:00:00 2001 From: Pablo Date: Mon, 13 Jul 2026 15:04:34 -0600 Subject: [PATCH 11/23] fixc: increase unit test coverage Signed-off-by: Pablo --- sdk/solana/chain_metadata.go | 11 ++-- sdk/solana/chain_metadata_test.go | 32 +++++++++ sdk/solana/timelock_converter.go | 38 ++++++----- sdk/solana/timelock_converter_test.go | 95 +++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 23 deletions(-) diff --git a/sdk/solana/chain_metadata.go b/sdk/solana/chain_metadata.go index 3be0e52b..1a4b3284 100644 --- a/sdk/solana/chain_metadata.go +++ b/sdk/solana/chain_metadata.go @@ -17,11 +17,7 @@ type AdditionalFieldsMetadata struct { ProposerRoleAccessController solana.PublicKey `json:"proposerRoleAccessController" validate:"required"` CancellerRoleAccessController solana.PublicKey `json:"cancellerRoleAccessController" validate:"required"` BypasserRoleAccessController solana.PublicKey `json:"bypasserRoleAccessController" validate:"required"` - // ExecutePayer, when set, is the account that will pay for (and therefore sign) - // the MCM execute transaction on this chain. If it also appears in a bypass op's - // remaining accounts, the converter marks it as a signer so the off-chain Merkle - // leaf matches on-chain proof verification (runtime always presents the fee payer - // as a signer). Optional; ignored for schedule/cancel. + // ExecutePayer is the optional outer MCM execute fee payer (bypass only). ExecutePayer *solana.PublicKey `json:"executePayer,omitempty"` } @@ -31,6 +27,11 @@ func (f AdditionalFieldsMetadata) WithExecutePayer(pk solana.PublicKey) Addition return f } +// HasExecutePayer reports whether ExecutePayer is set to a non-zero public key. +func (f AdditionalFieldsMetadata) HasExecutePayer() bool { + return f.ExecutePayer != nil && !f.ExecutePayer.IsZero() +} + func (f AdditionalFieldsMetadata) Validate() error { var validate = validator.New() if err := validate.Struct(f); err != nil { diff --git a/sdk/solana/chain_metadata_test.go b/sdk/solana/chain_metadata_test.go index de88bb7f..bef47862 100644 --- a/sdk/solana/chain_metadata_test.go +++ b/sdk/solana/chain_metadata_test.go @@ -157,6 +157,38 @@ func TestAdditionalFieldsMetadata_ExecutePayerJSON(t *testing.T) { }) } +func TestAdditionalFieldsMetadata_WithExecutePayer(t *testing.T) { + t.Parallel() + + base := AdditionalFieldsMetadata{ + ProposerRoleAccessController: solana.NewWallet().PublicKey(), + CancellerRoleAccessController: solana.NewWallet().PublicKey(), + BypasserRoleAccessController: solana.NewWallet().PublicKey(), + } + payer := solana.NewWallet().PublicKey() + + require.False(t, base.HasExecutePayer()) + + updated := base.WithExecutePayer(payer) + require.Nil(t, base.ExecutePayer, "original must remain unchanged") + require.False(t, base.HasExecutePayer()) + require.NotNil(t, updated.ExecutePayer) + require.True(t, updated.HasExecutePayer()) + require.True(t, updated.ExecutePayer.Equals(payer)) + require.True(t, updated.ProposerRoleAccessController.Equals(base.ProposerRoleAccessController)) + + require.NoError(t, updated.Validate()) + + raw, err := json.Marshal(updated) + require.NoError(t, err) + require.NoError(t, ValidateChainMetadata(types.ChainMetadata{AdditionalFields: raw})) + + zero := solana.PublicKey{} + withZero := base + withZero.ExecutePayer = &zero + require.False(t, withZero.HasExecutePayer(), "zero public key must not count as set") +} + func TestAdditionalFieldsMetadata_Validate(t *testing.T) { t.Parallel() diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index e75d0d59..7ece9ef7 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -101,19 +101,9 @@ func (t TimelockConverter) ConvertBatchToChainOperations( instructions, err = cancelInstructions(timelockPDASeed, operationID, additionalFields.CancellerRoleAccessController, operationPDA, configPDA, mcmSignerPDA) case types.TimelockActionBypass: - accounts, rerr := getAccountsFromBatchOperation(batchOp) - if rerr != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) - } - // If the expected execute payer is recorded in chain metadata, mark that - // account as a signer in the bypass remaining accounts. At execution time - // the payer is the outer transaction fee payer, which the Solana runtime - // always presents as a signer; without this override the off-chain Merkle - // leaf would hash it as IsSigner=false and on-chain proof verification - // would fail with ProofCannotBeVerified. - if additionalFields.ExecutePayer != nil && !additionalFields.ExecutePayer.IsZero() { - applyExecutePayerSignerOverride(accounts, *additionalFields.ExecutePayer) - } + // Transaction fields were already validated by getInstructionDataFromBatchOperation, + // so account extraction uses the same inputs and should not fail here. + accounts, _ := bypassRemainingAccounts(batchOp, additionalFields) instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec accounts) @@ -262,11 +252,23 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco return uniqueAccounts, nil } -// applyExecutePayerSignerOverride marks the payer account as a signer in the -// given account metas (in place). Used for Solana bypass operations where the -// execute payer also appears in the instruction's remaining accounts: the -// Solana runtime always presents the fee payer as a signer, so the off-chain -// Merkle leaf must hash it the same way for proof verification to succeed. +// bypassRemainingAccounts collects remaining accounts for a bypass op and applies +// the execute-payer signer override when ExecutePayer is set in chain metadata. +func bypassRemainingAccounts( + batchOp types.BatchOperation, additionalFields AdditionalFieldsMetadata, +) ([]*solana.AccountMeta, error) { + accounts, err := getAccountsFromBatchOperation(batchOp) + if err != nil { + return nil, err + } + if additionalFields.HasExecutePayer() { + applyExecutePayerSignerOverride(accounts, *additionalFields.ExecutePayer) + } + + return accounts, nil +} + +// applyExecutePayerSignerOverride marks payer as IsSigner=true in accounts (in place). func applyExecutePayerSignerOverride(accounts []*solana.AccountMeta, payer solana.PublicKey) { for _, acc := range accounts { if acc.PublicKey.Equals(payer) { diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 03fb0796..10084a45 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -606,6 +606,22 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { withoutPayer := convert(t, metadataWithoutPayer, types.TimelockActionSchedule) require.Empty(t, cmp.Diff(withoutPayer, withPayer), "schedule conversion must ignore execute payers") }) + + t.Run("bypass, zero execute payer pointer: treated as unset", func(t *testing.T) { + t.Parallel() + zero := solana.PublicKey{} + metaBytesZero, zerr := json.Marshal(AdditionalFieldsMetadata{ + ProposerRoleAccessController: proposerAC.PublicKey(), + CancellerRoleAccessController: cancellerAC.PublicKey(), + BypasserRoleAccessController: bypasserAC.PublicKey(), + ExecutePayer: &zero, + }) + require.NoError(t, zerr) + metadataZeroPayer := types.ChainMetadata{MCMAddress: mcmAddress, AdditionalFields: metaBytesZero} + isSigner, found := payerIsSigner(t, convert(t, metadataZeroPayer, types.TimelockActionBypass)) + require.True(t, found) + require.False(t, isSigner, "zero ExecutePayer must not trigger the signer override") + }) } func TestApplyExecutePayerSignerOverride(t *testing.T) { @@ -622,6 +638,85 @@ func TestApplyExecutePayerSignerOverride(t *testing.T) { require.False(t, accounts[0].IsSigner, "non-payer account must be untouched") require.True(t, accounts[1].IsSigner, "payer account must be marked signer") + + t.Run("no match leaves accounts unchanged", func(t *testing.T) { + t.Parallel() + other := solana.NewWallet().PublicKey() + accts := []*solana.AccountMeta{{PublicKey: a, IsWritable: true}} + applyExecutePayerSignerOverride(accts, other) + require.False(t, accts[0].IsSigner) + }) + + t.Run("empty accounts is a no-op", func(t *testing.T) { + t.Parallel() + require.NotPanics(t, func() { + applyExecutePayerSignerOverride(nil, a) + applyExecutePayerSignerOverride([]*solana.AccountMeta{}, a) + }) + }) +} + +func TestBypassRemainingAccounts(t *testing.T) { + t.Parallel() + + payer := solana.NewWallet().PublicKey() + baseMeta := AdditionalFieldsMetadata{ + ProposerRoleAccessController: solana.NewWallet().PublicKey(), + CancellerRoleAccessController: solana.NewWallet().PublicKey(), + BypasserRoleAccessController: solana.NewWallet().PublicKey(), + } + + validBatch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "11111111111111111111111111111111", + Data: []byte{1}, + AdditionalFields: toJSON(t, AdditionalFields{Accounts: []*solana.AccountMeta{ + {PublicKey: payer, IsWritable: true}, + }}), + }}, + } + + t.Run("marks execute payer as signer", func(t *testing.T) { + t.Parallel() + accounts, err := bypassRemainingAccounts(validBatch, baseMeta.WithExecutePayer(payer)) + require.NoError(t, err) + require.Len(t, accounts, 2) // program id + payer + found := false + for _, acc := range accounts { + if acc.PublicKey.Equals(payer) { + found = true + require.True(t, acc.IsSigner) + } + } + require.True(t, found) + }) + + t.Run("without execute payer keeps non-signer", func(t *testing.T) { + t.Parallel() + accounts, err := bypassRemainingAccounts(validBatch, baseMeta) + require.NoError(t, err) + for _, acc := range accounts { + if acc.PublicKey.Equals(payer) { + require.False(t, acc.IsSigner) + } + } + }) + + t.Run("returns error for invalid program id", func(t *testing.T) { + t.Parallel() + badBatch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "not-a-valid-solana-program-id", + Data: []byte{1}, + AdditionalFields: toJSON(t, AdditionalFields{}), + }}, + } + _, err := bypassRemainingAccounts(badBatch, baseMeta) + require.Error(t, err) + require.ErrorContains(t, err, "unable to parse program id") + }) } func TestAppendIxDataChunkSize(t *testing.T) { From 505017e9fe460d37971be83e2f07d5994cf3809f Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:23:16 -0600 Subject: [PATCH 12/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/timelock_converter.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index 7ece9ef7..a7090e97 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -101,9 +101,10 @@ func (t TimelockConverter) ConvertBatchToChainOperations( instructions, err = cancelInstructions(timelockPDASeed, operationID, additionalFields.CancellerRoleAccessController, operationPDA, configPDA, mcmSignerPDA) case types.TimelockActionBypass: - // Transaction fields were already validated by getInstructionDataFromBatchOperation, - // so account extraction uses the same inputs and should not fail here. - accounts, _ := bypassRemainingAccounts(batchOp, additionalFields) +accounts, rerr := bypassRemainingAccounts(batchOp, additionalFields) +if rerr != nil { + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) +} instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec accounts) From 6a93d97814800180db69918d996683b8c3599284 Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:28:15 -0600 Subject: [PATCH 13/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/timelock_converter.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index a7090e97..f6afc7ea 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -101,10 +101,10 @@ func (t TimelockConverter) ConvertBatchToChainOperations( instructions, err = cancelInstructions(timelockPDASeed, operationID, additionalFields.CancellerRoleAccessController, operationPDA, configPDA, mcmSignerPDA) case types.TimelockActionBypass: -accounts, rerr := bypassRemainingAccounts(batchOp, additionalFields) -if rerr != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) -} + accounts, rerr := bypassRemainingAccounts(batchOp, additionalFields) + if rerr != nil { + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) + } instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec accounts) From 4ab690e8254c6bff6b46895ee7054b69a8579b2e Mon Sep 17 00:00:00 2001 From: Pablo Date: Mon, 13 Jul 2026 16:36:46 -0600 Subject: [PATCH 14/23] fix: copilot comments Signed-off-by: Pablo --- sdk/solana/timelock_converter.go | 5 ++++- sdk/solana/timelock_converter_test.go | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index f6afc7ea..bd0c7769 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -238,6 +238,9 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco accountsMap := map[solana.PublicKey]*solana.AccountMeta{} uniqueAccounts := make([]*solana.AccountMeta, 0) for _, account := range accounts { + if account == nil { + return nil, fmt.Errorf("nil account in batch operation additional fields") + } existingAccount, found := accountsMap[account.PublicKey] if found { // existingAccount.IsSigner = existingAccount.IsSigner || account.IsSigner @@ -272,7 +275,7 @@ func bypassRemainingAccounts( // applyExecutePayerSignerOverride marks payer as IsSigner=true in accounts (in place). func applyExecutePayerSignerOverride(accounts []*solana.AccountMeta, payer solana.PublicKey) { for _, acc := range accounts { - if acc.PublicKey.Equals(payer) { + if acc != nil && acc.PublicKey.Equals(payer) { acc.IsSigner = true return } diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 10084a45..7daf1aff 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -717,6 +717,21 @@ func TestBypassRemainingAccounts(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "unable to parse program id") }) + + t.Run("returns error for nil account in additional fields", func(t *testing.T) { + t.Parallel() + badBatch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "11111111111111111111111111111111", + Data: []byte{1}, + AdditionalFields: []byte(`{"accounts":[null]}`), + }}, + } + _, err := bypassRemainingAccounts(badBatch, baseMeta) + require.Error(t, err) + require.ErrorContains(t, err, "nil account in batch operation additional fields") + }) } func TestAppendIxDataChunkSize(t *testing.T) { From c26f14ea370d0e6a58b89e757e1409f6fdef3f27 Mon Sep 17 00:00:00 2001 From: Pablo Date: Mon, 13 Jul 2026 17:02:59 -0600 Subject: [PATCH 15/23] fix: copilot comments Signed-off-by: Pablo --- sdk/solana/timelock_converter.go | 39 ++++++--- sdk/solana/timelock_converter_test.go | 114 ++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 11 deletions(-) diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index bd0c7769..ce8f289c 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -21,7 +21,10 @@ import ( bindings "github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/v0_1_1/timelock" ) -var _ sdk.TimelockConverter = (*TimelockConverter)(nil) +var ( + _ sdk.TimelockConverter = (*TimelockConverter)(nil) + errNilAccountInAdditionalFields = fmt.Errorf("nil account in batch operation additional fields") +) type TimelockConverter struct{} @@ -53,6 +56,22 @@ func (t TimelockConverter) ConvertBatchToChainOperations( bindings.SetProgramID(timelockProgramID) tags := getTagsFromBatchOperation(batchOp) + + var additionalFields AdditionalFieldsMetadata + if err = json.Unmarshal(metadata.AdditionalFields, &additionalFields); err != nil { + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to unmarshal solana-specific additional fields from chain metada: %w", err) + } + + // Resolve bypass remaining accounts before hashing so malformed account + // metadata fails early (including the ConvertBatch error path under test). + var bypassAccounts []*solana.AccountMeta + if action == types.TimelockActionBypass { + bypassAccounts, err = bypassRemainingAccounts(batchOp, additionalFields) + if err != nil { + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", err) + } + } + instructionsData, err := getInstructionDataFromBatchOperation(batchOp) if err != nil { return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to convert batch operation to solana instructions: %w", err) @@ -86,10 +105,7 @@ func (t TimelockConverter) ConvertBatchToChainOperations( if err != nil { return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to find mcm signer address: %w", err) } - var additionalFields AdditionalFieldsMetadata - if err = json.Unmarshal(metadata.AdditionalFields, &additionalFields); err != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to unmarshal solana-specific additional fields from chain metada: %w", err) - } + // encode the data based on the operation var instructions []solana.Instruction switch action { @@ -101,13 +117,9 @@ func (t TimelockConverter) ConvertBatchToChainOperations( instructions, err = cancelInstructions(timelockPDASeed, operationID, additionalFields.CancellerRoleAccessController, operationPDA, configPDA, mcmSignerPDA) case types.TimelockActionBypass: - accounts, rerr := bypassRemainingAccounts(batchOp, additionalFields) - if rerr != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", rerr) - } instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec - accounts) + bypassAccounts) default: err = fmt.Errorf("invalid timelock operation: %s", string(action)) } @@ -205,6 +217,11 @@ func getInstructionDataFromBatchOperation(batchOp types.BatchOperation) ([]bindi return nil, fmt.Errorf("unable to unmarshal Solana additional fields: %w\n%v", err, string(tx.AdditionalFields)) } } + for _, account := range additionalFields.Accounts { + if account == nil { + return nil, errNilAccountInAdditionalFields + } + } instructionsData = append(instructionsData, bindings.InstructionData{ ProgramId: toProgramID, @@ -239,7 +256,7 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco uniqueAccounts := make([]*solana.AccountMeta, 0) for _, account := range accounts { if account == nil { - return nil, fmt.Errorf("nil account in batch operation additional fields") + return nil, errNilAccountInAdditionalFields } existingAccount, found := accountsMap[account.PublicKey] if found { diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 7daf1aff..029aaa4a 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -622,6 +622,55 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { require.True(t, found) require.False(t, isSigner, "zero ExecutePayer must not trigger the signer override") }) + + t.Run("bypass, nil remaining account: ConvertBatch returns wrapped error", func(t *testing.T) { + t.Parallel() + badBatch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "11111111111111111111111111111111", + Data: []byte{1}, + AdditionalFields: []byte(`{"accounts":[null]}`), + }}, + } + _, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(context.Background(), metadataWithPayer, badBatch, + timelockAddress, mcmAddress, types.NewDuration(time.Second), types.TimelockActionBypass, common.Hash{}, + common.HexToHash("0x01")) + require.Error(t, cerr) + require.ErrorContains(t, cerr, "unable to get accounts from batch operation") + require.ErrorContains(t, cerr, "nil account in batch operation additional fields") + }) + + t.Run("schedule, nil remaining account: ConvertBatch returns error", func(t *testing.T) { + t.Parallel() + badBatch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: "11111111111111111111111111111111", + Data: []byte{1}, + AdditionalFields: []byte(`{"accounts":[null]}`), + }}, + } + _, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(context.Background(), metadataWithPayer, badBatch, + timelockAddress, mcmAddress, types.NewDuration(time.Second), types.TimelockActionSchedule, common.Hash{}, + common.HexToHash("0x01")) + require.Error(t, cerr) + require.ErrorContains(t, cerr, "unable to convert batch operation to solana instructions") + require.ErrorContains(t, cerr, "nil account in batch operation additional fields") + }) + + t.Run("invalid chain metadata additional fields", func(t *testing.T) { + t.Parallel() + badMetadata := types.ChainMetadata{ + MCMAddress: mcmAddress, + AdditionalFields: []byte(`not-json`), + } + _, _, cerr := TimelockConverter{}.ConvertBatchToChainOperations(context.Background(), badMetadata, batchOp(), + timelockAddress, mcmAddress, types.NewDuration(time.Second), types.TimelockActionBypass, common.Hash{}, + common.HexToHash("0x01")) + require.Error(t, cerr) + require.ErrorContains(t, cerr, "unable to unmarshal solana-specific additional fields from chain metada") + }) } func TestApplyExecutePayerSignerOverride(t *testing.T) { @@ -654,6 +703,71 @@ func TestApplyExecutePayerSignerOverride(t *testing.T) { applyExecutePayerSignerOverride([]*solana.AccountMeta{}, a) }) }) + + t.Run("skips nil entries without panicking", func(t *testing.T) { + t.Parallel() + accts := []*solana.AccountMeta{nil, {PublicKey: b, IsWritable: true}} + require.NotPanics(t, func() { + applyExecutePayerSignerOverride(accts, b) + }) + require.True(t, accts[1].IsSigner) + }) +} + +func TestGetAccountsFromBatchOperation(t *testing.T) { + t.Parallel() + + pk := solana.NewWallet().PublicKey() + programID := "11111111111111111111111111111111" + + t.Run("merges duplicate accounts as writable", func(t *testing.T) { + t.Parallel() + batch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{ + { + To: programID, + Data: []byte{1}, + AdditionalFields: toJSON(t, AdditionalFields{Accounts: []*solana.AccountMeta{ + {PublicKey: pk, IsWritable: false}, + }}), + }, + { + To: programID, + Data: []byte{2}, + AdditionalFields: toJSON(t, AdditionalFields{Accounts: []*solana.AccountMeta{ + {PublicKey: pk, IsWritable: true}, + }}), + }, + }, + } + accounts, err := getAccountsFromBatchOperation(batch) + require.NoError(t, err) + var found bool + for _, acc := range accounts { + if acc.PublicKey.Equals(pk) { + found = true + require.True(t, acc.IsWritable, "duplicate pubkey must OR IsWritable") + require.False(t, acc.IsSigner) + } + } + require.True(t, found) + }) + + t.Run("returns error for invalid additional fields json", func(t *testing.T) { + t.Parallel() + batch := types.BatchOperation{ + ChainSelector: chaintest.Chain4Selector, + Transactions: []types.Transaction{{ + To: programID, + Data: []byte{1}, + AdditionalFields: []byte(`{not-json`), + }}, + } + _, err := getAccountsFromBatchOperation(batch) + require.Error(t, err) + require.ErrorContains(t, err, "unable to unmarshal additional fields") + }) } func TestBypassRemainingAccounts(t *testing.T) { From 0391a1a0eab237122be2ff57f58aee191eda4677 Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:07:35 -0600 Subject: [PATCH 16/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/timelock_converter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 029aaa4a..3f7f023f 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -669,7 +669,7 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { timelockAddress, mcmAddress, types.NewDuration(time.Second), types.TimelockActionBypass, common.Hash{}, common.HexToHash("0x01")) require.Error(t, cerr) - require.ErrorContains(t, cerr, "unable to unmarshal solana-specific additional fields from chain metada") + require.ErrorContains(t, cerr, "unable to unmarshal solana-specific additional fields from chain metadata") }) } From ce5068595199329e96c764201c1fbcc49bd7cced Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:29:32 -0600 Subject: [PATCH 17/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/timelock_converter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index ce8f289c..38bc5171 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -59,7 +59,7 @@ func (t TimelockConverter) ConvertBatchToChainOperations( var additionalFields AdditionalFieldsMetadata if err = json.Unmarshal(metadata.AdditionalFields, &additionalFields); err != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to unmarshal solana-specific additional fields from chain metada: %w", err) + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to unmarshal solana-specific additional fields from chain metadata: %w", err) } // Resolve bypass remaining accounts before hashing so malformed account From 6b1f4f5dce273fd0d7a9552cd978cf5cdaee3d6d Mon Sep 17 00:00:00 2001 From: Pablo Date: Tue, 14 Jul 2026 13:29:37 -0600 Subject: [PATCH 18/23] fix: address review comments Signed-off-by: Pablo --- CHANGELOG.md | 6 - sdk/solana/chain_metadata_test.go | 446 +++++--------------------- sdk/solana/encoder.go | 10 +- sdk/solana/encoder_test.go | 2 +- sdk/solana/executor.go | 8 +- sdk/solana/executor_test.go | 2 +- sdk/solana/simulator.go | 7 +- sdk/solana/simulator_test.go | 2 +- sdk/solana/timelock_converter.go | 45 +-- sdk/solana/timelock_converter_test.go | 6 +- sdk/solana/timelock_executor_test.go | 5 +- sdk/solana/transaction.go | 31 +- sdk/solana/transaction_test.go | 2 +- 13 files changed, 141 insertions(+), 431 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e851999a..0e6f3327 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,5 @@ # @smartcontractkit/mcms -## Unreleased - -### Features - -* **solana:** optional `executePayer` on Timelock chain metadata so bypass Merkle leaves match Solana fee-payer signer bits - ## [0.51.0](https://github.com/smartcontractkit/mcms/compare/v0.50.1...v0.51.0) (2026-07-07) diff --git a/sdk/solana/chain_metadata_test.go b/sdk/solana/chain_metadata_test.go index bef47862..55e164b3 100644 --- a/sdk/solana/chain_metadata_test.go +++ b/sdk/solana/chain_metadata_test.go @@ -1,269 +1,125 @@ package solana import ( - "context" "encoding/json" "errors" "testing" - "github.com/stretchr/testify/require" - - "gotest.tools/v3/assert" - - "github.com/google/go-cmp/cmp" - - "github.com/smartcontractkit/mcms/types" - "github.com/gagliardetto/solana-go" "github.com/gagliardetto/solana-go/rpc" + "github.com/stretchr/testify/require" "github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/v0_1_1/timelock" "github.com/smartcontractkit/mcms/sdk/solana/mocks" + "github.com/smartcontractkit/mcms/types" ) +func validAdditionalFields(t *testing.T) AdditionalFieldsMetadata { + t.Helper() + return AdditionalFieldsMetadata{ + ProposerRoleAccessController: solana.NewWallet().PublicKey(), + CancellerRoleAccessController: solana.NewWallet().PublicKey(), + BypasserRoleAccessController: solana.NewWallet().PublicKey(), + } +} + func TestNewChainMetadataFromTimelock(t *testing.T) { t.Parallel() - type params struct { - startingOpCount uint64 - mcmProgramID solana.PublicKey - mcmInstanceSeed PDASeed - timelock solana.PublicKey - timelockSeed PDASeed - } - programID := solana.NewWallet().PublicKey() timelockProgramID := solana.NewWallet().PublicKey() - MCMSeed := PDASeed([32]byte{1, 2, 3, 4}) + mcmSeed := PDASeed([32]byte{1, 2, 3, 4}) timelockSeed := PDASeed([32]byte{1, 2, 3, 4}) configPDA, err := FindTimelockConfigPDA(timelockProgramID, timelockSeed) require.NoError(t, err) - tests := []struct { - name string - params params - setupMock func(mock *mocks.JSONRPCClient) - wantMetadata *types.ChainMetadata - wantErr error - }{ - { - name: "valid metadata", - params: params{ - startingOpCount: 100, - mcmProgramID: programID, - mcmInstanceSeed: MCMSeed, - timelock: timelockProgramID, - timelockSeed: timelockSeed, - }, - setupMock: func(mockJSONRPCClient *mocks.JSONRPCClient) { - mockGetAccountInfo(t, mockJSONRPCClient, configPDA, &timelock.Config{}, nil) - }, - wantMetadata: &types.ChainMetadata{ - StartingOpCount: 100, - MCMAddress: ContractAddress(programID, MCMSeed), - AdditionalFields: json.RawMessage(`{"proposerRoleAccessController":"11111111111111111111111111111111","cancellerRoleAccessController":"11111111111111111111111111111111","bypasserRoleAccessController":"11111111111111111111111111111111"}`), - }, - }, - { - name: "error rpc call", - params: params{ - startingOpCount: 100, - mcmProgramID: programID, - mcmInstanceSeed: MCMSeed, - timelock: timelockProgramID, - timelockSeed: timelockSeed, - }, - wantErr: errors.New("unable to read timelock config pda: rpc error"), - setupMock: func(mockJSONRPCClient *mocks.JSONRPCClient) { - err := errors.New("rpc error") - mockGetAccountInfo(t, mockJSONRPCClient, configPDA, &timelock.Config{}, err) - }, - }, + newClient := func(t *testing.T, rpcErr error) *rpc.Client { + t.Helper() + jsonRPC := mocks.NewJSONRPCClient(t) + mockGetAccountInfo(t, jsonRPC, configPDA, &timelock.Config{}, rpcErr) + return rpc.NewWithCustomRPCClient(jsonRPC) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - jsonRPC := mocks.NewJSONRPCClient(t) - tt.setupMock(jsonRPC) - client := rpc.NewWithCustomRPCClient(jsonRPC) - metadata, err := NewChainMetadataFromTimelock( - context.Background(), - client, - tt.params.startingOpCount, - tt.params.mcmProgramID, - tt.params.mcmInstanceSeed, - tt.params.timelock, - tt.params.timelockSeed) - if tt.wantErr == nil { - require.NoError(t, err, "expected no error but got one") - require.Empty(t, cmp.Diff(tt.wantMetadata, &metadata)) - } else { - // Assert the error message matches the expected error. - require.NotNil(t, metadata) - require.EqualError(t, err, tt.wantErr.Error()) - } - }) - } + t.Run("returns metadata from timelock config", func(t *testing.T) { + t.Parallel() + metadata, err := NewChainMetadataFromTimelock( + t.Context(), newClient(t, nil), 100, programID, mcmSeed, timelockProgramID, timelockSeed) + require.NoError(t, err) + require.Equal(t, uint64(100), metadata.StartingOpCount) + require.Equal(t, ContractAddress(programID, mcmSeed), metadata.MCMAddress) + }) + + t.Run("wraps RPC errors", func(t *testing.T) { + t.Parallel() + _, err := NewChainMetadataFromTimelock( + t.Context(), newClient(t, errors.New("rpc error")), 100, programID, mcmSeed, timelockProgramID, timelockSeed) + require.EqualError(t, err, "unable to read timelock config pda: rpc error") + }) } -func TestAdditionalFieldsMetadata_ExecutePayerJSON(t *testing.T) { +func TestAdditionalFieldsMetadata_ExecutePayer(t *testing.T) { t.Parallel() - proposer := solana.NewWallet().PublicKey() - canceller := solana.NewWallet().PublicKey() - bypasser := solana.NewWallet().PublicKey() + base := validAdditionalFields(t) payer := solana.NewWallet().PublicKey() - t.Run("omits executePayer when nil", func(t *testing.T) { + t.Run("WithExecutePayer returns copy without mutating original", func(t *testing.T) { + t.Parallel() + updated := base.WithExecutePayer(payer) + require.Nil(t, base.ExecutePayer) + require.True(t, updated.ExecutePayer.Equals(payer)) + require.True(t, updated.ProposerRoleAccessController.Equals(base.ProposerRoleAccessController)) + }) + + t.Run("HasExecutePayer is false for nil and zero key, true when set", func(t *testing.T) { t.Parallel() + require.False(t, base.HasExecutePayer()) - fields := AdditionalFieldsMetadata{ - ProposerRoleAccessController: proposer, - CancellerRoleAccessController: canceller, - BypasserRoleAccessController: bypasser, - } - raw, err := json.Marshal(fields) - require.NoError(t, err) - require.NotContains(t, string(raw), "executePayer") + zero := solana.PublicKey{} + withZero := base + withZero.ExecutePayer = &zero + require.False(t, withZero.HasExecutePayer()) - var roundTrip AdditionalFieldsMetadata - require.NoError(t, json.Unmarshal(raw, &roundTrip)) - require.Nil(t, roundTrip.ExecutePayer) - require.True(t, roundTrip.ProposerRoleAccessController.Equals(proposer)) + require.True(t, base.WithExecutePayer(payer).HasExecutePayer()) }) - t.Run("serializes executePayer as base58 when set", func(t *testing.T) { + t.Run("JSON round-trips executePayer, omits when nil", func(t *testing.T) { t.Parallel() - fields := AdditionalFieldsMetadata{ - ProposerRoleAccessController: proposer, - CancellerRoleAccessController: canceller, - BypasserRoleAccessController: bypasser, - }.WithExecutePayer(payer) - raw, err := json.Marshal(fields) + raw, err := json.Marshal(base) require.NoError(t, err) + require.NotContains(t, string(raw), "executePayer") - var asMap map[string]any - require.NoError(t, json.Unmarshal(raw, &asMap)) - require.Equal(t, payer.String(), asMap["executePayer"]) + raw, err = json.Marshal(base.WithExecutePayer(payer)) + require.NoError(t, err) var roundTrip AdditionalFieldsMetadata require.NoError(t, json.Unmarshal(raw, &roundTrip)) - require.NotNil(t, roundTrip.ExecutePayer) require.True(t, roundTrip.ExecutePayer.Equals(payer)) }) } -func TestAdditionalFieldsMetadata_WithExecutePayer(t *testing.T) { - t.Parallel() - - base := AdditionalFieldsMetadata{ - ProposerRoleAccessController: solana.NewWallet().PublicKey(), - CancellerRoleAccessController: solana.NewWallet().PublicKey(), - BypasserRoleAccessController: solana.NewWallet().PublicKey(), - } - payer := solana.NewWallet().PublicKey() - - require.False(t, base.HasExecutePayer()) - - updated := base.WithExecutePayer(payer) - require.Nil(t, base.ExecutePayer, "original must remain unchanged") - require.False(t, base.HasExecutePayer()) - require.NotNil(t, updated.ExecutePayer) - require.True(t, updated.HasExecutePayer()) - require.True(t, updated.ExecutePayer.Equals(payer)) - require.True(t, updated.ProposerRoleAccessController.Equals(base.ProposerRoleAccessController)) - - require.NoError(t, updated.Validate()) - - raw, err := json.Marshal(updated) - require.NoError(t, err) - require.NoError(t, ValidateChainMetadata(types.ChainMetadata{AdditionalFields: raw})) - - zero := solana.PublicKey{} - withZero := base - withZero.ExecutePayer = &zero - require.False(t, withZero.HasExecutePayer(), "zero public key must not count as set") -} - func TestAdditionalFieldsMetadata_Validate(t *testing.T) { t.Parallel() - // Create valid public keys for testing. - validPK1, err := solana.NewRandomPrivateKey() - require.NoError(t, err) - validPK2, err := solana.NewRandomPrivateKey() - require.NoError(t, err) - validPK3, err := solana.NewRandomPrivateKey() - require.NoError(t, err) - zeroPK := solana.PublicKey{} // zero value public key - - tests := []struct { - name string - fields AdditionalFieldsMetadata - expectedErr error - }{ - { - name: "all valid keys", - fields: AdditionalFieldsMetadata{ - ProposerRoleAccessController: validPK1.PublicKey(), - CancellerRoleAccessController: validPK2.PublicKey(), - BypasserRoleAccessController: validPK3.PublicKey(), - }, - expectedErr: nil, - }, - { - name: "valid keys with optional execute payer", - fields: AdditionalFieldsMetadata{ - ProposerRoleAccessController: validPK1.PublicKey(), - CancellerRoleAccessController: validPK2.PublicKey(), - BypasserRoleAccessController: validPK3.PublicKey(), - }.WithExecutePayer(validPK1.PublicKey()), - expectedErr: nil, - }, - { - name: "zero proposer key", - fields: AdditionalFieldsMetadata{ - ProposerRoleAccessController: zeroPK, - CancellerRoleAccessController: validPK2.PublicKey(), - BypasserRoleAccessController: validPK3.PublicKey(), - }, - expectedErr: errors.New("Key: 'AdditionalFieldsMetadata.ProposerRoleAccessController' Error:Field validation for 'ProposerRoleAccessController' failed on the 'required' tag"), - }, - { - name: "zero canceller key", - fields: AdditionalFieldsMetadata{ - ProposerRoleAccessController: validPK1.PublicKey(), - CancellerRoleAccessController: zeroPK, - BypasserRoleAccessController: validPK3.PublicKey(), - }, - expectedErr: errors.New("Key: 'AdditionalFieldsMetadata.CancellerRoleAccessController' Error:Field validation for 'CancellerRoleAccessController' failed on the 'required' tag"), - }, - { - name: "zero bypasser key", - fields: AdditionalFieldsMetadata{ - ProposerRoleAccessController: validPK1.PublicKey(), - CancellerRoleAccessController: validPK2.PublicKey(), - BypasserRoleAccessController: zeroPK, - }, - expectedErr: errors.New("Key: 'AdditionalFieldsMetadata.BypasserRoleAccessController' Error:Field validation for 'BypasserRoleAccessController' failed on the 'required' tag"), - }, - } + require.NoError(t, validAdditionalFields(t).Validate(), "all valid keys") + require.NoError(t, validAdditionalFields(t).WithExecutePayer(solana.NewWallet().PublicKey()).Validate(), "with execute payer") - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, field := range []string{"ProposerRoleAccessController", "CancellerRoleAccessController", "BypasserRoleAccessController"} { + t.Run("rejects zero "+field, func(t *testing.T) { t.Parallel() - - err := tt.fields.Validate() - if tt.expectedErr == nil { - require.NoError(t, err, "expected no error but got one") - } else { - // Assert the error message matches the expected error. - require.EqualError(t, err, tt.expectedErr.Error()) + fields := validAdditionalFields(t) + switch field { + case "ProposerRoleAccessController": + fields.ProposerRoleAccessController = solana.PublicKey{} + case "CancellerRoleAccessController": + fields.CancellerRoleAccessController = solana.PublicKey{} + case "BypasserRoleAccessController": + fields.BypasserRoleAccessController = solana.PublicKey{} } + require.ErrorContains(t, fields.Validate(), field) }) } } @@ -271,160 +127,36 @@ func TestAdditionalFieldsMetadata_Validate(t *testing.T) { func TestValidateChainMetadata(t *testing.T) { t.Parallel() - // Create some public keys for testing. - zeroPK := solana.PublicKey{} // zero value public key - - // Valid additional fields. - validFields := AdditionalFieldsMetadata{ - ProposerRoleAccessController: solana.NewWallet().PublicKey(), - CancellerRoleAccessController: solana.NewWallet().PublicKey(), - BypasserRoleAccessController: solana.NewWallet().PublicKey(), - } - validJSON, err := json.Marshal(validFields) + raw, err := json.Marshal(validAdditionalFields(t)) require.NoError(t, err) + require.NoError(t, ValidateChainMetadata(types.ChainMetadata{AdditionalFields: raw}), "valid fields") - // Missing required field. - // Here we omit CancellerRoleAccessController so that field remains at its zero value. - // Using an inline struct with only two fields. - missingField := struct { - ProposerRoleAccessController solana.PublicKey `json:"proposerRoleAccessController"` - BypasserRoleAccessController solana.PublicKey `json:"bypasserRoleAccessController"` - }{ - ProposerRoleAccessController: validFields.ProposerRoleAccessController, - BypasserRoleAccessController: validFields.BypasserRoleAccessController, - } - missingFieldJSON, err := json.Marshal(missingField) - require.NoError(t, err) + require.ErrorContains(t, ValidateChainMetadata(types.ChainMetadata{AdditionalFields: []byte("bad")}), "unable to unmarshal") - // Zero value field: Proposer is zero. - zeroField := AdditionalFieldsMetadata{ - ProposerRoleAccessController: zeroPK, - CancellerRoleAccessController: validFields.CancellerRoleAccessController, - BypasserRoleAccessController: validFields.BypasserRoleAccessController, - } - zeroFieldJSON, err := json.Marshal(zeroField) + invalid := validAdditionalFields(t) + invalid.ProposerRoleAccessController = solana.PublicKey{} + raw, err = json.Marshal(invalid) require.NoError(t, err) - - tests := []struct { - name string - metadata types.ChainMetadata - expectedErr bool - }{ - { - name: "valid additional fields", - metadata: types.ChainMetadata{ - AdditionalFields: validJSON, - }, - expectedErr: false, - }, - { - name: "invalid JSON", - metadata: types.ChainMetadata{ - AdditionalFields: []byte("not a json"), - }, - expectedErr: true, - }, - { - name: "missing required field", - metadata: types.ChainMetadata{ - AdditionalFields: missingFieldJSON, - }, - expectedErr: true, - }, - { - name: "zero value in one field", - metadata: types.ChainMetadata{ - AdditionalFields: zeroFieldJSON, - }, - expectedErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - err := ValidateChainMetadata(tt.metadata) - if tt.expectedErr { - require.Error(t, err, "expected an error for test case: %s", tt.name) - } else { - require.NoError(t, err, "expected no error for test case: %s", tt.name) - } - }) - } + require.ErrorContains(t, ValidateChainMetadata(types.ChainMetadata{AdditionalFields: raw}), "additional fields are invalid") } -func TestNewSolanaChainMetadata(t *testing.T) { +func TestNewChainMetadata(t *testing.T) { t.Parallel() - // Create sample public keys. - mcmProgramID, err := solana.NewRandomPrivateKey() - require.NoError(t, err) - - proposerKey, err := solana.NewRandomPrivateKey() - require.NoError(t, err) - - cancellerKey, err := solana.NewRandomPrivateKey() - require.NoError(t, err) + proposer := solana.NewWallet().PublicKey() + canceller := solana.NewWallet().PublicKey() + bypasser := solana.NewWallet().PublicKey() + programID := solana.NewWallet().PublicKey() + seed := PDASeed([32]byte{1, 2, 3, 4}) - bypasserKey, err := solana.NewRandomPrivateKey() + metadata, err := NewChainMetadata(100, programID, seed, proposer, canceller, bypasser) require.NoError(t, err) - - tests := []struct { - name string - startingOpCount uint64 - mcmProgramID solana.PublicKey - mcmInstanceSeed PDASeed - proposerKey solana.PublicKey - cancellerKey solana.PublicKey - bypasserKey solana.PublicKey - wantErr string - }{ - { - name: "valid metadata", - startingOpCount: 100, - mcmProgramID: mcmProgramID.PublicKey(), - mcmInstanceSeed: PDASeed([32]byte{1, 2, 3, 4}), - proposerKey: proposerKey.PublicKey(), - cancellerKey: cancellerKey.PublicKey(), - bypasserKey: bypasserKey.PublicKey(), - }, - { - name: "invalid metadata", - startingOpCount: 100, - mcmProgramID: solana.PublicKey{}, - mcmInstanceSeed: PDASeed([32]byte{1, 2, 3, 4}), - proposerKey: proposerKey.PublicKey(), - cancellerKey: cancellerKey.PublicKey(), - bypasserKey: bypasserKey.PublicKey(), - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - metadata, err := NewChainMetadata(tc.startingOpCount, tc.mcmProgramID, tc.mcmInstanceSeed, tc.proposerKey, tc.cancellerKey, tc.bypasserKey) - if tc.wantErr != "" { - require.EqualError(t, err, tc.wantErr) - return - } - require.NoError(t, err) - - assert.Equal(t, tc.startingOpCount, metadata.StartingOpCount) - - expectedMCMAddress := ContractAddress(tc.mcmProgramID, tc.mcmInstanceSeed) - assert.Equal(t, expectedMCMAddress, metadata.MCMAddress) - - var additionalFields AdditionalFieldsMetadata - err = json.Unmarshal(metadata.AdditionalFields, &additionalFields) - require.NoError(t, err) - - expectedAdditionalFields := AdditionalFieldsMetadata{ - ProposerRoleAccessController: tc.proposerKey, - CancellerRoleAccessController: tc.cancellerKey, - BypasserRoleAccessController: tc.bypasserKey, - } - assert.Equal(t, expectedAdditionalFields, additionalFields) - }) - } + require.Equal(t, uint64(100), metadata.StartingOpCount) + require.Equal(t, ContractAddress(programID, seed), metadata.MCMAddress) + + var additional AdditionalFieldsMetadata + require.NoError(t, json.Unmarshal(metadata.AdditionalFields, &additional)) + require.True(t, additional.ProposerRoleAccessController.Equals(proposer)) + require.True(t, additional.CancellerRoleAccessController.Equals(canceller)) + require.True(t, additional.BypasserRoleAccessController.Equals(bypasser)) } diff --git a/sdk/solana/encoder.go b/sdk/solana/encoder.go index b1ee1d75..8abe981b 100644 --- a/sdk/solana/encoder.go +++ b/sdk/solana/encoder.go @@ -3,7 +3,6 @@ package solana import ( "bytes" "encoding/binary" - "encoding/json" "fmt" "github.com/ethereum/go-ethereum/common" @@ -61,12 +60,9 @@ func (e *Encoder) HashOperation( return common.Hash{}, fmt.Errorf("unable to prase program id from To field: %w", err) } - // Parse Additional fields to get the ix accounts - var additionalFields AdditionalFields - if op.Transaction.AdditionalFields != nil { - if err = json.Unmarshal(op.Transaction.AdditionalFields, &additionalFields); err != nil { - return common.Hash{}, fmt.Errorf("unable to unmarshal additional fields: %w", err) - } + additionalFields, err := ParseAdditionalFields(op.Transaction.AdditionalFields) + if err != nil { + return common.Hash{}, err } buffers := [][]byte{ diff --git a/sdk/solana/encoder_test.go b/sdk/solana/encoder_test.go index c6be9317..157b6a59 100644 --- a/sdk/solana/encoder_test.go +++ b/sdk/solana/encoder_test.go @@ -140,7 +140,7 @@ func TestEncoder_HashOperation(t *testing.T) { AdditionalFields: []byte(`invalid`), }, }, - wantErr: "unable to unmarshal additional fields: invalid character 'i' looking for beginning of value", + wantErr: "unable to unmarshal solana additional fields: invalid character 'i' looking for beginning of value", }, { name: "failure: invalid 'to' address", diff --git a/sdk/solana/executor.go b/sdk/solana/executor.go index d10dcde1..6beb7855 100644 --- a/sdk/solana/executor.go +++ b/sdk/solana/executor.go @@ -2,7 +2,6 @@ package solana import ( "context" - "encoding/json" "fmt" "math" "regexp" @@ -90,10 +89,9 @@ func (e *Executor) ExecuteOperation( return types.TransactionResult{}, err } - // Unmarshal the AdditionalFields from the operation - var additionalFields AdditionalFields - if err = json.Unmarshal(op.Transaction.AdditionalFields, &additionalFields); err != nil { - return types.TransactionResult{}, fmt.Errorf("unable to unmarshal additional fields: %w", err) + additionalFields, err := ParseAdditionalFields(op.Transaction.AdditionalFields) + if err != nil { + return types.TransactionResult{}, err } toProgramID, err := ParseProgramID(op.Transaction.To) if err != nil { diff --git a/sdk/solana/executor_test.go b/sdk/solana/executor_test.go index 99278322..d7404a4e 100644 --- a/sdk/solana/executor_test.go +++ b/sdk/solana/executor_test.go @@ -158,7 +158,7 @@ func TestExecutor_ExecuteOperation(t *testing.T) { //nolint:paralleltest want: "", wantErr: errors.New("invalid contract ID provided"), assertion: func(t assert.TestingT, err error, i ...any) bool { - return assert.EqualError(t, err, "unable to unmarshal additional fields: invalid character 'b' looking for beginning of value") + return assert.EqualError(t, err, "unable to unmarshal solana additional fields: invalid character 'b' looking for beginning of value") }, }, } diff --git a/sdk/solana/simulator.go b/sdk/solana/simulator.go index 72d78fb7..52bc7e45 100644 --- a/sdk/solana/simulator.go +++ b/sdk/solana/simulator.go @@ -2,7 +2,6 @@ package solana import ( "context" - "encoding/json" "fmt" "time" @@ -48,9 +47,9 @@ func (s *Simulator) SimulateSetRoot( func (s *Simulator) SimulateOperation( ctx context.Context, metadata types.ChainMetadata, operation types.Operation, ) error { - var additionalFields AdditionalFields - if err := json.Unmarshal(operation.Transaction.AdditionalFields, &additionalFields); err != nil { - return fmt.Errorf("unable to unmarshal additional fields: %w", err) + additionalFields, err := ParseAdditionalFields(operation.Transaction.AdditionalFields) + if err != nil { + return err } toProgramID, err := ParseProgramID(operation.Transaction.To) diff --git a/sdk/solana/simulator_test.go b/sdk/solana/simulator_test.go index 22bff9b7..edd5276c 100644 --- a/sdk/solana/simulator_test.go +++ b/sdk/solana/simulator_test.go @@ -159,7 +159,7 @@ func TestSimulator_SimulateOperation(t *testing.T) { setupMocks: func(t *testing.T, m *mocks.JSONRPCClient) { t.Helper() }, - expectedError: "unable to unmarshal additional fields: invalid character 'i' looking for beginning of value", + expectedError: "unable to unmarshal solana additional fields: invalid character 'i' looking for beginning of value", }, { name: "error: block hash fetch failed", diff --git a/sdk/solana/timelock_converter.go b/sdk/solana/timelock_converter.go index 38bc5171..8de3d6ed 100644 --- a/sdk/solana/timelock_converter.go +++ b/sdk/solana/timelock_converter.go @@ -21,10 +21,7 @@ import ( bindings "github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/v0_1_1/timelock" ) -var ( - _ sdk.TimelockConverter = (*TimelockConverter)(nil) - errNilAccountInAdditionalFields = fmt.Errorf("nil account in batch operation additional fields") -) +var _ sdk.TimelockConverter = (*TimelockConverter)(nil) type TimelockConverter struct{} @@ -62,16 +59,6 @@ func (t TimelockConverter) ConvertBatchToChainOperations( return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to unmarshal solana-specific additional fields from chain metadata: %w", err) } - // Resolve bypass remaining accounts before hashing so malformed account - // metadata fails early (including the ConvertBatch error path under test). - var bypassAccounts []*solana.AccountMeta - if action == types.TimelockActionBypass { - bypassAccounts, err = bypassRemainingAccounts(batchOp, additionalFields) - if err != nil { - return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", err) - } - } - instructionsData, err := getInstructionDataFromBatchOperation(batchOp) if err != nil { return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to convert batch operation to solana instructions: %w", err) @@ -117,6 +104,10 @@ func (t TimelockConverter) ConvertBatchToChainOperations( instructions, err = cancelInstructions(timelockPDASeed, operationID, additionalFields.CancellerRoleAccessController, operationPDA, configPDA, mcmSignerPDA) case types.TimelockActionBypass: + bypassAccounts, bypassErr := bypassRemainingAccounts(batchOp, additionalFields) + if bypassErr != nil { + return []types.Operation{}, common.Hash{}, fmt.Errorf("unable to get accounts from batch operation: %w", bypassErr) + } instructions, err = bypassInstructions(timelockPDASeed, operationID, additionalFields.BypasserRoleAccessController, operationBypasserPDA, configPDA, signerPDA, mcmSignerPDA, salt, uint32(len(batchOp.Transactions)), instructionsData, //nolint:gosec bypassAccounts) @@ -210,17 +201,9 @@ func getInstructionDataFromBatchOperation(batchOp types.BatchOperation) ([]bindi return nil, fmt.Errorf("unable to parse program id from To field: %w", err) } - var additionalFields AdditionalFields - if len(tx.AdditionalFields) > 0 { - err = json.Unmarshal(tx.AdditionalFields, &additionalFields) - if err != nil { - return nil, fmt.Errorf("unable to unmarshal Solana additional fields: %w\n%v", err, string(tx.AdditionalFields)) - } - } - for _, account := range additionalFields.Accounts { - if account == nil { - return nil, errNilAccountInAdditionalFields - } + additionalFields, err := ParseAdditionalFields(tx.AdditionalFields) + if err != nil { + return nil, err } instructionsData = append(instructionsData, bindings.InstructionData{ @@ -242,12 +225,9 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco } accounts = append(accounts, &solana.AccountMeta{PublicKey: toProgramID}) - var additionalFields AdditionalFields - if len(tx.AdditionalFields) > 0 { - err = json.Unmarshal(tx.AdditionalFields, &additionalFields) - if err != nil { - return nil, fmt.Errorf("unable to unmarshal additional fields: %w\n%v", err, string(tx.AdditionalFields)) - } + additionalFields, err := ParseAdditionalFields(tx.AdditionalFields) + if err != nil { + return nil, err } accounts = append(accounts, additionalFields.Accounts...) } @@ -255,9 +235,6 @@ func getAccountsFromBatchOperation(batchOp types.BatchOperation) ([]*solana.Acco accountsMap := map[solana.PublicKey]*solana.AccountMeta{} uniqueAccounts := make([]*solana.AccountMeta, 0) for _, account := range accounts { - if account == nil { - return nil, errNilAccountInAdditionalFields - } existingAccount, found := accountsMap[account.PublicKey] if found { // existingAccount.IsSigner = existingAccount.IsSigner || account.IsSigner diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 3f7f023f..61ed49f4 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -637,7 +637,7 @@ func TestTimelockConverter_ExecutePayerSignerOverride(t *testing.T) { timelockAddress, mcmAddress, types.NewDuration(time.Second), types.TimelockActionBypass, common.Hash{}, common.HexToHash("0x01")) require.Error(t, cerr) - require.ErrorContains(t, cerr, "unable to get accounts from batch operation") + require.ErrorContains(t, cerr, "unable to convert batch operation to solana instructions") require.ErrorContains(t, cerr, "nil account in batch operation additional fields") }) @@ -766,7 +766,7 @@ func TestGetAccountsFromBatchOperation(t *testing.T) { } _, err := getAccountsFromBatchOperation(batch) require.Error(t, err) - require.ErrorContains(t, err, "unable to unmarshal additional fields") + require.ErrorContains(t, err, "unable to unmarshal solana additional fields") }) } @@ -959,7 +959,7 @@ func TestOperationID(t *testing.T) { action: types.TimelockActionSchedule, predecessor: common.HexToHash("0x0123"), salt: common.HexToHash("0xabcd"), - wantErr: "unable to convert batch operation to solana instructions: unable to unmarshal Solana additional fields: invalid character", + wantErr: "unable to convert batch operation to solana instructions: unable to unmarshal solana additional fields: invalid character", }, } for _, tt := range tests { diff --git a/sdk/solana/timelock_executor_test.go b/sdk/solana/timelock_executor_test.go index 47af60a0..54d748a5 100644 --- a/sdk/solana/timelock_executor_test.go +++ b/sdk/solana/timelock_executor_test.go @@ -114,9 +114,8 @@ func TestTimelockExecutor_Execute(t *testing.T) { //nolint:paralleltest }, setup: func(t *testing.T, e *TimelockExecutor, m *mocks.JSONRPCClient) { t.Helper() }, assertion: assertErrorEquals("unable to get InstructionData from batch operation: " + - "unable to unmarshal Solana additional fields: " + - "invalid character 'i' looking for beginning of value\n" + - "invalid JSON"), + "unable to unmarshal solana additional fields: " + + "invalid character 'i' looking for beginning of value"), }, { name: "error: invalid To program field", diff --git a/sdk/solana/transaction.go b/sdk/solana/transaction.go index 26fea03f..84643471 100644 --- a/sdk/solana/transaction.go +++ b/sdk/solana/transaction.go @@ -2,6 +2,7 @@ package solana import ( "encoding/json" + "errors" "fmt" "math/big" @@ -13,16 +14,13 @@ import ( const rbacTimelockContractType = "RBACTimelock" +var errNilAccountInAdditionalFields = errors.New("nil account in batch operation additional fields") + func ValidateAdditionalFields(additionalFields json.RawMessage) error { - fields := AdditionalFields{ - Value: big.NewInt(0), - } - if len(additionalFields) != 0 { - if err := json.Unmarshal(additionalFields, &fields); err != nil { - return fmt.Errorf("failed to unmarshal solana additional fields: %w", err) - } + fields, err := ParseAdditionalFields(additionalFields) + if err != nil { + return err } - return fields.Validate() } @@ -31,6 +29,23 @@ type AdditionalFields struct { Value *big.Int `json:"value" validate:"omitempty"` } +// ParseAdditionalFields unmarshals raw JSON into AdditionalFields and validates +// that no account entry is nil. Returns a zero-value AdditionalFields when raw is empty. +func ParseAdditionalFields(raw json.RawMessage) (AdditionalFields, error) { + var fields AdditionalFields + if len(raw) > 0 { + if err := json.Unmarshal(raw, &fields); err != nil { + return AdditionalFields{}, fmt.Errorf("unable to unmarshal solana additional fields: %w", err) + } + } + for _, account := range fields.Accounts { + if account == nil { + return AdditionalFields{}, errNilAccountInAdditionalFields + } + } + return fields, nil +} + // Validate ensures the solana-specific fields are correct func (f AdditionalFields) Validate() error { return validator.New().Struct(f) diff --git a/sdk/solana/transaction_test.go b/sdk/solana/transaction_test.go index 0ccd2d39..163b4fa6 100644 --- a/sdk/solana/transaction_test.go +++ b/sdk/solana/transaction_test.go @@ -105,7 +105,7 @@ func TestValidateAdditionalFields(t *testing.T) { name: "malformed json", input: []byte(`invalid json`), wantErr: true, - errContains: "failed to unmarshal", + errContains: "unable to unmarshal solana additional fields", }, { name: "empty input", From d30e0ef2e630fc0fdf04a42faf29de9f15dc3583 Mon Sep 17 00:00:00 2001 From: Pablo Date: Tue, 14 Jul 2026 13:36:53 -0600 Subject: [PATCH 19/23] fix: address review comments Signed-off-by: Pablo --- sdk/solana/encoder_test.go | 2 +- sdk/solana/executor_test.go | 2 +- sdk/solana/simulator_test.go | 2 +- sdk/solana/transaction_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/solana/encoder_test.go b/sdk/solana/encoder_test.go index 157b6a59..c6be9317 100644 --- a/sdk/solana/encoder_test.go +++ b/sdk/solana/encoder_test.go @@ -140,7 +140,7 @@ func TestEncoder_HashOperation(t *testing.T) { AdditionalFields: []byte(`invalid`), }, }, - wantErr: "unable to unmarshal solana additional fields: invalid character 'i' looking for beginning of value", + wantErr: "unable to unmarshal additional fields: invalid character 'i' looking for beginning of value", }, { name: "failure: invalid 'to' address", diff --git a/sdk/solana/executor_test.go b/sdk/solana/executor_test.go index d7404a4e..99278322 100644 --- a/sdk/solana/executor_test.go +++ b/sdk/solana/executor_test.go @@ -158,7 +158,7 @@ func TestExecutor_ExecuteOperation(t *testing.T) { //nolint:paralleltest want: "", wantErr: errors.New("invalid contract ID provided"), assertion: func(t assert.TestingT, err error, i ...any) bool { - return assert.EqualError(t, err, "unable to unmarshal solana additional fields: invalid character 'b' looking for beginning of value") + return assert.EqualError(t, err, "unable to unmarshal additional fields: invalid character 'b' looking for beginning of value") }, }, } diff --git a/sdk/solana/simulator_test.go b/sdk/solana/simulator_test.go index edd5276c..22bff9b7 100644 --- a/sdk/solana/simulator_test.go +++ b/sdk/solana/simulator_test.go @@ -159,7 +159,7 @@ func TestSimulator_SimulateOperation(t *testing.T) { setupMocks: func(t *testing.T, m *mocks.JSONRPCClient) { t.Helper() }, - expectedError: "unable to unmarshal solana additional fields: invalid character 'i' looking for beginning of value", + expectedError: "unable to unmarshal additional fields: invalid character 'i' looking for beginning of value", }, { name: "error: block hash fetch failed", diff --git a/sdk/solana/transaction_test.go b/sdk/solana/transaction_test.go index 163b4fa6..0ccd2d39 100644 --- a/sdk/solana/transaction_test.go +++ b/sdk/solana/transaction_test.go @@ -105,7 +105,7 @@ func TestValidateAdditionalFields(t *testing.T) { name: "malformed json", input: []byte(`invalid json`), wantErr: true, - errContains: "unable to unmarshal solana additional fields", + errContains: "failed to unmarshal", }, { name: "empty input", From 0060afbc7aa446f86c4e34c32f18505457980bc0 Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:44:34 -0600 Subject: [PATCH 20/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/solana/transaction.go b/sdk/solana/transaction.go index 84643471..1625fcbf 100644 --- a/sdk/solana/transaction.go +++ b/sdk/solana/transaction.go @@ -14,7 +14,7 @@ import ( const rbacTimelockContractType = "RBACTimelock" -var errNilAccountInAdditionalFields = errors.New("nil account in batch operation additional fields") +var errNilAccountInAdditionalFields = errors.New("nil account in solana additional fields") func ValidateAdditionalFields(additionalFields json.RawMessage) error { fields, err := ParseAdditionalFields(additionalFields) From c8c1d88bdf8b7d6cbdb875919b889030bc543ee2 Mon Sep 17 00:00:00 2001 From: Pablo Estrada <139084212+ecPablo@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:44:44 -0600 Subject: [PATCH 21/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sdk/solana/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/solana/transaction.go b/sdk/solana/transaction.go index 1625fcbf..81909358 100644 --- a/sdk/solana/transaction.go +++ b/sdk/solana/transaction.go @@ -35,7 +35,7 @@ func ParseAdditionalFields(raw json.RawMessage) (AdditionalFields, error) { var fields AdditionalFields if len(raw) > 0 { if err := json.Unmarshal(raw, &fields); err != nil { - return AdditionalFields{}, fmt.Errorf("unable to unmarshal solana additional fields: %w", err) + return AdditionalFields{}, fmt.Errorf("unable to unmarshal additional fields: %w", err) } } for _, account := range fields.Accounts { From 031122ffb3a250f90a94b15da8384ddd5d890928 Mon Sep 17 00:00:00 2001 From: Pablo Date: Tue, 14 Jul 2026 13:56:17 -0600 Subject: [PATCH 22/23] fix: address review comments Signed-off-by: Pablo --- sdk/solana/chain_metadata_test.go | 1 + sdk/solana/timelock_converter_test.go | 4 ++-- sdk/solana/timelock_executor_test.go | 2 +- sdk/solana/transaction.go | 2 ++ sdk/solana/transaction_test.go | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/sdk/solana/chain_metadata_test.go b/sdk/solana/chain_metadata_test.go index 55e164b3..c24daa6c 100644 --- a/sdk/solana/chain_metadata_test.go +++ b/sdk/solana/chain_metadata_test.go @@ -39,6 +39,7 @@ func TestNewChainMetadataFromTimelock(t *testing.T) { t.Helper() jsonRPC := mocks.NewJSONRPCClient(t) mockGetAccountInfo(t, jsonRPC, configPDA, &timelock.Config{}, rpcErr) + return rpc.NewWithCustomRPCClient(jsonRPC) } diff --git a/sdk/solana/timelock_converter_test.go b/sdk/solana/timelock_converter_test.go index 61ed49f4..1ce28867 100644 --- a/sdk/solana/timelock_converter_test.go +++ b/sdk/solana/timelock_converter_test.go @@ -766,7 +766,7 @@ func TestGetAccountsFromBatchOperation(t *testing.T) { } _, err := getAccountsFromBatchOperation(batch) require.Error(t, err) - require.ErrorContains(t, err, "unable to unmarshal solana additional fields") + require.ErrorContains(t, err, "unable to unmarshal additional fields") }) } @@ -959,7 +959,7 @@ func TestOperationID(t *testing.T) { action: types.TimelockActionSchedule, predecessor: common.HexToHash("0x0123"), salt: common.HexToHash("0xabcd"), - wantErr: "unable to convert batch operation to solana instructions: unable to unmarshal solana additional fields: invalid character", + wantErr: "unable to convert batch operation to solana instructions: unable to unmarshal additional fields: invalid character", }, } for _, tt := range tests { diff --git a/sdk/solana/timelock_executor_test.go b/sdk/solana/timelock_executor_test.go index 54d748a5..5b977351 100644 --- a/sdk/solana/timelock_executor_test.go +++ b/sdk/solana/timelock_executor_test.go @@ -114,7 +114,7 @@ func TestTimelockExecutor_Execute(t *testing.T) { //nolint:paralleltest }, setup: func(t *testing.T, e *TimelockExecutor, m *mocks.JSONRPCClient) { t.Helper() }, assertion: assertErrorEquals("unable to get InstructionData from batch operation: " + - "unable to unmarshal solana additional fields: " + + "unable to unmarshal additional fields: " + "invalid character 'i' looking for beginning of value"), }, { diff --git a/sdk/solana/transaction.go b/sdk/solana/transaction.go index 81909358..945afca3 100644 --- a/sdk/solana/transaction.go +++ b/sdk/solana/transaction.go @@ -21,6 +21,7 @@ func ValidateAdditionalFields(additionalFields json.RawMessage) error { if err != nil { return err } + return fields.Validate() } @@ -43,6 +44,7 @@ func ParseAdditionalFields(raw json.RawMessage) (AdditionalFields, error) { return AdditionalFields{}, errNilAccountInAdditionalFields } } + return fields, nil } diff --git a/sdk/solana/transaction_test.go b/sdk/solana/transaction_test.go index 0ccd2d39..ad529b63 100644 --- a/sdk/solana/transaction_test.go +++ b/sdk/solana/transaction_test.go @@ -105,7 +105,7 @@ func TestValidateAdditionalFields(t *testing.T) { name: "malformed json", input: []byte(`invalid json`), wantErr: true, - errContains: "failed to unmarshal", + errContains: "unable to unmarshal additional fields", }, { name: "empty input", From 31f55cd3e8fd536a7812bc7433f735a23216d953 Mon Sep 17 00:00:00 2001 From: Pablo Date: Tue, 14 Jul 2026 14:00:45 -0600 Subject: [PATCH 23/23] fix: unit tests Signed-off-by: Pablo --- sdk/solana/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/solana/transaction.go b/sdk/solana/transaction.go index 945afca3..edb52780 100644 --- a/sdk/solana/transaction.go +++ b/sdk/solana/transaction.go @@ -14,7 +14,7 @@ import ( const rbacTimelockContractType = "RBACTimelock" -var errNilAccountInAdditionalFields = errors.New("nil account in solana additional fields") +var errNilAccountInAdditionalFields = errors.New("nil account in batch operation additional fields") func ValidateAdditionalFields(additionalFields json.RawMessage) error { fields, err := ParseAdditionalFields(additionalFields)