Better tests

ObolNetwork · Jan 24, 2025 · 7d35950 · 7d35950
1 parent 1841d58
commit 7d35950
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 19 deletions.
diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol
@@ -186,7 +186,7 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
     bytes[] calldata sourcePubKeys,
     bytes calldata targetPubKey
   ) external payable onlyOwnerOrRoles(CONSOLIDATION_ROLE) {
-    if (sourcePubKeys.length == 0) revert InvalidRequest_Params();
+    if (sourcePubKeys.length == 0 || targetPubKey.length != 48) revert InvalidRequest_Params();
 
     uint256 remainingFee = msg.value;
     uint256 len = sourcePubKeys.length;
@@ -312,8 +312,8 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
   /// -----------------------------------------------------------------------
 
   /// Compute system contracts fee
-  function _computeSystemContractFee(address systemContractAddress) internal returns (uint256) {
-    (bool ok, bytes memory result) = systemContractAddress.call(new bytes(0));
+  function _computeSystemContractFee(address systemContractAddress) internal view returns (uint256) {
+    (bool ok, bytes memory result) = systemContractAddress.staticcall("");
     if (!ok) revert InvalidRequest_SystemGetFee();
 
     return uint256(bytes32(result));
@@ -330,7 +330,7 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
     //  +--------+--------+
     //      48       48
 
-    (bool ok, ) = consolidationSystemContract.call{value: fee}(abi.encodePacked(source, target));
+    (bool ok, ) = consolidationSystemContract.call{value: fee}(bytes.concat(source, target));
     if (!ok) revert InvalidConsolidation_Failed();
 
     emit ConsolidationRequested(msg.sender, source, target);

diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol
@@ -120,11 +120,24 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
     bytes[] memory empty = new bytes[](0);
     owrETH.requestConsolidation{value: 1 ether}(empty, new bytes(48));
 
-    // Not enough fee (2 wei is the minimum fee)
+    // Not enough fee (1 wei is the minimum fee)
     vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector);
     bytes[] memory single = new bytes[](1);
     single[0] = new bytes(48);
-    owrETH.requestConsolidation{value: 1 wei}(single, new bytes(48));
+    owrETH.requestConsolidation{value: 0}(single, new bytes(48));
+
+    // Failed get_fee() request
+    uint256 realFee = consolidationMock.fakeExponential(0);
+    consolidationMock.setFailNextFeeRequest(true);
+    vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_SystemGetFee.selector);
+    owrETH.requestConsolidation{value: realFee}(single, new bytes(48));
+    consolidationMock.setFailNextFeeRequest(false);
+
+    // Failed add_request() request
+    consolidationMock.setFailNextAddRequest(true);
+    vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidConsolidation_Failed.selector);
+    owrETH.requestConsolidation{value: realFee}(single, new bytes(48));
+    consolidationMock.setFailNextAddRequest(false);
   }
 
   function testRequestSingleConsolidation() public {
@@ -139,6 +152,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
 
     address _user = vm.addr(0x1);
     owrETH.grantRoles(_user, owrETH.CONSOLIDATION_ROLE());
+    uint256 realFee = consolidationMock.fakeExponential(0);
 
     vm.deal(_user, 1 ether);
     vm.startPrank(_user);
@@ -151,19 +165,19 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
     bytes[] memory requestsMade = consolidationMock.getRequests();
     assertEq(requestsMade.length, 1);
     assertEq(requestsMade[0], encodedData);
-    assertEq(address(consolidationMock).balance, 2 wei);
-    assertEq(_user.balance, 1 ether - 2 wei);
+    assertEq(address(consolidationMock).balance, realFee);
+    assertEq(_user.balance, 1 ether - realFee);
   }
 
   function testRequestBatchConsolidation() public {
-    uint256 excessFee = 100 wei;
-    uint256 expectedTotalFee;
     uint256 numRequests = 10;
+    uint256 expectedTotalFee;
+    uint256 excessFee = 100 wei;
     bytes[] memory srcPubkeys = new bytes[](numRequests);
     bytes memory dstPubkey = new bytes(48);
 
     for (uint8 i = 0; i < numRequests; i++) {
-      expectedTotalFee += 2 << i; // must match SystemContractMock logic
+      expectedTotalFee += consolidationMock.fakeExponential(i);
 
       bytes memory srcPubkey = new bytes(48);
       for (uint8 j = 0; j < 48; j++) {
@@ -202,11 +216,24 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
     bytes[] memory empty = new bytes[](0);
     owrETH.requestWithdrawal{value: 1 ether}(empty, new uint64[](1));
 
-    // Not enough fee (2 wei is the minimum fee)
+    // Not enough fee (1 wei is the minimum fee)
     vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector);
     bytes[] memory single = new bytes[](1);
     single[0] = new bytes(48);
-    owrETH.requestWithdrawal{value: 1 wei}(single, new uint64[](1));
+    owrETH.requestWithdrawal{value: 0}(single, new uint64[](1));
+
+    // Failed get_fee() request
+    uint256 realFee = withdrawalMock.fakeExponential(0);
+    withdrawalMock.setFailNextFeeRequest(true);
+    vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_SystemGetFee.selector);
+    owrETH.requestWithdrawal{value: realFee}(single, new uint64[](1));
+    withdrawalMock.setFailNextFeeRequest(false);
+
+    // Failed add_request() request
+    withdrawalMock.setFailNextAddRequest(true);
+    vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidWithdrawal_Failed.selector);
+    owrETH.requestWithdrawal{value: realFee}(single, new uint64[](1));
+    withdrawalMock.setFailNextAddRequest(false);
   }
 
   function testRequestSingleWithdrawal() public {
@@ -222,6 +249,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
 
     address _user = vm.addr(0x2);
     owrETH.grantRoles(_user, owrETH.WITHDRAWAL_ROLE());
+    uint256 realFee = withdrawalMock.fakeExponential(0);
 
     vm.deal(_user, 1 ether);
     vm.startPrank(_user);
@@ -234,8 +262,8 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
     bytes[] memory requestsMade = withdrawalMock.getRequests();
     assertEq(requestsMade.length, 1);
     assertEq(requestsMade[0], encodedData);
-    assertEq(address(withdrawalMock).balance, 2 wei);
-    assertEq(_user.balance, 1 ether - 2 wei);
+    assertEq(address(withdrawalMock).balance, realFee);
+    assertEq(_user.balance, 1 ether - realFee);
   }
 
   function testRequestBatchWithdrawal() public {
@@ -246,7 +274,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
     uint64[] memory amounts = new uint64[](numRequests);
 
     for (uint8 i = 0; i < numRequests; i++) {
-      expectedTotalFee += 2 << i; // must match SystemContractMock logic
+      expectedTotalFee += withdrawalMock.fakeExponential(i);
 
       bytes memory pubkey = new bytes(48);
       for (uint8 j = 0; j < 48; j++) {

diff --git a/src/test/owr/pectra/SystemContractMock.sol b/src/test/owr/pectra/SystemContractMock.sol
@@ -8,11 +8,13 @@ import "forge-std/console.sol";
 /// @dev This contract is used for testing purposes only.
 ///      The receive() function omitted intentionally to catch all requests with fallback().
 ///      Ignore the warning:
-///        Warning (3628): This contract has a payable fallback function, but no receive ether function. 
+///        Warning (3628): This contract has a payable fallback function, but no receive ether function.
 ///        Consider adding a receive ether function.
 contract SystemContractMock {
   uint256 internal immutable requestSize;
   bytes[] internal requests;
+  bool internal failNextFeeRequest;
+  bool internal failNextAddRequest;
 
   /// @notice Constructor
   /// @param _requestSize The expected request size: 96 for consolidation, 56 for withdrawal.
@@ -25,12 +27,39 @@ contract SystemContractMock {
     return requests;
   }
 
+  function setFailNextFeeRequest(bool _failNextFeeRequest) external {
+    failNextFeeRequest = _failNextFeeRequest;
+  }
+
+  function setFailNextAddRequest(bool _failNextAddRequest) external {
+    failNextAddRequest = _failNextAddRequest;
+  }
+
+  function fakeExponential(uint256 numerator) public pure returns (uint256) {
+    uint256 DENOMINATOR = 17;
+    uint256 i = 1;
+    uint256 output = 0;
+    uint256 numeratorAccum = DENOMINATOR;
+
+    while (numeratorAccum > 0) {
+      output += numeratorAccum;
+      numeratorAccum = (numeratorAccum * numerator) / (DENOMINATOR * i);
+      i += 1;
+    }
+
+    return output / DENOMINATOR;
+  }
+
   fallback(bytes calldata) external payable returns (bytes memory) {
-    // First request fee is 2 wei, then power of two
-    uint256 feeWei = 2 << requests.length;
+    uint256 feeWei = fakeExponential(requests.length);
 
     // If calldata is empty, return the fee
     if (msg.data.length == 0) {
+      if (failNextFeeRequest) {
+        failNextFeeRequest = false;
+        revert("fee request failed");
+      }
+
       return abi.encodePacked(bytes32(feeWei));
     }
 
@@ -39,6 +68,11 @@ contract SystemContractMock {
       revert("insufficient fee");
     }
 
+    if (failNextAddRequest) {
+      failNextAddRequest = false;
+      revert("add request failed");
+    }
+
     // If calldata is not empty, consider it as a valid request
     if (msg.data.length != requestSize) {
       console.log("invalid calldata length, expected: ", requestSize, " received: ", msg.data.length);