include review suggestions

zeta-chain · Sep 23, 2024 · af858ab · af858ab
1 parent dbec3ca
commit af858ab
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 42 deletions.
diff --git a/precompiles/bank/bank.go b/precompiles/bank/bank.go
@@ -151,7 +151,15 @@ func (c *Contract) Run(evm *vm.EVM, contract *vm.Contract, readOnly bool) ([]byt
 			return err
 		})
 		if execErr != nil {
-			return method.Outputs.Pack(false)
+			res, errPack := method.Outputs.Pack(false)
+			if errPack != nil {
+				return nil, errPack
+			}
+
+			// Return the proper result (true/false) and the error message.
+			// This way we make bank compliant with smart contracts which would expect a true/false.
+			// And also with Go bindings which would expect an error.
+			return res, err
 		}
 		return res, nil
 

diff --git a/precompiles/bank/method_deposit.go b/precompiles/bank/method_deposit.go
@@ -61,10 +61,17 @@ func (c *Contract) deposit(
 
 	// Safety check: token has to be a valid whitelisted ZRC20 and not be paused.
 	t, found := c.fungibleKeeper.GetForeignCoins(ctx, zrc20Addr.String())
-	if !found || t.Paused {
+	if !found {
 		return nil, &ptypes.ErrInvalidToken{
 			Got:    zrc20Addr.String(),
-			Reason: "token is not a whitelisted ZRC20 or it's paused",
+			Reason: "token is not a whitelisted ZRC20",
+		}
+	}
+
+	if t.Paused {
+		return nil, &ptypes.ErrInvalidToken{
+			Got:    zrc20Addr.String(),
+			Reason: "token is paused",
 		}
 	}
 
@@ -86,7 +93,13 @@ func (c *Contract) deposit(
 	}
 
 	balance, ok := resBalanceOf[0].(*big.Int)
-	if !ok || balance.Cmp(amount) < 0 || balance.Cmp(big.NewInt(0)) <= 0 {
+	if !ok {
+		return nil, &ptypes.ErrUnexpected{
+			Got: "ZRC20 balanceOf returned an unexpected type",
+		}
+	}
+
+	if balance.Cmp(amount) < 0 || balance.Cmp(big.NewInt(0)) <= 0 {
 		return nil, &ptypes.ErrInvalidAmount{
 			Got: balance.String(),
 		}
@@ -110,7 +123,13 @@ func (c *Contract) deposit(
 	}
 
 	allowance, ok := resAllowance[0].(*big.Int)
-	if !ok || allowance.Cmp(amount) < 0 || allowance.Cmp(big.NewInt(0)) <= 0 {
+	if !ok {
+		return nil, &ptypes.ErrUnexpected{
+			Got: "ZRC20 allowance returned an unexpected type",
+		}
+	}
+
+	if allowance.Cmp(amount) < 0 || allowance.Cmp(big.NewInt(0)) <= 0 {
 		return nil, &ptypes.ErrInvalidAmount{
 			Got: allowance.String(),
 		}
@@ -152,8 +171,7 @@ func (c *Contract) deposit(
 	}
 
 	// 3. Interactions: create cosmos coin and send.
-	err = c.bankKeeper.MintCoins(ctx, types.ModuleName, coinSet)
-	if err != nil {
+	if err := c.bankKeeper.MintCoins(ctx, types.ModuleName, coinSet); err != nil {
 		return nil, &ptypes.ErrUnexpected{
 			When: "MintCoins",
 			Got:  err.Error(),

diff --git a/precompiles/bank/method_test.go b/precompiles/bank/method_test.go
@@ -98,7 +98,14 @@ func Test_Methods(t *testing.T) {
 		)
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.ErrorAs(
+			t, 
+			ptypes.ErrInvalidAmount{
+				Got: "0",
+			},
+			err,
+		)
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)
@@ -127,7 +134,14 @@ func Test_Methods(t *testing.T) {
 		)
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.ErrorAs(
+			t, 
+			ptypes.ErrInvalidAmount{
+				Got: "0",
+			},
+			err,
+		)
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)
@@ -158,7 +172,14 @@ func Test_Methods(t *testing.T) {
 		)
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.ErrorAs(
+			t, 
+			ptypes.ErrInvalidAmount{
+				Got: "500",
+			},
+			err,
+		)
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)
@@ -205,7 +226,14 @@ func Test_Methods(t *testing.T) {
 		)
 
 		success, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.ErrorAs(
+			t, 
+			ptypes.ErrInvalidAmount{
+				Got: "1000",
+			},
+			err,
+		)
 
 		res, err := ts.bankABI.Methods[DepositMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)
@@ -409,7 +437,15 @@ func Test_Methods(t *testing.T) {
 		)
 
 		success, err = ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
-		require.NoError(t, err)
+		require.Error(t, err)
+		require.ErrorAs(
+			t, 
+			ptypes.ErrInsufficientBalance{
+				Requested: "501",
+				Got: "500",
+			},
+			err,
+		)
 
 		res, err = ts.bankABI.Methods[WithdrawMethodName].Outputs.Unpack(success)
 		require.NoError(t, err)

diff --git a/precompiles/bank/method_withdraw.go b/precompiles/bank/method_withdraw.go
@@ -57,19 +57,26 @@ func (c *Contract) withdraw(
 
 	// Safety check: token has to be a valid whitelisted ZRC20 and not be paused.
 	t, found := c.fungibleKeeper.GetForeignCoins(ctx, zrc20Addr.String())
-	if !found || t.Paused {
+	if !found {
 		return nil, &ptypes.ErrInvalidToken{
 			Got:    zrc20Addr.String(),
-			Reason: "token is not a whitelisted ZRC20 or it's paused",
+			Reason: "token is not a whitelisted ZRC20",
+		}
+	}
+
+	if t.Paused {
+		return nil, &ptypes.ErrInvalidToken{
+			Got:    zrc20Addr.String(),
+			Reason: "token is paused",
 		}
 	}
 
 	// Caller has to have enough cosmos coin balance to withdraw the requested amount.
 	coin := c.bankKeeper.GetBalance(ctx, fromAddr, ZRC20ToCosmosDenom(zrc20Addr))
-	if coin.Amount.IsNil() {
+	if !coin.IsValid() {
 		return nil, &ptypes.ErrInsufficientBalance{
 			Requested: amount.String(),
-			Got:       "nil",
+			Got:       "invalid coin",
 		}
 	}
 
@@ -103,39 +110,19 @@ func (c *Contract) withdraw(
 	}
 
 	balance, ok := resBalanceOf[0].(*big.Int)
-	if !ok || balance.Cmp(amount) == -1 {
-		return nil, &ptypes.ErrInvalidAmount{
-			Got: "not enough bank balance",
-		}
-	}
-
-	// 2. Effect: transfer balance.
-
-	// function transfer(address recipient, uint256 amount) public virtual override returns (bool)
-	resTransferFrom, err := c.CallContract(
-		ctx,
-		&c.fungibleKeeper,
-		c.zrc20ABI,
-		zrc20Addr,
-		"transfer",
-		[]interface{}{caller /* sender */, amount},
-	)
-	if err != nil {
+	if !ok {
 		return nil, &ptypes.ErrUnexpected{
-			When: "transferFrom",
-			Got:  err.Error(),
+			Got: "ZRC20 balanceOf returned an unexpected type",
 		}
 	}
 
-	transferred, ok := resTransferFrom[0].(bool)
-	if !ok || !transferred {
-		return nil, &ptypes.ErrUnexpected{
-			When: "transferFrom",
-			Got:  "transaction not successful",
+	if balance.Cmp(amount) == -1 {
+		return nil, &ptypes.ErrInvalidAmount{
+			Got: balance.String(),
 		}
 	}
 
-	// 3. Interactions: send to module and burn.
+	// 2. Effect: burn cosmos coin balance.
 	if err := c.bankKeeper.SendCoinsFromAccountToModule(ctx, fromAddr, types.ModuleName, coinSet); err != nil {
 		return nil, &ptypes.ErrUnexpected{
 			When: "SendCoinsFromAccountToModule",
@@ -157,6 +144,38 @@ func (c *Contract) withdraw(
 		}
 	}
 
+	// 3. Interactions: send to module and burn.
+
+	// function transfer(address recipient, uint256 amount) public virtual override returns (bool)
+	resTransfer, err := c.CallContract(
+		ctx,
+		&c.fungibleKeeper,
+		c.zrc20ABI,
+		zrc20Addr,
+		"transfer",
+		[]interface{}{caller /* sender */, amount},
+	)
+	if err != nil {
+		return nil, &ptypes.ErrUnexpected{
+			When: "transfer",
+			Got:  err.Error(),
+		}
+	}
+
+	transferred, ok := resTransfer[0].(bool)
+	if !ok {
+		return nil, &ptypes.ErrUnexpected{
+			Got: "ZRC20 transfer returned an unexpected type",
+		}
+	}
+
+	if !transferred {
+		return nil, &ptypes.ErrUnexpected{
+			When: "transfer",
+			Got:  "transaction not successful",
+		}
+	}
+
 	return method.Outputs.Pack(true)
 }