Merge pull request #163 from multiversx/fixed-nonce-transaction-handl…

…er-v2

Fixed nonce transaction handler v2

blockchain/addressGenerator.go

@@ -2,6 +2,7 @@ package blockchain
 
 import (
 	"bytes"
+
 	mxChainCore "github.com/multiversx/mx-chain-core-go/core"
 	"github.com/multiversx/mx-chain-core-go/core/check"
 	"github.com/multiversx/mx-chain-core-go/data/typeConverters/uint64ByteSlice"
@@ -89,9 +90,9 @@ func (ag *addressGenerator) CompatibleDNSAddressFromUsername(username string) (c
 	return ag.CompatibleDNSAddress(lastByte)
 }
 
-// ComputeArwenScAddress will return the smart contract address that will be generated by the Arwen VM providing
+// ComputeWasmVMScAddress will return the smart contract address that will be generated by the Wasm VM providing
 // the owner's address & nonce
-func (ag *addressGenerator) ComputeArwenScAddress(address core.AddressHandler, nonce uint64) (core.AddressHandler, error) {
+func (ag *addressGenerator) ComputeWasmVMScAddress(address core.AddressHandler, nonce uint64) (core.AddressHandler, error) {
 	if check.IfNil(address) {
 		return nil, ErrNilAddress
 	}

blockchain/addressGenerator_test.go

@@ -54,7 +54,7 @@ func TestGenerateSameDNSAddress(t *testing.T) {
 	assert.Equal(t, "erd1qqqqqqqqqqqqqpgqvrsdh798pvd4x09x0argyscxc9h7lzfhqz4sttlatg", newDnsAsBech32)
 }
 
-func TestAddressGenerator_ComputeArwenScAddress(t *testing.T) {
+func TestAddressGenerator_ComputeWasmVMScAddress(t *testing.T) {
 	t.Parallel()
 
 	coord, err := NewShardCoordinator(3, 0)
@@ -65,7 +65,7 @@ func TestAddressGenerator_ComputeArwenScAddress(t *testing.T) {
 	owner, err := data.NewAddressFromBech32String("erd1dglncxk6sl9a3xumj78n6z2xux4ghp5c92cstv5zsn56tjgtdwpsk46qrs")
 	require.Nil(t, err)
 
-	scAddress, err := ag.ComputeArwenScAddress(owner, 10)
+	scAddress, err := ag.ComputeWasmVMScAddress(owner, 10)
 	require.Nil(t, err)
 
 	scAddressAsBech32, err := scAddress.AddressAsBech32String()

interactors/errors.go

@@ -29,8 +29,8 @@ var ErrNilTransaction = errors.New("nil transaction")
 // ErrTxAlreadySent signals that a transaction was already sent
 var ErrTxAlreadySent = errors.New("transaction already sent")
 
+// ErrTxWithSameNonceAndGasPriceAlreadySent signals that a transaction with the same nonce & gas price was already sent
+var ErrTxWithSameNonceAndGasPriceAlreadySent = errors.New("transaction with the same nonce & gas price was already sent")
+
 // ErrGapNonce signals that a gap nonce between the lowest nonce of the transactions from the cache and the blockchain nonce has been detected
 var ErrGapNonce = errors.New("gap nonce detected")
-
-// ErrNilAddressNonceHandlerCreator signals that a nil AddressNonceHandlerCreator was provided
-var ErrNilAddressNonceHandlerCreator = errors.New("nil AddressNonceHandlerCreator")

interactors/interface.go

@@ -56,9 +56,3 @@ type TransactionNonceHandlerV2 interface {
 	Close() error
 	IsInterfaceNil() bool
 }
-
-// AddressNonceHandlerCreator defines the component able to create AddressNonceHandler instances
-type AddressNonceHandlerCreator interface {
-	Create(proxy Proxy, address core.AddressHandler) (AddressNonceHandler, error)
-	IsInterfaceNil() bool
-}

interactors/nonceHandlerV2/addressNonceHandler.go

@@ -1,7 +1,6 @@
 package nonceHandlerV2
 
 import (
-	"bytes"
 	"context"
 	"sync"
 
@@ -12,9 +11,6 @@ import (
 	"github.com/multiversx/mx-sdk-go/interactors"
 )
 
-//TODO EN-13182: create a baseAddressNonceHandler component that can remove the duplicate code as much as possible from the
-// addressNonceHandler and singleTransactionAddressNonceHandler
-
 // addressNonceHandler is the handler used for one address. It is able to handle the current
 // nonce as max(current_stored_nonce, account_nonce). After each call of the getNonce function
 // the current_stored_nonce is incremented. This will prevent "nonce too low in transaction"
@@ -51,9 +47,9 @@ func NewAddressNonceHandler(proxy interactors.Proxy, address sdkCore.AddressHand
 
 // ApplyNonceAndGasPrice will apply the computed nonce to the given FrontendTransaction
 func (anh *addressNonceHandler) ApplyNonceAndGasPrice(ctx context.Context, tx *transaction.FrontendTransaction) error {
-	oldTx, alreadyExists := anh.isTxAlreadySent(tx)
-	if alreadyExists {
-		err := anh.handleTxAlreadyExists(oldTx, tx)
+	oldTx := anh.getOlderTxWithSameNonce(tx)
+	if oldTx != nil {
+		err := anh.handleTxWithSameNonce(oldTx, tx)
 		if err != nil {
 			return err
 		}
@@ -70,7 +66,7 @@ func (anh *addressNonceHandler) ApplyNonceAndGasPrice(ctx context.Context, tx *t
 	return nil
 }
 
-func (anh *addressNonceHandler) handleTxAlreadyExists(oldTx *transaction.FrontendTransaction, tx *transaction.FrontendTransaction) error {
+func (anh *addressNonceHandler) handleTxWithSameNonce(oldTx *transaction.FrontendTransaction, tx *transaction.FrontendTransaction) error {
 	if oldTx.GasPrice < tx.GasPrice {
 		return nil
 	}
@@ -79,7 +75,7 @@ func (anh *addressNonceHandler) handleTxAlreadyExists(oldTx *transaction.Fronten
 		return nil
 	}
 
-	return interactors.ErrTxAlreadySent
+	return interactors.ErrTxWithSameNonceAndGasPriceAlreadySent
 }
 
 func (anh *addressNonceHandler) fetchGasPriceIfRequired(ctx context.Context, nonce uint64) {
@@ -189,18 +185,11 @@ func (anh *addressNonceHandler) DropTransactions() {
 	anh.mut.Unlock()
 }
 
-func (anh *addressNonceHandler) isTxAlreadySent(tx *transaction.FrontendTransaction) (*transaction.FrontendTransaction, bool) {
+func (anh *addressNonceHandler) getOlderTxWithSameNonce(tx *transaction.FrontendTransaction) *transaction.FrontendTransaction {
 	anh.mut.RLock()
 	defer anh.mut.RUnlock()
-	for _, oldTx := range anh.transactions {
-		isTheSameReceiverDataValue := oldTx.Receiver == tx.Receiver &&
-			bytes.Equal(oldTx.Data, tx.Data) &&
-			oldTx.Value == tx.Value
-		if isTheSameReceiverDataValue {
-			return oldTx, true
-		}
-	}
-	return nil, false
+
+	return anh.transactions[tx.Nonce]
 }
 
 // IsInterfaceNil returns true if there is no value under the interface

interactors/nonceHandlerV2/addressNonceHandler_test.go

@@ -58,12 +58,15 @@ func TestAddressNonceHandler_ApplyNonceAndGasPrice(t *testing.T) {
 		anh, err := NewAddressNonceHandlerWithPrivateAccess(&testsCommon.ProxyStub{}, testAddress)
 		require.Nil(t, err)
 
+		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
+		require.Nil(t, err)
+
 		_, err = anh.SendTransaction(context.Background(), &tx)
 		require.Nil(t, err)
 
 		anh.gasPrice = tx.GasPrice
 		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
-		require.Equal(t, interactors.ErrTxAlreadySent, err)
+		require.Equal(t, interactors.ErrTxWithSameNonceAndGasPriceAlreadySent, err)
 	})
 	t.Run("tx already sent; oldTx.GasPrice < txArgs.GasPrice", func(t *testing.T) {
 		t.Parallel()
@@ -75,12 +78,18 @@ func TestAddressNonceHandler_ApplyNonceAndGasPrice(t *testing.T) {
 		anh, err := NewAddressNonceHandlerWithPrivateAccess(&testsCommon.ProxyStub{}, testAddress)
 		require.Nil(t, err)
 
+		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
+		require.Nil(t, err)
+
 		_, err = anh.SendTransaction(context.Background(), &tx)
 		require.Nil(t, err)
 
 		anh.gasPrice = initialGasPrice
 		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
 		require.Nil(t, err)
+
+		_, err = anh.SendTransaction(context.Background(), &tx)
+		require.Nil(t, err)
 	})
 	t.Run("oldTx.GasPrice == txArgs.GasPrice && oldTx.GasPrice < anh.gasPrice", func(t *testing.T) {
 		t.Parallel()
@@ -89,12 +98,40 @@ func TestAddressNonceHandler_ApplyNonceAndGasPrice(t *testing.T) {
 		anh, err := NewAddressNonceHandlerWithPrivateAccess(&testsCommon.ProxyStub{}, testAddress)
 		require.Nil(t, err)
 
+		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
+		require.Nil(t, err)
+
 		_, err = anh.SendTransaction(context.Background(), &tx)
 		require.Nil(t, err)
 
 		anh.gasPrice = tx.GasPrice + 1
 		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx)
 		require.Nil(t, err)
+
+		_, err = anh.SendTransaction(context.Background(), &tx)
+		require.Nil(t, err)
+	})
+	t.Run("same transaction but with different nonce should work", func(t *testing.T) {
+		t.Parallel()
+
+		tx1 := createDefaultTx()
+		tx2 := createDefaultTx()
+		tx2.Nonce++
+
+		anh, err := NewAddressNonceHandlerWithPrivateAccess(&testsCommon.ProxyStub{}, testAddress)
+		require.Nil(t, err)
+
+		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx1)
+		require.Nil(t, err)
+
+		_, err = anh.SendTransaction(context.Background(), &tx1)
+		require.Nil(t, err)
+
+		err = anh.ApplyNonceAndGasPrice(context.Background(), &tx2)
+		require.Nil(t, err)
+
+		_, err = anh.SendTransaction(context.Background(), &tx2)
+		require.Nil(t, err)
 	})
 }
 

interactors/nonceHandlerV2/nonceTransactionsHandler.go

@@ -22,7 +22,6 @@ var log = logger.GetOrCreate("mx-sdk-go/interactors/nonceHandlerV2")
 type ArgsNonceTransactionsHandlerV2 struct {
 	Proxy            interactors.Proxy
 	IntervalToResend time.Duration
-	Creator          interactors.AddressNonceHandlerCreator
 }
 
 // nonceTransactionsHandlerV2 is the handler used for an unlimited number of addresses.
@@ -36,7 +35,6 @@ type ArgsNonceTransactionsHandlerV2 struct {
 type nonceTransactionsHandlerV2 struct {
 	proxy            interactors.Proxy
 	mutHandlers      sync.RWMutex
-	creator          interactors.AddressNonceHandlerCreator
 	handlers         map[string]interactors.AddressNonceHandler
 	cancelFunc       func()
 	intervalToResend time.Duration
@@ -51,15 +49,11 @@ func NewNonceTransactionHandlerV2(args ArgsNonceTransactionsHandlerV2) (*nonceTr
 	if args.IntervalToResend < minimumIntervalToResend {
 		return nil, fmt.Errorf("%w for intervalToResend in NewNonceTransactionHandlerV2", interactors.ErrInvalidValue)
 	}
-	if check.IfNil(args.Creator) {
-		return nil, interactors.ErrNilAddressNonceHandlerCreator
-	}
 
 	nth := &nonceTransactionsHandlerV2{
 		proxy:            args.Proxy,
 		handlers:         make(map[string]interactors.AddressNonceHandler),
 		intervalToResend: args.IntervalToResend,
-		creator:          args.Creator,
 	}
 
 	ctx, cancelFunc := context.WithCancel(context.Background())
@@ -115,7 +109,8 @@ func (nth *nonceTransactionsHandlerV2) createAddressNonceHandler(address core.Ad
 	if found {
 		return anh, nil
 	}
-	anh, err := nth.creator.Create(nth.proxy, address)
+
+	anh, err := NewAddressNonceHandler(nth.proxy, address)
 	if err != nil {
 		return nil, err
 	}
@@ -130,7 +125,11 @@ func (nth *nonceTransactionsHandlerV2) SendTransaction(ctx context.Context, tx *
 		return "", interactors.ErrNilTransaction
 	}
 
-	addrAsBech32 := tx.Sender
+	// Work with a full copy of the provided transaction so the provided one can change without affecting this component.
+	// Abnormal and unpredictable behaviors due to the resending mechanism are prevented this way
+	txCopy := *tx
+
+	addrAsBech32 := txCopy.Sender
 	address, err := data.NewAddressFromBech32String(addrAsBech32)
 	if err != nil {
 		return "", fmt.Errorf("%w while creating address handler for string %s", err, addrAsBech32)
@@ -141,7 +140,7 @@ func (nth *nonceTransactionsHandlerV2) SendTransaction(ctx context.Context, tx *
 		return "", err
 	}
 
-	sentHash, err := anh.SendTransaction(ctx, tx)
+	sentHash, err := anh.SendTransaction(ctx, &txCopy)
 	if err != nil {
 		return "", fmt.Errorf("%w while sending transaction for address %s", err, addrAsBech32)
 	}

interactors/nonceHandlerV2/nonceTransactionsHandler_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/multiversx/mx-sdk-go/data"
 	"github.com/multiversx/mx-sdk-go/interactors"
 	"github.com/multiversx/mx-sdk-go/testsCommon"
-	testsInteractors "github.com/multiversx/mx-sdk-go/testsCommon/interactors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -30,15 +29,6 @@ func TestNewNonceTransactionHandlerV2(t *testing.T) {
 		require.Nil(t, nth)
 		assert.Equal(t, interactors.ErrNilProxy, err)
 	})
-	t.Run("nil AddressNonceHandlerCreator", func(t *testing.T) {
-		t.Parallel()
-
-		args := createMockArgsNonceTransactionsHandlerV2()
-		args.Creator = nil
-		nth, err := NewNonceTransactionHandlerV2(args)
-		require.Nil(t, nth)
-		assert.Equal(t, interactors.ErrNilAddressNonceHandlerCreator, err)
-	})
 	t.Run("should work", func(t *testing.T) {
 		t.Parallel()
 
@@ -375,7 +365,8 @@ func TestNonceTransactionsHandlerV2_SendTransactionsWithGetNonce(t *testing.T) {
 }
 
 func TestNonceTransactionsHandlerV2_SendDuplicateTransactions(t *testing.T) {
-	currentNonce := uint64(664)
+	initialNonce := uint64(664)
+	currentNonce := initialNonce
 
 	numCalls := 0
 
@@ -393,7 +384,7 @@ func TestNonceTransactionsHandlerV2_SendDuplicateTransactions(t *testing.T) {
 		},
 		SendTransactionCalled: func(tx *transaction.FrontendTransaction) (string, error) {
 			require.LessOrEqual(t, numCalls, 1)
-			currentNonce++
+			atomic.AddUint64(&currentNonce, 1)
 			return "", nil
 		},
 	}
@@ -419,15 +410,15 @@ func TestNonceTransactionsHandlerV2_SendDuplicateTransactions(t *testing.T) {
 	require.True(t, ok)
 
 	// after sending first tx, nonce shall increase
-	require.Equal(t, accWithPrivateAccess.computedNonce+1, currentNonce)
+	require.Equal(t, atomic.LoadUint64(&currentNonce), accWithPrivateAccess.computedNonce+1)
 
 	// trying to apply nonce for the same tx, NonceTransactionHandler shall return ErrTxAlreadySent
 	// and computedNonce shall not increase
-	tx.Nonce = 0
+	tx.Nonce = initialNonce
 	err = nth.ApplyNonceAndGasPrice(context.Background(), testAddress, tx)
-	require.Equal(t, err, interactors.ErrTxAlreadySent)
-	require.Equal(t, tx.Nonce, uint64(0))
-	require.Equal(t, accWithPrivateAccess.computedNonce+1, currentNonce)
+	require.Equal(t, interactors.ErrTxWithSameNonceAndGasPriceAlreadySent, err)
+	require.Equal(t, initialNonce, tx.Nonce)
+	require.Equal(t, currentNonce, accWithPrivateAccess.computedNonce+1)
 }
 
 func createMockTransactionsWithGetNonce(
@@ -500,10 +491,5 @@ func createMockArgsNonceTransactionsHandlerV2() ArgsNonceTransactionsHandlerV2 {
 	return ArgsNonceTransactionsHandlerV2{
 		Proxy:            &testsCommon.ProxyStub{},
 		IntervalToResend: time.Second * 2,
-		Creator: &testsInteractors.AddressNonceHandlerCreatorStub{
-			CreateCalled: func(proxy interactors.Proxy, address core.AddressHandler) (interactors.AddressNonceHandler, error) {
-				return NewAddressNonceHandler(proxy, address)
-			},
-		},
 	}
 }