From 6adb5ec9bdbdf6b1239764fda7509a62bdf27ecb Mon Sep 17 00:00:00 2001 From: Francisco de Borja Aranda Castillejo Date: Fri, 11 Oct 2024 19:35:36 +0200 Subject: [PATCH] add reviewed changes --- precompiles/bank/method_deposit.go | 2 +- precompiles/bank/method_test.go | 8 +++- .../keeper/zrc20_cosmos_coin_mapping_test.go | 35 ++++++++++------ .../keeper/zrc20_cosmos_coins_mapping.go | 41 ++++++++++--------- x/fungible/keeper/zrc20_methods.go | 32 +++++++-------- x/fungible/keeper/zrc20_methods_test.go | 24 ++++++++++- 6 files changed, 88 insertions(+), 54 deletions(-) diff --git a/precompiles/bank/method_deposit.go b/precompiles/bank/method_deposit.go index ae620c34a9..60f166de97 100644 --- a/precompiles/bank/method_deposit.go +++ b/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(), diff --git a/precompiles/bank/method_test.go b/precompiles/bank/method_test.go index f6601bccbe..8921c787af 100644 --- a/precompiles/bank/method_test.go +++ b/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) diff --git a/x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go b/x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go index 578ee2dfb9..941b81e98c 100644 --- a/x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go +++ b/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) { diff --git a/x/fungible/keeper/zrc20_cosmos_coins_mapping.go b/x/fungible/keeper/zrc20_cosmos_coins_mapping.go index 2013514758..f7d152f749 100644 --- a/x/fungible/keeper/zrc20_cosmos_coins_mapping.go +++ b/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 { diff --git a/x/fungible/keeper/zrc20_methods.go b/x/fungible/keeper/zrc20_methods.go index b3b636dfe4..83a9b184da 100644 --- a/x/fungible/keeper/zrc20_methods.go +++ b/x/fungible/keeper/zrc20_methods.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" @@ -53,7 +54,7 @@ func (k Keeper) ZRC20Allowance( args..., ) if err != nil { - return nil, err + return nil, errors.Wrap(err, "EVM error calling ZRC20 allowance function") } if res.VmError != "" { @@ -62,7 +63,7 @@ func (k Keeper) ZRC20Allowance( ret, err := zrc20ABI.Methods[allowance].Outputs.Unpack(res.Ret) if err != nil { - return nil, err + return nil, errors.Wrap(err, "failed to unpack ZRC20 allowance return value") } if len(ret) == 0 { @@ -108,7 +109,7 @@ func (k Keeper) ZRC20BalanceOf( owner, ) if err != nil { - return nil, err + return nil, errors.Wrap(err, "EVM error calling ZRC20 balanceOf function") } if res.VmError != "" { @@ -117,7 +118,7 @@ func (k Keeper) ZRC20BalanceOf( ret, err := zrc20ABI.Methods[balanceOf].Outputs.Unpack(res.Ret) if err != nil { - return nil, err + return nil, errors.Wrap(err, "failed to unpack ZRC20 balanceOf return value") } if len(ret) == 0 { @@ -159,7 +160,7 @@ func (k Keeper) ZRC20TotalSupply( totalSupply, ) if err != nil { - return nil, err + return nil, errors.Wrap(err, "EVM error calling ZRC20 totalSupply function") } if res.VmError != "" { @@ -168,7 +169,7 @@ func (k Keeper) ZRC20TotalSupply( ret, err := zrc20ABI.Methods[totalSupply].Outputs.Unpack(res.Ret) if err != nil { - return nil, err + return nil, errors.Wrap(err, "failed to unpack ZRC20 totalSupply return value") } if len(ret) == 0 { @@ -202,7 +203,6 @@ func (k Keeper) ZRC20Transfer( return false, err } - // transfer from the EOA locking the assets to the owner. args := []interface{}{to, amount} res, err := k.CallEVM( ctx, @@ -217,7 +217,7 @@ func (k Keeper) ZRC20Transfer( args..., ) if err != nil { - return false, err + return false, errors.Wrap(err, "EVM error calling ZRC20 transfer function") } if res.VmError != "" { @@ -226,7 +226,7 @@ func (k Keeper) ZRC20Transfer( ret, err := zrc20ABI.Methods[transfer].Outputs.Unpack(res.Ret) if err != nil { - return false, err + return false, errors.Wrap(err, "failed to unpack ZRC20 transfer return value") } if len(ret) == 0 { @@ -241,20 +241,20 @@ func (k Keeper) ZRC20Transfer( return transferred, nil } -// ZRC20TransferFrom transfers ZRC20 tokens from the owner to the spender. +// ZRC20TransferFrom transfers ZRC20 tokens "from" to the EOA "to". // The transaction is started by the spender. -// This requires the spender to have been approved by the owner. +// Requisite: the original EOA must have approved the spender to spend the tokens. func (k Keeper) ZRC20TransferFrom( ctx sdk.Context, zrc20ABI *abi.ABI, - zrc20Address, from, to common.Address, + zrc20Address, spender, from, to common.Address, amount *big.Int, ) (bool, error) { if zrc20ABI == nil { return false, fungibletypes.ErrZRC20NilABI } - if crypto.IsEmptyAddress(from) || crypto.IsEmptyAddress(to) { + if crypto.IsEmptyAddress(from) || crypto.IsEmptyAddress(to) || crypto.IsEmptyAddress(spender) { return false, fungibletypes.ErrZeroAddress } @@ -266,7 +266,7 @@ func (k Keeper) ZRC20TransferFrom( res, err := k.CallEVM( ctx, *zrc20ABI, - to, + spender, zrc20Address, big.NewInt(0), nil, @@ -276,7 +276,7 @@ func (k Keeper) ZRC20TransferFrom( args..., ) if err != nil { - return false, err + return false, errors.Wrap(err, "EVM error calling ZRC20 transferFrom function") } if res.VmError != "" { @@ -285,7 +285,7 @@ func (k Keeper) ZRC20TransferFrom( ret, err := zrc20ABI.Methods[transferFrom].Outputs.Unpack(res.Ret) if err != nil { - return false, err + return false, errors.Wrap(err, "failed to unpack ZRC20 transferFrom return value") } if len(ret) == 0 { diff --git a/x/fungible/keeper/zrc20_methods_test.go b/x/fungible/keeper/zrc20_methods_test.go index 2fb6992508..a5010c332a 100644 --- a/x/fungible/keeper/zrc20_methods_test.go +++ b/x/fungible/keeper/zrc20_methods_test.go @@ -239,17 +239,19 @@ func Test_ZRC20TransferFrom(t *testing.T) { ts.zrc20Address, common.Address{}, common.Address{}, + common.Address{}, big.NewInt(0), ) require.Error(t, err) require.ErrorAs(t, err, &fungibletypes.ErrZRC20NilABI) }) - t.Run("should fail when owner is zero address", func(t *testing.T) { + t.Run("should fail when from is zero address", func(t *testing.T) { _, err := ts.fungibleKeeper.ZRC20TransferFrom( ts.ctx, zrc20ABI, ts.zrc20Address, + sample.EthAddress(), common.Address{}, sample.EthAddress(), big.NewInt(0), @@ -258,13 +260,28 @@ func Test_ZRC20TransferFrom(t *testing.T) { require.ErrorAs(t, err, &fungibletypes.ErrZeroAddress) }) - t.Run("should fail when spender is zero address", func(t *testing.T) { + t.Run("should fail when to is zero address", func(t *testing.T) { _, err := ts.fungibleKeeper.ZRC20TransferFrom( ts.ctx, zrc20ABI, ts.zrc20Address, sample.EthAddress(), + sample.EthAddress(), + common.Address{}, + big.NewInt(0), + ) + require.Error(t, err) + require.ErrorAs(t, err, &fungibletypes.ErrZeroAddress) + }) + + t.Run("should fail when spender is zero address", func(t *testing.T) { + _, err := ts.fungibleKeeper.ZRC20TransferFrom( + ts.ctx, + zrc20ABI, + ts.zrc20Address, common.Address{}, + sample.EthAddress(), + sample.EthAddress(), big.NewInt(0), ) require.Error(t, err) @@ -277,6 +294,7 @@ func Test_ZRC20TransferFrom(t *testing.T) { zrc20ABI, common.Address{}, sample.EthAddress(), + sample.EthAddress(), fungibletypes.ModuleAddressZEVM, big.NewInt(0), ) @@ -293,6 +311,7 @@ func Test_ZRC20TransferFrom(t *testing.T) { ts.ctx, zrc20ABI, ts.zrc20Address, + fungibletypes.ModuleAddressZEVM, sample.EthAddress(), fungibletypes.ModuleAddressZEVM, big.NewInt(10), @@ -312,6 +331,7 @@ func Test_ZRC20TransferFrom(t *testing.T) { ts.ctx, zrc20ABI, ts.zrc20Address, + fungibletypes.ModuleAddressZEVM, sample.EthAddress(), fungibletypes.ModuleAddressZEVM, big.NewInt(10),