fix: review

lidofinance · Nov 10, 2023 · 2fa5111 · 2fa5111
1 parent ff72061
commit 2fa5111
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 21 deletions.
diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol
@@ -66,6 +66,11 @@ contract CSAccountingBase {
         uint256 penaltyETH,
         uint256 coveringETH
     );
+
+    error NotOwnerToClaim(address msgSender, address owner);
+    error InvalidBlockedBondRetentionPeriod();
+    error InvalidStolenAmount();
+    error InvalidSender();
 }
 
 contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
@@ -109,11 +114,6 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
     mapping(uint256 => uint256) internal _bondShares;
     mapping(uint256 => BlockedBond) internal _blockedBondEther;
 
-    error NotOwnerToClaim(address msgSender, address owner);
-    error InvalidBlockedBondRetentionPeriod();
-    error InvalidStolenAmount();
-    error InvalidSender();
-
     /// @param commonBondSize common bond size in ETH for all node operators.
     /// @param admin admin role member address
     /// @param lidoLocator lido locator contract address
@@ -459,6 +459,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
         uint256 nodeOperatorId,
         uint256 stETHAmount
     ) external returns (uint256) {
+        // todo: can it be two functions rather than one with `from` param and condition?
         from = _validateDepositSender(from);
         return _depositStETH(from, nodeOperatorId, stETHAmount);
     }
@@ -472,6 +473,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
         uint256 stETHAmount,
         PermitInput calldata permit
     ) external returns (uint256) {
+        // todo: can it be two functions rather than one with `from` param and condition?
         from = _validateDepositSender(from);
         // preventing revert for already used permit
         if (_lido().allowance(from, address(this)) < permit.value) {
@@ -511,6 +513,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
         uint256 nodeOperatorId,
         uint256 wstETHAmount
     ) external returns (uint256) {
+        // todo: can it be two functions rather than one with `from` param and condition?
         from = _validateDepositSender(from);
         return _depositWstETH(from, nodeOperatorId, wstETHAmount);
     }
@@ -524,6 +527,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable {
         uint256 wstETHAmount,
         PermitInput calldata permit
     ) external returns (uint256) {
+        // todo: can it be two functions rather than one with `from` param and condition?
         from = _validateDepositSender(from);
         // preventing revert for already used permit
         if (WSTETH.allowance(from, address(this)) < permit.value) {

diff --git a/test/CSAccounting.blockedBond.t.sol b/test/CSAccounting.blockedBond.t.sol
@@ -432,7 +432,7 @@ contract CSAccountingTest is Test, Fixtures, CSAccountingBase {
     function test_initELRewardsStealingPenalty_revertWhenZero() public {
         _createNodeOperator({ ongoingVals: 1, withdrawnVals: 0 });
 
-        vm.expectRevert(0x7edd4cfd);
+        vm.expectRevert(InvalidStolenAmount.selector);
 
         vm.prank(admin);
         accounting.initELRewardsStealingPenalty({

diff --git a/test/CSAccounting.t.sol b/test/CSAccounting.t.sol
@@ -562,19 +562,19 @@ contract CSAccountingTest is
     }
 
     function test_depositETH_RevertIfInvalidSender() public {
-        vm.expectRevert(0xddb5de5e);
+        vm.expectRevert(InvalidSender.selector);
         vm.prank(stranger);
         accounting.depositETH{ value: 0 }(user, 0);
     }
 
     function test_depositStETH_RevertIfInvalidSender() public {
-        vm.expectRevert(0xddb5de5e);
+        vm.expectRevert(InvalidSender.selector);
         vm.prank(stranger);
         accounting.depositStETH(user, 0, 32 ether);
     }
 
     function test_depositStETHWithPermit_RevertIfInvalidSender() public {
-        vm.expectRevert(0xddb5de5e);
+        vm.expectRevert(InvalidSender.selector);
         vm.prank(stranger);
         accounting.depositStETHWithPermit(
             user,
@@ -592,13 +592,13 @@ contract CSAccountingTest is
     }
 
     function test_depositWstETH_RevertIfInvalidSender() public {
-        vm.expectRevert(0xddb5de5e);
+        vm.expectRevert(InvalidSender.selector);
         vm.prank(stranger);
         accounting.depositWstETH(user, 0, 32 ether);
     }
 
     function test_depositWstETHWithPermit_RevertIfInvalidSender() public {
-        vm.expectRevert(0xddb5de5e);
+        vm.expectRevert(InvalidSender.selector);
         vm.prank(stranger);
         accounting.depositWstETHWithPermit(
             user,
@@ -993,11 +993,7 @@ contract CSAccountingTest is
         _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 });
 
         vm.expectRevert(
-            abi.encodeWithSelector(
-                CSAccounting.NotOwnerToClaim.selector,
-                stranger,
-                user
-            )
+            abi.encodeWithSelector(NotOwnerToClaim.selector, stranger, user)
         );
         vm.prank(stranger);
         accounting.claimRewardsStETH(new bytes32[](1), 0, 1, 1 ether);
@@ -1104,11 +1100,7 @@ contract CSAccountingTest is
         _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 });
 
         vm.expectRevert(
-            abi.encodeWithSelector(
-                CSAccounting.NotOwnerToClaim.selector,
-                stranger,
-                user
-            )
+            abi.encodeWithSelector(NotOwnerToClaim.selector, stranger, user)
         );
         vm.prank(stranger);
         accounting.claimRewardsWstETH(new bytes32[](1), 0, 1, 1 ether);