fix: set ErrorData on all errors from handling a turnstone message (#439

) * chore: return all compass call errors * chore: return compass errors processing messages * chore: cleanup error handling
palomachain · Sep 20, 2024 · 455b91c · 455b91c
1 parent 6067bfc
commit 455b91c
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 76 deletions.
diff --git a/chain/evm/compass.go b/chain/evm/compass.go
@@ -188,18 +188,7 @@ func (t compass) updateValset(
 		estimate,
 	})
 	if err != nil {
-		logger.WithError(err).Error("call_compass error")
-		if opts.estimateOnly == false {
-			isSmartContractError, setErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err)
-			if setErr != nil {
-				return nil, 0, setErr
-			}
-			if isSmartContractError {
-				logger.Debug("smart contract error. recovering...")
-				return nil, 0, nil
-			}
-		}
-		return nil, 0, fmt.Errorf("call_compass error: %w", err)
+		return nil, 0, err
 	}
 
 	return tx, currentValsetID, nil
@@ -281,18 +270,6 @@ func (t compass) submitLogicCall(
 	logger.WithField("consensus", con).WithField("args", args).Debug("submitting logic call")
 	tx, err := t.callCompass(ctx, opts, "submit_logic_call", args)
 	if err != nil {
-		logger.WithError(err).Error("submitLogicCall: error calling DeployContract")
-		if opts.estimateOnly == false {
-			isSmartContractError, setErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err)
-			if setErr != nil {
-				return nil, 0, setErr
-			}
-			if isSmartContractError {
-				logger.Debug("smart contract error. recovering...")
-				return nil, 0, nil
-			}
-		}
-
 		return nil, 0, err
 	}
 
@@ -359,18 +336,6 @@ func (t compass) compass_handover(
 	logger.WithField("consensus", con).WithField("args", args).Debug("compass handover")
 	tx, err := t.callCompass(ctx, opts, "compass_update_batch", args)
 	if err != nil {
-		logger.WithError(err).Error("CompassHandover: error calling DeployContract")
-		if opts.estimateOnly == false {
-			isSmartContractError, setErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err)
-			if setErr != nil {
-				return nil, 0, setErr
-			}
-			if isSmartContractError {
-				logger.Debug("smart contract error. recovering...")
-				return nil, 0, nil
-			}
-		}
-
 		return nil, 0, err
 	}
 
@@ -410,20 +375,6 @@ func (t compass) uploadSmartContract(
 		constructorInput,
 	)
 	if err != nil {
-		logger.
-			WithError(err).
-			WithField("input", constructorInput).
-			Error("uploadSmartContract: error calling DeployContract")
-
-		isSmartContractError, setErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err)
-		if setErr != nil {
-			return nil, setErr
-		}
-		if isSmartContractError {
-			logger.Debug("smart contract error. recovering...")
-			return nil, nil
-		}
-
 		return nil, err
 	}
 
@@ -501,29 +452,22 @@ func (t compass) uploadUserSmartContract(
 		Debug("deploying user smart contract")
 	tx, err := t.callCompass(ctx, opts, "deploy_contract", args)
 	if err != nil {
-		logger.WithError(err).Error("deploy_contract: error calling compass")
-		if opts.estimateOnly == false {
-			isSmartContractError, setErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err)
-			if setErr != nil {
-				return nil, 0, setErr
-			}
-			if isSmartContractError {
-				logger.Debug("smart contract error. recovering...")
-				return nil, 0, nil
-			}
-		}
-
 		return nil, 0, err
 	}
 
 	return tx, valsetID, nil
 }
 
-func (t compass) SetErrorData(ctx context.Context, queueTypeName string, msgID uint64, errToProcess error) (bool, error) {
+func (t compass) SetErrorData(
+	ctx context.Context,
+	queueTypeName string,
+	msgID uint64,
+	errToProcess error,
+) error {
+	data := []byte(errToProcess.Error())
+
 	var jsonRpcErr rpc.DataError
-	if !errors.As(errToProcess, &jsonRpcErr) {
-		return false, t.paloma.SetErrorData(ctx, queueTypeName, msgID, []byte(errToProcess.Error()))
-	} else {
+	if errors.As(errToProcess, &jsonRpcErr) {
 		liblog.WithContext(ctx).WithFields(
 			log.Fields{
 				"queue-type-name": queueTypeName,
@@ -532,13 +476,10 @@ func (t compass) SetErrorData(ctx context.Context, queueTypeName string, msgID u
 			},
 		).Warn("smart contract returned an error")
 
-		err := t.paloma.SetErrorData(ctx, queueTypeName, msgID, []byte(jsonRpcErr.Error()))
-		if err != nil {
-			return false, err
-		}
-
-		return true, nil
+		data = []byte(jsonRpcErr.Error())
 	}
+
+	return t.paloma.SetErrorData(ctx, queueTypeName, msgID, data)
 }
 
 func (t compass) findLastValsetMessageID(ctx context.Context) (uint64, error) {
@@ -877,6 +818,18 @@ func (t compass) processMessages(ctx context.Context, queueTypeName string, msgs
 			}
 		default:
 			logger.WithError(processingErr).Error("processing error")
+
+			if !opts.estimateOnly {
+				// If we're not just estimating, we want to set the error data
+				// on the message
+				setErr := t.SetErrorData(ctx, queueTypeName, rawMsg.ID, processingErr)
+				if setErr != nil {
+					// If we got an error setting the error data, this is the error
+					// we want to log
+					processingErr = setErr
+				}
+			}
+
 			if err := t.paloma.NewStatus().
 				WithChainReferenceID(t.ChainReferenceID).
 				WithQueueType(queueTypeName).
@@ -1719,7 +1672,7 @@ func (t compass) getFeeArgs(
 
 	if userFunds.Cmp(totalFundsNeeded) < 0 {
 		err := fmt.Errorf("insufficient funds for fees: %s < %s", userFunds, totalFundsNeeded)
-		if _, sendErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err); sendErr != nil {
+		if sendErr := t.SetErrorData(ctx, queueTypeName, origMessage.ID, err); sendErr != nil {
 			err = fmt.Errorf("failed to set error data: %w", sendErr)
 		}
 		return feeArgs, err

diff --git a/chain/evm/compass_test.go b/chain/evm/compass_test.go
@@ -268,6 +268,7 @@ func TestIsArbitraryCallAlreadyExecuted(t *testing.T) {
 
 func TestMessageProcessing(t *testing.T) {
 	dummyErr := whoops.String("dummy")
+	rpcErr := fakeJsonRpcError("bla")
 
 	addValidSignature := func(pk *ecdsa.PrivateKey) chain.ValidatorSignature {
 		return signMessage(ethCompatibleBytesToSign, pk)
@@ -339,6 +340,7 @@ func TestMessageProcessing(t *testing.T) {
 					},
 				}
 				paloma.On("QueryGetSnapshotByID", mock.Anything, uint64(0)).Return(sn, nil)
+				paloma.On("SetErrorData", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 
 				return evm, paloma
 			},
@@ -1647,7 +1649,6 @@ func TestMessageProcessing(t *testing.T) {
 			},
 			setup: func(t *testing.T) (*mockEvmClienter, *evmmocks.PalomaClienter) {
 				evm, paloma := newMockEvmClienter(t), evmmocks.NewPalomaClienter(t)
-				fakeErr := fakeJsonRpcError("bla")
 
 				paloma.On("QueryGetEVMValsetByID", mock.Anything, uint64(0), "internal-chain-id").Return(
 					&types.Valset{
@@ -1657,11 +1658,12 @@ func TestMessageProcessing(t *testing.T) {
 					},
 					nil,
 				)
-				evm.On("DeployContract", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, fakeErr)
+				evm.On("DeployContract", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, rpcErr)
+				paloma.On("NewStatus").Return(&StatusUpdater{})
 				paloma.On("SetErrorData", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 				return evm, paloma
 			},
-			expErr: nil,
+			expErr: rpcErr,
 		},
 		{
 			name: "upload_smart_contract/when smart contract returns an error and sending it to paloma fails, it returns it back",