Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mx 15181 #292

Closed
wants to merge 4 commits into from
Closed

Mx 15181 #292

wants to merge 4 commits into from

Conversation

dragos-rebegea
Copy link
Contributor

No description provided.

@iulianpascalau iulianpascalau self-requested a review March 8, 2024 07:31
isNativeOnMultiversX := executor.isNativeOnMultiversX(ctx, convertedToken)

if isNativeOnEthereum && isNativeOnMultiversX {
return fmt.Errorf("%w isNativeOnEthereum = %v, isNativeOnMultiversX = %v", ErrInvalidSetup, isNativeOnEthereum, isNativeOnMultiversX)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return executor.ethereumClient.CheckRequiredBalance(ctx, token, amount)
}
// transfer to MultiversX
return executor.multiversXClient.CheckRequiredBalance(ctx, convertedToken, amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from linter:

return executor.multiversXClient.CheckRequiredBalance(ctx, convertedToken, amount)
Check failure on line 626 in bridges/ethMultiversX/bridgeExecutor.go

GitHub
Actions
/ Unit
executor.multiversXClient.CheckRequiredBalance undefined (type MultiversXClient has no field or method CheckRequiredBalance)


burnBalances, err := executor.ethereumClient.BurnBalances(ctx, token)
if err != nil {
return big.NewInt(0), err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return nil, err ? Here and on L640

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
burnBalances, err := executor.multiversXClient.BurnBalances(ctx, token)
if err != nil {
return big.NewInt(0), err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return nil, err ? Here and on L659

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

func (executor *bridgeExecutor) isNativeOnEthereum(ctx context.Context, erc20Address common.Address) bool {
isNative, err := executor.ethereumClient.NativeTokens(ctx, erc20Address)
if err != nil {
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this ok to assume here that the erc20 address is not native in case of an error? Also on isMintBurnOnMultiversX function. I think we should stop the further processing in case of an error as to not force the nodes to work in an undefined way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, refactored to return also error

@@ -29,6 +29,7 @@ func ExtractListMvxToEth(batch *clients.TransferBatch) *ArgListsBatch {
arg.EthTokens = append(arg.EthTokens, token)

amount := big.NewInt(0).Set(dt.Amount)
amount = big.NewInt(0).Neg(amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored

Comment on lines 588 to 593
if isNativeOnEthereum && isNativeOnMultiversX {
return fmt.Errorf("%w isNativeOnEthereum = %v, isNativeOnMultiversX = %v", ErrInvalidSetup, isNativeOnEthereum, isNativeOnMultiversX)
}
return isMintBurnOnEthereum, nil
}

func (executor *bridgeExecutor) checkRequiredMintBurnBalance(ctx context.Context, ethToken common.Address, mvxToken []byte) error {
ethBalance, err := executor.ethereumClient.TokenMintedBalances(ctx, ethToken)
if err != nil {
return err
if !isMintBurnOnEthereum && !isMintBurnOnMultiversX {
return fmt.Errorf("%w isMintBurnOnEthereum = %v, isMintBurnOnMultiversX = %v", ErrInvalidSetup, isMintBurnOnEthereum, isMintBurnOnMultiversX)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be reduced to one if if isMintBurnOnEthereum == isMintBurnOnMultiversX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one cannot be reduce to that because it is allowed to have both as mintBurn tokens

return big.NewInt(0), err
}

if isNative {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also check if burnBalances >= mintBalances? similar for L64, L662, L664

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@dragos-rebegea dragos-rebegea changed the base branch from feat/chain-simulator to feat-v3-arm64 March 11, 2024 16:03
@dragos-rebegea dragos-rebegea changed the base branch from feat-v3-arm64 to feat/v3-arm64 March 11, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants