Conversation
There was a problem hiding this comment.
Pull request overview
Updates repository dependencies and remappings, and applies formatting/refactors to align with the new dependency setup.
Changes:
- Moved Forge remappings from
remappings.txtintofoundry.tomland updated submodule pins/branches. - Relocated helper contracts under
src/helpers/and updated imports in tests and deployment scripts. - Replaced some upgradeable OZ utilities with non-upgradeable/transient variants and reformatted many function signatures/calls.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/VaultUserLtvTracker.t.sol | Updates import path to the helper location. |
| test/StakeHelpers.t.sol | Updates import path; formatting changes to literals/struct inits. |
| test/EthAaveLeverageStrategy.t.sol | Formatting-only changes to call/struct formatting. |
| test/BoostHelpers.t.sol | Updates import path; formatting changes to struct inits. |
| src/mocks/ComposableCowMock.sol | Formatting-only changes to function/constructor signatures. |
| src/mocks/BalancerVaultMock.sol | Formatting-only changes to constructor signature. |
| src/mocks/AaveMock.sol | Formatting-only changes to signatures. |
| src/leverage/interfaces/ILeverageStrategy.sol | Formatting-only changes to interface signatures. |
| src/leverage/LeverageStrategy.sol | Formatting-only changes to signatures/call layout (incl. try/catch formatting). |
| src/leverage/GnoAaveLeverageStrategy.sol | Formatting-only changes to proxy execute calls and function signature layout. |
| src/leverage/EthAaveLeverageStrategy.sol | Formatting-only changes to proxy execute calls and function signature layout. |
| src/leverage/AaveLeverageStrategy.sol | Formatting-only changes to signatures and proxy execute call layout. |
| src/interfaces/IStrategyProxy.sol | Formatting-only changes to interface signatures. |
| src/interfaces/IStrategiesRegistry.sol | Formatting-only changes to interface signatures. |
| src/interfaces/IMerkleDistributor.sol | Formatting-only changes to interface signatures. |
| src/helpers/interfaces/IBoostHelpers.sol | Fixes relative import path and formats signature. |
| src/helpers/VaultUserLtvTracker.sol | Formatting-only changes to constructor signature. |
| src/helpers/StakeHelpers.sol | Formatting-only changes to constructor signature. |
| src/helpers/BoostHelpers.sol | Fixes relative import paths after helpers move; minor formatting. |
| src/converters/interfaces/IComposableCow.sol | Formatting-only changes to interface signature. |
| src/converters/interfaces/IBaseTokensConverter.sol | Formatting-only changes to interface signature. |
| src/converters/BaseTokensConverter.sol | Switches reentrancy guard implementation and removes reentrancy init; formatting. |
| src/StrategyProxy.sol | Switches OZ imports and reentrancy guard implementation; formats signatures. |
| src/StrategiesRegistry.sol | Formatting-only changes to function signatures. |
| src/OsTokenVaultEscrowAuth.sol | Formatting-only changes to constructor/function signature. |
| src/MerkleDistributor.sol | Formats function signature and proof verification if-statement layout. |
| snapshots/StrategiesRegistryTest.json | Updates gas snapshot values. |
| snapshots/MerkleDistributorTest.json | Updates gas snapshot values. |
| snapshots/EthAaveLeverageStrategyTest.json | Updates gas snapshot values. |
| script/DeployVaultUserLtvTracker.s.sol | Updates import path to the helper location. |
| script/DeployStakeHelpers.s.sol | Updates import path and formats constructor invocation. |
| script/DeployAaveBoostHelpers.s.sol | Updates import path to the helper location. |
| remappings.txt | Removed in favor of foundry.toml remappings. |
| lib/v3-core | Updates submodule commit pin. |
| lib/openzeppelin-contracts-upgradeable | Updates submodule commit pin. |
| lib/forge-std | Updates submodule commit pin. |
| lib/composable-cow | Updates submodule commit pin. |
| lib/aave-v3-origin | Updates submodule commit pin. |
| foundry.toml | Adds explicit remappings array. |
| CLAUDE.md | Adds repo guidance documentation for Claude tooling. |
| .gitmodules | Updates submodule branch tracking metadata. |
| .claude/settings.json | Adds Claude hook to auto-run forge fmt on edits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @notice Defines common functionality for converting tokens to asset token (e.g. ETH/GNO) and returning them to the Vault | ||
| */ | ||
| abstract contract BaseTokensConverter is Initializable, ReentrancyGuardUpgradeable, Multicall, IBaseTokensConverter { | ||
| abstract contract BaseTokensConverter is Initializable, ReentrancyGuardTransient, Multicall, IBaseTokensConverter { |
There was a problem hiding this comment.
Replacing ReentrancyGuardUpgradeable with ReentrancyGuardTransient changes inherited storage layout (upgradeable guard has storage; transient guard typically does not). If any concrete converter inheriting BaseTokensConverter is deployed behind a proxy and upgraded in-place, this can cause storage collisions and corrupt state. Safer options: keep ReentrancyGuardUpgradeable for upgradeable deployments, or only use ReentrancyGuardTransient in contracts that are never upgraded / always freshly deployed, and explicitly preserve layout (e.g., reserve the old storage slot(s)) in an upgrade-safe way.
| constructor( | ||
| address composableCoW, | ||
| address swapOrderHandler, | ||
| address assetToken, | ||
| address relayer | ||
| ) { |
There was a problem hiding this comment.
Replacing ReentrancyGuardUpgradeable with ReentrancyGuardTransient changes inherited storage layout (upgradeable guard has storage; transient guard typically does not). If any concrete converter inheriting BaseTokensConverter is deployed behind a proxy and upgraded in-place, this can cause storage collisions and corrupt state. Safer options: keep ReentrancyGuardUpgradeable for upgradeable deployments, or only use ReentrancyGuardTransient in contracts that are never upgraded / always freshly deployed, and explicitly preserve layout (e.g., reserve the old storage slot(s)) in an upgrade-safe way.
| function isValidSignature( | ||
| bytes32 _hash, | ||
| bytes memory signature | ||
| ) public view returns (bytes4) { |
There was a problem hiding this comment.
Replacing ReentrancyGuardUpgradeable with ReentrancyGuardTransient changes inherited storage layout (upgradeable guard has storage; transient guard typically does not). If any concrete converter inheriting BaseTokensConverter is deployed behind a proxy and upgraded in-place, this can cause storage collisions and corrupt state. Safer options: keep ReentrancyGuardUpgradeable for upgradeable deployments, or only use ReentrancyGuardTransient in contracts that are never upgraded / always freshly deployed, and explicitly preserve layout (e.g., reserve the old storage slot(s)) in an upgrade-safe way.
| function __BaseTokensConverter_init( | ||
| address _vault | ||
| ) internal onlyInitializing { | ||
| __ReentrancyGuard_init(); | ||
| if (IVaultVersion(_vault).version() < _supportedVaultVersion()) { |
There was a problem hiding this comment.
Replacing ReentrancyGuardUpgradeable with ReentrancyGuardTransient changes inherited storage layout (upgradeable guard has storage; transient guard typically does not). If any concrete converter inheriting BaseTokensConverter is deployed behind a proxy and upgraded in-place, this can cause storage collisions and corrupt state. Safer options: keep ReentrancyGuardUpgradeable for upgradeable deployments, or only use ReentrancyGuardTransient in contracts that are never upgraded / always freshly deployed, and explicitly preserve layout (e.g., reserve the old storage slot(s)) in an upgrade-safe way.
|
|
||
| import {Initializable} from '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; | ||
| import {ReentrancyGuardUpgradeable} from '@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol'; | ||
| import {Initializable} from '@openzeppelin/contracts/proxy/utils/Initializable.sol'; |
There was a problem hiding this comment.
This mixes non-upgradeable Initializable (from @openzeppelin/contracts/...) with OwnableUpgradeable (from @openzeppelin/contracts-upgradeable/...). In most setups this introduces a duplicate Initializable contract definition in the compilation unit (both packages define Initializable), which can fail compilation and/or create inconsistent initialization semantics. Prefer using the upgradeable Initializable import here (to match OwnableUpgradeable), or switch fully to non-upgradeable patterns (Ownable + non-upgradeable Initializable) to avoid duplicate symbols and inconsistent upgradeability assumptions.
| import {Initializable} from '@openzeppelin/contracts/proxy/utils/Initializable.sol'; | |
| import {Initializable} from '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; |
| * @notice Proxy contract for executing transactions on behalf of the Strategy. | ||
| */ | ||
| contract StrategyProxy is Initializable, ReentrancyGuardUpgradeable, OwnableUpgradeable, IStrategyProxy { | ||
| contract StrategyProxy is Initializable, ReentrancyGuardTransient, OwnableUpgradeable, IStrategyProxy { |
There was a problem hiding this comment.
This mixes non-upgradeable Initializable (from @openzeppelin/contracts/...) with OwnableUpgradeable (from @openzeppelin/contracts-upgradeable/...). In most setups this introduces a duplicate Initializable contract definition in the compilation unit (both packages define Initializable), which can fail compilation and/or create inconsistent initialization semantics. Prefer using the upgradeable Initializable import here (to match OwnableUpgradeable), or switch fully to non-upgradeable patterns (Ownable + non-upgradeable Initializable) to avoid duplicate symbols and inconsistent upgradeability assumptions.
| function initialize( | ||
| address initialOwner | ||
| ) external initializer { | ||
| __ReentrancyGuard_init(); | ||
| __Ownable_init(initialOwner); | ||
| } |
There was a problem hiding this comment.
This mixes non-upgradeable Initializable (from @openzeppelin/contracts/...) with OwnableUpgradeable (from @openzeppelin/contracts-upgradeable/...). In most setups this introduces a duplicate Initializable contract definition in the compilation unit (both packages define Initializable), which can fail compilation and/or create inconsistent initialization semantics. Prefer using the upgradeable Initializable import here (to match OwnableUpgradeable), or switch fully to non-upgradeable patterns (Ownable + non-upgradeable Initializable) to avoid duplicate symbols and inconsistent upgradeability assumptions.
|
Forge code coverage:
|
1 participant
No description provided.