feat: Add Compound V3 Position Manager for EVM #380
Conversation
WalkthroughThis update introduces the CompoundV3PositionManager library and its comprehensive test suite, including integration and mock contracts. The changes add new interface definitions for Compound V3, expand environment configuration, and update the .gitignore to exclude sensitive environment files. No modifications were made to existing logic outside these additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Processor
participant CompoundV3PositionManager
participant InputAccount
participant CompoundV3Market
participant OutputAccount
Processor->>CompoundV3PositionManager: supply(amount)
CompoundV3PositionManager->>InputAccount: execute(approve + supply)
InputAccount->>CompoundV3Market: approve(asset, amount)
InputAccount->>CompoundV3Market: supply(asset, amount)
Processor->>CompoundV3PositionManager: withdraw(amount)
CompoundV3PositionManager->>InputAccount: execute(withdraw)
InputAccount->>CompoundV3Market: withdraw(asset, amount, OutputAccount)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
solidity/test/mocks/MockBaseAccount.sol (2)
13-19: Consider adding input validation for robustness.The
executefunction accepts any parameters without validation. While this is acceptable for a mock contract, consider adding basic checks to make tests more reliable:function execute(address target, uint256 value, bytes calldata data) external returns (bytes memory result) { + require(target != address(0), "Invalid target address"); ExecuteParams memory params = ExecuteParams({target: target, value: value, data: data});
21-22: Consider adding a withdrawal function for test cleanup.The contract can receive ETH but has no withdrawal mechanism. While this might be intentional for testing, consider adding a simple withdrawal function to prevent ETH from being locked in test scenarios.
solidity/test/mocks/MockCompoundV3Market.sol (1)
4-4: Remove unused import.The
IERC20interface is imported but not used in this contract. Consider removing it to keep the code clean.-import {IERC20} from "forge-std/src/interfaces/IERC20.sol";solidity/test/libraries/CompoundV3PositionManager.t.sol (1)
56-56: Remove or uncomment the commented codeThis line appears to be commented out. If the library approval is needed for the tests, it should be uncommented. Otherwise, remove it to keep the code clean.
solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol (1)
31-31: Add error handling for missing environment variableConsider using
vm.envOrto provide a default or skip the test if the environment variable is not set.- vm.createSelectFork(vm.envString("MAINNET_RPC_URL"), 22638149); // Adjust the block number as needed + string memory rpcUrl = vm.envOr("MAINNET_RPC_URL", string("")); + if (bytes(rpcUrl).length == 0) { + vm.skip(true); + return; + } + vm.createSelectFork(rpcUrl, 22638149); // Adjust the block number as neededsolidity/src/libraries/CompoundV3PositionManager.sol (1)
140-141: Clean up comment formattingRemove the extra slashes in the comment.
- // // get the withdrawable amount of base asset from the market + // get the withdrawable amount of base asset from the marketsolidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol (2)
217-217: Consider reordering constants for better readability.
MAX_COLLATERAL_FACTORreferencesFACTOR_SCALEwhich is defined later on line 242. While this works in Solidity, reordering constants by dependency would improve code readability.
321-470: Well-structured interface with comprehensive error handling.The interface is well-designed with extensive custom errors and properly indexed events. The modular separation of concerns (Configuration, Math, Storage, Core) provides good architectural foundation.
Consider documenting the rationale for having separate internal storage variables (lines 161-169) alongside the TotalsBasic struct to help future maintainers understand this design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
solidity/.env.example(1 hunks)solidity/.gitignore(1 hunks)solidity/src/libraries/CompoundV3PositionManager.sol(1 hunks)solidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol(1 hunks)solidity/test/libraries/CompoundV3PositionManager.t.sol(1 hunks)solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol(1 hunks)solidity/test/mocks/MockBaseAccount.sol(1 hunks)solidity/test/mocks/MockCompoundV3Market.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-deploy
- GitHub Check: setup-local-ic
🔇 Additional comments (8)
solidity/.gitignore (1)
9-9: LGTM! Good security practice.Adding
.envto gitignore prevents sensitive environment variables from being accidentally committed to version control.solidity/.env.example (1)
1-1: LGTM! Standard environment variable documentation.The placeholder format clearly indicates what type of value is expected for the mainnet RPC URL configuration.
solidity/test/mocks/MockCompoundV3Market.sol (1)
6-11: LGTM! Appropriate minimal mock design.The contract provides the necessary
baseTokenaddress functionality for testing the Compound V3 Position Manager without unnecessary complexity.solidity/test/libraries/CompoundV3PositionManager.t.sol (1)
203-276: Well-structured supply testsThe supply tests comprehensively cover all scenarios including exact amounts, zero amounts (entire balance), approval verification, and access control. The use of revert patterns to inspect internal call parameters is clever.
solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol (1)
61-152: Excellent integration test coverageThe integration tests effectively validate real protocol interactions including:
- Supply operations with zero and exact amounts
- Interest accrual over time
- Withdraw operations with accumulated interest
The helper functions are well-organized and the tests provide confidence in the implementation.
solidity/src/libraries/CompoundV3PositionManager.sol (2)
48-73: Robust configuration validationThe validation logic properly checks all critical configuration parameters:
- Non-zero addresses for all accounts and market proxy
- Verification that the base asset matches the market's base token
This ensures the contract cannot be misconfigured.
98-116: Well-implemented supply logicThe supply implementation correctly:
- Handles zero amount as "supply entire balance"
- Approves the exact amount before supplying
- Works for both base assets and collateral
- Uses memory storage for gas efficiency
solidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol (1)
160-169: Verify the storage layout pattern with duplicated fields.The individual storage variables duplicate the fields from the
TotalsBasicstruct. While this pattern can be used for gas optimization or upgradeability, it requires careful synchronization to avoid inconsistencies.Please confirm this duplication is intentional and that the implementation properly manages both the struct and individual fields to maintain consistency.
MMujtabaRoohani-BlockApex
force-pushed
the
main
branch
from
08a985e to
f25bc7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
solidity/src/libraries/Splitter.sol (1)
143-154: Consider optimizing the validation logic to avoid redundant checks.The second validation loop checks the same token constraints multiple times if there are multiple splits for the same token. Consider tracking which tokens have been validated to avoid redundant checks.
+ // Track validated tokens to avoid redundant checks + mapping(IERC20 => bool) memory validatedTokens; + // checking if sum of all ratios is 1 and conflicting types are not provided for (uint256 i = 0; i < splits.length; i++) { SplitConfig memory split = splits[i]; + // Skip if token already validated + if (validatedTokens[split.token]) continue; + validatedTokens[split.token] = true; + if(split.splitType == SplitType.FixedAmount) { require(tokenRatioSplitSum[split.token] == 0, "Invalid split config: cannot combine different split types for same token."); } else if(split.splitType == SplitType.FixedRatio) { require(tokenRatioSplitSum[split.token] == 10 ** DECIMALS, "Invalid split config: sum of ratios is not equal to 1."); require(tokenAmountSplitSum[split.token] == 0, "Invalid split config: cannot combine different split types for same token."); } else { require(tokenAmountSplitSum[split.token] == 0 && tokenRatioSplitSum[split.token] == 0, "Invalid split config: cannot combine different split types for same token."); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
solidity/src/libraries/Splitter.sol(1 hunks)solidity/test/libraries/Splitter.t.sol(1 hunks)
🔇 Additional comments (2)
solidity/src/libraries/Splitter.sol (1)
9-14: Verify PR title consistency.The PR title mentions "Compound V3 Position Manager" but this contract implements a Splitter library for distributing funds. Please ensure the PR title accurately reflects the changes.
Likely an incorrect or invalid review comment.
solidity/test/libraries/Splitter.t.sol (1)
1-340: Well-structured and comprehensive test coverage!The tests thoroughly cover all validation scenarios and edge cases for the Splitter configuration logic. The test naming convention is clear and follows best practices.
MMujtabaRoohani-BlockApex
force-pushed
the
main
branch
3 times, most recently
from
f25bc7a to
ede0d8a
Compare
Co-authored-by: HareemSaad <[email protected]> Co-authored-by: Ayush Shaw <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
solidity/test/libraries/CompoundV3PositionManager.t.sol (2)
315-315:⚠️ Potential issueReplace UINT256_MAX with type(uint256).max.
Based on the codebase standards, use
type(uint256).maxinstead of the undefinedUINT256_MAX.- CometMainInterface.withdrawTo.selector, address(outputAccount), address(baseToken), UINT256_MAX + CometMainInterface.withdrawTo.selector, address(outputAccount), address(baseToken), type(uint256).max
371-371:⚠️ Potential issueReplace UINT256_MAX with type(uint256).max.
Use
type(uint256).maxfor consistency with the codebase standards.- abi.encodeWithSelector(CometMainInterface.withdrawTo.selector, address(outputAccount), token, UINT256_MAX); + abi.encodeWithSelector(CometMainInterface.withdrawTo.selector, address(outputAccount), token, type(uint256).max);
🧹 Nitpick comments (7)
solidity/test/libraries/CompoundV3PositionManager.t.sol (3)
4-4: Remove unusedconsoleimport.The
consoleimport from forge-std is not used anywhere in this test file.-import {Test, console} from "forge-std/src/Test.sol"; +import {Test} from "forge-std/src/Test.sol";
56-56: Remove commented-out code.This commented line should be removed to maintain code cleanliness.
compoundV3PositionManager = new CompoundV3PositionManager(owner, processor, abi.encode(config)); - // inputAccount.approveLibrary(address(compoundV3PositionManager)); vm.stopPrank();
214-215: Consider using a clearer pattern for verifying the number of execute calls.The current pattern of using
vm.expectRevert()to verify that only a specific number of calls were made is not immediately clear. Consider adding a helper function or using a more explicit assertion.Example helper function:
function assertExecuteCallCount(MockBaseAccount account, uint256 expectedCount) internal { // Try to access one more than expected count vm.expectRevert(); account.executeParams(expectedCount); }Then use it in tests:
- vm.expectRevert(); - MockBaseAccount(inputAccount).executeParams(2); + assertExecuteCallCount(inputAccount, 2);Also applies to: 235-236, 256-257, 289-290, 309-310, 345-346, 365-366, 403-404, 425-426, 446-447
solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol (3)
31-31: Consider using a more recent block number or making it configurable.The hardcoded block number
22638149may become stale over time. Consider using a more recent block or making it configurable through environment variables to ensure tests remain valid as the blockchain progresses.- vm.createSelectFork(vm.envString("MAINNET_RPC_URL"), 22638149); // Adjust the block number as needed + vm.createSelectFork(vm.envString("MAINNET_RPC_URL"), vm.envOr("FORK_BLOCK_NUMBER", uint256(22638149)));
81-81: Fix inconsistent comment in assertion.The comment states "500 cUSDC" but the deal amount is 1000 USDC, making the assertion expect approximately 1000 cUSDC.
- assertApproxEqAbs(cusdBalance, dealAmount, 2, "Input account should have approximately 500 cUSDC after supply"); + assertApproxEqAbs(cusdBalance, dealAmount, 2, "Input account should have approximately 1000 cUSDC after supply");
86-87: Fix inconsistent variable naming and comments.The comment on line 87 states "500 USDC" but the
supplyAmountis actually 400e6 (400 USDC). This inconsistency could confuse readers.uint256 dealAmount = 500e6; // 500 USDC - uint256 supplyAmount = 400e6; // 500 USDC + uint256 supplyAmount = 400e6; // 400 USDCsolidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol (1)
191-312: Well-structured core contract with comprehensive constants.The helper functions for value conversions are mathematically sound. The overflow warnings in comments (lines 299-300, 307-308) are valuable for maintainers.
Minor suggestion: Consider grouping the scale-related constants together (lines 233-242) for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
solidity/.env.example(1 hunks)solidity/.gitignore(1 hunks)solidity/src/libraries/CompoundV3PositionManager.sol(1 hunks)solidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol(1 hunks)solidity/test/libraries/CompoundV3PositionManager.t.sol(1 hunks)solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol(1 hunks)solidity/test/mocks/MockBaseAccount.sol(1 hunks)solidity/test/mocks/MockCompoundV3Market.sol(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- solidity/.env.example
- solidity/.gitignore
- solidity/test/mocks/MockCompoundV3Market.sol
🧰 Additional context used
🧠 Learnings (1)
solidity/test/libraries/CompoundV3PositionManager.t.sol (6)
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:315-315
Timestamp: 2025-06-05T13:22:22.057Z
Learning: In Foundry test files, UINT256_MAX is a predefined constant provided by the Test.sol contract from forge-std. Do not suggest replacing it with type(uint256).max as it's already available and commonly used in Foundry tests.
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:315-315
Timestamp: 2025-06-05T13:22:22.058Z
Learning: In Foundry test files that inherit from forge-std/Test.sol, UINT256_MAX is a predefined constant provided by the Test contract. Do not suggest replacing it with type(uint256).max as it's already available and commonly used in Foundry tests.
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:315-315
Timestamp: 2025-06-05T13:22:22.058Z
Learning: In Foundry test files that inherit from forge-std/Test.sol, UINT256_MAX is a predefined constant provided by the Test contract. Do not suggest replacing it with type(uint256).max as it's already available and commonly used in Foundry tests.
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:315-315
Timestamp: 2025-06-05T13:22:22.058Z
Learning: UINT256_MAX is not provided as a public constant in Foundry's forge-std/Test.sol. It exists as a private constant in StdCheats.sol but is not accessible outside that contract. The standard approach is to use type(uint256).max directly or define UINT256_MAX locally in test contracts.
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:315-315
Timestamp: 2025-06-05T13:22:22.058Z
Learning: UINT256_MAX is not provided as a public constant in Foundry's forge-std/Test.sol. It exists as a private constant in StdCheats.sol but is not accessible outside that contract. The standard approach is to use type(uint256).max directly or define UINT256_MAX locally in test contracts.
Learnt from: HareemSaad
PR: timewave-computer/valence-protocol#380
File: solidity/test/libraries/CompoundV3PositionManager.t.sol:0-0
Timestamp: 2025-06-05T13:22:38.034Z
Learning: In the valence-protocol codebase, the standard approach for representing the maximum uint256 value is `type(uint256).max`, which is used consistently throughout the test files rather than any custom UINT256_MAX constant.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: setup-local-ic
- GitHub Check: solidity contracts
- GitHub Check: build-and-deploy
🔇 Additional comments (12)
solidity/test/libraries/integration_tests/CompoundV3PositionManager.t.sol (2)
112-138: Excellent integration test for interest accrual.This test effectively validates the core functionality of the Compound V3 position manager by:
- Supplying assets to earn interest
- Simulating time progression to allow interest accrual
- Verifying that withdrawal includes both principal and earned interest
- Ensuring proper fund transfer to the output account
The time manipulation logic and interest verification demonstrate thorough testing of the position manager's real-world behavior.
157-163: Well-implemented time manipulation for testing.The
_skipTimefunction correctly simulates both timestamp advancement and block progression, accounting for the typical 12-second block time. This is essential for testing interest accrual in time-dependent DeFi protocols.solidity/src/libraries/CompoundV3PositionManager.sol (5)
42-73: Excellent configuration validation with comprehensive checks.The
validateConfigfunction implements robust validation:
- Zero address checks for all critical components
- Verification that the provided base asset matches the market's actual base token
- Clear, descriptive error messages
This validation prevents configuration errors that could lead to failed transactions or incorrect behavior.
82-84: Clean separation of concerns in public functions.The public
supplyfunction correctly delegates to the internal_supplyfunction, maintaining a clean interface while allowing code reuse between base asset and collateral operations.
99-114: Well-implemented supply logic with proper abstractions.The
_supplyfunction demonstrates several best practices:
- Handles zero amount as "use entire balance" semantics
- Properly approves tokens before supply operation
- Uses BaseAccount abstraction for secure execution
- Correctly encodes calls to the Compound V3 interface
The implementation correctly handles both base assets and collateral tokens through the same logic path.
135-146: Correct withdrawal implementation using Compound V3 patterns.The withdrawal logic properly:
- Uses
type(uint256).maxfor withdrawing entire position (Compound V3 convention)- Calls
withdrawToto send funds directly to the output account- Maintains consistency with the supply pattern using BaseAccount execution
152-164: Proper configuration management with access controls.The configuration management functions correctly:
- Use
onlyOwneraccess control for updates- Validate configuration changes through the same validation logic
- Follow the Library pattern for initialization
This ensures only authorized parties can modify the position manager's behavior while maintaining consistency.
solidity/test/mocks/MockBaseAccount.sol (1)
4-23: LGTM! Well-structured mock contract for testing.The mock implementation correctly captures execution parameters for verification in tests without performing actual executions, which is the appropriate behavior for a test mock.
solidity/src/libraries/interfaces/compoundV3/CometMainInterface.sol (4)
10-49: Configuration structures are well-designed.The use of packed structs with appropriately sized types (uint64, uint104, etc.) optimizes storage efficiency. The bytes32 fields for name and symbol in ExtConfiguration are a gas-efficient choice.
58-111: Robust math utility functions with proper bounds checking.All casting functions correctly validate inputs before conversion, preventing overflow/underflow issues. The custom errors provide clear failure reasons.
120-187: Excellent storage optimization with packed structs.The storage layout maximizes efficiency through careful struct packing. The explicit slot comments (e.g., "512 bits total = 2 slots") help maintainers understand the optimization strategy.
321-470: Comprehensive and well-organized protocol interface.The interface provides a complete set of functions for interacting with the Compound V3 protocol. The error definitions are descriptive, and events are properly structured with appropriate indexed parameters for efficient log filtering.
Summary by CodeRabbit
New Features
Bug Fixes
Tests