Fix code review issues for redemptions

contracts/interfaces/IEthMetaVaultFactory.sol

@@ -10,11 +10,13 @@ pragma solidity ^0.8.22;
 interface IEthMetaVaultFactory {
     /**
      * @notice Event emitted on a MetaVault creation
+     * @dev the caller address is redundant, but keep it in the event for backward compatibility
+     * @param caller The address of the factory caller
      * @param admin The address of the Vault admin
      * @param vault The address of the created Vault
      * @param params The encoded parameters for initializing the Vault contract
      */
-    event MetaVaultCreated(address indexed admin, address indexed vault, bytes params);
+    event MetaVaultCreated(address indexed caller, address indexed admin, address indexed vault, bytes params);
 
     /**
      * @notice The address of the Vault implementation contract used for proxy creation

contracts/interfaces/IGnoMetaVaultFactory.sol

@@ -10,11 +10,13 @@ pragma solidity ^0.8.22;
 interface IGnoMetaVaultFactory {
     /**
      * @notice Event emitted on a MetaVault creation
+     * @dev the caller address is redundant, but keep it in the event for backward compatibility
+     * @param caller The address of the factory caller
      * @param admin The address of the Vault admin
      * @param vault The address of the created Vault
      * @param params The encoded parameters for initializing the Vault contract
      */
-    event MetaVaultCreated(address indexed admin, address indexed vault, bytes params);
+    event MetaVaultCreated(address indexed caller, address indexed admin, address indexed vault, bytes params);
 
     /**
      * @notice The address of the Vault implementation contract used for proxy creation

contracts/libraries/SubVaultUtils.sol

@@ -108,6 +108,60 @@ library SubVaultUtils {
         }
     }
 
+    /**
+     * @dev Calculates the required sub-vaults exit requests to fulfill the assets to redeem
+     * @param subVaultsStates The mapping of sub-vault addresses to their states
+     * @param subVaultsCurator The address of the sub-vaults curator
+     * @param vaults The addresses of the sub-vaults
+     * @param assetsToRedeem The amount of assets to redeem
+     * @param withdrawableAssets The amount of withdrawable assets in the meta vault
+     * @param ejectingSubVault The address of the ejecting sub-vault
+     * @param ejectingSubVaultShares The shares of the ejecting sub-vault
+     * @return redeemRequests The array of sub-vaults exit requests
+     */
+    function calculateSubVaultsRedemptions(
+        mapping(address vault => IVaultSubVaults.SubVaultState state) storage subVaultsStates,
+        address subVaultsCurator,
+        address[] memory vaults,
+        uint256 assetsToRedeem,
+        uint256 withdrawableAssets,
+        address ejectingSubVault,
+        uint256 ejectingSubVaultShares
+    ) external view returns (ISubVaultsCurator.ExitRequest[] memory redeemRequests) {
+        // check whether enough assets available
+        unchecked {
+            assetsToRedeem -= Math.min(assetsToRedeem, withdrawableAssets);
+        }
+        if (assetsToRedeem == 0) {
+            // if enough withdrawable assets, return empty array
+            return redeemRequests;
+        }
+
+        // check whether ejecting shares can be consumed
+        if (ejectingSubVault != address(0) && ejectingSubVaultShares != 0) {
+            uint256 ejectingVaultAssets = IVaultState(ejectingSubVault).convertToAssets(ejectingSubVaultShares);
+            unchecked {
+                assetsToRedeem -= Math.min(assetsToRedeem, ejectingVaultAssets);
+            }
+        }
+
+        if (assetsToRedeem == 0) {
+            // if no assets to redeem, return empty array
+            return redeemRequests;
+        }
+
+        // check vaults length
+        uint256 vaultsLength = vaults.length;
+        if (vaultsLength == 0) revert Errors.EmptySubVaults();
+
+        // fetch current sub-vaults balances
+        uint256[] memory balances;
+        (balances,) = getSubVaultsBalances(subVaultsStates, vaults, false);
+
+        // fetch redeems from the curator
+        return ISubVaultsCurator(subVaultsCurator).getExitRequests(assetsToRedeem, vaults, balances, ejectingSubVault);
+    }
+
     /**
      * @dev Processes the given redeem requests
      * @param subVaultsStates The mapping of sub-vault addresses to their states
@@ -217,73 +271,6 @@ library SubVaultUtils {
         }
     }
 
-    /**
-     * @dev Internal function to check whether a sub-vault is collateralized
-     * @param subVault The address of the sub-vault
-     * @return true if the sub-vault is collateralized
-     */
-    function _isSubVaultCollateralized(address keeper, address subVault) private view returns (bool) {
-        try IVaultSubVaults(subVault).isCollateralized() returns (bool collateralized) {
-            return collateralized;
-        } catch {}
-
-        return IKeeperRewards(keeper).isCollateralized(subVault);
-    }
-
-    /**
-     * @dev Calculates the required sub-vaults exit requests to fulfill the assets to redeem
-     * @param subVaultsStates The mapping of sub-vault addresses to their states
-     * @param subVaultsCurator The address of the sub-vaults curator
-     * @param vaults The addresses of the sub-vaults
-     * @param assetsToRedeem The amount of assets to redeem
-     * @param withdrawableAssets The amount of withdrawable assets in the meta vault
-     * @param ejectingSubVault The address of the ejecting sub-vault
-     * @param ejectingSubVaultShares The shares of the ejecting sub-vault
-     * @return redeemRequests The array of sub-vaults exit requests
-     */
-    function calculateSubVaultsRedemptions(
-        mapping(address vault => IVaultSubVaults.SubVaultState state) storage subVaultsStates,
-        address subVaultsCurator,
-        address[] memory vaults,
-        uint256 assetsToRedeem,
-        uint256 withdrawableAssets,
-        address ejectingSubVault,
-        uint256 ejectingSubVaultShares
-    ) external view returns (ISubVaultsCurator.ExitRequest[] memory redeemRequests) {
-        // check whether enough assets available
-        unchecked {
-            assetsToRedeem -= Math.min(assetsToRedeem, withdrawableAssets);
-        }
-        if (assetsToRedeem == 0) {
-            // if enough withdrawable assets, return empty array
-            return redeemRequests;
-        }
-
-        // check whether ejecting shares can be consumed
-        if (ejectingSubVault != address(0) && ejectingSubVaultShares != 0) {
-            uint256 ejectingVaultAssets = IVaultState(ejectingSubVault).convertToAssets(ejectingSubVaultShares);
-            unchecked {
-                assetsToRedeem -= Math.min(assetsToRedeem, ejectingVaultAssets);
-            }
-        }
-
-        if (assetsToRedeem == 0) {
-            // if no assets to redeem, return empty array
-            return redeemRequests;
-        }
-
-        // check vaults length
-        uint256 vaultsLength = vaults.length;
-        if (vaultsLength == 0) revert Errors.EmptySubVaults();
-
-        // fetch current sub-vaults balances
-        uint256[] memory balances;
-        (balances,) = getSubVaultsBalances(subVaultsStates, vaults, false);
-
-        // fetch redeems from the curator
-        return ISubVaultsCurator(subVaultsCurator).getExitRequests(assetsToRedeem, vaults, balances, ejectingSubVault);
-    }
-
     /**
      * @dev Ejects a sub-vault from the meta vault
      * @param subVaults The set of currently added sub-vaults
@@ -337,4 +324,17 @@ library SubVaultUtils {
             return (true, 0);
         }
     }
+
+    /**
+     * @dev Internal function to check whether a sub-vault is collateralized
+     * @param subVault The address of the sub-vault
+     * @return true if the sub-vault is collateralized
+     */
+    function _isSubVaultCollateralized(address keeper, address subVault) private view returns (bool) {
+        try IVaultSubVaults(subVault).isCollateralized() returns (bool collateralized) {
+            return collateralized;
+        } catch {}
+
+        return IKeeperRewards(keeper).isCollateralized(subVault);
+    }
 }

