Merge pull request #448 from multiversx/execute-transfer-fix

Execute transfer fix

Author: cosmatudor

clients/sui/client.go

@@ -6,6 +6,7 @@ import (
 	"encoding/base64"
 	"fmt"
 	"math/big"
+	"sort"
 	"sync"
 
 	"github.com/block-vision/sui-go-sdk/models"
@@ -264,8 +265,13 @@ func (c *client) GenerateMessageHash(batch *batchProcessor.ArgListsBatch, batchI
 	}
 
 	var hash []byte
-	groups := c.groupTransfersByTokenType(batch)
-	for _, group := range groups {
+	groups, sortedTokenTypes, err := c.groupTransfersByTokenType(batch)
+	if err != nil {
+		return nil, fmt.Errorf("error grouping transfers by token type: %v", err)
+	}
+
+	for _, tokenType := range sortedTokenTypes {
+		group := groups[tokenType]
 		groupHash, err := c.getHashForTokenGroupData(group, batchId)
 		if err != nil {
 			return nil, fmt.Errorf("error getting hash for token group data: %v", err)
@@ -365,9 +371,12 @@ func (c *client) ExecuteTransfer(
 		serializedSignatures = serializedSignatures[:quorum]
 	}
 
-	tokenGroups := c.groupTransfersByTokenType(argLists)
+	groups, sortedTokenTypes, err := c.groupTransfersByTokenType(argLists)
+	if err != nil {
+		return "", fmt.Errorf("error grouping transfers by token type: %v", err)
+	}
 
-	err = c.processSignaturesOfRelayers(tokenGroups, serializedSignatures)
+	err = c.processSignaturesOfRelayers(groups, sortedTokenTypes, serializedSignatures)
 	if err != nil {
 		return "", err
 	}
@@ -377,15 +386,15 @@ func (c *client) ExecuteTransfer(
 		return "", err
 	}
 
-	calls, err := c.prepareExecuteTransferCallArgs(batchID, tokenGroups)
+	calls, err := c.prepareExecuteTransferCallArgs(batchID, groups, sortedTokenTypes)
 	if err != nil {
 		return "", err
 	}
 
 	return c.txHandler.SendTransactionReturnHash(ctx, gasCoin, calls)
 }
 
-func (c *client) groupTransfersByTokenType(argLists *batchProcessor.ArgListsBatch) map[string]*TokenTransferGroup {
+func (c *client) groupTransfersByTokenType(argLists *batchProcessor.ArgListsBatch) (map[string]*TokenTransferGroup, []string, error) {
 	groups := make(map[string]*TokenTransferGroup)
 
 	for i := 0; i < len(argLists.PeerTokens); i++ {
@@ -403,7 +412,7 @@ func (c *client) groupTransfersByTokenType(argLists *batchProcessor.ArgListsBatc
 		suiAddress := suiAddressFromBytes(argLists.Recipients[i])
 		suiAddressBytes, err := transaction.ConvertSuiAddressStringToBytes(models.SuiAddress(suiAddress))
 		if err != nil {
-			return nil
+			return nil, nil, err
 		}
 
 		groups[tokenTypeStr].Recipients = append(groups[tokenTypeStr].Recipients, *suiAddressBytes)
@@ -412,15 +421,17 @@ func (c *client) groupTransfersByTokenType(argLists *batchProcessor.ArgListsBatc
 		groups[tokenTypeStr].Nonces = append(groups[tokenTypeStr].Nonces, argLists.Nonces[i].Uint64())
 	}
 
-	return groups
-}
-
-func (c *client) processSignaturesOfRelayers(tokenGroups map[string]*TokenTransferGroup, serializedSignatures [][]byte) error {
-	var tokenTypes []string
-	for key := range tokenGroups {
+	// Sort token types for deterministic ordering
+	tokenTypes := make([]string, 0, len(groups))
+	for key := range groups {
 		tokenTypes = append(tokenTypes, key)
 	}
+	sort.Strings(tokenTypes)
+
+	return groups, tokenTypes, nil
+}
 
+func (c *client) processSignaturesOfRelayers(tokenGroups map[string]*TokenTransferGroup, sortedTokenTypes []string, serializedSignatures [][]byte) error {
 	for _, serializedSigsOfRelayer := range serializedSignatures {
 		n := len(serializedSigsOfRelayer) / EncodedSignatureLength
 		for i := 0; i < n; i++ {
@@ -434,7 +445,7 @@ func (c *client) processSignaturesOfRelayers(tokenGroups map[string]*TokenTransf
 			}
 
 			sig := [signatureLength]byte(_bytes[signatureSchemePrefixSize:]) // remove the signature scheme byte
-			tokenGroups[tokenTypes[i]].Signatures = append(tokenGroups[tokenTypes[i]].Signatures, sig)
+			tokenGroups[sortedTokenTypes[i]].Signatures = append(tokenGroups[sortedTokenTypes[i]].Signatures, sig)
 		}
 	}
 
@@ -459,25 +470,24 @@ func (c *client) getGasCoinOfRelayer(ctx context.Context) (*transaction.SuiObjec
 	)
 }
 
-func (c *client) prepareExecuteTransferCallArgs(batchID uint64, tokenGroups map[string]*TokenTransferGroup) ([]bridgeCore.SuiPTBOperation, error) {
-	i := 0
+func (c *client) prepareExecuteTransferCallArgs(batchID uint64, tokenGroups map[string]*TokenTransferGroup, sortedTokenTypes []string) ([]bridgeCore.SuiPTBOperation, error) {
 	n := len(tokenGroups)
 	calls := make([]bridgeCore.SuiPTBOperation, 0, n)
-	for coinType, group := range tokenGroups {
-		i++
-		isBatchComplete := i == n
+	for i, tokenType := range sortedTokenTypes {
+		group := tokenGroups[tokenType]
+		isBatchComplete := i == n-1
 
 		localGroup := group
 		localIsBatchComplete := isBatchComplete
 
-		coinParts, err := parseCoinType(coinType)
+		coinParts, err := parseCoinType(tokenType)
 		if err != nil {
-			return nil, fmt.Errorf("failed to parse coin type %s: %w", coinType, err)
+			return nil, fmt.Errorf("failed to parse coin type %s: %w", tokenType, err)
 		}
 
 		coinIdBytes, err := transaction.ConvertSuiAddressStringToBytes(models.SuiAddress(coinParts[0]))
 		if err != nil {
-			return nil, fmt.Errorf("failed to convert coin type %s: %w", coinType, err)
+			return nil, fmt.Errorf("failed to convert coin type %s: %w", tokenType, err)
 		}
 
 		coinIdBytesVal := *coinIdBytes

clients/sui/client_test.go

@@ -775,45 +775,94 @@ func TestClient_GenerateMessageHash(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, expectedHash, hash)
 	})
-	t.Run("should return correct hash for batch with deposits with different tokens", func(t *testing.T) {
+	t.Run("should return same hash for batches with same deposits in different order", func(t *testing.T) {
 		args := createMockSuiClientArgs()
 		c, _ := NewSuiClient(args)
 
-		batch := &batchProcessor.ArgListsBatch{
+		batchID := uint64(456)
+		batch1 := &batchProcessor.ArgListsBatch{
 			PeerTokens:    [][]byte{[]byte("token1"), []byte("token2")},
 			Recipients:    [][]byte{[]byte("recipient1"), []byte("recipient2")},
 			MvxTokenBytes: [][]byte{[]byte("mvxToken1"), []byte("mvxToken2")},
 			Amounts:       []*big.Int{big.NewInt(900), big.NewInt(560)},
 			Nonces:        []*big.Int{big.NewInt(640), big.NewInt(310)},
 			Direction:     batchProcessor.FromMultiversX,
 		}
-		batchID := uint64(456)
+		batch2 := &batchProcessor.ArgListsBatch{
+			PeerTokens:    [][]byte{[]byte("token2"), []byte("token1")},
+			Recipients:    [][]byte{[]byte("recipient2"), []byte("recipient1")},
+			MvxTokenBytes: [][]byte{[]byte("mvxToken2"), []byte("mvxToken1")},
+			Amounts:       []*big.Int{big.NewInt(560), big.NewInt(900)},
+			Nonces:        []*big.Int{big.NewInt(310), big.NewInt(640)},
+			Direction:     batchProcessor.FromMultiversX,
+		}
 
-		suiAddress1 := suiAddressFromBytes(batch.Recipients[0])
+		hash1, err := c.GenerateMessageHash(batch1, batchID)
+		assert.NoError(t, err)
+		assert.NotNil(t, hash1)
+
+		hash2, err := c.GenerateMessageHash(batch2, batchID)
+		assert.NoError(t, err)
+		assert.NotNil(t, hash2)
+
+		assert.True(t, bytes.Equal(hash1, hash2))
+
+		suiAddress1 := suiAddressFromBytes(batch1.Recipients[0])
 		suiAddressBytes1, _ := transaction.ConvertSuiAddressStringToBytes(models.SuiAddress(suiAddress1))
 
-		suiAddress2 := suiAddressFromBytes(batch.Recipients[1])
+		suiAddress2 := suiAddressFromBytes(batch1.Recipients[1])
 		suiAddressBytes2, _ := transaction.ConvertSuiAddressStringToBytes(models.SuiAddress(suiAddress2))
 
+		// we use batch1 data to generate expected hashes because is ordered correctly
 		expectedHashForToken1 := generateHashForTokenGroup(t, batchID, &TokenTransferGroup{
-			Tokens:     [][]byte{batch.PeerTokens[0]},
+			Tokens:     [][]byte{batch1.PeerTokens[0]},
 			Recipients: []models.SuiAddressBytes{*suiAddressBytes1},
-			Amounts:    []uint64{batch.Amounts[0].Uint64()},
-			Nonces:     []uint64{batch.Nonces[0].Uint64()},
+			Amounts:    []uint64{batch1.Amounts[0].Uint64()},
+			Nonces:     []uint64{batch1.Nonces[0].Uint64()},
 		})
 
 		expectedHashForToken2 := generateHashForTokenGroup(t, batchID, &TokenTransferGroup{
-			Tokens:     [][]byte{batch.PeerTokens[1]},
+			Tokens:     [][]byte{batch1.PeerTokens[1]},
 			Recipients: []models.SuiAddressBytes{*suiAddressBytes2},
-			Amounts:    []uint64{batch.Amounts[1].Uint64()},
-			Nonces:     []uint64{batch.Nonces[1].Uint64()},
+			Amounts:    []uint64{batch1.Amounts[1].Uint64()},
+			Nonces:     []uint64{batch1.Nonces[1].Uint64()},
 		})
 
-		hash, err := c.GenerateMessageHash(batch, batchID)
-		assert.NoError(t, err)
-		assert.True(t, bytes.Contains(hash, expectedHashForToken1))
-		assert.True(t, bytes.Contains(hash, expectedHashForToken2))
-		assert.Equal(t, len(expectedHashForToken1)+len(expectedHashForToken2), len(hash))
+		assert.True(t, bytes.Equal(expectedHashForToken1, hash1[:32]))
+		assert.True(t, bytes.Equal(expectedHashForToken2, hash1[32:]))
+	})
+	t.Run("should be deterministic over multiple invocations", func(t *testing.T) {
+		args := createMockSuiClientArgs()
+		c, _ := NewSuiClient(args)
+
+		batch := &batchProcessor.ArgListsBatch{
+			PeerTokens:    [][]byte{[]byte("token1"), []byte("token2")},
+			Recipients:    [][]byte{[]byte("recipient1"), []byte("recipient2")},
+			MvxTokenBytes: [][]byte{[]byte("mvxToken1"), []byte("mvxToken2")},
+			Amounts:       []*big.Int{big.NewInt(900), big.NewInt(560)},
+			Nonces:        []*big.Int{big.NewInt(640), big.NewInt(310)},
+			Direction:     batchProcessor.FromMultiversX,
+		}
+		batchID := uint64(456)
+
+		const iterations = 100
+		var hashes [][]byte
+
+		for i := 0; i < iterations; i++ {
+			hash, err := c.GenerateMessageHash(batch, batchID)
+			assert.NoError(t, err, "iteration %d failed", i)
+			assert.NotNil(t, hash)
+			assert.NotEmpty(t, hash)
+
+			hashes = append(hashes, hash)
+		}
+
+		referenceHash := hashes[0]
+		for i := 1; i < len(hashes); i++ {
+			assert.True(t, bytes.Equal(referenceHash, hashes[i]),
+				"Hash mismatch at iteration %d.\nExpected: %x\nGot: %x",
+				i, referenceHash, hashes[i])
+		}
 	})
 }