From c1afd24792ce66a2783a39a5f3f987f70ea4f8db Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Sat, 23 Sep 2023 14:00:48 +0100 Subject: [PATCH 1/3] Handle idempotent retry after error during init phase - introducing OpPhase enum Signed-off-by: Peter Broadhurst --- Makefile | 4 +- internal/assets/manager.go | 2 +- internal/assets/operations.go | 28 ++-- internal/assets/operations_test.go | 62 ++++----- internal/assets/token_approval.go | 16 ++- internal/assets/token_approval_test.go | 16 +-- internal/assets/token_pool.go | 7 +- internal/assets/token_pool_test.go | 16 +-- internal/assets/token_transfer.go | 16 ++- internal/assets/token_transfer_test.go | 22 +-- internal/broadcast/manager.go | 20 +-- internal/broadcast/manager_test.go | 25 ++-- internal/broadcast/operations.go | 30 ++-- internal/broadcast/operations_test.go | 50 +++---- internal/contracts/manager.go | 6 +- internal/contracts/manager_test.go | 12 +- internal/contracts/operations.go | 14 +- internal/contracts/operations_test.go | 20 +-- internal/definitions/sender_tokenpool_test.go | 21 +++ internal/events/batch_pin_complete.go | 2 +- internal/events/batch_pin_complete_test.go | 8 +- internal/events/persist_batch.go | 2 +- internal/multiparty/manager.go | 16 +-- internal/multiparty/manager_test.go | 32 ++--- internal/multiparty/operations.go | 13 +- internal/multiparty/operations_test.go | 16 +-- internal/operations/manager.go | 84 +++++++---- internal/operations/manager_test.go | 130 ++++++++++++++---- internal/orchestrator/orchestrator.go | 2 +- internal/orchestrator/orchestrator_test.go | 2 +- internal/privatemessaging/message_test.go | 4 +- internal/privatemessaging/operations.go | 14 +- internal/privatemessaging/operations_test.go | 26 ++-- internal/privatemessaging/privatemessaging.go | 8 +- .../privatemessaging/privatemessaging_test.go | 14 +- internal/shareddownload/download_manager.go | 26 ++-- .../shareddownload/download_manager_test.go | 19 +-- internal/shareddownload/download_worker.go | 20 +-- internal/shareddownload/operations.go | 26 ++-- internal/tokens/fftokens/fftokens.go | 22 +-- internal/tokens/fftokens/fftokens_test.go | 44 +++--- mocks/assetmocks/manager.go | 10 +- mocks/broadcastmocks/manager.go | 10 +- mocks/contractmocks/manager.go | 10 +- mocks/multipartymocks/manager.go | 30 ++-- mocks/operationmocks/manager.go | 25 ++-- mocks/privatemessagingmocks/manager.go | 10 +- mocks/shareddownloadmocks/manager.go | 20 +-- mocks/tokenmocks/plugin.go | 20 +-- pkg/core/operation.go | 8 ++ pkg/tokens/plugin.go | 4 +- 51 files changed, 603 insertions(+), 461 deletions(-) diff --git a/Makefile b/Makefile index 7c9b3225d..f675bc5a5 100644 --- a/Makefile +++ b/Makefile @@ -73,12 +73,12 @@ $(eval $(call makemock, internal/assets, Manager, assetm $(eval $(call makemock, internal/contracts, Manager, contractmocks)) $(eval $(call makemock, internal/spievents, Manager, spieventsmocks)) $(eval $(call makemock, internal/orchestrator, Orchestrator, orchestratormocks)) -$(eval $(call makemock, internal/apiserver, FFISwaggerGen, apiservermocks)) -$(eval $(call makemock, internal/apiserver, Server, apiservermocks)) $(eval $(call makemock, internal/cache, Manager, cachemocks)) $(eval $(call makemock, internal/metrics, Manager, metricsmocks)) $(eval $(call makemock, internal/operations, Manager, operationmocks)) $(eval $(call makemock, internal/multiparty, Manager, multipartymocks)) +$(eval $(call makemock, internal/apiserver, FFISwaggerGen, apiservermocks)) +$(eval $(call makemock, internal/apiserver, Server, apiservermocks)) firefly-nocgo: ${GOFILES} CGO_ENABLED=0 $(VGO) build -o ${BINARY_NAME}-nocgo -ldflags "-X main.buildDate=$(DATE) -X main.buildVersion=$(BUILD_VERSION) -X 'github.com/hyperledger/firefly/cmd.BuildVersionOverride=$(BUILD_VERSION)' -X 'github.com/hyperledger/firefly/cmd.BuildDate=$(DATE)' -X 'github.com/hyperledger/firefly/cmd.BuildCommit=$(GIT_REF)'" -tags=prod -tags=prod -v diff --git a/internal/assets/manager.go b/internal/assets/manager.go index 68e3f53ca..77ceaf824 100644 --- a/internal/assets/manager.go +++ b/internal/assets/manager.go @@ -70,7 +70,7 @@ type Manager interface { // From operations.OperationHandler PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) } type assetManager struct { diff --git a/internal/assets/operations.go b/internal/assets/operations.go index a3f6ec726..9e79e2783 100644 --- a/internal/assets/operations.go +++ b/internal/assets/operations.go @@ -99,36 +99,36 @@ func (am *assetManager) PrepareOperation(ctx context.Context, op *core.Operation } } -func (am *assetManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (am *assetManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case createPoolData: plugin, err := am.selectTokenPlugin(ctx, data.Pool.Connector) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } - complete, err = plugin.CreateTokenPool(ctx, op.NamespacedIDString(), data.Pool) - return nil, complete, err + phase, err = plugin.CreateTokenPool(ctx, op.NamespacedIDString(), data.Pool) + return nil, phase, err case activatePoolData: plugin, err := am.selectTokenPlugin(ctx, data.Pool.Connector) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } - complete, err = plugin.ActivateTokenPool(ctx, data.Pool) - return nil, complete, err + phase, err = plugin.ActivateTokenPool(ctx, data.Pool) + return nil, phase, err case transferData: plugin, err := am.selectTokenPlugin(ctx, data.Pool.Connector) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } switch data.Transfer.Type { case core.TokenTransferTypeMint: - return nil, false, plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + return nil, core.OpPhaseInitializing, plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) case core.TokenTransferTypeTransfer: - return nil, false, plugin.TransferTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + return nil, core.OpPhaseInitializing, plugin.TransferTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) case core.TokenTransferTypeBurn: - return nil, false, plugin.BurnTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + return nil, core.OpPhaseInitializing, plugin.BurnTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) default: panic(fmt.Sprintf("unknown transfer type: %v", data.Transfer.Type)) } @@ -136,12 +136,12 @@ func (am *assetManager) RunOperation(ctx context.Context, op *core.PreparedOpera case approvalData: plugin, err := am.selectTokenPlugin(ctx, data.Pool.Connector) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } - return nil, false, plugin.TokensApproval(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Approval, data.Pool.Methods) + return nil, core.OpPhaseInitializing, plugin.TokensApproval(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Approval, data.Pool.Methods) default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } diff --git a/internal/assets/operations_test.go b/internal/assets/operations_test.go index 1020e9193..e261ecf08 100644 --- a/internal/assets/operations_test.go +++ b/internal/assets/operations_test.go @@ -46,15 +46,15 @@ func TestPrepareAndRunCreatePool(t *testing.T) { assert.NoError(t, err) mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) - mti.On("CreateTokenPool", context.Background(), "ns1:"+op.ID.String(), pool).Return(false, nil) + mti.On("CreateTokenPool", context.Background(), "ns1:"+op.ID.String(), pool).Return(core.OpPhaseComplete, nil) po, err := am.PrepareOperation(context.Background(), op) assert.NoError(t, err) assert.Equal(t, pool, po.Data.(createPoolData).Pool) - _, complete, err := am.RunOperation(context.Background(), po) + _, phase, err := am.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -78,16 +78,16 @@ func TestPrepareAndRunActivatePool(t *testing.T) { mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) mdi := am.database.(*databasemocks.Plugin) - mti.On("ActivateTokenPool", context.Background(), pool).Return(true, nil) + mti.On("ActivateTokenPool", context.Background(), pool).Return(core.OpPhaseComplete, nil) mdi.On("GetTokenPoolByID", context.Background(), "ns1", pool.ID).Return(pool, nil) po, err := am.PrepareOperation(context.Background(), op) assert.NoError(t, err) assert.Equal(t, pool, po.Data.(activatePoolData).Pool) - _, complete, err := am.RunOperation(context.Background(), po) + _, phase, err := am.RunOperation(context.Background(), po) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -124,9 +124,9 @@ func TestPrepareAndRunTransfer(t *testing.T) { assert.Equal(t, pool, po.Data.(transferData).Pool) assert.Equal(t, transfer, po.Data.(transferData).Transfer) - _, complete, err := am.RunOperation(context.Background(), po) + _, phase, err := am.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -163,9 +163,9 @@ func TestPrepareAndRunApproval(t *testing.T) { assert.Equal(t, pool, po.Data.(approvalData).Pool) assert.Equal(t, approval, po.Data.(approvalData).Approval) - _, complete, err := am.RunOperation(context.Background(), po) + _, phase, err := am.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -352,9 +352,9 @@ func TestRunOperationNotSupported(t *testing.T) { am, cancel := newTestAssets(t) defer cancel() - _, complete, err := am.RunOperation(context.Background(), &core.PreparedOperation{}) + _, phase, err := am.RunOperation(context.Background(), &core.PreparedOperation{}) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10378", err) } @@ -365,9 +365,9 @@ func TestRunOperationCreatePoolBadPlugin(t *testing.T) { op := &core.Operation{} pool := &core.TokenPool{} - _, complete, err := am.RunOperation(context.Background(), opCreatePool(op, pool)) + _, phase, err := am.RunOperation(context.Background(), opCreatePool(op, pool)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10272", err) } @@ -384,11 +384,11 @@ func TestRunOperationCreatePool(t *testing.T) { } mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) - mti.On("CreateTokenPool", context.Background(), "ns1:"+op.ID.String(), pool).Return(false, nil) + mti.On("CreateTokenPool", context.Background(), "ns1:"+op.ID.String(), pool).Return(core.OpPhaseInitializing, nil) - _, complete, err := am.RunOperation(context.Background(), opCreatePool(op, pool)) + _, phase, err := am.RunOperation(context.Background(), opCreatePool(op, pool)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -401,9 +401,9 @@ func TestRunOperationActivatePoolBadPlugin(t *testing.T) { op := &core.Operation{} pool := &core.TokenPool{} - _, complete, err := am.RunOperation(context.Background(), opActivatePool(op, pool)) + _, phase, err := am.RunOperation(context.Background(), opActivatePool(op, pool)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10272", err) } @@ -415,9 +415,9 @@ func TestRunOperationTransferBadPlugin(t *testing.T) { pool := &core.TokenPool{} transfer := &core.TokenTransfer{} - _, complete, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) + _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10272", err) } @@ -429,9 +429,9 @@ func TestRunOperationApprovalBadPlugin(t *testing.T) { pool := &core.TokenPool{} approval := &core.TokenApproval{} - _, complete, err := am.RunOperation(context.Background(), opApproval(op, pool, approval)) + _, phase, err := am.RunOperation(context.Background(), opApproval(op, pool, approval)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10272", err) } @@ -473,9 +473,9 @@ func TestRunOperationTransferMint(t *testing.T) { mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) mti.On("MintTokens", context.Background(), "ns1:"+op.ID.String(), "F1", transfer, (*fftypes.JSONAny)(nil)).Return(nil) - _, complete, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) + _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -501,9 +501,9 @@ func TestRunOperationTransferMintWithInterface(t *testing.T) { mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) mti.On("MintTokens", context.Background(), "ns1:"+op.ID.String(), "F1", transfer, pool.Methods).Return(nil) - _, complete, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) + _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -528,9 +528,9 @@ func TestRunOperationTransferBurn(t *testing.T) { mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) mti.On("BurnTokens", context.Background(), "ns1:"+op.ID.String(), "F1", transfer, (*fftypes.JSONAny)(nil)).Return(nil) - _, complete, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) + _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -555,9 +555,9 @@ func TestRunOperationTransfer(t *testing.T) { mti := am.tokens["magic-tokens"].(*tokenmocks.Plugin) mti.On("TransferTokens", context.Background(), "ns1:"+op.ID.String(), "F1", transfer, (*fftypes.JSONAny)(nil)).Return(nil) - _, complete, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) + _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mti.AssertExpectations(t) diff --git a/internal/assets/token_approval.go b/internal/assets/token_approval.go index 1235ae513..ed96b21c2 100644 --- a/internal/assets/token_approval.go +++ b/internal/assets/token_approval.go @@ -34,10 +34,11 @@ func (am *assetManager) GetTokenApprovals(ctx context.Context, filter ffapi.AndF } type approveSender struct { - mgr *assetManager - approval *core.TokenApprovalInput - resolved bool - msgSender syncasync.Sender + mgr *assetManager + approval *core.TokenApprovalInput + resolved bool + msgSender syncasync.Sender + idempotentSubmit bool } func (s *approveSender) Prepare(ctx context.Context) error { @@ -58,8 +59,9 @@ func (s *approveSender) setDefaults() { func (am *assetManager) NewApproval(approval *core.TokenApprovalInput) syncasync.Sender { sender := &approveSender{ - mgr: am, - approval: approval, + mgr: am, + approval: approval, + idempotentSubmit: approval.IdempotencyKey != "", } sender.setDefaults() return sender @@ -194,7 +196,7 @@ func (s *approveSender) sendInternal(ctx context.Context, method sendMethod) (er } } - _, err = s.mgr.operations.RunOperation(ctx, opApproval(op, pool, &s.approval.TokenApproval)) + _, err = s.mgr.operations.RunOperation(ctx, opApproval(op, pool, &s.approval.TokenApproval), s.idempotentSubmit) return err } diff --git a/internal/assets/token_approval_test.go b/internal/assets/token_approval_test.go index c27031720..2593aa78c 100644 --- a/internal/assets/token_approval_test.go +++ b/internal/assets/token_approval_test.go @@ -81,7 +81,7 @@ func TestTokenApprovalSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, false) assert.NoError(t, err) @@ -121,7 +121,7 @@ func TestTokenApprovalSuccessUnknownIdentity(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, false) assert.NoError(t, err) @@ -208,7 +208,7 @@ func TestApprovalDefaultPoolSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == tokenPools[0] && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, false) assert.NoError(t, err) @@ -474,7 +474,7 @@ func TestApprovalFail(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) _, err := am.TokenApproval(context.Background(), approval, false) assert.EqualError(t, err, "pop") @@ -555,7 +555,7 @@ func TestApprovalWithBroadcastMessage(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, false) assert.NoError(t, err) @@ -751,7 +751,7 @@ func TestApprovalWithPrivateMessage(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, false) assert.NoError(t, err) @@ -908,7 +908,7 @@ func TestTokenApprovalConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) msa.On("WaitForTokenApproval", context.Background(), mock.Anything, mock.Anything). Run(func(args mock.Arguments) { @@ -989,7 +989,7 @@ func TestApprovalWithBroadcastConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(approvalData) return op.Type == core.OpTypeTokenApproval && data.Pool == pool && data.Approval == &approval.TokenApproval - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TokenApproval(context.Background(), approval, true) assert.NoError(t, err) diff --git a/internal/assets/token_pool.go b/internal/assets/token_pool.go index 0142a65b0..63d0bbc8d 100644 --- a/internal/assets/token_pool.go +++ b/internal/assets/token_pool.go @@ -123,7 +123,7 @@ func (am *assetManager) createTokenPoolInternal(ctx context.Context, pool *core. return nil, err } - _, err = am.operations.RunOperation(ctx, opCreatePool(newOperation, &pool.TokenPool)) + _, err = am.operations.RunOperation(ctx, opCreatePool(newOperation, &pool.TokenPool), pool.IdempotencyKey != "") return &pool.TokenPool, err } @@ -154,7 +154,10 @@ func (am *assetManager) ActivateTokenPool(ctx context.Context, pool *core.TokenP return err } - _, err = am.operations.RunOperation(ctx, opActivatePool(op, pool)) + _, err = am.operations.RunOperation(ctx, opActivatePool(op, pool), + false, // TODO: this operation should be made idempotent, but cannot inherit this from the TX per our normal semantics + // as the transaction is only on the submitting side and this is triggered on all parties. + ) return err } diff --git a/internal/assets/token_pool_test.go b/internal/assets/token_pool_test.go index b493ba282..368291a36 100644 --- a/internal/assets/token_pool_test.go +++ b/internal/assets/token_pool_test.go @@ -110,7 +110,7 @@ func TestCreateTokenPoolDefaultConnectorSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(createPoolData) return op.Type == core.OpTypeTokenCreatePool && data.Pool == &pool.TokenPool - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.CreateTokenPool(context.Background(), pool, false) assert.NoError(t, err) @@ -369,7 +369,7 @@ func TestCreateTokenPoolFail(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(createPoolData) return op.Type == core.OpTypeTokenCreatePool && data.Pool == &pool.TokenPool - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) _, err := am.CreateTokenPool(context.Background(), pool, false) assert.Regexp(t, "pop", err) @@ -487,7 +487,7 @@ func TestCreateTokenPoolSyncSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(createPoolData) return op.Type == core.OpTypeTokenCreatePool && data.Pool == &pool.TokenPool - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.CreateTokenPool(context.Background(), pool, false) assert.NoError(t, err) @@ -521,7 +521,7 @@ func TestCreateTokenPoolAsyncSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(createPoolData) return op.Type == core.OpTypeTokenCreatePool && data.Pool == &pool.TokenPool - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.CreateTokenPool(context.Background(), pool, false) assert.NoError(t, err) @@ -562,7 +562,7 @@ func TestCreateTokenPoolConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(createPoolData) return op.Type == core.OpTypeTokenCreatePool && data.Pool == &pool.TokenPool - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.CreateTokenPool(context.Background(), pool, true) assert.NoError(t, err) @@ -596,7 +596,7 @@ func TestActivateTokenPool(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(activatePoolData) return op.Type == core.OpTypeTokenActivatePool && data.Pool == pool - })).Return(nil, nil) + }), false).Return(nil, nil) err := am.ActivateTokenPool(context.Background(), pool) assert.NoError(t, err) @@ -665,7 +665,7 @@ func TestActivateTokenPoolFail(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(activatePoolData) return op.Type == core.OpTypeTokenActivatePool && data.Pool == pool - })).Return(nil, fmt.Errorf("pop")) + }), false).Return(nil, fmt.Errorf("pop")) err := am.ActivateTokenPool(context.Background(), pool) assert.EqualError(t, err, "pop") @@ -737,7 +737,7 @@ func TestActivateTokenPoolSyncSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(activatePoolData) return op.Type == core.OpTypeTokenActivatePool && data.Pool == pool - })).Return(nil, nil) + }), false).Return(nil, nil) err := am.ActivateTokenPool(context.Background(), pool) assert.NoError(t, err) diff --git a/internal/assets/token_transfer.go b/internal/assets/token_transfer.go index cbf6e992a..f70b6c1ee 100644 --- a/internal/assets/token_transfer.go +++ b/internal/assets/token_transfer.go @@ -43,18 +43,20 @@ func (am *assetManager) GetTokenTransferByID(ctx context.Context, id string) (*c func (am *assetManager) NewTransfer(transfer *core.TokenTransferInput) syncasync.Sender { sender := &transferSender{ - mgr: am, - transfer: transfer, + mgr: am, + transfer: transfer, + idempotentSubmit: transfer.IdempotencyKey != "", } sender.setDefaults() return sender } type transferSender struct { - mgr *assetManager - transfer *core.TokenTransferInput - resolved bool - msgSender syncasync.Sender + mgr *assetManager + transfer *core.TokenTransferInput + resolved bool + msgSender syncasync.Sender + idempotentSubmit bool } // sendMethod is the specific operation requested of the transferSender. @@ -284,7 +286,7 @@ func (s *transferSender) sendInternal(ctx context.Context, method sendMethod) (e } } - _, err = s.mgr.operations.RunOperation(ctx, opTransfer(op, pool, &s.transfer.TokenTransfer)) + _, err = s.mgr.operations.RunOperation(ctx, opTransfer(op, pool, &s.transfer.TokenTransfer), s.idempotentSubmit) return err } diff --git a/internal/assets/token_transfer_test.go b/internal/assets/token_transfer_test.go index 9f4cf1872..6835530cc 100644 --- a/internal/assets/token_transfer_test.go +++ b/internal/assets/token_transfer_test.go @@ -101,7 +101,7 @@ func TestMintTokensSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &mint.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.MintTokens(context.Background(), mint, false) assert.NoError(t, err) @@ -270,7 +270,7 @@ func TestMintTokenDefaultPoolSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == tokenPools[0] && data.Transfer == &mint.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.MintTokens(context.Background(), mint, false) assert.NoError(t, err) @@ -461,7 +461,7 @@ func TestMintTokensFail(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &mint.TokenTransfer - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) _, err := am.MintTokens(context.Background(), mint, false) assert.EqualError(t, err, "pop") @@ -541,7 +541,7 @@ func TestMintTokensConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &mint.TokenTransfer - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) _, err := am.MintTokens(context.Background(), mint, true) assert.NoError(t, err) @@ -578,7 +578,7 @@ func TestBurnTokensSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &burn.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.BurnTokens(context.Background(), burn, false) assert.NoError(t, err) @@ -654,7 +654,7 @@ func TestBurnTokensConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &burn.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.BurnTokens(context.Background(), burn, true) assert.NoError(t, err) @@ -694,7 +694,7 @@ func TestTransferTokensSuccess(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &transfer.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TransferTokens(context.Background(), transfer, false) assert.NoError(t, err) @@ -869,7 +869,7 @@ func TestTransferTokensWithBroadcastMessage(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &transfer.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TransferTokens(context.Background(), transfer, false) assert.NoError(t, err) @@ -1069,7 +1069,7 @@ func TestTransferTokensWithPrivateMessage(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &transfer.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TransferTokens(context.Background(), transfer, false) assert.NoError(t, err) @@ -1195,7 +1195,7 @@ func TestTransferTokensConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &transfer.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TransferTokens(context.Background(), transfer, true) assert.NoError(t, err) @@ -1269,7 +1269,7 @@ func TestTransferTokensWithBroadcastConfirm(t *testing.T) { mom.On("RunOperation", context.Background(), mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferData) return op.Type == core.OpTypeTokenTransfer && data.Pool == pool && data.Transfer == &transfer.TokenTransfer - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := am.TransferTokens(context.Background(), transfer, true) assert.NoError(t, err) diff --git a/internal/broadcast/manager.go b/internal/broadcast/manager.go index afef2aeee..9800468fa 100644 --- a/internal/broadcast/manager.go +++ b/internal/broadcast/manager.go @@ -55,7 +55,7 @@ type Manager interface { // From operations.OperationHandler PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) } type broadcastManager struct { @@ -137,7 +137,7 @@ func (bm *broadcastManager) Name() string { func (bm *broadcastManager) dispatchBatch(ctx context.Context, payload *batch.DispatchPayload) error { // Ensure all the blobs are published - if err := bm.uploadBlobs(ctx, payload.Batch.TX.ID, payload.Data); err != nil { + if err := bm.uploadBlobs(ctx, payload.Batch.TX.ID, payload.Data, false /* batch processing does not currently use idempotency keys */); err != nil { return err } @@ -156,20 +156,20 @@ func (bm *broadcastManager) dispatchBatch(ctx context.Context, payload *batch.Di // We are in an (indefinite) retry cycle from the batch processor to dispatch this batch, that is only // terminated with shutdown. So we leave the operation pending on failure, as it is still being retried. // The user will still have the failure details recorded. - outputs, err := bm.operations.RunOperation(ctx, opUploadBatch(op, batch), operations.RemainPendingOnFailure) + outputs, err := bm.operations.RunOperation(ctx, opUploadBatch(op, batch), false /* batch processing does not currently use idempotency keys */) if err != nil { return err } payloadRef := outputs.GetString("payloadRef") log.L(ctx).Infof("Pinning broadcast batch %s with author=%s key=%s payloadRef=%s", batch.ID, batch.Author, batch.Key, payloadRef) - return bm.multiparty.SubmitBatchPin(ctx, &payload.Batch, payload.Pins, payloadRef) + return bm.multiparty.SubmitBatchPin(ctx, &payload.Batch, payload.Pins, payloadRef, false /* batch processing does not currently use idempotency keys */) } -func (bm *broadcastManager) uploadBlobs(ctx context.Context, tx *fftypes.UUID, data core.DataArray) error { +func (bm *broadcastManager) uploadBlobs(ctx context.Context, tx *fftypes.UUID, data core.DataArray, idempotentSubmit bool) error { for _, d := range data { // We only need to send a blob if there is one, and it's not been uploaded to the shared storage if d.Blob != nil && d.Blob.Hash != nil && d.Blob.Public == "" { - if err := bm.uploadDataBlob(ctx, tx, d); err != nil { + if err := bm.uploadDataBlob(ctx, tx, d, idempotentSubmit); err != nil { return err } } @@ -195,7 +195,7 @@ func (bm *broadcastManager) resolveData(ctx context.Context, id string) (*core.D return d, nil } -func (bm *broadcastManager) uploadDataBlob(ctx context.Context, tx *fftypes.UUID, d *core.Data) error { +func (bm *broadcastManager) uploadDataBlob(ctx context.Context, tx *fftypes.UUID, d *core.Data, idempotentSubmit bool) error { if d.Blob == nil || d.Blob.Hash == nil { return i18n.NewError(ctx, coremsgs.MsgDataDoesNotHaveBlob) } @@ -218,7 +218,7 @@ func (bm *broadcastManager) uploadDataBlob(ctx context.Context, tx *fftypes.UUID return i18n.NewError(ctx, coremsgs.MsgBlobNotFound, d.Blob.Hash) } - _, err = bm.operations.RunOperation(ctx, opUploadBlob(op, d, blobs[0])) + _, err = bm.operations.RunOperation(ctx, opUploadBlob(op, d, blobs[0]), idempotentSubmit) return err } @@ -257,7 +257,7 @@ func (bm *broadcastManager) PublishDataValue(ctx context.Context, id string, ide return nil, err } - if _, err := bm.operations.RunOperation(ctx, opUploadValue(op, d)); err != nil { + if _, err := bm.operations.RunOperation(ctx, opUploadValue(op, d), idempotencyKey != ""); err != nil { return nil, err } @@ -289,7 +289,7 @@ func (bm *broadcastManager) PublishDataBlob(ctx context.Context, id string, idem return d, err } - if err = bm.uploadDataBlob(ctx, txid, d); err != nil { + if err = bm.uploadDataBlob(ctx, txid, d, idempotencyKey != ""); err != nil { return nil, err } diff --git a/internal/broadcast/manager_test.go b/internal/broadcast/manager_test.go index 0590feb7f..c1b9caa9a 100644 --- a/internal/broadcast/manager_test.go +++ b/internal/broadcast/manager_test.go @@ -28,7 +28,6 @@ import ( "github.com/hyperledger/firefly/internal/coremsgs" "github.com/hyperledger/firefly/internal/data" "github.com/hyperledger/firefly/internal/database/sqlcommon" - "github.com/hyperledger/firefly/internal/operations" "github.com/hyperledger/firefly/mocks/batchmocks" "github.com/hyperledger/firefly/mocks/blockchainmocks" "github.com/hyperledger/firefly/mocks/databasemocks" @@ -245,7 +244,7 @@ func TestDispatchBatchUploadFail(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadBatchData) return op.Type == core.OpTypeSharedStorageUploadBatch && data.Batch.ID.Equals(state.Batch.ID) - }), operations.RemainPendingOnFailure).Return(nil, fmt.Errorf("pop")) + }), false).Return(nil, fmt.Errorf("pop")) err := bm.dispatchBatch(context.Background(), state) assert.EqualError(t, err, "pop") @@ -270,11 +269,11 @@ func TestDispatchBatchSubmitBatchPinSucceed(t *testing.T) { mmp := bm.multiparty.(*multipartymocks.Manager) mom := bm.operations.(*operationmocks.Manager) mom.On("AddOrReuseOperation", mock.Anything, mock.Anything).Return(nil) - mmp.On("SubmitBatchPin", mock.Anything, mock.Anything, mock.Anything, "payload1").Return(nil) + mmp.On("SubmitBatchPin", mock.Anything, mock.Anything, mock.Anything, "payload1", false).Return(nil) mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadBatchData) return op.Type == core.OpTypeSharedStorageUploadBatch && data.Batch.ID.Equals(state.Batch.ID) - }), operations.RemainPendingOnFailure).Return(getUploadBatchOutputs("payload1"), nil) + }), false).Return(getUploadBatchOutputs("payload1"), nil) err := bm.dispatchBatch(context.Background(), state) assert.NoError(t, err) @@ -302,11 +301,11 @@ func TestDispatchBatchSubmitBroadcastFail(t *testing.T) { mmp := bm.multiparty.(*multipartymocks.Manager) mom := bm.operations.(*operationmocks.Manager) mom.On("AddOrReuseOperation", mock.Anything, mock.Anything).Return(nil) - mmp.On("SubmitBatchPin", mock.Anything, mock.Anything, mock.Anything, "payload1").Return(fmt.Errorf("pop")) + mmp.On("SubmitBatchPin", mock.Anything, mock.Anything, mock.Anything, "payload1", false).Return(fmt.Errorf("pop")) mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadBatchData) return op.Type == core.OpTypeSharedStorageUploadBatch && data.Batch.ID.Equals(state.Batch.ID) - }), operations.RemainPendingOnFailure).Return(getUploadBatchOutputs("payload1"), nil) + }), false).Return(getUploadBatchOutputs("payload1"), nil) err := bm.dispatchBatch(context.Background(), state) assert.EqualError(t, err, "pop") @@ -344,7 +343,7 @@ func TestUploadBlobPublishFail(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadBlobData) return op.Type == core.OpTypeSharedStorageUploadBlob && data.Blob == blob - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) _, err := bm.PublishDataBlob(ctx, d.ID.String(), "idem1") assert.EqualError(t, err, "pop") @@ -482,7 +481,7 @@ func TestUploadBlobsGetBlobFail(t *testing.T) { Hash: blob.Hash, }, }, - }) + }, false) assert.Regexp(t, "pop", err) mdi.AssertExpectations(t) @@ -514,7 +513,7 @@ func TestUploadBlobsGetBlobNotFound(t *testing.T) { Hash: blob.Hash, }, }, - }) + }, false) assert.Regexp(t, "FF10239", err) mdi.AssertExpectations(t) @@ -543,7 +542,7 @@ func TestUploadBlobsGetBlobInsertOpFail(t *testing.T) { Hash: blob.Hash, }, }, - }) + }, true) assert.EqualError(t, err, "pop") mom.AssertExpectations(t) @@ -627,7 +626,7 @@ func TestUploadValueFail(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadValue) return op.Type == core.OpTypeSharedStorageUploadValue && data.Data.ID.Equals(d.ID) - })).Return(nil, fmt.Errorf("pop")) + }), false).Return(nil, fmt.Errorf("pop")) mtx := bm.txHelper.(*txcommonmocks.Helper) mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("")).Return(fftypes.NewUUID(), nil) @@ -656,7 +655,7 @@ func TestUploadValueOK(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadValue) return op.Type == core.OpTypeSharedStorageUploadValue && data.Data.ID.Equals(d.ID) - })).Return(nil, nil) + }), false).Return(nil, nil) mtx := bm.txHelper.(*txcommonmocks.Helper) mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("")).Return(fftypes.NewUUID(), nil) @@ -808,7 +807,7 @@ func TestUploadBlobOK(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(uploadBlobData) return op.Type == core.OpTypeSharedStorageUploadBlob && data.Blob == blob - })).Return(nil, nil) + }), false).Return(nil, nil) mtx := bm.txHelper.(*txcommonmocks.Helper) mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("")).Return(fftypes.NewUUID(), nil) diff --git a/internal/broadcast/operations.go b/internal/broadcast/operations.go index 68f74f412..80d98b39c 100644 --- a/internal/broadcast/operations.go +++ b/internal/broadcast/operations.go @@ -140,7 +140,7 @@ func (bm *broadcastManager) PrepareOperation(ctx context.Context, op *core.Opera } } -func (bm *broadcastManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (bm *broadcastManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case uploadBatchData: return bm.uploadBatch(ctx, data) @@ -149,71 +149,71 @@ func (bm *broadcastManager) RunOperation(ctx context.Context, op *core.PreparedO case uploadValue: return bm.uploadValue(ctx, data) default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } // uploadBatch uploads the serialized batch to public storage -func (bm *broadcastManager) uploadBatch(ctx context.Context, data uploadBatchData) (outputs fftypes.JSONObject, complete bool, err error) { +func (bm *broadcastManager) uploadBatch(ctx context.Context, data uploadBatchData) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { // Serialize the full payload, which has already been sealed for us by the BatchManager data.Batch.Namespace = bm.namespace.NetworkName payload, err := json.Marshal(data.Batch) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgSerializationFailed) + return nil, core.OpPhaseInitializing, i18n.WrapError(ctx, err, coremsgs.MsgSerializationFailed) } // Write it to IPFS to get a payload reference payloadRef, err := bm.sharedstorage.UploadData(ctx, bytes.NewReader(payload)) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } log.L(ctx).Infof("Published batch '%s' to shared storage: '%s'", data.Batch.ID, payloadRef) - return getUploadBatchOutputs(payloadRef), true, nil + return getUploadBatchOutputs(payloadRef), core.OpPhaseComplete, nil } // uploadBlob streams a blob from the local data exchange, to public storage -func (bm *broadcastManager) uploadBlob(ctx context.Context, data uploadBlobData) (outputs fftypes.JSONObject, complete bool, err error) { +func (bm *broadcastManager) uploadBlob(ctx context.Context, data uploadBlobData) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { // Stream from the local data exchange ... reader, err := bm.exchange.DownloadBlob(ctx, data.Blob.PayloadRef) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgDownloadBlobFailed, data.Blob.PayloadRef) + return nil, core.OpPhaseInitializing, i18n.WrapError(ctx, err, coremsgs.MsgDownloadBlobFailed, data.Blob.PayloadRef) } defer reader.Close() // ... to the shared storage data.Data.Blob.Public, err = bm.sharedstorage.UploadData(ctx, reader) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } // Update the data in the DB err = bm.database.UpdateData(ctx, bm.namespace.Name, data.Data.ID, database.DataQueryFactory.NewUpdate(ctx).Set("blob.public", data.Data.Blob.Public)) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } log.L(ctx).Infof("Published blob with hash '%s' for data '%s' to shared storage: '%s'", data.Data.Blob.Hash, data.Data.ID, data.Data.Blob.Public) - return getUploadBlobOutputs(data.Data.Blob.Public), true, nil + return getUploadBlobOutputs(data.Data.Blob.Public), core.OpPhaseComplete, nil } // uploadValue streams the value JSON from a data record to public storage -func (bm *broadcastManager) uploadValue(ctx context.Context, data uploadValue) (outputs fftypes.JSONObject, complete bool, err error) { +func (bm *broadcastManager) uploadValue(ctx context.Context, data uploadValue) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { // Upload to shared storage data.Data.Public, err = bm.sharedstorage.UploadData(ctx, bytes.NewReader(data.Data.Value.Bytes())) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } // Update the public reference for the data in the DB err = bm.database.UpdateData(ctx, bm.namespace.Name, data.Data.ID, database.DataQueryFactory.NewUpdate(ctx).Set("public", data.Data.Public)) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } log.L(ctx).Infof("Published value for data '%s' to shared storage: '%s'", data.Data.ID, data.Data.Public) - return getUploadBlobOutputs(data.Data.Public), true, nil + return getUploadBlobOutputs(data.Data.Public), core.OpPhaseComplete, nil } func (bm *broadcastManager) OnOperationUpdate(ctx context.Context, op *core.Operation, update *core.OperationUpdate) error { diff --git a/internal/broadcast/operations_test.go b/internal/broadcast/operations_test.go index 43210812c..ceec07c76 100644 --- a/internal/broadcast/operations_test.go +++ b/internal/broadcast/operations_test.go @@ -6,7 +6,7 @@ // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // -// http://www.apache.org/licenses/LICENSE-2.0 +// http://www.apache.org/licenses/LICENSE-2.0 // // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, @@ -61,9 +61,9 @@ func TestPrepareAndRunBatchBroadcast(t *testing.T) { assert.NoError(t, err) assert.Equal(t, bp.ID, po.Data.(uploadBatchData).Batch.ID) - _, complete, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mps.AssertExpectations(t) @@ -159,9 +159,9 @@ func TestRunOperationNotSupported(t *testing.T) { bm, cancel := newTestBroadcast(t) defer cancel() - _, complete, err := bm.RunOperation(context.Background(), &core.PreparedOperation{}) + _, phase, err := bm.RunOperation(context.Background(), &core.PreparedOperation{}) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10378", err) } @@ -178,9 +178,9 @@ func TestRunOperationBatchBroadcastInvalidData(t *testing.T) { }, } - _, complete, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10137", err) } @@ -198,9 +198,9 @@ func TestRunOperationBatchBroadcastPublishFail(t *testing.T) { mps := bm.sharedstorage.(*sharedstoragemocks.Plugin) mps.On("UploadData", context.Background(), mock.Anything).Return("", fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.EqualError(t, err, "pop") mps.AssertExpectations(t) @@ -221,10 +221,10 @@ func TestRunOperationBatchBroadcast(t *testing.T) { mdi := bm.database.(*databasemocks.Plugin) mps.On("UploadData", context.Background(), mock.Anything).Return("123", nil) - outputs, complete, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) + outputs, phase, err := bm.RunOperation(context.Background(), opUploadBatch(op, batch)) assert.Equal(t, "123", outputs["payloadRef"]) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mps.AssertExpectations(t) @@ -273,10 +273,10 @@ func TestPrepareAndRunUploadBlob(t *testing.T) { assert.Equal(t, data, po.Data.(uploadBlobData).Data) assert.Equal(t, blob, po.Data.(uploadBlobData).Blob) - outputs, complete, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) + outputs, phase, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) assert.Equal(t, "123", outputs["payloadRef"]) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mps.AssertExpectations(t) @@ -316,10 +316,10 @@ func TestPrepareAndRunValue(t *testing.T) { assert.NoError(t, err) assert.Equal(t, data, po.Data.(uploadValue).Data) - outputs, complete, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) + outputs, phase, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) assert.Equal(t, "123", outputs["payloadRef"]) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) mps.AssertExpectations(t) @@ -527,9 +527,9 @@ func TestRunOperationUploadBlobUpdateFail(t *testing.T) { mps.On("UploadData", context.Background(), mock.Anything).Return("123", nil) mdi.On("UpdateData", context.Background(), "ns1", data.ID, mock.Anything).Return(fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mps.AssertExpectations(t) @@ -553,9 +553,9 @@ func TestRunOperationUploadValueUpdateFail(t *testing.T) { mps.On("UploadData", context.Background(), mock.Anything).Return("123", nil) mdi.On("UpdateData", context.Background(), "ns1", data.ID, mock.Anything).Return(fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) + _, phase, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mps.AssertExpectations(t) @@ -584,9 +584,9 @@ func TestRunOperationUploadBlobUploadFail(t *testing.T) { mdx.On("DownloadBlob", context.Background(), mock.Anything).Return(reader, nil) mps.On("UploadData", context.Background(), mock.Anything).Return("", fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mps.AssertExpectations(t) @@ -606,9 +606,9 @@ func TestRunOperationUploadValueUploadFail(t *testing.T) { mps := bm.sharedstorage.(*sharedstoragemocks.Plugin) mps.On("UploadData", context.Background(), mock.Anything).Return("", fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) + _, phase, err := bm.RunOperation(context.Background(), opUploadValue(op, data)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mps.AssertExpectations(t) @@ -635,9 +635,9 @@ func TestRunOperationUploadBlobDownloadFail(t *testing.T) { reader := ioutil.NopCloser(strings.NewReader("some data")) mdx.On("DownloadBlob", context.Background(), mock.Anything).Return(reader, fmt.Errorf("pop")) - _, complete, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) + _, phase, err := bm.RunOperation(context.Background(), opUploadBlob(op, data, blob)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mps.AssertExpectations(t) diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index 64838f054..f710214fa 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -81,7 +81,7 @@ type Manager interface { // From operations.OperationHandler PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) } type contractManager struct { @@ -370,7 +370,7 @@ func (cm *contractManager) DeployContract(ctx context.Context, req *core.Contrac } send := func(ctx context.Context) error { - _, err := cm.operations.RunOperation(ctx, opBlockchainContractDeploy(op, req)) + _, err := cm.operations.RunOperation(ctx, opBlockchainContractDeploy(op, req), req.IdempotencyKey != "") return err } if waitConfirm { @@ -434,7 +434,7 @@ func (cm *contractManager) InvokeContract(ctx context.Context, req *core.Contrac return op, msgSender.Send(ctx) } send := func(ctx context.Context) error { - _, err := cm.operations.RunOperation(ctx, txcommon.OpBlockchainInvoke(op, req, nil)) + _, err := cm.operations.RunOperation(ctx, txcommon.OpBlockchainInvoke(op, req, nil), req.IdempotencyKey != "") return err } if waitConfirm { diff --git a/internal/contracts/manager_test.go b/internal/contracts/manager_test.go index dfed650e6..9275dfb63 100644 --- a/internal/contracts/manager_test.go +++ b/internal/contracts/manager_test.go @@ -1686,7 +1686,7 @@ func TestDeployContract(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(blockchainContractDeployData) return op.Type == core.OpTypeBlockchainContractDeploy && data.Request == req - })).Return(nil, nil) + }), true).Return(nil, nil) _, err := cm.DeployContract(context.Background(), req, false) @@ -1917,7 +1917,7 @@ func TestInvokeContract(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BlockchainInvokeData) return op.Type == core.OpTypeBlockchainInvoke && data.Request == req - })).Return(nil, nil) + }), true).Return(nil, nil) opaqueData := "anything" mbi.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) mbi.On("ValidateInvokeRequest", mock.Anything, opaqueData, req.Input, false).Return(nil) @@ -1964,7 +1964,7 @@ func TestInvokeContractViaFFI(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BlockchainInvokeData) return op.Type == core.OpTypeBlockchainInvoke && data.Request == req - })).Return(nil, nil) + }), true).Return(nil, nil) opaqueData := "anything" mbi.On("ParseInterface", context.Background(), method, errors).Return(opaqueData, nil) mbi.On("ValidateInvokeRequest", mock.Anything, opaqueData, req.Input, false).Return(nil) @@ -2505,7 +2505,7 @@ func TestInvokeContractConfirm(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BlockchainInvokeData) return op.Type == core.OpTypeBlockchainInvoke && data.Request == req - })).Return(nil, nil) + }), true).Return(nil, nil) msa.On("WaitForInvokeOperation", mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { send := args[2].(syncasync.SendFunction) @@ -2557,7 +2557,7 @@ func TestInvokeContractFail(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BlockchainInvokeData) return op.Type == core.OpTypeBlockchainInvoke && data.Request == req - })).Return(nil, fmt.Errorf("pop")) + }), true).Return(nil, fmt.Errorf("pop")) opaqueData := "anything" mbi.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) mbi.On("ValidateInvokeRequest", mock.Anything, opaqueData, req.Input, false).Return(nil) @@ -3100,7 +3100,7 @@ func TestInvokeContractAPI(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BlockchainInvokeData) return op.Type == core.OpTypeBlockchainInvoke && data.Request == req - })).Return(nil, nil) + }), true).Return(nil, nil) opaqueData := "anything" mbi.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) mbi.On("ValidateInvokeRequest", mock.Anything, opaqueData, req.Input, false).Return(nil) diff --git a/internal/contracts/operations.go b/internal/contracts/operations.go index 2e9c6399e..a47398772 100644 --- a/internal/contracts/operations.go +++ b/internal/contracts/operations.go @@ -25,6 +25,7 @@ import ( "github.com/hyperledger/firefly/internal/batch" "github.com/hyperledger/firefly/internal/coremsgs" "github.com/hyperledger/firefly/internal/data" + "github.com/hyperledger/firefly/internal/operations" "github.com/hyperledger/firefly/internal/txcommon" "github.com/hyperledger/firefly/pkg/blockchain" "github.com/hyperledger/firefly/pkg/core" @@ -112,7 +113,7 @@ func (cm *contractManager) PrepareOperation(ctx context.Context, op *core.Operat } } -func (cm *contractManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (cm *contractManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case txcommon.BlockchainInvokeData: req := data.Request @@ -128,15 +129,16 @@ func (cm *contractManager) RunOperation(ctx context.Context, op *core.PreparedOp } bcParsedMethod, err := cm.validateInvokeContractRequest(ctx, req, false /* do-not revalidate with the blockchain connector - just send it */) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } - return nil, false, cm.blockchain.InvokeContract(ctx, op.NamespacedIDString(), req.Key, req.Location, bcParsedMethod, req.Input, req.Options, batchPin) - + err = cm.blockchain.InvokeContract(ctx, op.NamespacedIDString(), req.Key, req.Location, bcParsedMethod, req.Input, req.Options, batchPin) + return nil, operations.ErrTernary(err, core.OpPhaseInitializing, core.OpPhasePending), err case blockchainContractDeployData: req := data.Request - return nil, false, cm.blockchain.DeployContract(ctx, op.NamespacedIDString(), req.Key, req.Definition, req.Contract, req.Input, req.Options) + err = cm.blockchain.DeployContract(ctx, op.NamespacedIDString(), req.Key, req.Definition, req.Contract, req.Input, req.Options) + return nil, operations.ErrTernary(err, core.OpPhaseInitializing, core.OpPhasePending), err default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } diff --git a/internal/contracts/operations_test.go b/internal/contracts/operations_test.go index 871541a73..abc8e70e7 100644 --- a/internal/contracts/operations_test.go +++ b/internal/contracts/operations_test.go @@ -93,9 +93,9 @@ func TestPrepareAndRunBlockchainInvoke(t *testing.T) { assert.NoError(t, err) assert.Equal(t, req, po.Data.(txcommon.BlockchainInvokeData).Request) - _, complete, err := cm.RunOperation(context.Background(), po) + _, phase, err := cm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mbi.AssertExpectations(t) @@ -131,9 +131,9 @@ func TestPrepareAndRunBlockchainInvokeValidateFail(t *testing.T) { assert.NoError(t, err) assert.Equal(t, req, po.Data.(txcommon.BlockchainInvokeData).Request) - _, complete, err := cm.RunOperation(context.Background(), po) + _, phase, err := cm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "pop", err) mbi.AssertExpectations(t) @@ -164,9 +164,9 @@ func TestPrepareAndRunBlockchainContractDeploy(t *testing.T) { assert.NoError(t, err) assert.Equal(t, req, po.Data.(blockchainContractDeployData).Request) - _, complete, err := cm.RunOperation(context.Background(), po) + _, phase, err := cm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mbi.AssertExpectations(t) @@ -595,9 +595,9 @@ func TestRunBlockchainInvokeWithBatch(t *testing.T) { Contexts: []*fftypes.Bytes32{pin}, PayloadRef: "test-payload", }) - _, complete, err := cm.RunOperation(context.Background(), po) + _, phase, err := cm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mbi.AssertExpectations(t) @@ -606,9 +606,9 @@ func TestRunBlockchainInvokeWithBatch(t *testing.T) { func TestRunOperationNotSupported(t *testing.T) { cm := newTestContractManager() - _, complete, err := cm.RunOperation(context.Background(), &core.PreparedOperation{}) + _, phase, err := cm.RunOperation(context.Background(), &core.PreparedOperation{}) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10378", err) } diff --git a/internal/definitions/sender_tokenpool_test.go b/internal/definitions/sender_tokenpool_test.go index 4e2cb1b7f..8e15bfdb3 100644 --- a/internal/definitions/sender_tokenpool_test.go +++ b/internal/definitions/sender_tokenpool_test.go @@ -184,6 +184,27 @@ func TestDefineTokenPoolNonMultipartyTokenPoolFail(t *testing.T) { assert.Regexp(t, "pop", err) } +func TestDefineTokenPoolNonMultipartyTokenPoolFailInner(t *testing.T) { + ds := newTestDefinitionSender(t) + defer ds.cleanup(t) + + pool := &core.TokenPool{ + ID: fftypes.NewUUID(), + Namespace: "ns1", + Name: "mypool", + Type: core.TokenTypeNonFungible, + Locator: "N1", + Symbol: "COIN", + Connector: "connector1", + Published: false, + } + + ds.mdi.On("InsertOrGetTokenPool", mock.Anything, pool).Return(nil, fmt.Errorf("error2: [%w]", fmt.Errorf("pop"))) + + err := ds.DefineTokenPool(context.Background(), pool, false) + assert.Regexp(t, "pop", err) +} + func TestDefineTokenPoolBadName(t *testing.T) { ds := newTestDefinitionSender(t) defer ds.cleanup(t) diff --git a/internal/events/batch_pin_complete.go b/internal/events/batch_pin_complete.go index da0c6c57e..7dccbe5cb 100644 --- a/internal/events/batch_pin_complete.go +++ b/internal/events/batch_pin_complete.go @@ -90,7 +90,7 @@ func (em *eventManager) postBlockchainBatchPinEventInsert(ctx context.Context, e } // Kick off a download for broadcast batches if the batch isn't already persisted if !private && batch == nil { - if err := em.sharedDownload.InitiateDownloadBatch(ctx, batchPin.TransactionID, batchPin.BatchPayloadRef); err != nil { + if err := em.sharedDownload.InitiateDownloadBatch(ctx, batchPin.TransactionID, batchPin.BatchPayloadRef, false /* batch processing does not currently use idempotency keys */); err != nil { return err } } diff --git a/internal/events/batch_pin_complete_test.go b/internal/events/batch_pin_complete_test.go index 23e1ed4b5..fc78d63ac 100644 --- a/internal/events/batch_pin_complete_test.go +++ b/internal/events/batch_pin_complete_test.go @@ -126,7 +126,7 @@ func TestBatchPinCompleteOkBroadcast(t *testing.T) { })).Return(nil).Once() em.mdi.On("InsertPins", mock.Anything, mock.Anything).Return(nil).Once() em.mdi.On("GetBatchByID", mock.Anything, "ns1", mock.Anything).Return(nil, nil) - em.msd.On("InitiateDownloadBatch", mock.Anything, batchPin.TransactionID, batchPin.BatchPayloadRef).Return(nil) + em.msd.On("InitiateDownloadBatch", mock.Anything, batchPin.TransactionID, batchPin.BatchPayloadRef, false).Return(nil) err := em.BlockchainEventBatch([]*blockchain.EventToDispatch{ { @@ -369,7 +369,7 @@ func TestSequencedBroadcastInitiateDownloadFail(t *testing.T) { em.mdi.On("InsertEvent", mock.Anything, mock.Anything).Return(nil) em.mdi.On("InsertPins", mock.Anything, mock.Anything).Return(nil) em.mdi.On("GetBatchByID", mock.Anything, "ns1", mock.Anything).Return(nil, nil) - em.msd.On("InitiateDownloadBatch", mock.Anything, batchPin.TransactionID, batchPin.BatchPayloadRef).Return(fmt.Errorf("pop")) + em.msd.On("InitiateDownloadBatch", mock.Anything, batchPin.TransactionID, batchPin.BatchPayloadRef, false).Return(fmt.Errorf("pop")) err := em.BlockchainEventBatch([]*blockchain.EventToDispatch{ { @@ -769,7 +769,7 @@ func TestPersistBatchDataWithPublicInitiateDownload(t *testing.T) { em.mdi.On("GetBlobs", mock.Anything, mock.Anything, mock.Anything).Return([]*core.Blob{}, nil, nil) - em.msd.On("InitiateDownloadBlob", mock.Anything, batch.Payload.TX.ID, data.ID, "ref1").Return(nil) + em.msd.On("InitiateDownloadBlob", mock.Anything, batch.Payload.TX.ID, data.ID, "ref1", false).Return(nil) valid, err := em.checkAndInitiateBlobDownloads(context.Background(), batch, 0, data) assert.Nil(t, err) @@ -794,7 +794,7 @@ func TestPersistBatchDataWithPublicInitiateDownloadFail(t *testing.T) { em.mdi.On("GetBlobs", mock.Anything, mock.Anything, mock.Anything).Return([]*core.Blob{}, nil, nil) - em.msd.On("InitiateDownloadBlob", mock.Anything, batch.Payload.TX.ID, data.ID, "ref1").Return(fmt.Errorf("pop")) + em.msd.On("InitiateDownloadBlob", mock.Anything, batch.Payload.TX.ID, data.ID, "ref1", false).Return(fmt.Errorf("pop")) valid, err := em.checkAndInitiateBlobDownloads(context.Background(), batch, 0, data) assert.Regexp(t, "pop", err) diff --git a/internal/events/persist_batch.go b/internal/events/persist_batch.go index b31a3bb05..831cfb493 100644 --- a/internal/events/persist_batch.go +++ b/internal/events/persist_batch.go @@ -177,7 +177,7 @@ func (em *eventManager) checkAndInitiateBlobDownloads(ctx context.Context, batch log.L(ctx).Errorf("Invalid data entry %d id=%s in batch '%s' - missing public blob reference", i, data.ID, batch.ID) return false, nil } - if err = em.sharedDownload.InitiateDownloadBlob(ctx, batch.Payload.TX.ID, data.ID, data.Blob.Public); err != nil { + if err = em.sharedDownload.InitiateDownloadBlob(ctx, batch.Payload.TX.ID, data.ID, data.Blob.Public, false /* batch processing does not currently use idempotency keys */); err != nil { return false, err } } diff --git a/internal/multiparty/manager.go b/internal/multiparty/manager.go index 3fc3277f4..b0c7e1a61 100644 --- a/internal/multiparty/manager.go +++ b/internal/multiparty/manager.go @@ -56,14 +56,14 @@ type Manager interface { GetNetworkVersion() int // SubmitBatchPin sequences a batch of message globally to all viewers of a given ledger - SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string) error + SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string, idempotentSubmit bool) error // SubmitNetworkAction writes a special "BatchPin" event which signals the plugin to take an action - SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction) error + SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction, idempotentSubmit bool) error // From operations.OperationHandler PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) } type Config struct { @@ -204,7 +204,7 @@ func (mm *multipartyManager) GetNetworkVersion() int { return mm.namespace.Contracts.Active.Info.Version } -func (mm *multipartyManager) SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction) error { +func (mm *multipartyManager) SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction, idempotentSubmit bool) error { if action.Type != core.NetworkActionTerminate { return i18n.NewError(ctx, coremsgs.MsgUnrecognizedNetworkAction, action.Type) } @@ -224,7 +224,7 @@ func (mm *multipartyManager) SubmitNetworkAction(ctx context.Context, signingKey return err } - _, err = mm.operations.RunOperation(ctx, opNetworkAction(op, action.Type, signingKey)) + _, err = mm.operations.RunOperation(ctx, opNetworkAction(op, action.Type, signingKey), idempotentSubmit) return err } @@ -244,13 +244,13 @@ func (mm *multipartyManager) prepareInvokeOperation(ctx context.Context, batch * }), nil } -func (mm *multipartyManager) SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string) error { +func (mm *multipartyManager) SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string, idempotentSubmit bool) error { if batch.TX.Type == core.TransactionTypeContractInvokePin { preparedOp, err := mm.prepareInvokeOperation(ctx, batch, contexts, payloadRef) if err != nil { return err } else if preparedOp != nil { - _, err = mm.operations.RunOperation(ctx, preparedOp) + _, err = mm.operations.RunOperation(ctx, preparedOp, idempotentSubmit) return err } log.L(ctx).Warnf("No invoke operation found on transaction %s", batch.TX.ID) @@ -269,6 +269,6 @@ func (mm *multipartyManager) SubmitBatchPin(ctx context.Context, batch *core.Bat if mm.metrics.IsMetricsEnabled() { mm.metrics.CountBatchPin() } - _, err := mm.operations.RunOperation(ctx, opBatchPin(op, batch, contexts, payloadRef)) + _, err := mm.operations.RunOperation(ctx, opBatchPin(op, batch, contexts, payloadRef), idempotentSubmit) return err } diff --git a/internal/multiparty/manager_test.go b/internal/multiparty/manager_test.go index f9dc8e885..8dfc1218c 100644 --- a/internal/multiparty/manager_test.go +++ b/internal/multiparty/manager_test.go @@ -275,11 +275,11 @@ func TestSubmitNetworkAction(t *testing.T) { mp.mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(networkActionData) return op.Type == core.OpTypeBlockchainNetworkAction && data.Type == core.NetworkActionTerminate - })).Return(nil, nil) + }), false).Return(nil, nil) err := mp.ConfigureContract(context.Background()) assert.NoError(t, err) - err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}) + err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}, false) assert.Nil(t, err) mp.mbi.AssertExpectations(t) @@ -310,7 +310,7 @@ func TestSubmitNetworkActionTXFail(t *testing.T) { err := mp.ConfigureContract(context.Background()) assert.NoError(t, err) - err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}) + err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}, false) assert.EqualError(t, err, "pop") mp.mbi.AssertExpectations(t) @@ -343,7 +343,7 @@ func TestSubmitNetworkActionOpFail(t *testing.T) { err := mp.ConfigureContract(context.Background()) assert.NoError(t, err) - err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}) + err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: core.NetworkActionTerminate}, false) assert.EqualError(t, err, "pop") mp.mbi.AssertExpectations(t) @@ -373,7 +373,7 @@ func TestSubmitNetworkActionBadType(t *testing.T) { err := mp.ConfigureContract(context.Background()) assert.NoError(t, err) - err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: "BAD"}) + err = mp.SubmitNetworkAction(context.Background(), "0x123", &core.NetworkAction{Type: "BAD"}, false) assert.Regexp(t, "FF10397", err) mp.mbi.AssertExpectations(t) @@ -410,9 +410,9 @@ func TestSubmitBatchPinOk(t *testing.T) { mp.mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BatchPinData) return op.Type == core.OpTypeBlockchainPinBatch && data.Batch == batch - })).Return(nil, nil) + }), false).Return(nil, nil) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.NoError(t, err) } @@ -448,9 +448,9 @@ func TestSubmitPinnedBatchWithMetricsOk(t *testing.T) { mp.mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BatchPinData) return op.Type == core.OpTypeBlockchainPinBatch && data.Batch == batch - })).Return(nil, nil) + }), false).Return(nil, nil) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.NoError(t, err) } @@ -483,9 +483,9 @@ func TestSubmitBatchPinWithBatchOk(t *testing.T) { assert.Equal(t, contexts, data.BatchPin.Contexts) assert.Equal(t, "payload1", data.BatchPin.PayloadRef) return op.Type == core.OpTypeBlockchainInvoke && data.BatchPin.Batch == batch - })).Return(nil, nil) + }), false).Return(nil, nil) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.NoError(t, err) } @@ -511,7 +511,7 @@ func TestSubmitBatchPinWithBatchOpFailure(t *testing.T) { mp.mth.On("FindOperationInTransaction", ctx, batch.TX.ID, core.OpTypeBlockchainInvoke).Return(nil, fmt.Errorf("pop")) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.EqualError(t, err, "pop") } @@ -543,7 +543,7 @@ func TestSubmitBatchPinWithBatchOpMalformed(t *testing.T) { mp.mth.On("FindOperationInTransaction", ctx, batch.TX.ID, core.OpTypeBlockchainInvoke).Return(invokeOp, nil) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.Regexp(t, "FF00127", err) } @@ -581,9 +581,9 @@ func TestSubmitBatchPinWithBatchOpNotFound(t *testing.T) { mp.mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(txcommon.BatchPinData) return op.Type == core.OpTypeBlockchainPinBatch && data.Batch == batch - })).Return(nil, nil) + }), false).Return(nil, nil) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.NoError(t, err) } @@ -608,7 +608,7 @@ func TestSubmitPinnedBatchOpFail(t *testing.T) { mp.mbi.On("Name").Return("ut") mp.mom.On("AddOrReuseOperation", ctx, mock.Anything).Return(fmt.Errorf("pop")) - err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1") + err := mp.SubmitBatchPin(ctx, batch, contexts, "payload1", false) assert.Regexp(t, "pop", err) } diff --git a/internal/multiparty/operations.go b/internal/multiparty/operations.go index d2267613d..945e7aca2 100644 --- a/internal/multiparty/operations.go +++ b/internal/multiparty/operations.go @@ -22,6 +22,7 @@ import ( "github.com/hyperledger/firefly-common/pkg/fftypes" "github.com/hyperledger/firefly-common/pkg/i18n" "github.com/hyperledger/firefly/internal/coremsgs" + "github.com/hyperledger/firefly/internal/operations" "github.com/hyperledger/firefly/internal/txcommon" "github.com/hyperledger/firefly/pkg/blockchain" "github.com/hyperledger/firefly/pkg/core" @@ -98,25 +99,25 @@ func (mm *multipartyManager) PrepareOperation(ctx context.Context, op *core.Oper } } -func (mm *multipartyManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (mm *multipartyManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case txcommon.BatchPinData: batch := data.Batch contract := mm.namespace.Contracts.Active - return nil, false, mm.blockchain.SubmitBatchPin(ctx, op.NamespacedIDString(), batch.Namespace, batch.Key, &blockchain.BatchPin{ + err = mm.blockchain.SubmitBatchPin(ctx, op.NamespacedIDString(), batch.Namespace, batch.Key, &blockchain.BatchPin{ TransactionID: batch.TX.ID, BatchID: batch.ID, BatchHash: batch.Hash, BatchPayloadRef: data.PayloadRef, Contexts: data.Contexts, }, contract.Location) - + return nil, operations.ErrTernary(err, core.OpPhaseInitializing, core.OpPhasePending), err case networkActionData: contract := mm.namespace.Contracts.Active - return nil, false, mm.blockchain.SubmitNetworkAction(ctx, op.NamespacedIDString(), data.Key, data.Type, contract.Location) - + err = mm.blockchain.SubmitNetworkAction(ctx, op.NamespacedIDString(), data.Key, data.Type, contract.Location) + return nil, operations.ErrTernary(err, core.OpPhaseInitializing, core.OpPhasePending), err default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } diff --git a/internal/multiparty/operations_test.go b/internal/multiparty/operations_test.go index 79c075d51..b7ee65f09 100644 --- a/internal/multiparty/operations_test.go +++ b/internal/multiparty/operations_test.go @@ -59,9 +59,9 @@ func TestPrepareAndRunBatchPin(t *testing.T) { assert.NoError(t, err) assert.Equal(t, batch, po.Data.(txcommon.BatchPinData).Batch) - _, complete, err := mp.RunOperation(context.Background(), opBatchPin(op, batch, contexts, "payload1")) + _, phase, err := mp.RunOperation(context.Background(), opBatchPin(op, batch, contexts, "payload1")) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) } @@ -82,9 +82,9 @@ func TestPrepareAndRunNetworkAction(t *testing.T) { assert.NoError(t, err) assert.Equal(t, core.NetworkActionTerminate, po.Data.(networkActionData).Type) - _, complete, err := mp.RunOperation(context.Background(), opNetworkAction(op, core.NetworkActionTerminate, "0x123")) + _, phase, err := mp.RunOperation(context.Background(), opNetworkAction(op, core.NetworkActionTerminate, "0x123")) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mp.mbi.AssertExpectations(t) @@ -133,9 +133,9 @@ func TestRunOperationNotSupported(t *testing.T) { mp := newTestMultipartyManager() defer mp.cleanup(t) - _, complete, err := mp.RunOperation(context.Background(), &core.PreparedOperation{}) + _, phase, err := mp.RunOperation(context.Background(), &core.PreparedOperation{}) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10378", err) } @@ -204,9 +204,9 @@ func TestRunBatchPinV1(t *testing.T) { mp.mbi.On("SubmitBatchPin", context.Background(), "ns1:"+op.ID.String(), "ns1", "0x123", mock.Anything, mock.Anything).Return(nil) - _, complete, err := mp.RunOperation(context.Background(), opBatchPin(op, batch, contexts, "payload1")) + _, phase, err := mp.RunOperation(context.Background(), opBatchPin(op, batch, contexts, "payload1")) - assert.False(t, complete) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) } diff --git a/internal/operations/manager.go b/internal/operations/manager.go index 225022ef4..ccef670bf 100644 --- a/internal/operations/manager.go +++ b/internal/operations/manager.go @@ -35,14 +35,14 @@ import ( type OperationHandler interface { core.Named PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) OnOperationUpdate(ctx context.Context, op *core.Operation, update *core.OperationUpdate) error } type Manager interface { RegisterHandler(ctx context.Context, handler OperationHandler, ops []core.OpType) PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation, options ...RunOperationOption) (fftypes.JSONObject, error) + RunOperation(ctx context.Context, op *core.PreparedOperation, idempotentSubmit bool) (fftypes.JSONObject, error) RetryOperation(ctx context.Context, opID *fftypes.UUID) (*core.Operation, error) ResubmitOperations(ctx context.Context, txID *fftypes.UUID) ([]*core.Operation, error) AddOrReuseOperation(ctx context.Context, op *core.Operation, hooks ...database.PostCompletionHook) error @@ -59,17 +59,20 @@ type ConflictError interface { IsConflictError() bool } -type RunOperationOption int - -const ( - RemainPendingOnFailure RunOperationOption = iota -) +func ErrTernary(err error, ifErr, ifNoError core.OpPhase) core.OpPhase { + phase := ifErr + if err == nil { + phase = ifNoError + } + return phase +} type operationsManager struct { ctx context.Context namespace string database database.Plugin handlers map[core.OpType]OperationHandler + txHelper txcommon.Helper updater *operationUpdater cache cache.CInterface } @@ -96,6 +99,7 @@ func NewOperationsManager(ctx context.Context, ns string, di database.Plugin, tx ctx: ctx, namespace: ns, database: di, + txHelper: txHelper, handlers: make(map[core.OpType]OperationHandler), } om.updater = newOperationUpdater(ctx, om, di, txHelper) @@ -142,7 +146,7 @@ func (om *operationsManager) ResubmitOperations(ctx context.Context, txID *fftyp continue } prepOp, _ := om.PrepareOperation(ctx, nextInitializedOp) - _, resubmitErr = om.RunOperation(ctx, prepOp) + _, resubmitErr = om.RunOperation(ctx, prepOp, true /* we only call ResubmitOperations in idempotent submit cases */) if resubmitErr != nil { break } @@ -152,39 +156,49 @@ func (om *operationsManager) ResubmitOperations(ctx context.Context, txID *fftyp return resubmitted, resubmitErr } -func (om *operationsManager) RunOperation(ctx context.Context, op *core.PreparedOperation, options ...RunOperationOption) (fftypes.JSONObject, error) { - failState := core.OpStatusFailed - for _, o := range options { - if o == RemainPendingOnFailure { - failState = core.OpStatusPending - } - } - +func (om *operationsManager) RunOperation(ctx context.Context, op *core.PreparedOperation, idempotentSubmit bool) (fftypes.JSONObject, error) { handler, ok := om.handlers[op.Type] if !ok { return nil, i18n.NewError(ctx, coremsgs.MsgOperationNotSupported, op.Type) } log.L(ctx).Infof("Executing %s operation %s via handler %s", op.Type, op.ID, handler.Name()) log.L(ctx).Tracef("Operation detail: %+v", op) - outputs, complete, err := handler.RunOperation(ctx, op) + outputs, phase, err := handler.RunOperation(ctx, op) if err != nil { - conflictErr, ok := err.(ConflictError) - if ok && conflictErr.IsConflictError() { - log.L(ctx).Infof("Leaving operation %s operation %s status unchanged after conflict", op.Type, op.ID) - } else { - om.SubmitOperationUpdate(&core.OperationUpdate{ - NamespacedOpID: op.NamespacedIDString(), - Plugin: op.Plugin, - Status: failState, - ErrorMessage: err.Error(), - Output: outputs, - }) + conflictErr, conflictTestOk := err.(ConflictError) + var failState core.OpStatus + switch { + case conflictTestOk && conflictErr.IsConflictError(): + // We are now pending - we know the connector has the action we're attempting to submit + // + // The async processing in SubmitOperationUpdate does not allow us to go back to pending, if + // we have progressed to failed through an async event that gets ordered before this update. + // So this is safe + failState = core.OpStatusPending + log.L(ctx).Infof("Setting operation %s operation %s status to %s after conflict", op.Type, op.ID, failState) + case phase == core.OpPhaseInitializing && idempotentSubmit: + // We haven't submitted the operation yet - so we will reuse the operation if the user retires with the same idempotency key + failState = core.OpStatusInitialized + case phase == core.OpPhasePending: + // This error is past the point we have submitted to the connector - idempotency error from here on in on resubmit. + // This also implies we are continuing to progress the transaction, and expecting it to update through events. + failState = core.OpStatusPending + default: + // Ok, we're failed + failState = core.OpStatusFailed } + om.SubmitOperationUpdate(&core.OperationUpdate{ + NamespacedOpID: op.NamespacedIDString(), + Plugin: op.Plugin, + Status: failState, + ErrorMessage: err.Error(), + Output: outputs, + }) } else { // No error so move us from "Initialized" to "Pending" newState := core.OpStatusPending - if complete { + if phase == core.OpPhaseComplete { // If the operation is actually completed synchronously skip "Pending" state and go to "Succeeded" newState = core.OpStatusSucceeded } @@ -212,12 +226,21 @@ func (om *operationsManager) findLatestRetry(ctx context.Context, opID *fftypes. func (om *operationsManager) RetryOperation(ctx context.Context, opID *fftypes.UUID) (op *core.Operation, err error) { var po *core.PreparedOperation + var idempotencyKey core.IdempotencyKey err = om.database.RunAsGroup(ctx, func(ctx context.Context) error { op, err = om.findLatestRetry(ctx, opID) if err != nil { return err } + tx, err := om.updater.txHelper.GetTransactionByIDCached(ctx, op.Transaction) + if err != nil { + return err + } + if tx != nil { + idempotencyKey = tx.IdempotencyKey + } + // Create a copy of the operation with a new ID op.ID = fftypes.NewUUID() op.Status = core.OpStatusInitialized @@ -244,7 +267,8 @@ func (om *operationsManager) RetryOperation(ctx context.Context, opID *fftypes.U return nil, err } - _, err = om.RunOperation(ctx, po) + log.L(ctx).Debugf("Retry initiation for operation %s idempotencyKey=%s", po.NamespacedIDString(), idempotencyKey) + _, err = om.RunOperation(ctx, po, idempotencyKey != "") return op, err } diff --git a/internal/operations/manager_test.go b/internal/operations/manager_test.go index db4ba6aa2..598a54a8e 100644 --- a/internal/operations/manager_test.go +++ b/internal/operations/manager_test.go @@ -38,7 +38,8 @@ import ( ) type mockHandler struct { - Complete bool + Phase core.OpPhase + PrepErr error RunErr error Prepared *core.PreparedOperation Outputs fftypes.JSONObject @@ -62,11 +63,11 @@ func (m *mockHandler) Name() string { } func (m *mockHandler) PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) { - return m.Prepared, m.RunErr + return m.Prepared, m.PrepErr } -func (m *mockHandler) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { - return m.Outputs, m.Complete, m.RunErr +func (m *mockHandler) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { + return m.Outputs, m.Phase, m.RunErr } func (m *mockHandler) OnOperationUpdate(ctx context.Context, op *core.Operation, update *core.OperationUpdate) error { @@ -161,7 +162,7 @@ func TestRunOperationNotSupported(t *testing.T) { op := &core.PreparedOperation{} - _, err := om.RunOperation(context.Background(), op) + _, err := om.RunOperation(context.Background(), op, true) assert.Regexp(t, "FF10371", err) } @@ -175,7 +176,7 @@ func TestRunOperationSuccess(t *testing.T) { } om.RegisterHandler(ctx, &mockHandler{Outputs: fftypes.JSONObject{"test": "output"}}, []core.OpType{core.OpTypeBlockchainPinBatch}) - outputs, err := om.RunOperation(context.Background(), op) + outputs, err := om.RunOperation(context.Background(), op, true) assert.Equal(t, "output", outputs.GetString("test")) assert.NoError(t, err) @@ -197,13 +198,13 @@ func TestRunOperationSyncSuccess(t *testing.T) { Type: core.OpTypeBlockchainPinBatch, } - om.RegisterHandler(ctx, &mockHandler{Complete: true}, []core.OpType{core.OpTypeBlockchainPinBatch}) - _, err := om.RunOperation(ctx, op) + om.RegisterHandler(ctx, &mockHandler{Phase: core.OpPhaseComplete}, []core.OpType{core.OpTypeBlockchainPinBatch}) + _, err := om.RunOperation(ctx, op, true) assert.NoError(t, err) } -func TestRunOperationFail(t *testing.T) { +func TestRunOperationFailIdempotentInit(t *testing.T) { om, cancel := newTestOperations(t) defer cancel() @@ -218,11 +219,43 @@ func TestRunOperationFail(t *testing.T) { Type: core.OpTypeBlockchainPinBatch, } - om.RegisterHandler(ctx, &mockHandler{RunErr: fmt.Errorf("pop")}, []core.OpType{core.OpTypeBlockchainPinBatch}) - _, err := om.RunOperation(ctx, op) + om.RegisterHandler(ctx, &mockHandler{ + RunErr: fmt.Errorf("pop"), + Phase: core.OpPhaseInitializing, + }, []core.OpType{core.OpTypeBlockchainPinBatch}) + _, err := om.RunOperation(ctx, op, true) update := <-om.updater.workQueues[0] assert.Equal(t, "ns1:"+op.ID.String(), update.NamespacedOpID) + assert.Equal(t, core.OpStatusInitialized, update.Status) + + assert.EqualError(t, err, "pop") +} + +func TestRunOperationFailNonIdempotentInit(t *testing.T) { + om, cancel := newTestOperations(t) + defer cancel() + + om.updater.workQueues = []chan *core.OperationUpdate{ + make(chan *core.OperationUpdate, 1), + } + + ctx := context.Background() + op := &core.PreparedOperation{ + ID: fftypes.NewUUID(), + Namespace: "ns1", + Type: core.OpTypeBlockchainPinBatch, + } + + om.RegisterHandler(ctx, &mockHandler{ + RunErr: fmt.Errorf("pop"), + Phase: core.OpPhaseInitializing, + }, []core.OpType{core.OpTypeBlockchainPinBatch}) + _, err := om.RunOperation(ctx, op, false) + + update := <-om.updater.workQueues[0] + assert.Equal(t, "ns1:"+op.ID.String(), update.NamespacedOpID) + assert.Equal(t, core.OpStatusFailed, update.Status) assert.EqualError(t, err, "pop") } @@ -243,13 +276,11 @@ func TestRunOperationFailConflict(t *testing.T) { } om.RegisterHandler(ctx, &mockHandler{RunErr: &mockConflictErr{err: fmt.Errorf("pop")}}, []core.OpType{core.OpTypeBlockchainPinBatch}) - _, err := om.RunOperation(ctx, op) + _, err := om.RunOperation(ctx, op, true) - select { - case <-om.updater.workQueues[0]: - assert.Fail(t, "Should not have an update") - default: - } + update := <-om.updater.workQueues[0] + assert.Equal(t, "ns1:"+op.ID.String(), update.NamespacedOpID) + assert.Equal(t, core.OpStatusPending, update.Status) assert.EqualError(t, err, "pop") } @@ -270,8 +301,11 @@ func TestRunOperationFailRemainPending(t *testing.T) { Type: core.OpTypeBlockchainPinBatch, } - om.RegisterHandler(ctx, &mockHandler{RunErr: fmt.Errorf("pop")}, []core.OpType{core.OpTypeBlockchainPinBatch}) - _, err := om.RunOperation(ctx, op, RemainPendingOnFailure) + om.RegisterHandler(ctx, &mockHandler{ + RunErr: fmt.Errorf("pop"), + Phase: core.OpPhasePending, + }, []core.OpType{core.OpTypeBlockchainPinBatch}) + _, err := om.RunOperation(ctx, op, false) assert.EqualError(t, err, "pop") } @@ -282,12 +316,14 @@ func TestRetryOperationSuccess(t *testing.T) { ctx := context.Background() opID := fftypes.NewUUID() + txID := fftypes.NewUUID() op := &core.Operation{ - ID: opID, - Namespace: "ns1", - Plugin: "blockchain", - Type: core.OpTypeBlockchainPinBatch, - Status: core.OpStatusFailed, + ID: opID, + Namespace: "ns1", + Plugin: "blockchain", + Transaction: txID, + Type: core.OpTypeBlockchainPinBatch, + Status: core.OpStatusFailed, } po := &core.PreparedOperation{ ID: op.ID, @@ -315,6 +351,11 @@ func TestRetryOperationSuccess(t *testing.T) { assert.Equal(t, op.ID.String(), val) return true })).Return(true, nil) + mdi.On("GetTransactionByID", mock.Anything, "ns1", txID).Return(&core.Transaction{ + ID: txID, + Namespace: "ns1", + IdempotencyKey: "idem1", + }, nil) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) newOp, err := om.RetryOperation(ctx, op.ID) @@ -325,6 +366,39 @@ func TestRetryOperationSuccess(t *testing.T) { mdi.AssertExpectations(t) } +func TestRetryOperationGetTXFail(t *testing.T) { + om, cancel := newTestOperations(t) + defer cancel() + + ctx := context.Background() + opID := fftypes.NewUUID() + txID := fftypes.NewUUID() + op := &core.Operation{ + ID: opID, + Namespace: "ns1", + Plugin: "blockchain", + Transaction: txID, + Type: core.OpTypeBlockchainPinBatch, + Status: core.OpStatusFailed, + } + po := &core.PreparedOperation{ + ID: op.ID, + Type: op.Type, + } + + om.cache = cache.NewUmanagedCache(ctx, 100, 10*time.Minute) + om.cacheOperation(op) + + mdi := om.database.(*databasemocks.Plugin) + mdi.On("GetTransactionByID", mock.Anything, "ns1", txID).Return(nil, fmt.Errorf("pop")) + + om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) + _, err := om.RetryOperation(ctx, op.ID) + + assert.Regexp(t, "pop", err) + mdi.AssertExpectations(t) +} + func TestRetryOperationGetFail(t *testing.T) { om, cancel := newTestOperations(t) defer cancel() @@ -381,6 +455,7 @@ func TestRetryTwiceOperationInsertFail(t *testing.T) { mdi := om.database.(*databasemocks.Plugin) mdi.On("GetOperationByID", ctx, "ns1", opID).Return(op, nil) mdi.On("GetOperationByID", ctx, "ns1", opID2).Return(op2, nil) + mdi.On("GetTransactionByID", mock.Anything, "ns1", mock.Anything).Return(nil, nil) mdi.On("InsertOperation", ctx, mock.Anything).Return(fmt.Errorf("pop")) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) @@ -410,6 +485,7 @@ func TestRetryOperationInsertFail(t *testing.T) { mdi := om.database.(*databasemocks.Plugin) mdi.On("GetOperationByID", ctx, "ns1", opID).Return(op, nil) + mdi.On("GetTransactionByID", mock.Anything, "ns1", mock.Anything).Return(nil, nil) mdi.On("InsertOperation", ctx, mock.Anything).Return(fmt.Errorf("pop")) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) @@ -440,6 +516,7 @@ func TestRetryOperationUpdateFail(t *testing.T) { mdi := om.database.(*databasemocks.Plugin) mdi.On("GetOperationByID", ctx, "ns1", opID).Return(op, nil) + mdi.On("GetTransactionByID", mock.Anything, "ns1", mock.Anything).Return(nil, nil) mdi.On("InsertOperation", ctx, mock.Anything).Return(nil) mdi.On("UpdateOperation", ctx, "ns1", op.ID, mock.Anything, mock.Anything).Return(false, fmt.Errorf("pop")) @@ -659,3 +736,8 @@ func TestResubmitIdempotentOperationExecError(t *testing.T) { mdi.AssertExpectations(t) } + +func TestErrTernaryHelper(t *testing.T) { + assert.Equal(t, core.OpPhasePending, ErrTernary(nil, core.OpPhaseInitializing, core.OpPhasePending)) + assert.Equal(t, core.OpPhaseInitializing, ErrTernary(fmt.Errorf("pop"), core.OpPhaseInitializing, core.OpPhasePending)) +} diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 2b839166e..186782d63 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -583,7 +583,7 @@ func (or *orchestrator) SubmitNetworkAction(ctx context.Context, action *core.Ne if err != nil { return err } - return or.multiparty.SubmitNetworkAction(ctx, key, action) + return or.multiparty.SubmitNetworkAction(ctx, key, action, false /* network actions do not support idempotency keys currently */) } func (or *orchestrator) Authorize(ctx context.Context, authReq *fftypes.AuthReq) error { diff --git a/internal/orchestrator/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go index 4e8636862..a3a3d31ca 100644 --- a/internal/orchestrator/orchestrator_test.go +++ b/internal/orchestrator/orchestrator_test.go @@ -493,7 +493,7 @@ func TestNetworkAction(t *testing.T) { or.namespace.Name = core.LegacySystemNamespace action := &core.NetworkAction{Type: core.NetworkActionTerminate} or.mim.On("ResolveInputSigningKey", context.Background(), "", identity.KeyNormalizationBlockchainPlugin).Return("0x123", nil) - or.mmp.On("SubmitNetworkAction", context.Background(), "0x123", action).Return(nil) + or.mmp.On("SubmitNetworkAction", context.Background(), "0x123", action, false).Return(nil) err := or.SubmitNetworkAction(context.Background(), action) assert.NoError(t, err) } diff --git a/internal/privatemessaging/message_test.go b/internal/privatemessaging/message_test.go index 4010f79ae..be2084fd0 100644 --- a/internal/privatemessaging/message_test.go +++ b/internal/privatemessaging/message_test.go @@ -663,7 +663,7 @@ func TestDispatchedUnpinnedMessageOK(t *testing.T) { mom.On("RunOperation", pm.ctx, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(batchSendData) return op.Type == core.OpTypeDataExchangeSendBatch && *data.Node.ID == *node2.ID - })).Return(nil, nil) + }), false).Return(nil, nil) err := pm.dispatchUnpinnedBatch(pm.ctx, &batch.DispatchPayload{ Batch: core.BatchPersisted{ @@ -767,7 +767,7 @@ func TestSendDataTransferFail(t *testing.T) { mom.On("RunOperation", pm.ctx, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(batchSendData) return op.Type == core.OpTypeDataExchangeSendBatch && *data.Node.ID == *node2.ID - })).Return(nil, fmt.Errorf("pop")) + }), false).Return(nil, fmt.Errorf("pop")) err := pm.sendData(pm.ctx, &core.TransportWrapper{ Batch: &core.Batch{ diff --git a/internal/privatemessaging/operations.go b/internal/privatemessaging/operations.go index 7c5cf2174..5a6ef8c0f 100644 --- a/internal/privatemessaging/operations.go +++ b/internal/privatemessaging/operations.go @@ -137,29 +137,29 @@ func (pm *privateMessaging) PrepareOperation(ctx context.Context, op *core.Opera } } -func (pm *privateMessaging) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (pm *privateMessaging) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case transferBlobData: localNode, err := pm.identity.GetLocalNode(ctx) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } - return nil, false, pm.exchange.TransferBlob(ctx, op.NamespacedIDString(), data.Node.Profile, localNode.Profile, data.Blob.PayloadRef) + return nil, core.OpPhaseInitializing, pm.exchange.TransferBlob(ctx, op.NamespacedIDString(), data.Node.Profile, localNode.Profile, data.Blob.PayloadRef) case batchSendData: localNode, err := pm.identity.GetLocalNode(ctx) if err != nil { - return nil, false, err + return nil, core.OpPhaseInitializing, err } payload, err := json.Marshal(data.Transport) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgSerializationFailed) + return nil, core.OpPhaseInitializing, i18n.WrapError(ctx, err, coremsgs.MsgSerializationFailed) } - return nil, false, pm.exchange.SendMessage(ctx, op.NamespacedIDString(), data.Node.Profile, localNode.Profile, payload) + return nil, core.OpPhaseInitializing, pm.exchange.SendMessage(ctx, op.NamespacedIDString(), data.Node.Profile, localNode.Profile, payload) default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } diff --git a/internal/privatemessaging/operations_test.go b/internal/privatemessaging/operations_test.go index a506f37ea..8b1ad8df8 100644 --- a/internal/privatemessaging/operations_test.go +++ b/internal/privatemessaging/operations_test.go @@ -6,7 +6,7 @@ // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // -// http://www.apache.org/licenses/LICENSE-2.0 +// http://www.apache.org/licenses/LICENSE-2.0 // // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, @@ -81,9 +81,9 @@ func TestPrepareAndRunTransferBlob(t *testing.T) { assert.Equal(t, node, po.Data.(transferBlobData).Node) assert.Equal(t, blob, po.Data.(transferBlobData).Blob) - _, complete, err := pm.RunOperation(context.Background(), po) + _, phase, err := pm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mdi.AssertExpectations(t) @@ -150,9 +150,9 @@ func TestPrepareAndRunBatchSend(t *testing.T) { assert.Equal(t, group, po.Data.(batchSendData).Transport.Group) assert.Equal(t, batch, po.Data.(batchSendData).Transport.Batch) - _, complete, err := pm.RunOperation(context.Background(), po) + _, phase, err := pm.RunOperation(context.Background(), po) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.NoError(t, err) mdi.AssertExpectations(t) @@ -553,9 +553,9 @@ func TestRunOperationNotSupported(t *testing.T) { pm, cancel := newTestPrivateMessaging(t) defer cancel() - _, complete, err := pm.RunOperation(context.Background(), &core.PreparedOperation{}) + _, phase, err := pm.RunOperation(context.Background(), &core.PreparedOperation{}) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10378", err) } @@ -592,9 +592,9 @@ func TestRunOperationBatchSendInvalidData(t *testing.T) { }, } - _, complete, err := pm.RunOperation(context.Background(), opSendBatch(op, node, transport)) + _, phase, err := pm.RunOperation(context.Background(), opSendBatch(op, node, transport)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10137", err) } @@ -621,9 +621,9 @@ func TestRunOperationBatchSendNodeFail(t *testing.T) { }, } - _, complete, err := pm.RunOperation(context.Background(), opSendBatch(op, node, transport)) + _, phase, err := pm.RunOperation(context.Background(), opSendBatch(op, node, transport)) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.EqualError(t, err, "pop") } @@ -640,9 +640,9 @@ func TestRunOperationBlobSendNodeFail(t *testing.T) { mim := pm.identity.(*identitymanagermocks.Manager) mim.On("GetLocalNode", context.Background()).Return(nil, fmt.Errorf("pop")) - _, complete, err := pm.RunOperation(context.Background(), opSendBlob(op, node, &core.Blob{})) + _, phase, err := pm.RunOperation(context.Background(), opSendBlob(op, node, &core.Blob{})) - assert.False(t, complete) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.EqualError(t, err, "pop") } diff --git a/internal/privatemessaging/privatemessaging.go b/internal/privatemessaging/privatemessaging.go index c41e4ce4d..fef50c04d 100644 --- a/internal/privatemessaging/privatemessaging.go +++ b/internal/privatemessaging/privatemessaging.go @@ -54,7 +54,7 @@ type Manager interface { // From operations.OperationHandler PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) - RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) + RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) } type privateMessaging struct { @@ -179,7 +179,7 @@ func (pm *privateMessaging) dispatchPinnedBatch(ctx context.Context, payload *ba } log.L(ctx).Infof("Pinning private batch %s with author=%s key=%s group=%s", payload.Batch.ID, payload.Batch.Author, payload.Batch.Key, payload.Batch.Group) - return pm.multiparty.SubmitBatchPin(ctx, &payload.Batch, payload.Pins, "" /* no payloadRef for private */) + return pm.multiparty.SubmitBatchPin(ctx, &payload.Batch, payload.Pins, "" /* no payloadRef for private */, false /* batch processing does not currently use idempotency keys */) } func (pm *privateMessaging) dispatchUnpinnedBatch(ctx context.Context, payload *batch.DispatchPayload) error { @@ -263,7 +263,7 @@ func (pm *privateMessaging) submitBlobTransfersToDX(ctx context.Context, tracker go func(tracker *blobTransferTracker) { defer wg.Done() log.L(ctx).Debugf("Initiating DX transfer blob=%s data=%s operation=%s", tracker.blobHash, tracker.dataID, tracker.op.ID) - if _, err := pm.operations.RunOperation(ctx, tracker.op); err != nil { + if _, err := pm.operations.RunOperation(ctx, tracker.op, false /* batch processing does not currently use idempotency keys */); err != nil { log.L(ctx).Errorf("Failed to initiate DX transfer blob=%s data=%s operation=%s", tracker.blobHash, tracker.dataID, tracker.op.ID) if firstError == nil { firstError = err @@ -329,7 +329,7 @@ func (pm *privateMessaging) sendData(ctx context.Context, tw *core.TransportWrap } // Then initiate the batch transfer - if _, err = pm.operations.RunOperation(ctx, sendBatchOp); err != nil { + if _, err = pm.operations.RunOperation(ctx, sendBatchOp, false /* batch processing does not currently use idempotency keys */); err != nil { return err } } diff --git a/internal/privatemessaging/privatemessaging_test.go b/internal/privatemessaging/privatemessaging_test.go index ea994a2ea..731ef83cf 100644 --- a/internal/privatemessaging/privatemessaging_test.go +++ b/internal/privatemessaging/privatemessaging_test.go @@ -215,16 +215,16 @@ func TestDispatchBatchWithBlobs(t *testing.T) { } data := op.Data.(transferBlobData) return *data.Node.ID == *node2.ID - })).Return(nil, nil) + }), false).Return(nil, nil) mom.On("RunOperation", pm.ctx, mock.MatchedBy(func(op *core.PreparedOperation) bool { if op.Type != core.OpTypeDataExchangeSendBatch { return false } data := op.Data.(batchSendData) return *data.Node.ID == *node2.ID - })).Return(nil, nil) + }), false).Return(nil, nil) - mmp.On("SubmitBatchPin", pm.ctx, mock.Anything, mock.Anything, "").Return(nil) + mmp.On("SubmitBatchPin", pm.ctx, mock.Anything, mock.Anything, "", false).Return(nil) err := pm.dispatchPinnedBatch(pm.ctx, &batch.DispatchPayload{ Batch: core.BatchPersisted{ @@ -438,7 +438,7 @@ func TestSendSubmitBlobTransferFail(t *testing.T) { mom.On("RunOperation", pm.ctx, mock.MatchedBy(func(op *core.PreparedOperation) bool { data := op.Data.(transferBlobData) return op.Type == core.OpTypeDataExchangeSendBlob && *data.Node.ID == *node2.ID - })).Return(nil, fmt.Errorf("pop")) + }), false).Return(nil, fmt.Errorf("pop")) err := pm.dispatchPinnedBatch(pm.ctx, &batch.DispatchPayload{ Batch: core.BatchPersisted{ @@ -496,14 +496,14 @@ func TestWriteTransactionSubmitBatchPinFail(t *testing.T) { } data := op.Data.(transferBlobData) return *data.Node.ID == *node2.ID - })).Return(nil, nil) + }), false).Return(nil, nil) mom.On("RunOperation", pm.ctx, mock.MatchedBy(func(op *core.PreparedOperation) bool { if op.Type != core.OpTypeDataExchangeSendBatch { return false } data := op.Data.(batchSendData) return *data.Node.ID == *node2.ID - })).Return(nil, nil) + }), false).Return(nil, nil) mdi.On("GetBlobs", pm.ctx, "ns1", mock.Anything).Return([]*core.Blob{{ Hash: blob1, @@ -511,7 +511,7 @@ func TestWriteTransactionSubmitBatchPinFail(t *testing.T) { }}, nil, nil) mmp := pm.multiparty.(*multipartymocks.Manager) - mmp.On("SubmitBatchPin", pm.ctx, mock.Anything, mock.Anything, "").Return(fmt.Errorf("pop")) + mmp.On("SubmitBatchPin", pm.ctx, mock.Anything, mock.Anything, "", false).Return(fmt.Errorf("pop")) err := pm.dispatchPinnedBatch(pm.ctx, &batch.DispatchPayload{ Batch: core.BatchPersisted{ diff --git a/internal/shareddownload/download_manager.go b/internal/shareddownload/download_manager.go index fcfb8aa7a..56cf25877 100644 --- a/internal/shareddownload/download_manager.go +++ b/internal/shareddownload/download_manager.go @@ -39,8 +39,8 @@ type Manager interface { Start() error WaitStop() - InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string) error - InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string) error + InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string, idempotentSubmit bool) error + InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string, idempotentSubmit bool) error } // downloadManager operates a number of workers that can perform downloads/retries. Each download @@ -69,9 +69,10 @@ type downloadManager struct { } type downloadWork struct { - dispatchedAt time.Time - preparedOp *core.PreparedOperation - attempts int + dispatchedAt time.Time + preparedOp *core.PreparedOperation + attempts int + idempotentSubmit bool } type Callbacks interface { @@ -224,25 +225,26 @@ func (dm *downloadManager) waitAndRetryDownload(work *downloadWork) { dm.dispatchWork(work) } -func (dm *downloadManager) InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string) error { +func (dm *downloadManager) InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string, idempotentSubmit bool) error { op := core.NewOperation(dm.sharedstorage, dm.namespace.Name, tx, core.OpTypeSharedStorageDownloadBatch) addDownloadBatchInputs(op, payloadRef) - return dm.createAndDispatchOp(ctx, op, opDownloadBatch(op, payloadRef)) + return dm.createAndDispatchOp(ctx, op, opDownloadBatch(op, payloadRef), idempotentSubmit) } -func (dm *downloadManager) InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string) error { +func (dm *downloadManager) InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string, idempotentSubmit bool) error { op := core.NewOperation(dm.sharedstorage, dm.namespace.Name, tx, core.OpTypeSharedStorageDownloadBlob) addDownloadBlobInputs(op, dataID, payloadRef) - return dm.createAndDispatchOp(ctx, op, opDownloadBlob(op, dataID, payloadRef)) + return dm.createAndDispatchOp(ctx, op, opDownloadBlob(op, dataID, payloadRef), idempotentSubmit) } -func (dm *downloadManager) createAndDispatchOp(ctx context.Context, op *core.Operation, preparedOp *core.PreparedOperation) error { +func (dm *downloadManager) createAndDispatchOp(ctx context.Context, op *core.Operation, preparedOp *core.PreparedOperation, idempotentSubmit bool) error { err := dm.operations.AddOrReuseOperation(ctx, op, func() { // Use a closure hook to dispatch the work once the operation is successfully in the DB. // Note we have crash recovery of pending operations on startup. dm.dispatchWork(&downloadWork{ - dispatchedAt: time.Now(), - preparedOp: preparedOp, + dispatchedAt: time.Now(), + preparedOp: preparedOp, + idempotentSubmit: idempotentSubmit, }) }) if err != nil { diff --git a/internal/shareddownload/download_manager_test.go b/internal/shareddownload/download_manager_test.go index f7d794da1..fd217ec9a 100644 --- a/internal/shareddownload/download_manager_test.go +++ b/internal/shareddownload/download_manager_test.go @@ -102,17 +102,17 @@ func TestDownloadBatchE2EOk(t *testing.T) { assert.Equal(t, "ref1", op.Data.(downloadBatchData).PayloadRef) return true }), mock.Anything).Return(nil, nil).Run(func(args mock.Arguments) { - output, complete, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) + output, phase, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) assert.NoError(t, err) assert.Equal(t, fftypes.JSONObject{"batch": batchID}, output) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) close(called) }) mci := dm.callbacks.(*shareddownloadmocks.Callbacks) mci.On("SharedStorageBatchDownloaded", "ref1", []byte("some batch data")).Return(batchID, nil) - err := dm.InitiateDownloadBatch(dm.ctx, txID, "ref1") + err := dm.InitiateDownloadBatch(dm.ctx, txID, "ref1", false) assert.NoError(t, err) <-called @@ -169,21 +169,21 @@ func TestDownloadBlobWithRetryOk(t *testing.T) { assert.Equal(t, "ref1", op.Data.(downloadBlobData).PayloadRef) return true }), mock.Anything).Return(nil, nil).Run(func(args mock.Arguments) { - output, complete, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) + output, phase, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) assert.NoError(t, err) assert.Equal(t, fftypes.JSONObject{ "dxPayloadRef": "privateRef1", "hash": blobHash, "size": 12345, }.String(), output.String()) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) close(called) }).Once() mci := dm.callbacks.(*shareddownloadmocks.Callbacks) mci.On("SharedStorageBlobDownloaded", *blobHash, int64(12345), "privateRef1", dataID).Return(nil) - err := dm.InitiateDownloadBlob(dm.ctx, txID, dataID, "ref1") + err := dm.InitiateDownloadBlob(dm.ctx, txID, dataID, "ref1", false) assert.NoError(t, err) <-called @@ -209,7 +209,7 @@ func TestDownloadBlobInsertOpFail(t *testing.T) { mom := dm.operations.(*operationmocks.Manager) mom.On("AddOrReuseOperation", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("pop")) - err := dm.InitiateDownloadBlob(dm.ctx, txID, dataID, "ref1") + err := dm.InitiateDownloadBlob(dm.ctx, txID, dataID, "ref1", false) assert.Regexp(t, "pop", err) mom.AssertExpectations(t) @@ -288,14 +288,15 @@ func TestDownloadManagerStartupRecoveryCombinations(t *testing.T) { mom.On("RunOperation", mock.Anything, mock.MatchedBy(func(op *core.PreparedOperation) bool { return op.Type == core.OpTypeSharedStorageDownloadBatch && op.Data.(downloadBatchData).PayloadRef == "ref2" }), mock.Anything).Return(nil, nil).Run(func(args mock.Arguments) { - output, complete, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) + output, phase, err := dm.RunOperation(args[0].(context.Context), args[1].(*core.PreparedOperation)) assert.NoError(t, err) assert.Equal(t, fftypes.JSONObject{ "batch": batchID, }.String(), output.String()) - assert.True(t, complete) + assert.Equal(t, core.OpPhaseComplete, phase) called <- true }) + mom.On("SubmitOperationUpdate", mock.Anything).Return(nil) mci := dm.callbacks.(*shareddownloadmocks.Callbacks) mci.On("SharedStorageBatchDownloaded", "ref2", []byte("some batch data")).Return(batchID, nil) diff --git a/internal/shareddownload/download_worker.go b/internal/shareddownload/download_worker.go index ab5f86eab..cae26e501 100644 --- a/internal/shareddownload/download_worker.go +++ b/internal/shareddownload/download_worker.go @@ -1,4 +1,4 @@ -// Copyright © 2022 Kaleido, Inc. +// Copyright © 2023 Kaleido, Inc. // // SPDX-License-Identifier: Apache-2.0 // @@ -21,7 +21,7 @@ import ( "fmt" "github.com/hyperledger/firefly-common/pkg/log" - "github.com/hyperledger/firefly/internal/operations" + "github.com/hyperledger/firefly/pkg/core" ) type downloadWorker struct { @@ -59,15 +59,17 @@ func (dw *downloadWorker) attemptWork(work *downloadWork) { work.attempts++ isLastAttempt := work.attempts >= dw.dm.retryMaxAttempts - options := []operations.RunOperationOption{operations.RemainPendingOnFailure} - if isLastAttempt { - options = []operations.RunOperationOption{} - } - - _, err := dw.dm.operations.RunOperation(dw.ctx, work.preparedOp, options...) + _, err := dw.dm.operations.RunOperation(dw.ctx, work.preparedOp, work.idempotentSubmit) if err != nil { log.L(dw.ctx).Errorf("Download operation %s/%s attempt=%d/%d failed: %s", work.preparedOp.Type, work.preparedOp.ID, work.attempts, dw.dm.retryMaxAttempts, err) - if !isLastAttempt { + if isLastAttempt { + dw.dm.operations.SubmitOperationUpdate(&core.OperationUpdate{ + NamespacedOpID: work.preparedOp.NamespacedIDString(), + Plugin: work.preparedOp.Plugin, + Status: core.OpStatusFailed, + ErrorMessage: err.Error(), + }) + } else { go dw.dm.waitAndRetryDownload(work) } } diff --git a/internal/shareddownload/operations.go b/internal/shareddownload/operations.go index 2ee215010..94c8f2f9f 100644 --- a/internal/shareddownload/operations.go +++ b/internal/shareddownload/operations.go @@ -96,25 +96,25 @@ func (dm *downloadManager) PrepareOperation(ctx context.Context, op *core.Operat } } -func (dm *downloadManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, complete bool, err error) { +func (dm *downloadManager) RunOperation(ctx context.Context, op *core.PreparedOperation) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { switch data := op.Data.(type) { case downloadBatchData: return dm.downloadBatch(ctx, data) case downloadBlobData: return dm.downloadBlob(ctx, data) default: - return nil, false, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) + return nil, core.OpPhaseInitializing, i18n.NewError(ctx, coremsgs.MsgOperationDataIncorrect, op.Data) } } // downloadBatch retrieves a serialized batch from public storage, then persists it and drives a rewind // on the messages included (just like the event driven when we receive data over DX). -func (dm *downloadManager) downloadBatch(ctx context.Context, data downloadBatchData) (outputs fftypes.JSONObject, complete bool, err error) { +func (dm *downloadManager) downloadBatch(ctx context.Context, data downloadBatchData) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { // Download into memory for batches reader, err := dm.sharedstorage.DownloadData(ctx, data.PayloadRef) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) + return nil, core.OpPhaseInitializing, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) } defer reader.Close() @@ -123,42 +123,42 @@ func (dm *downloadManager) downloadBatch(ctx context.Context, data downloadBatch limitedReader := io.LimitReader(reader, maxReadLimit) batchBytes, err := io.ReadAll(limitedReader) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) + return nil, core.OpPhasePending, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) } if len(batchBytes) == int(maxReadLimit) { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgDownloadBatchMaxBytes, data.PayloadRef) + return nil, core.OpPhasePending, i18n.WrapError(ctx, err, coremsgs.MsgDownloadBatchMaxBytes, data.PayloadRef) } // Parse and store the batch batchID, err := dm.callbacks.SharedStorageBatchDownloaded(data.PayloadRef, batchBytes) if err != nil { - return nil, false, err + return nil, core.OpPhasePending, err } - return getDownloadBatchOutputs(batchID), true, nil + return getDownloadBatchOutputs(batchID), core.OpPhaseComplete, nil } -func (dm *downloadManager) downloadBlob(ctx context.Context, data downloadBlobData) (outputs fftypes.JSONObject, complete bool, err error) { +func (dm *downloadManager) downloadBlob(ctx context.Context, data downloadBlobData) (outputs fftypes.JSONObject, phase core.OpPhase, err error) { // Stream from shared storage ... reader, err := dm.sharedstorage.DownloadData(ctx, data.PayloadRef) if err != nil { - return nil, false, err + return nil, core.OpPhasePending, err } defer reader.Close() // ... to data exchange dxPayloadRef, hash, blobSize, err := dm.dataexchange.UploadBlob(ctx, dm.namespace.NetworkName, *data.DataID, reader) if err != nil { - return nil, false, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) + return nil, core.OpPhasePending, i18n.WrapError(ctx, err, coremsgs.MsgDownloadSharedFailed, data.PayloadRef) } log.L(ctx).Infof("Transferred blob '%s' (%s) from shared storage '%s' to local data exchange '%s'", hash, units.HumanSizeWithPrecision(float64(blobSize), 2), data.PayloadRef, dxPayloadRef) // then callback to store metadata if err := dm.callbacks.SharedStorageBlobDownloaded(*hash, blobSize, dxPayloadRef, data.DataID); err != nil { - return nil, false, err + return nil, core.OpPhasePending, err } - return getDownloadBlobOutputs(hash, blobSize, dxPayloadRef), true, nil + return getDownloadBlobOutputs(hash, blobSize, dxPayloadRef), core.OpPhaseComplete, nil } func (dm *downloadManager) OnOperationUpdate(ctx context.Context, op *core.Operation, update *core.OperationUpdate) error { diff --git a/internal/tokens/fftokens/fftokens.go b/internal/tokens/fftokens/fftokens.go index 20170f183..1d8132f2d 100644 --- a/internal/tokens/fftokens/fftokens.go +++ b/internal/tokens/fftokens/fftokens.go @@ -719,7 +719,7 @@ func wrapError(ctx context.Context, errRes *tokenError, res *resty.Response, err return ffresty.WrapRestErr(ctx, res, err, coremsgs.MsgTokensRESTErr) } -func (ft *FFTokens) CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (complete bool, err error) { +func (ft *FFTokens) CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (phase core.OpPhase, err error) { tokenData := &tokenData{ TX: pool.TX.ID, TXType: pool.TX.Type, @@ -739,22 +739,22 @@ func (ft *FFTokens) CreateTokenPool(ctx context.Context, nsOpID string, pool *co SetError(&errRes). Post("/api/v1/createpool") if err != nil || !res.IsSuccess() { - return false, wrapError(ctx, &errRes, res, err) + return core.OpPhaseInitializing, wrapError(ctx, &errRes, res, err) } if res.StatusCode() == 200 { // HTTP 200: Creation was successful, and pool details are in response body var obj fftypes.JSONObject if err := json.Unmarshal(res.Body(), &obj); err != nil { - return false, i18n.WrapError(ctx, err, i18n.MsgJSONObjectParseFailed, res.Body()) + return core.OpPhaseComplete, i18n.WrapError(ctx, err, i18n.MsgJSONObjectParseFailed, res.Body()) } obj["poolData"] = packPoolData(pool.Namespace, pool.ID) - return true, ft.handleTokenPoolCreate(ctx, obj, tokenData) + return core.OpPhaseComplete, ft.handleTokenPoolCreate(ctx, obj, tokenData) } // Default (HTTP 202): Request was accepted, and success/failure status will be delivered via websocket - return false, nil + return core.OpPhasePending, nil } -func (ft *FFTokens) ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (complete bool, err error) { +func (ft *FFTokens) ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (phase core.OpPhase, err error) { var errRes tokenError res, err := ft.client.R().SetContext(ctx). SetBody(&activatePool{ @@ -765,25 +765,25 @@ func (ft *FFTokens) ActivateTokenPool(ctx context.Context, pool *core.TokenPool) SetError(&errRes). Post("/api/v1/activatepool") if err != nil || !res.IsSuccess() { - return false, wrapError(ctx, &errRes, res, err) + return core.OpPhaseInitializing, wrapError(ctx, &errRes, res, err) } if res.StatusCode() == 200 { // HTTP 200: Activation was successful, and pool details are in response body var obj fftypes.JSONObject if err := json.Unmarshal(res.Body(), &obj); err != nil { - return false, i18n.WrapError(ctx, err, i18n.MsgJSONObjectParseFailed, res.Body()) + return core.OpPhaseComplete, i18n.WrapError(ctx, err, i18n.MsgJSONObjectParseFailed, res.Body()) } - return true, ft.handleTokenPoolCreate(ctx, obj, &tokenData{ + return core.OpPhaseComplete, ft.handleTokenPoolCreate(ctx, obj, &tokenData{ TX: pool.TX.ID, TXType: pool.TX.Type, }) } else if res.StatusCode() == 204 { // HTTP 204: Activation was successful, but pool details are not available // This will resolve the operation, but connector is responsible for re-delivering pool details on the websocket. - return true, nil + return core.OpPhaseComplete, nil } // Default (HTTP 202): Request was accepted, and success/failure status will be delivered via websocket - return false, nil + return core.OpPhasePending, nil } func (ft *FFTokens) DeactivateTokenPool(ctx context.Context, pool *core.TokenPool) error { diff --git a/internal/tokens/fftokens/fftokens_test.go b/internal/tokens/fftokens/fftokens_test.go index 189fd88e5..da03d619a 100644 --- a/internal/tokens/fftokens/fftokens_test.go +++ b/internal/tokens/fftokens/fftokens_test.go @@ -198,8 +198,8 @@ func TestCreateTokenPool(t *testing.T) { return res, nil }) - complete, err := h.CreateTokenPool(context.Background(), nsOpID, pool) - assert.False(t, complete) + phase, err := h.CreateTokenPool(context.Background(), nsOpID, pool) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) } @@ -222,8 +222,8 @@ func TestCreateTokenPoolError(t *testing.T) { })) nsOpID := "ns1:" + fftypes.NewUUID().String() - complete, err := h.CreateTokenPool(context.Background(), nsOpID, pool) - assert.False(t, complete) + phase, err := h.CreateTokenPool(context.Background(), nsOpID, pool) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10274.*Bad Request: Missing required field", err) } @@ -245,8 +245,8 @@ func TestCreateTokenPoolErrorMessageOnly(t *testing.T) { })) nsOpID := "ns1:" + fftypes.NewUUID().String() - complete, err := h.CreateTokenPool(context.Background(), nsOpID, pool) - assert.False(t, complete) + phase, err := h.CreateTokenPool(context.Background(), nsOpID, pool) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10274.*Missing required field", err) } @@ -266,8 +266,8 @@ func TestCreateTokenPoolUnexpectedError(t *testing.T) { httpmock.NewStringResponder(400, "Failed miserably")) nsOpID := "ns1:" + fftypes.NewUUID().String() - complete, err := h.CreateTokenPool(context.Background(), nsOpID, pool) - assert.False(t, complete) + phase, err := h.CreateTokenPool(context.Background(), nsOpID, pool) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10274.*Failed miserably", err) } @@ -324,8 +324,8 @@ func TestCreateTokenPoolSynchronous(t *testing.T) { return p.PoolLocator == "F1" && p.Type == core.TokenTypeFungible && *p.TX.ID == *pool.TX.ID })).Return(nil) - complete, err := h.CreateTokenPool(ctx, nsOpID, pool) - assert.True(t, complete) + phase, err := h.CreateTokenPool(ctx, nsOpID, pool) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) } @@ -367,8 +367,8 @@ func TestCreateTokenPoolSynchronousBadResponse(t *testing.T) { return res, nil }) - complete, err := h.CreateTokenPool(context.Background(), nsOpID, pool) - assert.False(t, complete) + phase, err := h.CreateTokenPool(context.Background(), nsOpID, pool) + assert.Equal(t, core.OpPhaseComplete, phase) assert.Regexp(t, "FF00127", err) } @@ -406,8 +406,8 @@ func TestActivateTokenPool(t *testing.T) { return res, nil }) - complete, err := h.ActivateTokenPool(context.Background(), pool) - assert.False(t, complete) + phase, err := h.ActivateTokenPool(context.Background(), pool) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) } @@ -426,8 +426,8 @@ func TestActivateTokenPoolError(t *testing.T) { httpmock.RegisterResponder("POST", fmt.Sprintf("%s/api/v1/activatepool", httpURL), httpmock.NewJsonResponderOrPanic(500, fftypes.JSONObject{})) - complete, err := h.ActivateTokenPool(context.Background(), pool) - assert.False(t, complete) + phase, err := h.ActivateTokenPool(context.Background(), pool) + assert.Equal(t, core.OpPhaseInitializing, phase) assert.Regexp(t, "FF10274", err) } @@ -475,8 +475,8 @@ func TestActivateTokenPoolSynchronous(t *testing.T) { return p.PoolLocator == "F1" && p.Type == core.TokenTypeFungible && p.TX.ID == nil && p.Event == nil })).Return(nil) - complete, err := h.ActivateTokenPool(context.Background(), pool) - assert.True(t, complete) + phase, err := h.ActivateTokenPool(context.Background(), pool) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) } @@ -520,8 +520,8 @@ func TestActivateTokenPoolSynchronousBadResponse(t *testing.T) { return p.PoolLocator == "F1" && p.Type == core.TokenTypeFungible && p.TX.ID == nil })).Return(nil) - complete, err := h.ActivateTokenPool(context.Background(), pool) - assert.False(t, complete) + phase, err := h.ActivateTokenPool(context.Background(), pool) + assert.Equal(t, core.OpPhaseComplete, phase) assert.Regexp(t, "FF00127", err) } @@ -556,8 +556,8 @@ func TestActivateTokenPoolNoContent(t *testing.T) { return res, nil }) - complete, err := h.ActivateTokenPool(context.Background(), pool) - assert.True(t, complete) + phase, err := h.ActivateTokenPool(context.Background(), pool) + assert.Equal(t, core.OpPhaseComplete, phase) assert.NoError(t, err) } diff --git a/mocks/assetmocks/manager.go b/mocks/assetmocks/manager.go index d5c235132..437b2f127 100644 --- a/mocks/assetmocks/manager.go +++ b/mocks/assetmocks/manager.go @@ -543,13 +543,13 @@ func (_m *Manager) ResolvePoolMethods(ctx context.Context, pool *core.TokenPool) } // RunOperation provides a mock function with given fields: ctx, op -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, bool, error) { +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error) { ret := _m.Called(ctx, op) var r0 fftypes.JSONObject - var r1 bool + var r1 core.OpPhase var r2 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error)); ok { return rf(ctx, op) } if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) fftypes.JSONObject); ok { @@ -560,10 +560,10 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) bool); ok { + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) core.OpPhase); ok { r1 = rf(ctx, op) } else { - r1 = ret.Get(1).(bool) + r1 = ret.Get(1).(core.OpPhase) } if rf, ok := ret.Get(2).(func(context.Context, *core.PreparedOperation) error); ok { diff --git a/mocks/broadcastmocks/manager.go b/mocks/broadcastmocks/manager.go index 35eb831bc..ad208a2fb 100644 --- a/mocks/broadcastmocks/manager.go +++ b/mocks/broadcastmocks/manager.go @@ -153,13 +153,13 @@ func (_m *Manager) PublishDataValue(ctx context.Context, id string, idempotencyK } // RunOperation provides a mock function with given fields: ctx, op -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, bool, error) { +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error) { ret := _m.Called(ctx, op) var r0 fftypes.JSONObject - var r1 bool + var r1 core.OpPhase var r2 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error)); ok { return rf(ctx, op) } if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) fftypes.JSONObject); ok { @@ -170,10 +170,10 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) bool); ok { + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) core.OpPhase); ok { r1 = rf(ctx, op) } else { - r1 = ret.Get(1).(bool) + r1 = ret.Get(1).(core.OpPhase) } if rf, ok := ret.Get(2).(func(context.Context, *core.PreparedOperation) error); ok { diff --git a/mocks/contractmocks/manager.go b/mocks/contractmocks/manager.go index cfc552dda..7b45ecc53 100644 --- a/mocks/contractmocks/manager.go +++ b/mocks/contractmocks/manager.go @@ -700,13 +700,13 @@ func (_m *Manager) ResolveFFIReference(ctx context.Context, ref *fftypes.FFIRefe } // RunOperation provides a mock function with given fields: ctx, op -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, bool, error) { +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error) { ret := _m.Called(ctx, op) var r0 fftypes.JSONObject - var r1 bool + var r1 core.OpPhase var r2 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error)); ok { return rf(ctx, op) } if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) fftypes.JSONObject); ok { @@ -717,10 +717,10 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) bool); ok { + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) core.OpPhase); ok { r1 = rf(ctx, op) } else { - r1 = ret.Get(1).(bool) + r1 = ret.Get(1).(core.OpPhase) } if rf, ok := ret.Get(2).(func(context.Context, *core.PreparedOperation) error); ok { diff --git a/mocks/multipartymocks/manager.go b/mocks/multipartymocks/manager.go index cd2c3167f..b5a072978 100644 --- a/mocks/multipartymocks/manager.go +++ b/mocks/multipartymocks/manager.go @@ -118,13 +118,13 @@ func (_m *Manager) RootOrg() multiparty.RootOrg { } // RunOperation provides a mock function with given fields: ctx, op -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, bool, error) { +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error) { ret := _m.Called(ctx, op) var r0 fftypes.JSONObject - var r1 bool + var r1 core.OpPhase var r2 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error)); ok { return rf(ctx, op) } if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) fftypes.JSONObject); ok { @@ -135,10 +135,10 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) bool); ok { + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) core.OpPhase); ok { r1 = rf(ctx, op) } else { - r1 = ret.Get(1).(bool) + r1 = ret.Get(1).(core.OpPhase) } if rf, ok := ret.Get(2).(func(context.Context, *core.PreparedOperation) error); ok { @@ -150,13 +150,13 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) return r0, r1, r2 } -// SubmitBatchPin provides a mock function with given fields: ctx, batch, contexts, payloadRef -func (_m *Manager) SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string) error { - ret := _m.Called(ctx, batch, contexts, payloadRef) +// SubmitBatchPin provides a mock function with given fields: ctx, batch, contexts, payloadRef, idempotentSubmit +func (_m *Manager) SubmitBatchPin(ctx context.Context, batch *core.BatchPersisted, contexts []*fftypes.Bytes32, payloadRef string, idempotentSubmit bool) error { + ret := _m.Called(ctx, batch, contexts, payloadRef, idempotentSubmit) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, *core.BatchPersisted, []*fftypes.Bytes32, string) error); ok { - r0 = rf(ctx, batch, contexts, payloadRef) + if rf, ok := ret.Get(0).(func(context.Context, *core.BatchPersisted, []*fftypes.Bytes32, string, bool) error); ok { + r0 = rf(ctx, batch, contexts, payloadRef, idempotentSubmit) } else { r0 = ret.Error(0) } @@ -164,13 +164,13 @@ func (_m *Manager) SubmitBatchPin(ctx context.Context, batch *core.BatchPersiste return r0 } -// SubmitNetworkAction provides a mock function with given fields: ctx, signingKey, action -func (_m *Manager) SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction) error { - ret := _m.Called(ctx, signingKey, action) +// SubmitNetworkAction provides a mock function with given fields: ctx, signingKey, action, idempotentSubmit +func (_m *Manager) SubmitNetworkAction(ctx context.Context, signingKey string, action *core.NetworkAction, idempotentSubmit bool) error { + ret := _m.Called(ctx, signingKey, action, idempotentSubmit) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, *core.NetworkAction) error); ok { - r0 = rf(ctx, signingKey, action) + if rf, ok := ret.Get(0).(func(context.Context, string, *core.NetworkAction, bool) error); ok { + r0 = rf(ctx, signingKey, action, idempotentSubmit) } else { r0 = ret.Error(0) } diff --git a/mocks/operationmocks/manager.go b/mocks/operationmocks/manager.go index 752952883..2d8c4f2eb 100644 --- a/mocks/operationmocks/manager.go +++ b/mocks/operationmocks/manager.go @@ -185,32 +185,25 @@ func (_m *Manager) RetryOperation(ctx context.Context, opID *fftypes.UUID) (*cor return r0, r1 } -// RunOperation provides a mock function with given fields: ctx, op, options -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation, options ...operations.RunOperationOption) (fftypes.JSONObject, error) { - _va := make([]interface{}, len(options)) - for _i := range options { - _va[_i] = options[_i] - } - var _ca []interface{} - _ca = append(_ca, ctx, op) - _ca = append(_ca, _va...) - ret := _m.Called(_ca...) +// RunOperation provides a mock function with given fields: ctx, op, idempotentSubmit +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation, idempotentSubmit bool) (fftypes.JSONObject, error) { + ret := _m.Called(ctx, op, idempotentSubmit) var r0 fftypes.JSONObject var r1 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation, ...operations.RunOperationOption) (fftypes.JSONObject, error)); ok { - return rf(ctx, op, options...) + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation, bool) (fftypes.JSONObject, error)); ok { + return rf(ctx, op, idempotentSubmit) } - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation, ...operations.RunOperationOption) fftypes.JSONObject); ok { - r0 = rf(ctx, op, options...) + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation, bool) fftypes.JSONObject); ok { + r0 = rf(ctx, op, idempotentSubmit) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(fftypes.JSONObject) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation, ...operations.RunOperationOption) error); ok { - r1 = rf(ctx, op, options...) + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation, bool) error); ok { + r1 = rf(ctx, op, idempotentSubmit) } else { r1 = ret.Error(1) } diff --git a/mocks/privatemessagingmocks/manager.go b/mocks/privatemessagingmocks/manager.go index 35b6677a9..20510a704 100644 --- a/mocks/privatemessagingmocks/manager.go +++ b/mocks/privatemessagingmocks/manager.go @@ -214,13 +214,13 @@ func (_m *Manager) ResolveInitGroup(ctx context.Context, msg *core.Message, crea } // RunOperation provides a mock function with given fields: ctx, op -func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, bool, error) { +func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error) { ret := _m.Called(ctx, op) var r0 fftypes.JSONObject - var r1 bool + var r1 core.OpPhase var r2 error - if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) (fftypes.JSONObject, core.OpPhase, error)); ok { return rf(ctx, op) } if rf, ok := ret.Get(0).(func(context.Context, *core.PreparedOperation) fftypes.JSONObject); ok { @@ -231,10 +231,10 @@ func (_m *Manager) RunOperation(ctx context.Context, op *core.PreparedOperation) } } - if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) bool); ok { + if rf, ok := ret.Get(1).(func(context.Context, *core.PreparedOperation) core.OpPhase); ok { r1 = rf(ctx, op) } else { - r1 = ret.Get(1).(bool) + r1 = ret.Get(1).(core.OpPhase) } if rf, ok := ret.Get(2).(func(context.Context, *core.PreparedOperation) error); ok { diff --git a/mocks/shareddownloadmocks/manager.go b/mocks/shareddownloadmocks/manager.go index 818a2ca04..7d1841809 100644 --- a/mocks/shareddownloadmocks/manager.go +++ b/mocks/shareddownloadmocks/manager.go @@ -14,13 +14,13 @@ type Manager struct { mock.Mock } -// InitiateDownloadBatch provides a mock function with given fields: ctx, tx, payloadRef -func (_m *Manager) InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string) error { - ret := _m.Called(ctx, tx, payloadRef) +// InitiateDownloadBatch provides a mock function with given fields: ctx, tx, payloadRef, idempotentSubmit +func (_m *Manager) InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, payloadRef string, idempotentSubmit bool) error { + ret := _m.Called(ctx, tx, payloadRef, idempotentSubmit) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID, string) error); ok { - r0 = rf(ctx, tx, payloadRef) + if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID, string, bool) error); ok { + r0 = rf(ctx, tx, payloadRef, idempotentSubmit) } else { r0 = ret.Error(0) } @@ -28,13 +28,13 @@ func (_m *Manager) InitiateDownloadBatch(ctx context.Context, tx *fftypes.UUID, return r0 } -// InitiateDownloadBlob provides a mock function with given fields: ctx, tx, dataID, payloadRef -func (_m *Manager) InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string) error { - ret := _m.Called(ctx, tx, dataID, payloadRef) +// InitiateDownloadBlob provides a mock function with given fields: ctx, tx, dataID, payloadRef, idempotentSubmit +func (_m *Manager) InitiateDownloadBlob(ctx context.Context, tx *fftypes.UUID, dataID *fftypes.UUID, payloadRef string, idempotentSubmit bool) error { + ret := _m.Called(ctx, tx, dataID, payloadRef, idempotentSubmit) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID, *fftypes.UUID, string) error); ok { - r0 = rf(ctx, tx, dataID, payloadRef) + if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID, *fftypes.UUID, string, bool) error); ok { + r0 = rf(ctx, tx, dataID, payloadRef, idempotentSubmit) } else { r0 = ret.Error(0) } diff --git a/mocks/tokenmocks/plugin.go b/mocks/tokenmocks/plugin.go index 05c61cc86..50ff57f28 100644 --- a/mocks/tokenmocks/plugin.go +++ b/mocks/tokenmocks/plugin.go @@ -22,18 +22,18 @@ type Plugin struct { } // ActivateTokenPool provides a mock function with given fields: ctx, pool -func (_m *Plugin) ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (bool, error) { +func (_m *Plugin) ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (core.OpPhase, error) { ret := _m.Called(ctx, pool) - var r0 bool + var r0 core.OpPhase var r1 error - if rf, ok := ret.Get(0).(func(context.Context, *core.TokenPool) (bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.TokenPool) (core.OpPhase, error)); ok { return rf(ctx, pool) } - if rf, ok := ret.Get(0).(func(context.Context, *core.TokenPool) bool); ok { + if rf, ok := ret.Get(0).(func(context.Context, *core.TokenPool) core.OpPhase); ok { r0 = rf(ctx, pool) } else { - r0 = ret.Get(0).(bool) + r0 = ret.Get(0).(core.OpPhase) } if rf, ok := ret.Get(1).(func(context.Context, *core.TokenPool) error); ok { @@ -102,18 +102,18 @@ func (_m *Plugin) CheckInterface(ctx context.Context, pool *core.TokenPool, meth } // CreateTokenPool provides a mock function with given fields: ctx, nsOpID, pool -func (_m *Plugin) CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (bool, error) { +func (_m *Plugin) CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (core.OpPhase, error) { ret := _m.Called(ctx, nsOpID, pool) - var r0 bool + var r0 core.OpPhase var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, *core.TokenPool) (bool, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, string, *core.TokenPool) (core.OpPhase, error)); ok { return rf(ctx, nsOpID, pool) } - if rf, ok := ret.Get(0).(func(context.Context, string, *core.TokenPool) bool); ok { + if rf, ok := ret.Get(0).(func(context.Context, string, *core.TokenPool) core.OpPhase); ok { r0 = rf(ctx, nsOpID, pool) } else { - r0 = ret.Get(0).(bool) + r0 = ret.Get(0).(core.OpPhase) } if rf, ok := ret.Get(1).(func(context.Context, string, *core.TokenPool) error); ok { diff --git a/pkg/core/operation.go b/pkg/core/operation.go index e47b2a21c..5d8d098b8 100644 --- a/pkg/core/operation.go +++ b/pkg/core/operation.go @@ -144,6 +144,14 @@ type PreparedOperation struct { Data interface{} `json:"data"` } +type OpPhase int + +const ( + OpPhaseComplete OpPhase = iota + OpPhasePending + OpPhaseInitializing +) + func (po *PreparedOperation) NamespacedIDString() string { return po.Namespace + ":" + po.ID.String() } diff --git a/pkg/tokens/plugin.go b/pkg/tokens/plugin.go index 15ec628f6..328f62c20 100644 --- a/pkg/tokens/plugin.go +++ b/pkg/tokens/plugin.go @@ -50,10 +50,10 @@ type Plugin interface { Capabilities() *Capabilities // CreateTokenPool creates a new (fungible or non-fungible) pool of tokens - CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (complete bool, err error) + CreateTokenPool(ctx context.Context, nsOpID string, pool *core.TokenPool) (phase core.OpPhase, err error) // ActivateTokenPool activates a pool in order to begin receiving events - ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (complete bool, err error) + ActivateTokenPool(ctx context.Context, pool *core.TokenPool) (phase core.OpPhase, err error) // DectivateTokenPool deactivates a pool in order to stop receiving events and remove underlying listeners DeactivateTokenPool(ctx context.Context, pool *core.TokenPool) error From 3f2a21f958b05780dbe029f2de1c8f1b29382528 Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Wed, 27 Sep 2023 01:41:43 -0400 Subject: [PATCH 2/3] Handle the case where we fail before writing the operations, but after creating the TX Signed-off-by: Peter Broadhurst --- internal/assets/token_approval.go | 14 ++++- internal/assets/token_approval_test.go | 67 ++++++++++++++++++++- internal/assets/token_pool.go | 15 ++++- internal/assets/token_pool_test.go | 44 +++++++++++++- internal/assets/token_transfer.go | 14 ++++- internal/assets/token_transfer_test.go | 45 +++++++++++++- internal/broadcast/manager.go | 34 ++++++++--- internal/broadcast/manager_test.go | 81 ++++++++++++++++++++++++-- internal/contracts/manager.go | 6 +- internal/contracts/manager_test.go | 12 ++-- internal/operations/manager.go | 18 ++++-- internal/operations/manager_test.go | 14 ++--- mocks/operationmocks/manager.go | 29 +++++---- 13 files changed, 328 insertions(+), 65 deletions(-) diff --git a/internal/assets/token_approval.go b/internal/assets/token_approval.go index ed96b21c2..406af13e6 100644 --- a/internal/assets/token_approval.go +++ b/internal/assets/token_approval.go @@ -109,20 +109,28 @@ func (s *approveSender) resolve(ctx context.Context) (opResubmitted bool, err er if err != nil { // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers + resubmitWholeTX := false if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := s.mgr.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + total, resubmitted, resubmitErr := s.mgr.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) if resubmitErr != nil { // Error doing resubmit, return the new error return false, resubmitErr } - if len(resubmitted) > 0 { + if total == 0 { + // We didn't do anything last time - just start again + txid = idemErr.ExistingTXID + resubmitWholeTX = true + err = nil + } else if len(resubmitted) > 0 { // We resubmitted something - translate the status code to 200 (true return) s.approval.TX.ID = idemErr.ExistingTXID s.approval.TX.Type = core.TransactionTypeTokenApproval return true, nil } } - return false, err + if !resubmitWholeTX { + return false, err + } } s.approval.TX.ID = txid s.approval.TX.Type = core.TransactionTypeTokenApproval diff --git a/internal/assets/token_approval_test.go b/internal/assets/token_approval_test.go index 2593aa78c..b56ec732a 100644 --- a/internal/assets/token_approval_test.go +++ b/internal/assets/token_approval_test.go @@ -243,7 +243,68 @@ func TestApprovalIdempotentOperationResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenApproval, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{op}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{op}, nil) + + // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error + _, err := am.TokenApproval(context.Background(), approval, false) + assert.NoError(t, err) + + mth.AssertExpectations(t) + mom.AssertExpectations(t) +} + +func TestApprovalIdempotentOperationResubmitAll(t *testing.T) { + am, cancel := newTestAssets(t) + defer cancel() + var id = fftypes.NewUUID() + + approval := &core.TokenApprovalInput{ + TokenApproval: core.TokenApproval{ + Approved: true, + Operator: "operator", + Key: "key", + }, + IdempotencyKey: "idem1", + } + + pool := &core.TokenPool{ + Connector: "magic-tokens", + Active: true, + } + + mth := am.txHelper.(*txcommonmocks.Helper) + mom := am.operations.(*operationmocks.Manager) + mdi := am.database.(*databasemocks.Plugin) + mim := am.identity.(*identitymanagermocks.Manager) + + fb := database.TokenPoolQueryFactory.NewFilter(context.Background()) + f := fb.And() + f.Limit(1).Count(true) + mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenApproval, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ + ExistingTXID: id, + OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) + mom.On("ResubmitOperations", context.Background(), id).Return(0, nil, nil) + mdi.On("GetTokenPool", context.Background(), "ns1", "pool1").Return(pool, nil) + mom.On("AddOrReuseOperation", context.Background(), mock.Anything).Return(nil) + mom.On("RunOperation", context.Background(), mock.Anything, true).Return(nil, nil) + + tokenPools := []*core.TokenPool{ + { + Name: "pool1", + Locator: "F1", + Connector: "magic-tokens", + Active: true, + }, + } + totalCount := int64(1) + filterResult := &ffapi.FilterResult{ + TotalCount: &totalCount, + } + mim.On("ResolveInputSigningKey", context.Background(), "key", identity.KeyNormalizationBlockchainPlugin).Return("0x12345", nil) + mdi.On("GetTokenPools", context.Background(), "ns1", mock.MatchedBy((func(f ffapi.AndFilter) bool { + info, _ := f.Finalize() + return info.Count && info.Limit == 1 + }))).Return(tokenPools, filterResult, nil) // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error _, err := am.TokenApproval(context.Background(), approval, false) @@ -275,7 +336,7 @@ func TestApprovalIdempotentNoOperationToResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenApproval, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* one total */, nil /* none to resubmit */, nil) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back _, err := am.TokenApproval(context.Background(), approval, false) @@ -308,7 +369,7 @@ func TestApprovalIdempotentOperationErrorOnResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenApproval, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error _, err := am.TokenApproval(context.Background(), approval, false) diff --git a/internal/assets/token_pool.go b/internal/assets/token_pool.go index 63d0bbc8d..36c88d4fd 100644 --- a/internal/assets/token_pool.go +++ b/internal/assets/token_pool.go @@ -86,19 +86,28 @@ func (am *assetManager) createTokenPoolInternal(ctx context.Context, pool *core. if err != nil { // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers. + resubmitWholeTX := false if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr = am.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + var total int + total, resubmitted, resubmitErr = am.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) if resubmitErr != nil { // Error doing resubmit, return the new error return resubmitErr } - if len(resubmitted) > 0 { + if total == 0 { + // We didn't do anything last time - just start again + txid = idemErr.ExistingTXID + resubmitWholeTX = true + err = nil + } else if len(resubmitted) > 0 { pool.TX.ID = idemErr.ExistingTXID pool.TX.Type = core.TransactionTypeTokenPool err = nil } } - return err + if !resubmitWholeTX { + return err + } } pool.TX.ID = txid diff --git a/internal/assets/token_pool_test.go b/internal/assets/token_pool_test.go index 368291a36..92025f5ca 100644 --- a/internal/assets/token_pool_test.go +++ b/internal/assets/token_pool_test.go @@ -150,7 +150,45 @@ func TestCreateTokenPoolIdempotentResubmit(t *testing.T) { Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{op}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{op}, nil) + + _, err := am.CreateTokenPool(context.Background(), pool, false) + + // SubmitNewTransction returned 409 idempotency clash, ResubmitOperations returned that it resubmitted an operation. Shouldn't + // see the original 409 Conflict error + assert.NoError(t, err) + + mdi.AssertExpectations(t) + mim.AssertExpectations(t) + mth.AssertExpectations(t) + mom.AssertExpectations(t) +} + +func TestCreateTokenPoolIdempotentResubmitAll(t *testing.T) { + am, cancel := newTestAssets(t) + defer cancel() + var id = fftypes.NewUUID() + + pool := &core.TokenPoolInput{ + TokenPool: core.TokenPool{ + Name: "testpool", + }, + IdempotencyKey: "idem1", + } + + mdi := am.database.(*databasemocks.Plugin) + mim := am.identity.(*identitymanagermocks.Manager) + mth := am.txHelper.(*txcommonmocks.Helper) + mom := am.operations.(*operationmocks.Manager) + mdi.On("GetTokenPool", context.Background(), "ns1", "testpool").Return(nil, nil) + mim.On("ResolveInputSigningKey", context.Background(), "", identity.KeyNormalizationBlockchainPlugin).Return("resolved-key", nil) + mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenPool, core.IdempotencyKey("idem1")). + Return(id, &sqlcommon.IdempotencyError{ + ExistingTXID: id, + OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) + mom.On("ResubmitOperations", context.Background(), id).Return(0, nil, nil) + mom.On("AddOrReuseOperation", context.Background(), mock.Anything).Return(nil) + mom.On("RunOperation", context.Background(), mock.Anything, true).Return(nil, nil) _, err := am.CreateTokenPool(context.Background(), pool, false) @@ -186,7 +224,7 @@ func TestCreateTokenPoolIdempotentNoOperationToResubmit(t *testing.T) { Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resubmit */, nil) _, err := am.CreateTokenPool(context.Background(), pool, false) @@ -223,7 +261,7 @@ func TestCreateTokenPoolIdempotentErrorOnOperationResubmit(t *testing.T) { Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) _, err := am.CreateTokenPool(context.Background(), pool, false) diff --git a/internal/assets/token_transfer.go b/internal/assets/token_transfer.go index f70b6c1ee..0d26c1c06 100644 --- a/internal/assets/token_transfer.go +++ b/internal/assets/token_transfer.go @@ -195,13 +195,19 @@ func (s *transferSender) resolve(ctx context.Context) (opResubmitted bool, err e if err != nil { // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers. Note that we'll return the result of resubmitting the operation, not a 409 Conflict error + resubmitWholeTX := false if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := s.mgr.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + total, resubmitted, resubmitErr := s.mgr.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) if resubmitErr != nil { // Error doing resubmit, return the new error err = resubmitErr } - if len(resubmitted) > 0 { + if total == 0 { + // We didn't do anything last time - just start again + txid = idemErr.ExistingTXID + resubmitWholeTX = true + err = nil + } else if len(resubmitted) > 0 { // We resubmitted something - translate the status code to 200 (true return) s.transfer.TX.ID = idemErr.ExistingTXID s.transfer.TX.Type = core.TransactionTypeTokenTransfer @@ -209,7 +215,9 @@ func (s *transferSender) resolve(ctx context.Context) (opResubmitted bool, err e } } - return false, err + if !resubmitWholeTX { + return false, err + } } s.transfer.TX.ID = txid s.transfer.TX.Type = core.TransactionTypeTokenTransfer diff --git a/internal/assets/token_transfer_test.go b/internal/assets/token_transfer_test.go index 6835530cc..cc900f4bd 100644 --- a/internal/assets/token_transfer_test.go +++ b/internal/assets/token_transfer_test.go @@ -132,7 +132,46 @@ func TestMintTokensIdempotentResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenTransfer, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{op}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{op}, nil) + + // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error + _, err := am.MintTokens(context.Background(), mint, false) + assert.NoError(t, err) + + mth.AssertExpectations(t) + mom.AssertExpectations(t) +} + +func TestMintTokensIdempotentResubmitAll(t *testing.T) { + am, cancel := newTestAssetsWithMetrics(t) + defer cancel() + var id = fftypes.NewUUID() + + mint := &core.TokenTransferInput{ + TokenTransfer: core.TokenTransfer{ + Amount: *fftypes.NewFFBigInt(5), + }, + Pool: "pool1", + IdempotencyKey: "idem1", + } + pool := &core.TokenPool{ + Name: "pool1", + Connector: "magic-tokens", + Active: true, + } + + mdi := am.database.(*databasemocks.Plugin) + mim := am.identity.(*identitymanagermocks.Manager) + mth := am.txHelper.(*txcommonmocks.Helper) + mom := am.operations.(*operationmocks.Manager) + mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenTransfer, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ + ExistingTXID: id, + OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) + mom.On("ResubmitOperations", context.Background(), id).Return(0, nil, nil) + mim.On("ResolveInputSigningKey", context.Background(), "", identity.KeyNormalizationBlockchainPlugin).Return("0x12345", nil) + mdi.On("GetTokenPool", context.Background(), "ns1", "pool1").Return(pool, nil) + mom.On("AddOrReuseOperation", context.Background(), mock.Anything).Return(nil) + mom.On("RunOperation", context.Background(), mock.Anything, true).Return(nil, nil) // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error _, err := am.MintTokens(context.Background(), mint, false) @@ -160,7 +199,7 @@ func TestMintTokensIdempotentNoOperationToResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenTransfer, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resumit */, nil) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back _, err := am.MintTokens(context.Background(), mint, false) @@ -189,7 +228,7 @@ func TestMintTokensIdempotentErrorOnResubmit(t *testing.T) { mth.On("SubmitNewTransaction", context.Background(), core.TransactionTypeTokenTransfer, core.IdempotencyKey("idem1")).Return(id, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back _, err := am.MintTokens(context.Background(), mint, false) diff --git a/internal/broadcast/manager.go b/internal/broadcast/manager.go index 9800468fa..52b93ece5 100644 --- a/internal/broadcast/manager.go +++ b/internal/broadcast/manager.go @@ -233,18 +233,27 @@ func (bm *broadcastManager) PublishDataValue(ctx context.Context, id string, ide if err != nil { // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers + resubmitWholeTX := false if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := bm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + total, resubmitted, resubmitErr := bm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) - if resubmitErr != nil { + switch { + case resubmitErr != nil: // Error doing resubmit, return the new error err = resubmitErr - } else if len(resubmitted) > 0 { + case total == 0: + // We didn't do anything last time - just start again + txid = idemErr.ExistingTXID + resubmitWholeTX = true + err = nil + case len(resubmitted) > 0: // We successfully resubmitted an initialized operation, return 2xx not 409 err = nil } } - return d, err + if !resubmitWholeTX { + return d, err + } } op := core.NewOperation( @@ -275,18 +284,27 @@ func (bm *broadcastManager) PublishDataBlob(ctx context.Context, id string, idem if err != nil { // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers + resubmitWholeTX := false if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := bm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + total, resubmitted, resubmitErr := bm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) - if resubmitErr != nil { + switch { + case resubmitErr != nil: // Error doing resubmit, return the new error err = resubmitErr - } else if len(resubmitted) > 0 { + case total == 0: + // We didn't do anything last time - just start again + txid = idemErr.ExistingTXID + resubmitWholeTX = true + err = nil + case len(resubmitted) > 0: // We successfully resubmitted an initialized operation, return 2xx not 409 err = nil } } - return d, err + if !resubmitWholeTX { + return d, err + } } if err = bm.uploadDataBlob(ctx, txid, d, idempotencyKey != ""); err != nil { diff --git a/internal/broadcast/manager_test.go b/internal/broadcast/manager_test.go index c1b9caa9a..a57347d11 100644 --- a/internal/broadcast/manager_test.go +++ b/internal/broadcast/manager_test.go @@ -378,7 +378,7 @@ func TestUploadBlobPublishIdempotentResubmitOperation(t *testing.T) { mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{op}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{op}, nil) mdi.On("GetDataByID", ctx, "ns1", d.ID, true).Return(d, nil) // If ResubmitOperations returns an operation it's because it found one to resubmit, we return 2xx not 409 and hence don't expect any errors here @@ -389,6 +389,43 @@ func TestUploadBlobPublishIdempotentResubmitOperation(t *testing.T) { mdi.AssertExpectations(t) } +func TestUploadBlobPublishIdempotentResubmitAll(t *testing.T) { + bm, cancel := newTestBroadcast(t) + var id = fftypes.NewUUID() + defer cancel() + mdi := bm.database.(*databasemocks.Plugin) + mom := bm.operations.(*operationmocks.Manager) + mtx := bm.txHelper.(*txcommonmocks.Helper) + + blob := &core.Blob{ + Hash: fftypes.NewRandB32(), + PayloadRef: "blob/1", + } + d := &core.Data{ + ID: fftypes.NewUUID(), + Blob: &core.BlobRef{ + Hash: blob.Hash, + }, + } + + ctx := context.Background() + mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ + ExistingTXID: id, + OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) + mom.On("ResubmitOperations", context.Background(), id).Return(0, nil, nil) + mdi.On("GetDataByID", ctx, "ns1", d.ID, true).Return(d, nil) + mom.On("AddOrReuseOperation", mock.Anything, mock.Anything).Return(nil) + mdi.On("GetBlobs", ctx, bm.namespace.Name, mock.Anything).Return([]*core.Blob{blob}, nil, nil) + mom.On("RunOperation", mock.Anything, mock.Anything, true).Return(nil, nil) + + // If ResubmitOperations returns an operation it's because it found one to resubmit, we return 2xx not 409 and hence don't expect any errors here + d, err := bm.PublishDataBlob(ctx, d.ID.String(), "idem1") + assert.NotNil(t, d) + assert.NoError(t, err) + + mdi.AssertExpectations(t) +} + func TestUploadBlobPublishIdempotentNoOperationToResubmit(t *testing.T) { bm, cancel := newTestBroadcast(t) var id = fftypes.NewUUID() @@ -412,7 +449,7 @@ func TestUploadBlobPublishIdempotentNoOperationToResubmit(t *testing.T) { mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resubmit */, nil) mdi.On("GetDataByID", ctx, "ns1", d.ID, true).Return(d, nil) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back @@ -446,7 +483,7 @@ func TestUploadBlobPublishIdempotentErrorOnOperationResubmit(t *testing.T) { mtx.On("SubmitNewTransaction", mock.Anything, core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) mdi.On("GetDataByID", ctx, "ns1", d.ID, true).Return(d, nil) // If ResubmitOperations returned an error trying to resubmit an operation we expect that error back, not the 409 conflict error @@ -689,7 +726,39 @@ func TestUploadValueIdempotentResubmitOperation(t *testing.T) { mtx.On("SubmitNewTransaction", context.Background(), core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{op}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{op}, nil) + + // If ResubmitOperations returns an operation it's because it found one to resubmit, we return 2xx not 409 and hence don't expect any errors here + d1, err := bm.PublishDataValue(context.Background(), d.ID.String(), "idem1") + assert.NoError(t, err) + assert.Equal(t, d.ID, d1.ID) + + mom.AssertExpectations(t) + mdi.AssertExpectations(t) +} + +func TestUploadValueIdempotentResubmitAll(t *testing.T) { + bm, cancel := newTestBroadcast(t) + var id = fftypes.NewUUID() + defer cancel() + + d := &core.Data{ + ID: fftypes.NewUUID(), + Value: fftypes.JSONAnyPtr(`{"some": "value"}`), + } + + mdi := bm.database.(*databasemocks.Plugin) + mdi.On("GetDataByID", mock.Anything, "ns1", d.ID, true).Return(d, nil) + + mom := bm.operations.(*operationmocks.Manager) + + mtx := bm.txHelper.(*txcommonmocks.Helper) + mtx.On("SubmitNewTransaction", context.Background(), core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ + ExistingTXID: id, + OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) + mom.On("ResubmitOperations", context.Background(), id).Return(0, nil, nil) + mom.On("AddOrReuseOperation", mock.Anything, mock.Anything).Return(nil) + mom.On("RunOperation", mock.Anything, mock.Anything, true).Return(nil, nil) // If ResubmitOperations returns an operation it's because it found one to resubmit, we return 2xx not 409 and hence don't expect any errors here d1, err := bm.PublishDataValue(context.Background(), d.ID.String(), "idem1") @@ -719,7 +788,7 @@ func TestUploadValueIdempotentNoOperationToResubmit(t *testing.T) { mtx.On("SubmitNewTransaction", context.Background(), core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resubmit */, nil) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back _, err := bm.PublishDataValue(context.Background(), d.ID.String(), "idem1") @@ -749,7 +818,7 @@ func TestUploadValueIdempotentErrorOnOperationResubmit(t *testing.T) { mtx.On("SubmitNewTransaction", context.Background(), core.TransactionTypeDataPublish, core.IdempotencyKey("idem1")).Return(fftypes.NewUUID(), &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back _, err := bm.PublishDataValue(context.Background(), d.ID.String(), "idem1") diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index f710214fa..5dc6a556e 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -298,7 +298,8 @@ func (cm *contractManager) writeInvokeTransaction(ctx context.Context, req *core // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := cm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + // Note we don't need to worry about re-entering this code zero-ops in this case, as we write everything as a batch in WriteTransactionAndOps. + _, resubmitted, resubmitErr := cm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) if resubmitErr != nil { // Error doing resubmit, return the new error @@ -338,7 +339,8 @@ func (cm *contractManager) writeDeployTransaction(ctx context.Context, req *core // Check if we've clashed on idempotency key. There might be operations still in "Initialized" state that need // submitting to their handlers if idemErr, ok := err.(*sqlcommon.IdempotencyError); ok { - resubmitted, resubmitErr := cm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) + // Note we don't need to worry about re-entering this code zero-ops in this case, as we write everything as a batch in WriteTransactionAndOps. + _, resubmitted, resubmitErr := cm.operations.ResubmitOperations(ctx, idemErr.ExistingTXID) if resubmitErr != nil { // Error doing resubmit, return the new error diff --git a/internal/contracts/manager_test.go b/internal/contracts/manager_test.go index 9275dfb63..133d84947 100644 --- a/internal/contracts/manager_test.go +++ b/internal/contracts/manager_test.go @@ -1719,7 +1719,7 @@ func TestDeployContractIdempotentResubmitOperation(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{{}}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{{}}, nil) mim.On("ResolveInputSigningKey", mock.Anything, signingKey, identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) // If ResubmitOperations returns an operation it's because it found one to resubmit, so we return 2xx not 409, and don't expect an error @@ -1752,7 +1752,7 @@ func TestDeployContractIdempotentNoOperationToResubmit(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resubmit */, nil) mim.On("ResolveInputSigningKey", mock.Anything, signingKey, identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) // If ResubmitOperations returns nil it's because there was no operation in initialized state, so we expect the regular 409 error back @@ -1786,7 +1786,7 @@ func TestDeployContractIdempotentErrorOnOperationResubmit(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) mim.On("ResolveInputSigningKey", mock.Anything, signingKey, identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) _, err := cm.DeployContract(context.Background(), req, false) @@ -2370,7 +2370,7 @@ func TestInvokeContractIdempotentResubmitOperation(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return([]*core.Operation{{}}, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1, []*core.Operation{{}}, nil) mim.On("ResolveInputSigningKey", mock.Anything, "", identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) opaqueData := "anything" mbm.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) @@ -2414,7 +2414,7 @@ func TestInvokeContractIdempotentNoOperationToResubmit(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, nil) + mom.On("ResubmitOperations", context.Background(), id).Return(1 /* total */, nil /* to resubmit */, nil) mim.On("ResolveInputSigningKey", mock.Anything, "", identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) opaqueData := "anything" mbm.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) @@ -2458,7 +2458,7 @@ func TestInvokeContractIdempotentErrorOnOperationResubmit(t *testing.T) { })).Return(nil, &sqlcommon.IdempotencyError{ ExistingTXID: id, OriginalError: i18n.NewError(context.Background(), coremsgs.MsgIdempotencyKeyDuplicateTransaction, "idem1", id)}) - mom.On("ResubmitOperations", context.Background(), id).Return(nil, fmt.Errorf("pop")) + mom.On("ResubmitOperations", context.Background(), id).Return(-1, nil, fmt.Errorf("pop")) mim.On("ResolveInputSigningKey", mock.Anything, "", identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil) opaqueData := "anything" mbm.On("ParseInterface", context.Background(), req.Method, req.Errors).Return(opaqueData, nil) diff --git a/internal/operations/manager.go b/internal/operations/manager.go index ccef670bf..65c3d0f8e 100644 --- a/internal/operations/manager.go +++ b/internal/operations/manager.go @@ -44,7 +44,7 @@ type Manager interface { PrepareOperation(ctx context.Context, op *core.Operation) (*core.PreparedOperation, error) RunOperation(ctx context.Context, op *core.PreparedOperation, idempotentSubmit bool) (fftypes.JSONObject, error) RetryOperation(ctx context.Context, opID *fftypes.UUID) (*core.Operation, error) - ResubmitOperations(ctx context.Context, txID *fftypes.UUID) ([]*core.Operation, error) + ResubmitOperations(ctx context.Context, txID *fftypes.UUID) (total int, resubmit []*core.Operation, err error) AddOrReuseOperation(ctx context.Context, op *core.Operation, hooks ...database.PostCompletionHook) error BulkInsertOperations(ctx context.Context, ops ...*core.Operation) error SubmitOperationUpdate(update *core.OperationUpdate) @@ -122,19 +122,25 @@ func (om *operationsManager) PrepareOperation(ctx context.Context, op *core.Oper return handler.PrepareOperation(ctx, op) } -func (om *operationsManager) ResubmitOperations(ctx context.Context, txID *fftypes.UUID) ([]*core.Operation, error) { +func (om *operationsManager) ResubmitOperations(ctx context.Context, txID *fftypes.UUID) (int, []*core.Operation, error) { var resubmitErr error fb := database.OperationQueryFactory.NewFilter(ctx) filter := fb.And( fb.Eq("tx", txID), - fb.Eq("status", core.OpStatusInitialized), ) - initializedOperations, _, opErr := om.database.GetOperations(ctx, om.namespace, filter) + allOperations, _, opErr := om.database.GetOperations(ctx, om.namespace, filter) if opErr != nil { // Couldn't query operations. Log and return the original error log.L(ctx).Errorf("Failed to lookup initialized operations for TX %v: %v", txID, opErr) - return nil, opErr + return -1, nil, opErr + } + + initializedOperations := make([]*core.Operation, 0, len(allOperations)) + for _, op := range allOperations { + if op.Status == core.OpStatusInitialized { + initializedOperations = append(initializedOperations, op) + } } resubmitted := []*core.Operation{} @@ -153,7 +159,7 @@ func (om *operationsManager) ResubmitOperations(ctx context.Context, txID *fftyp log.L(ctx).Infof("%d operation resubmitted as part of idempotent retry of TX %s", nextInitializedOp.ID, txID) resubmitted = append(resubmitted, nextInitializedOp) } - return resubmitted, resubmitErr + return len(allOperations), resubmitted, resubmitErr } func (om *operationsManager) RunOperation(ctx context.Context, op *core.PreparedOperation, idempotentSubmit bool) (fftypes.JSONObject, error) { diff --git a/internal/operations/manager_test.go b/internal/operations/manager_test.go index 598a54a8e..4e5448c01 100644 --- a/internal/operations/manager_test.go +++ b/internal/operations/manager_test.go @@ -620,12 +620,12 @@ func TestResubmitIdempotentOperation(t *testing.T) { fb := database.OperationQueryFactory.NewFilter(ctx) filter := fb.And( fb.Eq("tx", id), - fb.Eq("status", core.OpStatusInitialized), ) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) mdi.On("GetOperations", ctx, "ns1", filter).Return(operations, nil, nil) - resubmitted, err := om.ResubmitOperations(ctx, id) + total, resubmitted, err := om.ResubmitOperations(ctx, id) assert.NoError(t, err) + assert.Equal(t, total, 1) assert.Len(t, resubmitted, 1) mdi.AssertExpectations(t) @@ -658,12 +658,12 @@ func TestResubmitIdempotentOperationSkipCached(t *testing.T) { fb := database.OperationQueryFactory.NewFilter(ctx) filter := fb.And( fb.Eq("tx", id), - fb.Eq("status", core.OpStatusInitialized), ) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) mdi.On("GetOperations", ctx, "ns1", filter).Return(operations, nil, nil) - resubmitted, err := om.ResubmitOperations(ctx, id) + total, resubmitted, err := om.ResubmitOperations(ctx, id) assert.NoError(t, err) + assert.Equal(t, total, 1) assert.Empty(t, resubmitted) mdi.AssertExpectations(t) @@ -693,11 +693,10 @@ func TestResubmitIdempotentOperationLookupError(t *testing.T) { fb := database.OperationQueryFactory.NewFilter(ctx) filter := fb.And( fb.Eq("tx", id), - fb.Eq("status", core.OpStatusInitialized), ) om.RegisterHandler(ctx, &mockHandler{Prepared: po}, []core.OpType{core.OpTypeBlockchainPinBatch}) mdi.On("GetOperations", ctx, "ns1", filter).Return(operations, nil, fmt.Errorf("pop")) - _, err := om.ResubmitOperations(ctx, id) + _, _, err := om.ResubmitOperations(ctx, id) assert.Error(t, err) mdi.AssertExpectations(t) @@ -727,11 +726,10 @@ func TestResubmitIdempotentOperationExecError(t *testing.T) { fb := database.OperationQueryFactory.NewFilter(ctx) filter := fb.And( fb.Eq("tx", id), - fb.Eq("status", core.OpStatusInitialized), ) om.RegisterHandler(ctx, &mockHandler{Prepared: po, RunErr: fmt.Errorf("pop")}, []core.OpType{core.OpTypeBlockchainPinBatch}) mdi.On("GetOperations", ctx, "ns1", filter).Return(operations, nil, nil) - _, err := om.ResubmitOperations(ctx, id) + _, _, err := om.ResubmitOperations(ctx, id) assert.Error(t, err) mdi.AssertExpectations(t) diff --git a/mocks/operationmocks/manager.go b/mocks/operationmocks/manager.go index 2d8c4f2eb..65963faba 100644 --- a/mocks/operationmocks/manager.go +++ b/mocks/operationmocks/manager.go @@ -134,29 +134,36 @@ func (_m *Manager) ResolveOperationByID(ctx context.Context, opID *fftypes.UUID, } // ResubmitOperations provides a mock function with given fields: ctx, txID -func (_m *Manager) ResubmitOperations(ctx context.Context, txID *fftypes.UUID) ([]*core.Operation, error) { +func (_m *Manager) ResubmitOperations(ctx context.Context, txID *fftypes.UUID) (int, []*core.Operation, error) { ret := _m.Called(ctx, txID) - var r0 []*core.Operation - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID) ([]*core.Operation, error)); ok { + var r0 int + var r1 []*core.Operation + var r2 error + if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID) (int, []*core.Operation, error)); ok { return rf(ctx, txID) } - if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID) []*core.Operation); ok { + if rf, ok := ret.Get(0).(func(context.Context, *fftypes.UUID) int); ok { r0 = rf(ctx, txID) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]*core.Operation) - } + r0 = ret.Get(0).(int) } - if rf, ok := ret.Get(1).(func(context.Context, *fftypes.UUID) error); ok { + if rf, ok := ret.Get(1).(func(context.Context, *fftypes.UUID) []*core.Operation); ok { r1 = rf(ctx, txID) } else { - r1 = ret.Error(1) + if ret.Get(1) != nil { + r1 = ret.Get(1).([]*core.Operation) + } } - return r0, r1 + if rf, ok := ret.Get(2).(func(context.Context, *fftypes.UUID) error); ok { + r2 = rf(ctx, txID) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } // RetryOperation provides a mock function with given fields: ctx, opID From 9e3284162391a3ebc9d7f8c12f9d3953734b41af Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Thu, 28 Sep 2023 13:29:04 -0400 Subject: [PATCH 3/3] Only initializing if there's an error Signed-off-by: Peter Broadhurst --- internal/assets/operations.go | 8 +++++--- internal/assets/operations_test.go | 10 +++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/assets/operations.go b/internal/assets/operations.go index 9e79e2783..f16349180 100644 --- a/internal/assets/operations.go +++ b/internal/assets/operations.go @@ -24,6 +24,7 @@ import ( "github.com/hyperledger/firefly-common/pkg/i18n" "github.com/hyperledger/firefly-common/pkg/log" "github.com/hyperledger/firefly/internal/coremsgs" + "github.com/hyperledger/firefly/internal/operations" "github.com/hyperledger/firefly/internal/txcommon" "github.com/hyperledger/firefly/pkg/core" ) @@ -124,14 +125,15 @@ func (am *assetManager) RunOperation(ctx context.Context, op *core.PreparedOpera } switch data.Transfer.Type { case core.TokenTransferTypeMint: - return nil, core.OpPhaseInitializing, plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + err = plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) case core.TokenTransferTypeTransfer: - return nil, core.OpPhaseInitializing, plugin.TransferTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + err = plugin.TransferTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) case core.TokenTransferTypeBurn: - return nil, core.OpPhaseInitializing, plugin.BurnTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) + err = plugin.BurnTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods) default: panic(fmt.Sprintf("unknown transfer type: %v", data.Transfer.Type)) } + return nil, operations.ErrTernary(err, core.OpPhaseInitializing, core.OpPhasePending), err case approvalData: plugin, err := am.selectTokenPlugin(ctx, data.Pool.Connector) diff --git a/internal/assets/operations_test.go b/internal/assets/operations_test.go index e261ecf08..f4c79bb4e 100644 --- a/internal/assets/operations_test.go +++ b/internal/assets/operations_test.go @@ -126,7 +126,7 @@ func TestPrepareAndRunTransfer(t *testing.T) { _, phase, err := am.RunOperation(context.Background(), po) - assert.Equal(t, core.OpPhaseInitializing, phase) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -475,7 +475,7 @@ func TestRunOperationTransferMint(t *testing.T) { _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.Equal(t, core.OpPhaseInitializing, phase) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -503,7 +503,7 @@ func TestRunOperationTransferMintWithInterface(t *testing.T) { _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.Equal(t, core.OpPhaseInitializing, phase) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -530,7 +530,7 @@ func TestRunOperationTransferBurn(t *testing.T) { _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.Equal(t, core.OpPhaseInitializing, phase) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mti.AssertExpectations(t) @@ -557,7 +557,7 @@ func TestRunOperationTransfer(t *testing.T) { _, phase, err := am.RunOperation(context.Background(), opTransfer(op, pool, transfer)) - assert.Equal(t, core.OpPhaseInitializing, phase) + assert.Equal(t, core.OpPhasePending, phase) assert.NoError(t, err) mti.AssertExpectations(t)