contracts/tokens/EthOsTokenRedeemer.sol

@@ -2,10 +2,10 @@
 
 pragma solidity ^0.8.22;
 
-import {IEthOsTokenRedeemer} from "../interfaces/IEthOsTokenRedeemer.sol";
-import {OsTokenRedeemer} from "./OsTokenRedeemer.sol";
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+import {IEthOsTokenRedeemer} from "../interfaces/IEthOsTokenRedeemer.sol";
+import {OsTokenRedeemer} from "./OsTokenRedeemer.sol";
 
 /**
  * @title EthOsTokenRedeemer

contracts/tokens/GnoOsTokenRedeemer.sol

@@ -2,11 +2,11 @@
 
 pragma solidity ^0.8.22;
 
-import {IGnoOsTokenRedeemer} from "../interfaces/IGnoOsTokenRedeemer.sol";
-import {OsTokenRedeemer} from "./OsTokenRedeemer.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import {IERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol";
 import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IGnoOsTokenRedeemer} from "../interfaces/IGnoOsTokenRedeemer.sol";
+import {OsTokenRedeemer} from "./OsTokenRedeemer.sol";
 
 /**
  * @title GnoOsTokenRedeemer

contracts/tokens/OsTokenRedeemer.sol

@@ -2,22 +2,22 @@
 
 pragma solidity ^0.8.22;
 
-import {Multicall} from "../base/Multicall.sol";
+import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {IERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {MerkleProof} from "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol";
+import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
+import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import {IMetaVault} from "../interfaces/IMetaVault.sol";
 import {IOsTokenRedeemer} from "../interfaces/IOsTokenRedeemer.sol";
 import {IOsTokenVaultController} from "../interfaces/IOsTokenVaultController.sol";
 import {IVaultOsToken} from "../interfaces/IVaultOsToken.sol";
 import {IVaultSubVaults} from "../interfaces/IVaultSubVaults.sol";
 import {IVaultsRegistry} from "../interfaces/IVaultsRegistry.sol";
+import {Multicall} from "../base/Multicall.sol";
 import {Errors} from "../libraries/Errors.sol";
 import {ExitQueue} from "../libraries/ExitQueue.sol";
-import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
-import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import {IERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol";
-import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import {MerkleProof} from "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol";
-import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
-import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 
 /**
  * @title OsTokenRedeemer

contracts/vaults/ethereum/EthMetaVault.sol

@@ -2,6 +2,9 @@
 
 pragma solidity ^0.8.22;
 
+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import {Address} from "@openzeppelin/contracts/utils/Address.sol";
+import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import {IEthMetaVault} from "../../interfaces/IEthMetaVault.sol";
 import {IEthMetaVaultFactory} from "../../interfaces/IEthMetaVaultFactory.sol";
 import {IKeeperRewards} from "../../interfaces/IKeeperRewards.sol";
@@ -12,9 +15,6 @@ import {VaultEnterExit} from "../modules/VaultEnterExit.sol";
 import {VaultState} from "../modules/VaultState.sol";
 import {VaultSubVaults} from "../modules/VaultSubVaults.sol";
 import {IVaultVersion, VaultVersion} from "../modules/VaultVersion.sol";
-import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
-import {Address} from "@openzeppelin/contracts/utils/Address.sol";
-import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
 /**
  * @title EthMetaVault

contracts/vaults/ethereum/EthMetaVaultFactory.sol

@@ -50,6 +50,6 @@ contract EthMetaVaultFactory is IEthMetaVaultFactory {
         _vaultsRegistry.addVault(vault);
 
         // emit event
-        emit MetaVaultCreated(msg.sender, vault, params);
+        emit MetaVaultCreated(msg.sender, msg.sender, vault, params);
     }
 }

contracts/vaults/ethereum/EthPrivMetaVault.sol

@@ -2,14 +2,14 @@
 
 pragma solidity ^0.8.22;
 
+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import {IEthMetaVaultFactory} from "../../interfaces/IEthMetaVaultFactory.sol";
 import {IEthPrivMetaVault} from "../../interfaces/IEthPrivMetaVault.sol";
 import {IVaultOsToken, VaultOsToken} from "../modules/VaultOsToken.sol";
 import {IVaultVersion} from "../modules/VaultVersion.sol";
 import {VaultWhitelist} from "../modules/VaultWhitelist.sol";
 import {EthMetaVault, IEthMetaVault} from "./EthMetaVault.sol";
-import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
-import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
 /**
  * @title EthPrivMetaVault
@@ -41,6 +41,7 @@ contract EthPrivMetaVault is Initializable, EthMetaVault, VaultWhitelist, IEthPr
         override(IEthMetaVault, EthMetaVault)
         reinitializer(_version)
     {
+        // do not check for the upgrades since this is the first implementation of EthPrivMetaVault
         // initialize deployed vault
         address _admin = IEthMetaVaultFactory(msg.sender).vaultAdmin();
         __EthMetaVault_init(_admin, abi.decode(params, (MetaVaultInitParams)));

contracts/vaults/gnosis/GnoMetaVault.sol

@@ -2,6 +2,9 @@
 
 pragma solidity ^0.8.22;
 
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import {IGnoMetaVault} from "../../interfaces/IGnoMetaVault.sol";
 import {IGnoMetaVaultFactory} from "../../interfaces/IGnoMetaVaultFactory.sol";
 import {IVaultGnoStaking} from "../../interfaces/IVaultGnoStaking.sol";
@@ -11,9 +14,6 @@ import {VaultEnterExit} from "../modules/VaultEnterExit.sol";
 import {VaultState} from "../modules/VaultState.sol";
 import {IVaultSubVaults, VaultSubVaults} from "../modules/VaultSubVaults.sol";
 import {IVaultVersion, VaultVersion} from "../modules/VaultVersion.sol";
-import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
-import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /**
  * @title GnoMetaVault

contracts/vaults/gnosis/GnoMetaVaultFactory.sol

@@ -65,6 +65,6 @@ contract GnoMetaVaultFactory is IGnoMetaVaultFactory {
         _vaultsRegistry.addVault(vault);
 
         // emit event
-        emit MetaVaultCreated(msg.sender, vault, params);
+        emit MetaVaultCreated(msg.sender, msg.sender, vault, params);
     }
 }

contracts/vaults/gnosis/GnoPrivMetaVault.sol

@@ -2,13 +2,13 @@
 
 pragma solidity ^0.8.22;
 
+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import {IGnoMetaVaultFactory} from "../../interfaces/IGnoMetaVaultFactory.sol";
 import {IGnoPrivMetaVault} from "../../interfaces/IGnoPrivMetaVault.sol";
 import {IVaultOsToken, VaultOsToken} from "../modules/VaultOsToken.sol";
 import {IVaultVersion, VaultVersion} from "../modules/VaultVersion.sol";
 import {VaultWhitelist} from "../modules/VaultWhitelist.sol";
 import {GnoMetaVault, IGnoMetaVault} from "./GnoMetaVault.sol";
-import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
 /**
  * @title GnoPrivMetaVault
@@ -38,6 +38,7 @@ contract GnoPrivMetaVault is Initializable, GnoMetaVault, VaultWhitelist, IGnoPr
         override(IGnoMetaVault, GnoMetaVault)
         reinitializer(_version)
     {
+        // do not check for the upgrades since this is the first implementation of GnoPrivMetaVault
         // initialize deployed vault
         address _admin = IGnoMetaVaultFactory(msg.sender).vaultAdmin();
         __GnoMetaVault_init(_admin, abi.decode(params, (MetaVaultInitParams)));

script/Network.sol

@@ -2,12 +2,12 @@
 
 pragma solidity ^0.8.22;
 
-import {IOsTokenConfig} from "../contracts/interfaces/IOsTokenConfig.sol";
-import {IVaultVersion} from "../contracts/interfaces/IVaultVersion.sol";
-import {IVaultsRegistry} from "../contracts/interfaces/IVaultsRegistry.sol";
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 import {Script} from "forge-std/Script.sol";
 import {stdJson} from "forge-std/StdJson.sol";
+import {IOsTokenConfig} from "../contracts/interfaces/IOsTokenConfig.sol";
+import {IVaultVersion} from "../contracts/interfaces/IVaultVersion.sol";
+import {IVaultsRegistry} from "../contracts/interfaces/IVaultsRegistry.sol";
 
 /**
  * @title Network

script/UpgradeEthNetwork.s.sol

@@ -2,6 +2,7 @@
 
 pragma solidity ^0.8.22;
 
+import {console} from "forge-std/console.sol";
 import {IMetaVault} from "../contracts/interfaces/IMetaVault.sol";
 import {IVaultVersion} from "../contracts/interfaces/IVaultVersion.sol";
 import {IVaultsRegistry} from "../contracts/interfaces/IVaultsRegistry.sol";
@@ -11,7 +12,6 @@ import {EthMetaVault} from "../contracts/vaults/ethereum/EthMetaVault.sol";
 import {EthMetaVaultFactory} from "../contracts/vaults/ethereum/EthMetaVaultFactory.sol";
 import {EthPrivMetaVault} from "../contracts/vaults/ethereum/EthPrivMetaVault.sol";
 import {Network} from "./Network.sol";
-import {console} from "forge-std/console.sol";
 
 contract UpgradeEthNetwork is Network {
     address public osTokenRedeemerOwner;

script/UpgradeGnoNetwork.s.sol

@@ -2,6 +2,7 @@
 
 pragma solidity ^0.8.22;
 
+import {console} from "forge-std/console.sol";
 import {IMetaVault} from "../contracts/interfaces/IMetaVault.sol";
 import {IVaultVersion} from "../contracts/interfaces/IVaultVersion.sol";
 import {IVaultsRegistry} from "../contracts/interfaces/IVaultsRegistry.sol";
@@ -11,7 +12,6 @@ import {GnoMetaVault} from "../contracts/vaults/gnosis/GnoMetaVault.sol";
 import {GnoMetaVaultFactory} from "../contracts/vaults/gnosis/GnoMetaVaultFactory.sol";
 import {GnoPrivMetaVault} from "../contracts/vaults/gnosis/GnoPrivMetaVault.sol";
 import {Network} from "./Network.sol";
-import {console} from "forge-std/console.sol";
 
 contract UpgradeGnoNetwork is Network {
     address public osTokenRedeemerOwner;