From 3a0b6384e5d5200eb411b6eda7e0415c527bc3a1 Mon Sep 17 00:00:00 2001 From: Lei Date: Fri, 14 Feb 2025 10:36:43 -0800 Subject: [PATCH] [CAPPL-525] return error when receiver reverts or is invalid (#16343) * return error when receiver reverts or is invalid * address comment and lint --- .changeset/blue-ducks-compete.md | 5 + core/capabilities/targets/write_target.go | 23 +-- .../capabilities/targets/write_target_test.go | 143 +++++++++++++++++- 3 files changed, 157 insertions(+), 14 deletions(-) create mode 100644 .changeset/blue-ducks-compete.md diff --git a/.changeset/blue-ducks-compete.md b/.changeset/blue-ducks-compete.md new file mode 100644 index 00000000000..b873d7ededa --- /dev/null +++ b/.changeset/blue-ducks-compete.md @@ -0,0 +1,5 @@ +--- +"chainlink": minor +--- + +#bugfix diff --git a/core/capabilities/targets/write_target.go b/core/capabilities/targets/write_target.go index a312518910b..2d9074ffaf3 100644 --- a/core/capabilities/targets/write_target.go +++ b/core/capabilities/targets/write_target.go @@ -25,7 +25,8 @@ import ( ) var ( - _ capabilities.TargetCapability = &WriteTarget{} + _ capabilities.TargetCapability = &WriteTarget{} + ErrTxFailed = errors.New("submitted transaction failed") ) const transactionStatusCheckInterval = 2 * time.Second @@ -264,7 +265,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap return capabilities.CapabilityResponse{}, nil case transmissionInfo.State == TransmissionStateInvalidReceiver: cap.lggr.Infow("returning without a transmission attempt - transmission already attempted, receiver was marked as invalid", "executionID", request.Metadata.WorkflowExecutionID) - return capabilities.CapabilityResponse{}, nil + return capabilities.CapabilityResponse{}, ErrTxFailed case transmissionInfo.State == TransmissionStateFailed: receiverGasMinimum := cap.receiverGasMinimum if request.Config.GasLimit != nil { @@ -272,7 +273,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap } if transmissionInfo.GasLimit.Uint64() > receiverGasMinimum { cap.lggr.Infow("returning without a transmission attempt - transmission already attempted and failed, sufficient gas was provided", "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit) - return capabilities.CapabilityResponse{}, nil + return capabilities.CapabilityResponse{}, ErrTxFailed } else { cap.lggr.Infow("non-empty report - retrying a failed transmission - attempting to push to txmgr", "request", request, "reportLen", len(request.Inputs.SignedReport.Report), "reportContextLen", len(request.Inputs.SignedReport.Context), "nSignatures", len(request.Inputs.SignedReport.Signatures), "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit) } @@ -350,13 +351,13 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap // This is counterintuitive, but the tx manager is currently returning unconfirmed whenever the tx is confirmed // current implementation here: https://github.com/smartcontractkit/chainlink-framework/blob/main/chains/txmgr/txmgr.go#L697 - // so we need to check if we where able to write to the consumer contract to determine if the transaction was successful + // so we need to check if we were able to write to the consumer contract to determine if the transaction was successful if transmissionInfo.State == TransmissionStateSucceeded { cap.lggr.Debugw("Transaction confirmed", "request", request, "transaction", txID) return capabilities.CapabilityResponse{}, nil - } else { + } else if transmissionInfo.State == TransmissionStateFailed || transmissionInfo.State == TransmissionStateInvalidReceiver { cap.lggr.Errorw("Transaction written to the forwarder, but failed to be written to the consumer contract", "request", request, "transaction", txID, "transmissionState", transmissionInfo.State) - msg := "failed to submit transaction with ID: " + txID.String() + msg := "transaction written to the forwarder, but failed to be written to the consumer contract, transaction ID: " + txID.String() err = cap.emitter.With( platform.KeyWorkflowID, request.Metadata.WorkflowID, platform.KeyWorkflowName, request.Metadata.DecodedWorkflowName, @@ -366,7 +367,11 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap if err != nil { cap.lggr.Errorf("failed to send custom message with msg: %s, err: %v", msg, err) } - return capabilities.CapabilityResponse{}, errors.New("submitted transaction failed") + return capabilities.CapabilityResponse{}, ErrTxFailed + } else { + // TransmissionStateNotAttempted is not expected here, but we'll log it just in case + cap.lggr.Debugw("Transaction confirmed but transmission not attempted, this should never happen", "request", request, "transaction", txID) + return capabilities.CapabilityResponse{}, errors.New("transmission not attempted") } case commontypes.Finalized: cap.lggr.Debugw("Transaction finalized", "request", request, "transaction", txID) @@ -374,7 +379,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap case commontypes.Failed, commontypes.Fatal: cap.lggr.Error("Transaction failed", "request", request, "transaction", txID) - msg := "failed to submit transaction with ID: " + txID.String() + msg := "transaction failed to be written to the forwarder, transaction ID: " + txID.String() err = cap.emitter.With( platform.KeyWorkflowID, request.Metadata.WorkflowID, platform.KeyWorkflowName, request.Metadata.DecodedWorkflowName, @@ -384,7 +389,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap if err != nil { cap.lggr.Errorf("failed to send custom message with msg: %s, err: %v", msg, err) } - return capabilities.CapabilityResponse{}, errors.New("submitted transaction failed") + return capabilities.CapabilityResponse{}, ErrTxFailed default: cap.lggr.Debugw("Unexpected transaction status", "request", request, "transaction", txID, "status", txStatus) } diff --git a/core/capabilities/targets/write_target_test.go b/core/capabilities/targets/write_target_test.go index 6fde8387ed9..2cf05bbbca2 100644 --- a/core/capabilities/targets/write_target_test.go +++ b/core/capabilities/targets/write_target_test.go @@ -8,7 +8,6 @@ import ( "testing" "github.com/ethereum/go-ethereum/common" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -33,6 +32,7 @@ type testHarness struct { writeTarget *targets.WriteTarget forwarderAddr string binding types.BoundContract + gasLimit uint64 } func setup(t *testing.T) testHarness { @@ -44,7 +44,8 @@ func setup(t *testing.T) testHarness { forwarderA := testutils.NewAddress() forwarderAddr := forwarderA.Hex() - writeTarget := targets.NewWriteTarget(lggr, "test-write-target@1.0.0", cr, cw, forwarderAddr, 400_000) + var txGasLimit uint64 = 400_000 + writeTarget := targets.NewWriteTarget(lggr, "test-write-target@1.0.0", cr, cw, forwarderAddr, txGasLimit) require.NotNil(t, writeTarget) config, err := values.NewMap(map[string]any{ @@ -106,6 +107,7 @@ func setup(t *testing.T) testHarness { writeTarget: writeTarget, forwarderAddr: forwarderAddr, binding: binding, + gasLimit: txGasLimit - targets.ForwarderContractLogicGasCost, } } func TestWriteTarget(t *testing.T) { @@ -320,7 +322,7 @@ func TestWriteTarget_UnconfirmedTransaction(t *testing.T) { GasLimit: big.NewInt(0), InvalidReceiver: false, State: targets.TransmissionStateSucceeded, - Success: false, + Success: true, TransmissionId: [32]byte{}, Transmitter: common.HexToAddress("0x0"), } @@ -341,7 +343,7 @@ func TestWriteTarget_UnconfirmedTransaction(t *testing.T) { require.NotNil(t, response) }) - t.Run("transaction written to the forwarder, but failed to be written to the consumer contract", func(t *testing.T) { + t.Run("getTransmissionInfo twice, transaction written to the forwarder, but failed to be written to the consumer contract because of revert in receiver", func(t *testing.T) { th := setup(t) th.cw.On("SubmitTransaction", mock.Anything, "forwarder", "report", mock.Anything, mock.Anything, th.forwarderAddr, mock.Anything, mock.Anything).Return(nil).Once() callCount := 0 @@ -378,6 +380,137 @@ func TestWriteTarget_UnconfirmedTransaction(t *testing.T) { ctx := testutils.Context(t) _, err2 := th.writeTarget.Execute(ctx, req) require.Error(t, err2) - require.Contains(t, err2.Error(), "submitted transaction failed") + require.ErrorIs(t, err2, targets.ErrTxFailed) + }) + + t.Run("getTransmissionInfo twice, transaction written to the forwarder, but failed to be written to the consumer contract because of invalid receiver", func(t *testing.T) { + th := setup(t) + th.cw.On("SubmitTransaction", mock.Anything, "forwarder", "report", mock.Anything, mock.Anything, th.forwarderAddr, mock.Anything, mock.Anything).Return(nil).Once() + callCount := 0 + th.cr.On("GetLatestValue", mock.Anything, th.binding.ReadIdentifier("getTransmissionInfo"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + transmissionInfo := args.Get(4).(*targets.TransmissionInfo) + if callCount == 0 { + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0), + InvalidReceiver: false, + State: targets.TransmissionStateNotAttempted, + Success: false, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + } else { + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0), + InvalidReceiver: true, + State: targets.TransmissionStateInvalidReceiver, + Success: false, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + } + callCount++ + }) + req := capabilities.CapabilityRequest{ + Metadata: th.validMetadata, + Config: th.config, + Inputs: th.validInputs, + } + + th.cw.On("GetTransactionStatus", mock.Anything, mock.Anything).Return(types.Unconfirmed, nil).Once() + ctx := testutils.Context(t) + _, err2 := th.writeTarget.Execute(ctx, req) + require.Error(t, err2) + require.ErrorIs(t, err2, targets.ErrTxFailed) + }) + + t.Run("getTransmissionInfo once, transaction written to the forwarder, but failed to be written to the consumer contract because of invalid receiver", func(t *testing.T) { + th := setup(t) + th.cr.On("GetLatestValue", mock.Anything, th.binding.ReadIdentifier("getTransmissionInfo"), mock.Anything, mock.Anything, mock.Anything).Once().Return(nil).Run(func(args mock.Arguments) { + transmissionInfo := args.Get(4).(*targets.TransmissionInfo) + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0), + InvalidReceiver: true, + State: targets.TransmissionStateInvalidReceiver, + Success: false, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + }) + req := capabilities.CapabilityRequest{ + Metadata: th.validMetadata, + Config: th.config, + Inputs: th.validInputs, + } + + ctx := testutils.Context(t) + _, err2 := th.writeTarget.Execute(ctx, req) + require.Error(t, err2) + require.ErrorIs(t, err2, targets.ErrTxFailed) + }) + + t.Run("getTransmissionInfo once, transaction written to the forwarder, but failed to be written to the consumer contract because of revert in receiver", func(t *testing.T) { + th := setup(t) + th.cr.On("GetLatestValue", mock.Anything, th.binding.ReadIdentifier("getTransmissionInfo"), mock.Anything, mock.Anything, mock.Anything).Once().Return(nil).Run(func(args mock.Arguments) { + transmissionInfo := args.Get(4).(*targets.TransmissionInfo) + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0).SetUint64(th.gasLimit + 1), // has sufficient gas + InvalidReceiver: false, + State: targets.TransmissionStateFailed, + Success: false, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + }) + req := capabilities.CapabilityRequest{ + Metadata: th.validMetadata, + Config: th.config, + Inputs: th.validInputs, + } + + ctx := testutils.Context(t) + _, err2 := th.writeTarget.Execute(ctx, req) + require.Error(t, err2) + require.ErrorIs(t, err2, targets.ErrTxFailed) + }) + + t.Run("getTransmissionInfo twice, first time receiver reverted because of insufficient gas, second time succeeded", func(t *testing.T) { + th := setup(t) + th.cw.On("SubmitTransaction", mock.Anything, "forwarder", "report", mock.Anything, mock.Anything, th.forwarderAddr, mock.Anything, mock.Anything).Return(nil).Once() + callCount := 0 + th.cr.On("GetLatestValue", mock.Anything, th.binding.ReadIdentifier("getTransmissionInfo"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + transmissionInfo := args.Get(4).(*targets.TransmissionInfo) + if callCount == 0 { + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0).SetUint64(th.gasLimit - 1), // has insufficient gas + InvalidReceiver: false, + State: targets.TransmissionStateFailed, + Success: false, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + } else { + *transmissionInfo = targets.TransmissionInfo{ + GasLimit: big.NewInt(0).SetUint64(th.gasLimit + 1), // has sufficient gas + InvalidReceiver: false, + State: targets.TransmissionStateSucceeded, + Success: true, + TransmissionId: [32]byte{}, + Transmitter: common.HexToAddress("0x0"), + } + } + callCount++ + }) + req := capabilities.CapabilityRequest{ + Metadata: th.validMetadata, + Config: th.config, + Inputs: th.validInputs, + } + + th.cw.On("GetTransactionStatus", mock.Anything, mock.Anything).Return(types.Unconfirmed, nil).Once() + + ctx := testutils.Context(t) + response, err2 := th.writeTarget.Execute(ctx, req) + require.NoError(t, err2) + require.NotNil(t, response) }) }