Mint burn tokens #266

dragos-rebegea · 2023-11-27T13:05:40Z

No description provided.

iulianpascalau

some linter issues that prevent the unit tests correct running:

cannot use &bridge.MultiversXClientStub{} (type *bridge.MultiversXClientStub) as type MultiversXClient in field value:

iulianpascalau · 2023-11-28T09:30:51Z

bridges/ethMultiversX/bridgeExecutor.go

@@ -18,6 +19,14 @@ const splits = 10

 const minRetries = 1

+type ArgListsBatch struct {


missing comment on exported items

iulianpascalau · 2023-11-28T09:31:33Z

bridges/ethMultiversX/bridgeExecutor.go

@@ -403,6 +412,7 @@ func (executor *bridgeExecutor) PerformActionOnMultiversX(ctx context.Context) e
 		return ErrNilBatch
 	}

+	// TODO: check mintBurn balances before performing the action


TODO will be implemented in next PR?

bridges/ethMultiversX/bridgeExecutor.go

clients/ethereum/client.go

iulianpascalau · 2023-11-28T10:02:23Z

clients/multiversx/mxClientDataGetter.go

+	builder := dataGetter.createDefaultVmQueryBuilder()
+	builder.Function(getAccumulatedBurnedTokensFuncName).ArgBytes(token)
+
+	return dataGetter.executeQueryUint64FromBuilder(ctx, builder)


I think uint64 can easily overflow if we deal with 18-digits tokens.

ExecuteQueryReturningBigInt created

sstanculeanu

no other comments

sasurobert

only those from Iulian. He made a good review.

… mint-burn-tokens # Conflicts: # cmd/bridge/config/config.toml # go.mod # go.sum # integrationTests/testscommon.go

sstanculeanu · 2023-12-20T15:04:45Z

bridges/ethMultiversX/bridgeExecutor_test.go

@@ -1196,6 +1204,9 @@ func TestMultiversXToEthBridgeExecutor_PerformTransferOnEthereum(t *testing.T) {
 	})
 }

+func TestMultiversXToEthBridgeExecutor_checkAvailableTokens(t *testing.T) {


sstanculeanu · 2023-12-20T15:09:53Z

bridges/ethMultiversX/bridgeExecutor_test.go

@@ -1655,3 +1666,16 @@ func TestBridgeExecutor_ValidateBatch(t *testing.T) {
 	assert.True(t, result)
 	assert.True(t, validateBatchCalled)
 }
+
+// CompareBytes compares two byte slices and returns true if they are equal.
+func CompareBytes(a, b []byte) bool {


bytes.Equal(b1, b2) instead of this? or simply string(b1) == string(b2) (which bytes.Equal also does)

sstanculeanu · 2023-12-20T15:20:59Z

clients/ethereum/client_test.go

 		assert.Equal(t, "0xc5b2c658f5fa236c598a6e7fbf7f21413dc42e2a41dd982eb772b30707cba2eb", hash)
 		assert.Nil(t, err)
 		assert.True(t, wasCalled)
 	})
 }

