From 04ec00005ecabfb3e5556624a5179c56d4ded4de Mon Sep 17 00:00:00 2001 From: Luis Carvalho Date: Fri, 20 Sep 2024 15:32:40 +0100 Subject: [PATCH 1/3] chore: return all compass call errors --- chain/evm/compass.go | 85 +++++++++++---------------------------- chain/evm/compass_test.go | 1 + 2 files changed, 24 insertions(+), 62 deletions(-) diff --git a/chain/evm/compass.go b/chain/evm/compass.go index 0d892f0..84cee30 100644 --- 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,18 +452,6 @@ 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 } @@ -877,6 +816,28 @@ func (t compass) processMessages(ctx context.Context, queueTypeName string, msgs } default: logger.WithError(processingErr).Error("processing error") + + var isContractErr bool + var setErr error + + if !opts.estimateOnly { + // If we're not just estimating, we want to set the error data + // on the message + isContractErr, setErr = t.SetErrorData(ctx, queueTypeName, rawMsg.ID, processingErr) + } + + if setErr == nil && isContractErr { + // If it's a smart contract error we just ignore it, as we want + // to retry it + continue + } + + 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). diff --git a/chain/evm/compass_test.go b/chain/evm/compass_test.go index 6cb84d7..0ed52c5 100644 --- a/chain/evm/compass_test.go +++ b/chain/evm/compass_test.go @@ -339,6 +339,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 }, From 28bd68cb182d7e20333706736f7cd26e74ae7b35 Mon Sep 17 00:00:00 2001 From: Luis Carvalho Date: Fri, 20 Sep 2024 15:50:39 +0100 Subject: [PATCH 2/3] chore: return compass errors processing messages --- chain/evm/compass.go | 33 ++++++++++++++------------------- chain/evm/compass_test.go | 7 ++++--- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/chain/evm/compass.go b/chain/evm/compass.go index 84cee30..0dfe75c 100644 --- a/chain/evm/compass.go +++ b/chain/evm/compass.go @@ -458,11 +458,16 @@ func (t compass) uploadUserSmartContract( 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, @@ -471,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) { @@ -817,19 +819,12 @@ func (t compass) processMessages(ctx context.Context, queueTypeName string, msgs default: logger.WithError(processingErr).Error("processing error") - var isContractErr bool var setErr error if !opts.estimateOnly { // If we're not just estimating, we want to set the error data // on the message - isContractErr, setErr = t.SetErrorData(ctx, queueTypeName, rawMsg.ID, processingErr) - } - - if setErr == nil && isContractErr { - // If it's a smart contract error we just ignore it, as we want - // to retry it - continue + setErr = t.SetErrorData(ctx, queueTypeName, rawMsg.ID, processingErr) } if setErr != nil { @@ -1680,7 +1675,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 index 0ed52c5..a186671 100644 --- 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) @@ -1648,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{ @@ -1658,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", From bb3a7420b79f32db97e9e05c9ade7f82a72da126 Mon Sep 17 00:00:00 2001 From: Luis Carvalho Date: Fri, 20 Sep 2024 16:33:45 +0100 Subject: [PATCH 3/3] chore: cleanup error handling --- chain/evm/compass.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/chain/evm/compass.go b/chain/evm/compass.go index 0dfe75c..07e6bb4 100644 --- a/chain/evm/compass.go +++ b/chain/evm/compass.go @@ -819,18 +819,15 @@ func (t compass) processMessages(ctx context.Context, queueTypeName string, msgs default: logger.WithError(processingErr).Error("processing error") - var setErr 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 + 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().