feat: add MorphoVaultV1PositionManager library#437
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MMujtabaRoohani-BlockApex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new MorphoVaultV1PositionManager Solidity library, designed to facilitate seamless and automated interaction with the Morpho Vault V1.1 protocol. It enables users to deposit and withdraw assets from Morpho vaults through a BaseAccount, ensuring efficient and secure position management. The changes also include the necessary Morpho-specific interfaces and a comprehensive test suite to validate the library's functionality and configuration handling.
Highlights
- New Library: MorphoVaultV1PositionManager: Introduced a new Solidity library to manage positions within the Morpho Vault V1.1 protocol, enabling automated deposit and withdrawal operations.
- Integration with BaseAccount: The new position manager leverages the existing BaseAccount contract to initiate and execute transactions, ensuring consistent account interaction.
- Configuration Management and Validation: The library includes a robust configuration mechanism with validation checks for critical parameters such as vault address, input/output accounts, and asset addresses, enhancing security and preventing misconfigurations.
- New Morpho Interfaces: Added
IMetaMorphoV1_1.solandIMorpho.solinterfaces to provide the necessary contract definitions for interacting with the Morpho Vault V1.1 and core Morpho protocol. - Comprehensive Test Coverage: A dedicated test suite,
MorphoVaultV1PositionManager.t.sol, has been added to thoroughly test the new library's functionalities, including configuration updates, balance retrieval, deposit, and withdrawal processes, along with various revert conditions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a MorphoVaultV1PositionManager library to interact with Morpho Vault V1.1, along with necessary interfaces and comprehensive tests. The implementation is solid, but there are several areas for improvement regarding security, efficiency, and code clarity. Key points include addressing a potential vulnerability in the ERC20 approval mechanism, optimizing a view function for gas savings, improving code maintainability by using custom errors and correcting some minor copy-paste errors in comments and tests. There is also a potential license compatibility issue that should be reviewed.
| //Approve the MorphoVaultV1 to spend the base asset from the input account | ||
| bytes memory encodedApproveCall = abi.encodeCall(IERC20.approve, (storedConfig.vaultAddress, depositAmount)); | ||
|
|
||
| storedConfig.inputAccount.execute(address(asset), 0, encodedApproveCall); |
There was a problem hiding this comment.
The use of IERC20.approve can be unsafe for certain tokens (like USDT) that don't revert when changing a non-zero allowance. This can lead to a race condition. It's highly recommended to use a safe approval mechanism. Since you are encoding the call, you can't directly use OpenZeppelin's SafeERC20 library.
A common pattern to mitigate this is to first approve zero, and then the desired amount. However, this would require two separate execute calls, increasing gas costs.
Consider if the BaseAccount can be extended with a safeApprove function or a multicall capability. A simpler, though less flexible, approach would be to approve type(uint256).max once during configuration and remove the repeated approve calls from the deposit function.
There was a problem hiding this comment.
the inputAccount is meant to be interacting with only this library hence we don't expect a non-zero approval before or after this function call.
| @@ -0,0 +1,268 @@ | |||
| // SPDX-License-Identifier: GPL-2.0-or-later | |||
There was a problem hiding this comment.
The SPDX license identifier is GPL-2.0-or-later, while other new files in this PR like MorphoVaultV1PositionManager.sol use Apache-2.0. Using code with a GPL license can have implications for the licensing of the entire project, as GPL is a 'copyleft' license. Please ensure this is intentional and compliant with your project's licensing policy. This also applies to solidity/src/libraries/interfaces/morpho/IMorpho.sol.
There was a problem hiding this comment.
Yes, this is intentional, the interfaces are taken from Morpho and carries the license from there.
| if (decodedConfig.vaultAddress == address(0)) { | ||
| revert("Vault address can't be zero address"); | ||
| } |
There was a problem hiding this comment.
For gas efficiency and better error handling, it's recommended to use custom errors instead of revert strings. This applies to all require and revert statements in the contract.
For example:
error VaultAddressIsZero();
// ...
if (decodedConfig.vaultAddress == address(0)) {
revert VaultAddressIsZero();
}There was a problem hiding this comment.
We follow the string practice as it is easy to debug and these functions are meant to be used in rare conditions.
| function balance() public view returns (uint256) { | ||
| return IMetaMorphoV1_1(config.vaultAddress).previewRedeem( | ||
| IMetaMorphoV1_1(config.vaultAddress).balanceOf(address(config.inputAccount)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The balance function can be optimized for gas efficiency. The current implementation results in multiple SLOAD operations. Caching the config struct in memory will reduce gas costs by performing a single SLOAD for the struct and then reading from memory.
function balance() public view returns (uint256) {
MorphoVaultV1PositionManagerConfig memory storedConfig = config;
IMetaMorphoV1_1 vault = IMetaMorphoV1_1(storedConfig.vaultAddress);
return vault.previewRedeem(vault.balanceOf(address(storedConfig.inputAccount)));
}
There was a problem hiding this comment.
Perfect, made the balance function external view function so we don't have to worry about gas.
Co-authored-by: HareemSaad <hareems20014@outlook.com>
MMujtabaRoohani-BlockApex
force-pushed
the
main
branch
from
891d6e4 to
023f80b
Compare
2 participants
No description provided.