+func TestClient_CheckRequiredBalance(t *testing.T) {


t.Paralel() on the main test?

sstanculeanu · 2023-12-20T15:22:21Z

clients/ethereum/client_test.go

+// TokenMintedBalances
+// WhitelistedTokensMintBurn


Suggested change

// TokenMintedBalances

// WhitelistedTokensMintBurn

sstanculeanu · 2023-12-20T15:24:25Z

clients/ethereum/client_test.go

+
+// TokenMintedBalances
+// WhitelistedTokensMintBurn
+func TestClient_TokenMintedBalances(t *testing.T) {


t.Paralel() on main test + subtests?

sstanculeanu · 2023-12-20T15:25:01Z

clients/ethereum/client_test.go

+// WhitelistedTokensMintBurn
+func TestClient_TokenMintedBalances(t *testing.T) {
+	t.Run("error while getting token minted balances", func(t *testing.T) {
+		expectedErr := errors.New("expected error")


extract it globally and reuse the same expectedErr

in other context expectedErr has other value

sstanculeanu · 2023-12-20T15:25:52Z

clients/ethereum/client_test.go

+		args := createMockEthereumClientArgs()
+		args.ClientWrapper = &bridgeTests.EthereumClientWrapperStub{
+			TokenMintedBalancesCalled: func(ctx context.Context, token common.Address) (*big.Int, error) {
+				return big.NewInt(100), nil


extract providedBalance := big.NewInt(100) and check against it at the end of the test

sstanculeanu · 2023-12-20T15:26:16Z

clients/ethereum/client_test.go

+	})
+}
+
+func TestClient_WhitelistedTokensMintBurn(t *testing.T) {


t.Parallel() on main and subtests?

sstanculeanu · 2023-12-20T15:46:36Z

core/batchProcessor/batchProcessor.go

+	Nonces              []*big.Int
+}
+
+func ExtractList(batch *clients.TransferBatch) (*ArgListsBatch, error) {


missing comment

iulianpascalau · 2023-12-21T08:22:04Z

bridges/ethMultiversX/bridgeExecutor.go

@@ -3,6 +3,7 @@ package ethmultiversx
 import (
 	"context"
 	"fmt"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:25:36Z

bridges/ethMultiversX/bridgeExecutor.go

-	if mintedBalance.Cmp(burntBalanceBigInt) < 0 {
-		return fmt.Errorf("%w, minted: %s, burnt: %s for ERC20 token %s",
-			ErrMintBurnBalance, mintedBalance.String(), burntBalanceBigInt.String(), erc20Address.String())
+	if mintedBalance.Cmp(burntBalance) != 0 {


is this ok? When will the burn be completed? At the moment of transfer in the mvx safe contract or when the transfer is completed on eth side and marked as finished on mvx?

when the transfer is completed on eth side and marked as finished on mvx, thats why it should be equal at the moment of the batch execution

ok, should work then

clients/ethereum/client.go

iulianpascalau · 2023-12-21T08:30:23Z

bridges/ethMultiversX/bridgeExecutor.go

-		if !found {
-			existing = big.NewInt(0)
-			transfers[token] = existing
+		if _, exists := cumulatedAmounts[token]; exists {


The code reads worse in this changeset. How about this:

for i, token := range tokens { existingValue, exists := cumulatedAmounts[token] if exists { existingValue.Add(existingValue, amounts[i]) continue } cumulatedAmounts[token] = amounts[i] uniqueTokens = append(uniqueTokens, token) uniqueConvertedTokens = append(uniqueConvertedTokens, convertedTokens[i]) }

iulianpascalau · 2023-12-21T08:31:08Z

bridges/ethMultiversX/bridgeExecutor_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:33:03Z

bridges/ethMultiversX/bridgeExecutor_test.go

-				assert.True(t, providedBatch == batch)
+				assert.True(t, providedBatch.ID == batchId)
+				for i := 0; i < len(providedBatch.Deposits); i++ {
+					assert.True(t, providedBatch.Deposits[i].Amount == batch.Amounts[i])


can use assert.Equal instead of assert.True as they will be evaluated by value not pointer. Also, the test will show a clearer image of what is wrong with assert.Equal. Valid here and on all asserts.

assert.Equal used instead, also no need for extra convertion to string. good suggestion

iulianpascalau · 2023-12-21T08:33:12Z

bridges/ethMultiversX/interface.go

@@ -2,6 +2,7 @@ package ethmultiversx

 import (
 	"context"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:33:23Z

clients/ethereum/client.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/ecdsa"
 	"fmt"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:33:35Z

clients/ethereum/client_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:37:39Z

clients/multiversx/mxClientDataGetter.go

+func (dataGetter *mxClientDataGetter) ExecuteQueryReturningBigInt(ctx context.Context, request *data.VmValueRequest) (*big.Int, error) {
+	response, err := dataGetter.ExecuteQueryReturningBytes(ctx, request)
+	if err != nil {
+		return big.NewInt(0), err


iulianpascalau · 2023-12-21T08:38:58Z

testsCommon/bridge/ethereumClientStub.go

@@ -2,6 +2,7 @@ package bridge

 import (
 	"context"
+	"github.com/multiversx/mx-bridge-eth-go/core/batchProcessor"


iulianpascalau · 2023-12-21T08:39:27Z

testsCommon/bridge/multiversxClientStub.go

 	"github.com/multiversx/mx-bridge-eth-go/clients"
+	"math/big"


dragos-rebegea added 3 commits November 15, 2023 19:28

do the balance check on for non mintBurn tokens

6b6a315

Merge remote-tracking branch 'origin/main' into main

48c98ac

add mintBurn check for multiversx2ethereum

b190a3c

dragos-rebegea changed the base branch from main to feat/mint-burn November 28, 2023 08:00

iulianpascalau self-requested a review November 28, 2023 09:30

iulianpascalau reviewed Nov 28, 2023

View reviewed changes

sstanculeanu reviewed Nov 28, 2023

View reviewed changes

sasurobert reviewed Nov 28, 2023

View reviewed changes

dragos-rebegea added 4 commits December 4, 2023 19:27

update clients stub

3f245a3

update dependencies and update stubs

db6acdc

fixes after review

307e0db

fix linter

d16bb1c

dragos-rebegea changed the base branch from feat/mint-burn to feat/native-esdts-bridging December 20, 2023 14:30

dragos-rebegea added 2 commits December 20, 2023 16:39

Merge remote-tracking branch 'origin/feat/native-esdts-bridging' into…

dce9bdd

… mint-burn-tokens # Conflicts: # cmd/bridge/config/config.toml # go.mod # go.sum # integrationTests/testscommon.go

fixes after review

6b556f4

sstanculeanu reviewed Dec 20, 2023

View reviewed changes

fixes after review

d6580c8

iulianpascalau reviewed Dec 21, 2023

View reviewed changes

fixes after review

a8c7e8c

iulianpascalau approved these changes Dec 21, 2023

View reviewed changes

sstanculeanu approved these changes Dec 22, 2023

View reviewed changes

sasurobert approved these changes Dec 22, 2023

View reviewed changes

iulianpascalau merged commit 0267f24 into feat/native-esdts-bridging Dec 22, 2023
3 checks passed

iulianpascalau deleted the mint-burn-tokens branch December 22, 2023 11:23

		@@ -18,6 +19,14 @@ const splits = 10

		const minRetries = 1

		type ArgListsBatch struct {

Mint burn tokens #266

Mint burn tokens #266

Conversation

dragos-rebegea commented Nov 27, 2023

iulianpascalau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu left a comment

Choose a reason for hiding this comment

sasurobert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment