add reviewed changes

precompiles/bank/method_deposit.go

@@ -100,7 +100,7 @@ func (c *Contract) deposit(
 	}
 
 	// 2. Effect: subtract balance.
-	if err := c.fungibleKeeper.LockZRC20(ctx, c.zrc20ABI, zrc20Addr, caller, c.Address(), amount); err != nil {
+	if err := c.fungibleKeeper.LockZRC20(ctx, c.zrc20ABI, zrc20Addr, c.Address(), caller, c.Address(), amount); err != nil {
 		return nil, &ptypes.ErrUnexpected{
 			When: "LockZRC20InBank",
 			Got:  err.Error(),

precompiles/bank/method_test.go

@@ -135,7 +135,7 @@ func Test_Methods(t *testing.T) {
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "invalid allowance, got: 0")
+		require.Contains(t, err.Error(), "invalid allowance, got 0")
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)
@@ -194,7 +194,11 @@ func Test_Methods(t *testing.T) {
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "unexpected error in LockZRC20InBank: invalid allowance, got: 500")
+		require.Contains(
+			t,
+			err.Error(),
+			"unexpected error in LockZRC20InBank: failed allowance check: invalid allowance, got 500, wanted 501",
+		)
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)

x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go

@@ -38,28 +38,28 @@ func Test_LockZRC20(t *testing.T) {
 
 	t.Run("should fail when trying to lock zero amount", func(t *testing.T) {
 		// Check lock with zero amount.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, big.NewInt(0))
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, locker, owner, locker, big.NewInt(0))
 		require.Error(t, err)
 		require.ErrorIs(t, err, fungibletypes.ErrInvalidAmount)
 	})
 
 	t.Run("should fail when ZRC20 ABI is not properly initialized", func(t *testing.T) {
 		// Check lock with nil ABI.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, nil, ts.zrc20Address, owner, locker, big.NewInt(10))
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, nil, ts.zrc20Address, locker, owner, locker, big.NewInt(10))
 		require.Error(t, err)
 		require.ErrorIs(t, err, fungibletypes.ErrZRC20NilABI)
 	})
 
 	t.Run("should fail when trying to lock a zero address ZRC20", func(t *testing.T) {
 		// Check lock with ZRC20 zero address.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, common.Address{}, owner, locker, big.NewInt(10))
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, common.Address{}, locker, owner, locker, big.NewInt(10))
 		require.Error(t, err)
 		require.ErrorIs(t, err, fungibletypes.ErrZRC20ZeroAddress)
 	})
 
 	t.Run("should fail when trying to lock a non whitelisted ZRC20", func(t *testing.T) {
 		// Check lock with non whitelisted ZRC20.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, sample.EthAddress(), owner, locker, big.NewInt(10))
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, sample.EthAddress(), locker, owner, locker, big.NewInt(10))
 		require.Error(t, err)
 		require.ErrorIs(t, err, fungibletypes.ErrZRC20NotWhiteListed)
 	})
@@ -72,6 +72,7 @@ func Test_LockZRC20(t *testing.T) {
 			ts.ctx,
 			zrc20ABI,
 			ts.zrc20Address,
+			locker,
 			owner,
 			locker,
 			big.NewInt(1000000000000000),
@@ -84,7 +85,7 @@ func Test_LockZRC20(t *testing.T) {
 		approveAllowance(t, ts, zrc20ABI, owner, locker, big.NewInt(1001))
 
 		// Check allowance smaller, equal and bigger than the amount.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, big.NewInt(1001))
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, locker, owner, locker, big.NewInt(1001))
 		require.Error(t, err)
 
 		// We do not check in LockZRC20 explicitly if the amount is bigger than the balance.
@@ -96,15 +97,15 @@ func Test_LockZRC20(t *testing.T) {
 		approveAllowance(t, ts, zrc20ABI, owner, locker, allowanceTotal)
 
 		// Check allowance smaller, equal and bigger than the amount.
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, higherThanAllowance)
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, locker, owner, locker, higherThanAllowance)
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "invalid allowance, got: 100")
+		require.Contains(t, err.Error(), "invalid allowance, got 100")
 	})
 
 	t.Run("should pass when trying to lock a valid approved amount", func(t *testing.T) {
 		approveAllowance(t, ts, zrc20ABI, owner, locker, allowanceTotal)
 
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, allowanceTotal)
+		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, locker, owner, locker, allowanceTotal)
 		require.NoError(t, err)
 
 		ownerBalance, err := ts.fungibleKeeper.ZRC20BalanceOf(ts.ctx, zrc20ABI, ts.zrc20Address, owner)
@@ -119,7 +120,15 @@ func Test_LockZRC20(t *testing.T) {
 	t.Run("should pass when trying to lock an amount smaller than approved", func(t *testing.T) {
 		approveAllowance(t, ts, zrc20ABI, owner, locker, allowanceTotal)
 
-		err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, smallerThanAllowance)
+		err = ts.fungibleKeeper.LockZRC20(
+			ts.ctx,
+			zrc20ABI,
+			ts.zrc20Address,
+			locker,
+			owner,
+			locker,
+			smallerThanAllowance,
+		)
 		require.NoError(t, err)
 
 		// Note that balances are cumulative for all tests. That's why we check 801 and 199 here.
@@ -155,7 +164,7 @@ func Test_UnlockZRC20(t *testing.T) {
 	approveAllowance(t, ts, zrc20ABI, owner, locker, allowanceTotal)
 
 	// Lock 100 ZRC20.
-	err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, allowanceTotal)
+	err = ts.fungibleKeeper.LockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, locker, owner, locker, allowanceTotal)
 	require.NoError(t, err)
 
 	t.Run("should fail when trying to unlock zero amount", func(t *testing.T) {
@@ -185,7 +194,7 @@ func Test_UnlockZRC20(t *testing.T) {
 	t.Run("should fail when trying to unlock an amount bigger than locker's balance", func(t *testing.T) {
 		err = ts.fungibleKeeper.UnlockZRC20(ts.ctx, zrc20ABI, ts.zrc20Address, owner, locker, big.NewInt(1001))
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "invalid balance, got: 100")
+		require.Contains(t, err.Error(), "invalid balance, got 100")
 	})
 
 	t.Run("should pass when trying to unlock a correct amount", func(t *testing.T) {
@@ -231,7 +240,7 @@ func Test_CheckZRC20Allowance(t *testing.T) {
 	t.Run("should fail when allowance is not approved", func(t *testing.T) {
 		err = ts.fungibleKeeper.CheckZRC20Allowance(ts.ctx, zrc20ABI, owner, spender, ts.zrc20Address, big.NewInt(10))
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "invalid allowance, got: 0")
+		require.Contains(t, err.Error(), "invalid allowance, got 0")
 	})
 
 	t.Run("should fail when checking a higher amount than approved", func(t *testing.T) {
@@ -246,7 +255,7 @@ func Test_CheckZRC20Allowance(t *testing.T) {
 			higherThanAllowance,
 		)
 		require.Error(t, err)
-		require.Contains(t, err.Error(), "invalid allowance, got: 100")
+		require.Contains(t, err.Error(), "invalid allowance, got 100")
 	})
 
 	t.Run("should pass when checking the same amount as approved", func(t *testing.T) {

x/fungible/keeper/zrc20_cosmos_coins_mapping.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"math/big"
 
+	"cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/ethereum/go-ethereum/accounts/abi"
 	"github.com/ethereum/go-ethereum/common"
@@ -19,42 +20,42 @@ import (
 func (k Keeper) LockZRC20(
 	ctx sdk.Context,
 	zrc20ABI *abi.ABI,
-	zrc20Address, owner, locker common.Address,
+	zrc20Address, spender, owner, locker common.Address,
 	amount *big.Int,
 ) error {
 	// owner is the EOA owner of the ZRC20 tokens.
 	// locker is the address that will lock the ZRC20 tokens, i.e: bank precompile.
 	if err := k.CheckZRC20Allowance(ctx, zrc20ABI, owner, locker, zrc20Address, amount); err != nil {
-		return err
+		return errors.Wrap(err, "failed allowance check")
 	}
 
 	// Check amount_to_be_locked <= total_erc20_balance - already_locked
 	// Max amount of ZRC20 tokens that exists in zEVM are the total supply.
 	totalSupply, err := k.ZRC20TotalSupply(ctx, zrc20ABI, zrc20Address)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed totalSupply check")
 	}
 
 	// The alreadyLocked amount is the amount of ZRC20 tokens that have been locked by the locker.
-	// TODO: Implement list of whitelisted locker addresses.
+	// TODO: Implement list of whitelisted locker addresses (https://github.com/zeta-chain/node/issues/2991)
 	alreadyLocked, err := k.ZRC20BalanceOf(ctx, zrc20ABI, zrc20Address, locker)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed getting the ZRC20 already locked amount")
 	}
 
 	if !k.IsValidDepositAmount(totalSupply, alreadyLocked, amount) {
-		return fungibletypes.ErrInvalidAmount
+		return errors.Wrap(fungibletypes.ErrInvalidAmount, "amount to be locked is not valid")
 	}
 
 	// Initiate a transferFrom the owner to the locker. This will lock the ZRC20 tokens.
 	// locker has to initiate the transaction and have enough allowance from owner.
-	transferred, err := k.ZRC20TransferFrom(ctx, zrc20ABI, zrc20Address, owner, locker, amount)
+	transferred, err := k.ZRC20TransferFrom(ctx, zrc20ABI, zrc20Address, spender, owner, locker, amount)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed executing transferFrom")
 	}
 
 	if !transferred {
-		return fmt.Errorf("lock ZRC20 not successful")
+		return fmt.Errorf("transferFrom returned false (no success)")
 	}
 
 	return nil
@@ -71,17 +72,17 @@ func (k Keeper) UnlockZRC20(
 ) error {
 	// Check if the account locking the ZRC20 tokens has enough balance.
 	if err := k.CheckZRC20Balance(ctx, zrc20ABI, zrc20Address, locker, amount); err != nil {
-		return err
+		return errors.Wrap(err, "failed balance check")
 	}
 
 	// transfer from the EOA locking the assets to the owner.
 	transferred, err := k.ZRC20Transfer(ctx, zrc20ABI, zrc20Address, locker, owner, amount)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed executing transfer")
 	}
 
 	if !transferred {
-		return fmt.Errorf("unlock ZRC20 not successful")
+		return fmt.Errorf("transfer returned false (no success)")
 	}
 
 	return nil
@@ -108,16 +109,16 @@ func (k Keeper) CheckZRC20Allowance(
 	}
 
 	if err := k.IsValidZRC20(ctx, zrc20Address); err != nil {
-		return err
+		return errors.Wrap(err, "ZRC20 is not valid")
 	}
 
 	allowanceValue, err := k.ZRC20Allowance(ctx, zrc20ABI, zrc20Address, owner, spender)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed while checking spender's allowance")
 	}
 
 	if allowanceValue.Cmp(amount) < 0 || allowanceValue.Cmp(big.NewInt(0)) <= 0 {
-		return fmt.Errorf("invalid allowance, got: %s", allowanceValue.String())
+		return fmt.Errorf("invalid allowance, got %s, wanted %s", allowanceValue.String(), amount.String())
 	}
 
 	return nil
@@ -140,7 +141,7 @@ func (k Keeper) CheckZRC20Balance(
 	}
 
 	if err := k.IsValidZRC20(ctx, zrc20Address); err != nil {
-		return err
+		return errors.Wrap(err, "ZRC20 is not valid")
 	}
 
 	if crypto.IsEmptyAddress(owner) {
@@ -151,11 +152,11 @@ func (k Keeper) CheckZRC20Balance(
 	// function balanceOf(address account)
 	balance, err := k.ZRC20BalanceOf(ctx, zrc20ABI, zrc20Address, owner)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed getting owner's ZRC20 balance")
 	}
 
-	if balance.Cmp(amount) == -1 {
-		return fmt.Errorf("invalid balance, got: %s", balance.String())
+	if balance.Cmp(amount) < 0 {
+		return fmt.Errorf("invalid balance, got %s, wanted %s", balance.String(), amount.String())
 	}
 
 	return nil
@@ -179,7 +180,7 @@ func (k Keeper) IsValidZRC20(ctx sdk.Context, zrc20Address common.Address) error
 	return nil
 }
 
-// IsValidDepositAmount checks "totalSupply >= amount_to_be_deposited + amount_already_locked".
+// IsValidDepositAmount checks "totalSupply >= amount_to_be_locked + amount_already_locked".
 // A failure here means the user is trying to lock more than the available ZRC20 supply.
 // This suggests that an actor is minting ZRC20 tokens out of thin air.
 func (k Keeper) IsValidDepositAmount(totalSupply, alreadyLocked, amountToDeposit *big.Int) bool {