Merge pull request #260 from lumoswiz/test/erc20-transfer-facet

Align ERC-20 returns and complete ERC20TransferFacet fuzz unit tests

Author: lumoswiz

src/token/ERC20/ERC20/ERC20Mod.sol

@@ -132,8 +132,9 @@ function burn(address _account, uint256 _value) {
  * @param _from The address to send tokens from.
  * @param _to The address to send tokens to.
  * @param _value The number of tokens to transfer.
+ * @return True if the transfer was successful.
  */
-function transferFrom(address _from, address _to, uint256 _value) {
+function transferFrom(address _from, address _to, uint256 _value) returns (bool) {
     ERC20TransferStorage storage s = getStorage();
     if (_from == address(0)) {
         revert ERC20InvalidSender(address(0));
@@ -157,15 +158,17 @@ function transferFrom(address _from, address _to, uint256 _value) {
     }
     s.balanceOf[_to] += _value;
     emit Transfer(_from, _to, _value);
+    return true;
 }
 
 /**
  * @notice Transfers tokens from the caller to another address.
  * @dev Updates balances directly without allowance mechanism.
  * @param _to The address to send tokens to.
  * @param _value The number of tokens to transfer.
+ * @return True if the transfer was successful.
  */
-function transfer(address _to, uint256 _value) {
+function transfer(address _to, uint256 _value) returns (bool) {
     ERC20TransferStorage storage s = getStorage();
     if (_to == address(0)) {
         revert ERC20InvalidReceiver(address(0));
@@ -179,19 +182,22 @@ function transfer(address _to, uint256 _value) {
     }
     s.balanceOf[_to] += _value;
     emit Transfer(msg.sender, _to, _value);
+    return true;
 }
 
 /**
  * @notice Approves a spender to transfer tokens on behalf of the caller.
  * @dev Sets the allowance for the spender.
  * @param _spender The address to approve for spending.
  * @param _value The amount of tokens to approve.
+ * @return True if the approval was successful.
  */
-function approve(address _spender, uint256 _value) {
+function approve(address _spender, uint256 _value) returns (bool) {
     if (_spender == address(0)) {
         revert ERC20InvalidSpender(address(0));
     }
     ERC20TransferStorage storage s = getStorage();
     s.allowance[msg.sender][_spender] = _value;
     emit Approval(msg.sender, _spender, _value);
+    return true;
 }

test/harnesses/token/ERC20/ERC20/ERC20Harness.sol

@@ -30,22 +30,22 @@ contract ERC20Harness {
     /**
      * @notice Exposes ERC20Mod.transferFrom as an external function
      */
-    function transferFrom(address _from, address _to, uint256 _value) external {
-        ERC20Mod.transferFrom(_from, _to, _value);
+    function transferFrom(address _from, address _to, uint256 _value) external returns (bool) {
+        return ERC20Mod.transferFrom(_from, _to, _value);
     }
 
     /**
      * @notice Exposes ERC20Mod.transfer as an external function
      */
-    function transfer(address _to, uint256 _value) external {
-        ERC20Mod.transfer(_to, _value);
+    function transfer(address _to, uint256 _value) external returns (bool) {
+        return ERC20Mod.transfer(_to, _value);
     }
 
     /**
      * @notice Exposes ERC20Mod.approve as an external function
      */
-    function approve(address _spender, uint256 _value) external {
-        ERC20Mod.approve(_spender, _value);
+    function approve(address _spender, uint256 _value) external returns (bool) {
+        return ERC20Mod.approve(_spender, _value);
     }
 
     function totalSupply() external view returns (uint256) {

test/trees/ERC20.tree

@@ -0,0 +1,76 @@
+Approve
+├── when the spender is the zero address
+│   └── it should revert
+└── when the spender is not the zero address
+    ├── it should set the caller's allowance for the spender to value
+    ├── it should emit an {Approval} event
+    └── it should return true
+
+Transfer
+├── when the receiver is the zero address
+│   └── it should revert
+└── when the receiver is not the zero address
+    ├── given when the sender's balance is less than the transfer amount
+    │   └── it should revert
+    └── given when the sender's balance is greater than, or equal to, the transfer amount
+        ├── when the amount is zero
+        │   ├── it should emit a {Transfer} event
+        │   ├── it should not change balances
+        │   └── it should return true
+        └── when the amount is > 0
+            ├── it should decrement the sender's balance by the transfer amount
+            ├── it should increment the receiver's balance by the transfer amount
+            ├── it should emit a {Transfer} event
+            └── it should return true
+
+TransferFrom
+├── when the sender is the zero address
+│   └── it should revert
+├── when the receiver is the zero address
+│   └── it should revert
+└── when both sender and receiver are non-zero addresses
+    ├── given allowance < amount
+    │   └── it should revert
+    └── given allowance >= amount
+        ├── given balance < amount
+        │   └── it should revert
+        └── given balance >= amount
+            ├── when amount is zero
+            │   ├── it should not change balances
+            │   ├── it should not decrement allowance
+            │   ├── it should emit a {Transfer} event
+            │   └── it should return true
+            ├── when amount > 0 and allowance is max uint256
+            │   ├── it should decrement the sender's balance by the transfer amount
+            │   ├── it should increment the receiver's balance by the transfer amount
+            │   ├── it should not change allowance
+            │   ├── it should emit a {Transfer} event
+            │   └── it should return true
+            └── when the amount is greater than zero and allowance is finite
+                ├── it should decrement the allowance by the transfer amount
+                ├── it should decrement the sender's balance by the transfer amount
+                ├── it should increment the receiver's balance by the transfer amount
+                ├── it should emit a {Transfer} event
+                └── it should return true
+
+Mint
+├── when the account to mint to is the zero address
+│   └── it should revert
+└── when the account to mint to is not the zero address
+    ├── given the mint amount would overflow total supply
+    │   └── it should revert
+    └── given the mint amount does not overflow total supply
+        ├── it should increment total supply by the mint amount
+        ├── it should increment the account's balance by the mint amount
+        └── it should emit a {Transfer} event from zero address to the account
+
+Burn
+├── when the account to burn from is the zero address
+│   └── it should revert
+└── when the account to burn from is not the zero address
+    ├── when the account balance is less than the burn amount
+    │   └── it should revert
+    └── when the account balance is greater than or equal to the burn amount
+        ├── it should decrement the account balance by the burn amount
+        ├── it should decrement total supply by the burn amount
+        └── it should emit a {Transfer} event to the zero address

test/unit/fuzz/token/ERC20/ERC20/mod/approve.tree

@@ -9,21 +9,21 @@ import {stdError} from "forge-std/StdError.sol";
 import {Base_Test} from "test/Base.t.sol";
 import {ERC20Harness} from "test/harnesses/token/ERC20/ERC20/ERC20Harness.sol";
 
-import "src/token/ERC20/ERC20/ERC20Mod.sol" as ERC20Mod;
+import "src/token/ERC20/ERC20/ERC20Mod.sol";
 
+/**
+ *  @dev BTT spec: test/trees/ERC20.tree
+ */
 contract Approve_ERC20Mod_Fuzz_Unit_Test is Base_Test {
     ERC20Harness internal harness;
 
-    event Approval(address indexed _owner, address indexed _spender, uint256 _value);
-
     function setUp() public override {
         Base_Test.setUp();
-
         harness = new ERC20Harness();
     }
 
     function testFuzz_ShouldRevert_SpenderIsZeroAddress(uint256 value) external {
-        vm.expectRevert(abi.encodeWithSelector(ERC20Mod.ERC20InvalidSpender.selector, ADDRESS_ZERO));
+        vm.expectRevert(abi.encodeWithSelector(ERC20InvalidSpender.selector, ADDRESS_ZERO));
         harness.approve(ADDRESS_ZERO, value);
     }
 
@@ -32,9 +32,9 @@ contract Approve_ERC20Mod_Fuzz_Unit_Test is Base_Test {
 
         vm.expectEmit(address(harness));
         emit Approval(users.alice, spender, value);
-        harness.approve(spender, value);
+        bool result = harness.approve(spender, value);
 
+        assertEq(result, true, "approve failed");
         assertEq(harness.allowance(users.alice, spender), value);
     }
 }
-

test/unit/fuzz/token/ERC20/ERC20/mod/burn.tree

@@ -9,21 +9,21 @@ import {stdError} from "forge-std/StdError.sol";
 import {Base_Test} from "test/Base.t.sol";
 import {ERC20Harness} from "test/harnesses/token/ERC20/ERC20/ERC20Harness.sol";
 
-import "src/token/ERC20/ERC20/ERC20Mod.sol" as ERC20Mod;
+import "src/token/ERC20/ERC20/ERC20Mod.sol";
 
+/**
+ *  @dev BTT spec: test/trees/ERC20.tree
+ */
 contract Burn_ERC20Mod_Fuzz_Unit_Test is Base_Test {
     ERC20Harness internal harness;
 
-    event Transfer(address indexed _from, address indexed _to, uint256 _value);
-
     function setUp() public override {
         Base_Test.setUp();
-
         harness = new ERC20Harness();
     }
 
     function testFuzz_ShouldRevert_Account_ZeroAddress(uint256 value) external {
-        vm.expectRevert(abi.encodeWithSelector(ERC20Mod.ERC20InvalidSender.selector, address(0)));
+        vm.expectRevert(abi.encodeWithSelector(ERC20InvalidSender.selector, address(0)));
         harness.burn(ADDRESS_ZERO, value);
     }
 
@@ -37,7 +37,7 @@ contract Burn_ERC20Mod_Fuzz_Unit_Test is Base_Test {
 
         harness.mint(account, balance);
 
-        vm.expectRevert(abi.encodeWithSelector(ERC20Mod.ERC20InsufficientBalance.selector, account, balance, value));
+        vm.expectRevert(abi.encodeWithSelector(ERC20InsufficientBalance.selector, account, balance, value));
         harness.burn(account, value);
     }
 
@@ -63,4 +63,3 @@ contract Burn_ERC20Mod_Fuzz_Unit_Test is Base_Test {
         assertEq(harness.balanceOf(account), beforeBalanceOfAccount - value, "balanceOf(account)");
     }
 }
-