fix exisiting unit tests #54
Conversation
WalkthroughThe changes in this pull request enhance the testing framework across several contracts, including Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (4)
src/test/mocks/MockFeesFactory.sol (1)
7-7: Add a comment explaining the purpose of thegovernancevariable.While the
governancevariable is correctly declared as a public address, it would be beneficial to add a comment explaining its purpose and any specific roles or permissions associated with this address.Consider adding a comment like this:
/// @notice The address of the governance controller, responsible for [explain responsibilities] address public governance;src/test/Operation.t.sol (1)
Unresolved TODOs Detected in Test Files
The following TODO comments are still present in the test files, indicating incomplete unit tests:
src/test/TestUSDCSubmitOffer.t.sol: // TODO: fuzz thissrc/test/TestUSDCOffers.t.sol: // TODO: fuzz thissrc/test/Operation.t.sol: // TODO: add additional check on strat paramssrc/test/Operation.t.sol: // TODO: implement logic to simulate earning interest.src/test/TestUSDCSellRepoToken.t.sol: // TODO: fuzz thissrc/test/TestUSDCSellRepoToken.t.sol: // TODO: validate this math (weighted time to maturity)src/test/Shutdown.t.sol: // TODO: Add tests for any emergency function added.🔗 Analysis chain
Line range hint
1-24: Review TODOs and clarify intended test fixesWhile reviewing the entire file, I noticed that there are some TODO comments present:
- Line 16:
// TODO: add additional check on strat params- Line 62:
// TODO: implement logic to simulate earning interest.These TODOs might indicate areas where the tests are incomplete or could be improved. Given that the PR objective is to "fix existing unit tests", it might be worthwhile to address these TODOs as part of this PR.
To get a complete picture of the TODOs in the test files, we can run:
This will help identify if there are other test files with pending TODOs that might need attention.
Additionally, it's not immediately clear what specific fixes this PR is intended to make to the existing unit tests. Could you please clarify:
- Are there any specific issues with the existing tests that this PR is meant to address?
- If the logging statement is the only change, how does it contribute to fixing the tests?
Clarifying these points will help ensure that the PR meets its stated objective of fixing existing unit tests.
Also applies to: 25-48, 49-73, 74-110, 113-185
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TODO comments in test files rg --type solidity '// TODO:' src/test/Length of output: 74
Script:
#!/bin/bash # Search for TODO comments in Solidity test files by targeting .sol extensions rg '// TODO:' --glob '*.sol' src/test/Length of output: 664
Script:
#!/bin/bash # Verify if TODO comments have been addressed by checking their absence in the test files rg '// TODO:' --glob '*.sol' src/test/Length of output: 664
src/test/utils/Setup.sol (1)
101-101: Remove commented-out code.The line
// factory = strategy.FACTORY();is no longer needed due to the replacement offactorywithmockFactory. It's generally a good practice to remove commented-out code rather than leaving it in the codebase.Consider removing this line entirely:
-// factory = strategy.FACTORY();src/test/TestUSDCOffers.t.sol (1)
Line range hint
278-297: LGTM: Good addition of edge case tests, with suggestions for improvementThe new test functions
testEditOfferTotalGreaterThanCurrentLiquidityandtestEditOfferTotalLessThanCurrentLiquidityare valuable additions. They cover important edge cases for offer editing, verifying the system's behavior when dealing with amounts greater than or less than the current liquidity.Suggestions for improvement:
- Add assertions to verify the offer amounts after editing. This ensures that the offer was actually updated as expected.
- Consider adding error handling tests. For example, what happens if an offer is edited to an amount greater than the total available funds?
- Add comments explaining the purpose of each test for better readability.
Example improvement for
testEditOfferTotalGreaterThanCurrentLiquidity:function testEditOfferTotalGreaterThanCurrentLiquidity() public { bytes32 idHash1 = bytes32("offer id hash 1"); bytes32 offerId1 = _submitOffer(idHash1, 50e6); assertEq(termStrategy.totalLiquidBalance(), 50e6, "Initial liquid balance should be 50e6"); bytes32 editedOfferId = _submitOffer(idHash1, 100e6); assertEq(termStrategy.totalLiquidBalance(), 0, "Liquid balance should be 0 after edit"); assertEq(offerId1, editedOfferId, "Offer ID should remain the same after edit"); // Add an assertion to verify the new offer amount (uint256 offerAmount, , , ) = termStrategy.getOffer(editedOfferId); assertEq(offerAmount, 100e6, "Offer amount should be updated to 100e6"); }Would you like me to provide similar improvements for the
testEditOfferTotalLessThanCurrentLiquidityfunction?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/test/Operation.t.sol (1 hunks)
- src/test/TestUSDCOffers.t.sol (1 hunks)
- src/test/TestUSDCSubmitOffer.t.sol (1 hunks)
- src/test/mocks/MockFeesFactory.sol (1 hunks)
- src/test/utils/Setup.sol (6 hunks)
🔇 Additional comments (12)
src/test/mocks/MockFeesFactory.sol (4)
1-2: LGTM: License and Solidity version are correctly specified.The AGPL-3.0 license is properly declared, and the Solidity version ^0.8.18 is appropriately set, allowing for minor updates while ensuring compatibility.
6-6: LGTM: Contract declaration and inheritance are correct.The contract
MockFeesFactoryis properly declared and correctly inherits fromMockFactory. The naming convention (PascalCase) is followed, which is consistent with Solidity best practices.
1-12: Overall, the MockFeesFactory contract looks good for testing purposes.This contract appears to be a mock implementation for testing, extending the
MockFactorywith an additionalgovernanceconcept. While the implementation is generally correct, consider the suggestions made in previous comments to improve clarity and robustness, even in test code.Remember that maintaining good coding practices in test code can lead to more reliable and maintainable tests.
4-4: LGTM: Import statement is correct. Verify the import path.The import statement for
MockFactoryis syntactically correct. However, it's important to ensure that the import path "@tokenized-strategy/test/mocks/MockFactory.sol" is correct and consistent with your project structure.To verify the import path, run the following script:
src/test/Operation.t.sol (1)
111-112: Clarify the purpose of the added logging statementThe added logging statement provides additional visibility into the strategy's total assets after deposit. However, it's not clear how this relates to fixing existing unit tests, which is the stated objective of this PR.
Consider the following:
- Is this logging statement intended to be temporary for debugging purposes, or a permanent addition to the test?
- If it's permanent, please add a comment explaining why this specific log is necessary for this test.
- If it's temporary, consider removing it before merging or wrapping it in a debug flag condition.
To verify the necessity of this logging statement, we can check if similar logging is used in other test functions:
This will help determine if this is a one-off addition or part of a broader logging strategy in the tests.
✅ Verification successful
Clarify the necessity of the added logging statement
The logging statement
console2.log("total assets", strategy.totalAssets());is uniquely added in this test function and does not appear in other test files. Please consider:
- Is this logging statement intended for temporary debugging purposes?
- If so, remove it before merging.
- If it's necessary permanently, please add a comment explaining its purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar console logging in other test functions rg --type solidity 'console2\.log.*totalAssets' src/test/Length of output: 92
Script:
#!/bin/bash # Search for similar console logging in other solidity test functions rg 'console2\.log.*totalAssets' src/test/ --glob '*.sol'Length of output: 145
src/test/utils/Setup.sol (5)
17-17: LGTM: New imports are consistent with the changes.The addition of
MockFeesFactoryandMockUSDCimports aligns with the described changes in the AI summary. These new mocks will likely be used to simulate fee-related functionality and USDC token behavior in the tests.Also applies to: 20-21
71-71: LGTM: MockUSDC integration in setUp function.The creation of a
MockUSDCinstance and its use in the_setUpfunction aligns with the changes described in the AI summary. This modification allows for a more controlled testing environment by using a mock USDC token.Also applies to: 73-73
104-104: LGTM: Updated label for mockFactory.The label update for
mockFactoryis consistent with the replacement offactorywithmockFactorythroughout the file.
190-190: LGTM: Updated setFees function to use mockFactory.The changes in the
setFeesfunction correctly update the calls to usemockFactoryinstead of the previousIFactory. This is consistent with the AI summary and maintains the same functionality while using the newMockFeesFactory.Also applies to: 194-194, 197-197
59-59: LGTM: MockFeesFactory declaration added.The
mockFactorydeclaration replaces the previousfactoryvariable, which is consistent with the changes described in the AI summary.Note that the visibility has changed from
publictointernal. Please verify that this change doesn't break any existing test cases that might have directly accessed the factory variable.✅ Verification successful
LGTM: MockFeesFactory declaration added.
The
mockFactorydeclaration replaces the previousfactoryvariable, which is consistent with the changes described in the AI summary. The visibility has been changed frompublictointernal. No direct accesses tofactorywere found in the codebase, ensuring that this change does not break any existing test cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct access to 'factory' in test files rg --type-add 'sol:*.sol' --type sol 'factory\s*\.' test/Length of output: 105
Script:
#!/bin/bash # Search for any direct access to 'factory' in all Solidity files rg --type-add 'sol:*.sol' --type sol 'factory\s*\.' .Length of output: 55
src/test/TestUSDCOffers.t.sol (1)
239-240: LGTM: Improved assertion order for offer IDsThe change in the order of assertions correctly verifies that the first offer ID corresponds to
offerId1and the second toofferId2. This modification enhances the test's accuracy in validating the order of submitted offers, which aligns with the expected behavior of the offer submission system.src/test/TestUSDCSubmitOffer.t.sol (1)
262-263: Verify the change in expected order of pending offers.The assertion order for
offerId1andofferId2has been swapped, suggesting a change in the expected behavior of thependingOffers()function. This change looks good if it aligns with the intended behavior, but please verify:
- Does this change correctly reflect the expected order of pending offers?
- Are similar changes needed in other test cases or implementations?
- Does this change in order affect any other parts of the system that might rely on a specific order of pending offers?
Consider adding a comment explaining the expected order of pending offers to improve code clarity.
To help verify this change, you can run the following script to check for other occurrences of
pendingOffers()and their usage:✅ Verification successful
Change in pendingOffers() assertion order looks correct.
The assertion order for
offerId1andofferId2has been swapped, aligning with the expected behavior of thependingOffers()function. No additional assertions onpendingOffers()were found in other tests. Please ensure that this change does not affect other parts of the system that might rely on the specific order of pending offers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other usages of pendingOffers() to ensure consistency # Test: Search for pendingOffers() usage echo "Searching for pendingOffers() usage:" rg --type solidity -n 'pendingOffers\(\)' # Test: Search for assertions on pendingOffers() results echo "\nSearching for assertions on pendingOffers() results:" rg --type solidity -n 'assert.*pendingOffers'Length of output: 359
Script:
#!/bin/bash # Description: Search for other usages of pendingOffers() to ensure consistency without relying on rg file types # Test: Search for pendingOffers() usage in .sol files echo "Searching for pendingOffers() usage in Solidity files:" rg 'pendingOffers\(\)' --type-add 'sol=*.sol' -t sol -n # Test: Search for assertions on pendingOffers() results in .sol files echo "\nSearching for assertions on pendingOffers() results in Solidity files:" rg 'assert.*pendingOffers' --type-add 'sol=*.sol' -t sol -nLength of output: 513
Script:
#!/bin/bash # Description: Search for other usages of pendingOffers() to ensure consistency using glob patterns # Test: Search for pendingOffers() usage in .sol files using --glob echo "Searching for pendingOffers() usage in Solidity files using --glob:" rg 'pendingOffers\(\)' --glob '*.sol' -n # Test: Search for assertions on pendingOffers() results in .sol files using --glob echo "\nSearching for assertions on pendingOffers() results in Solidity files using --glob:" rg 'assert.*pendingOffers' --glob '*.sol' -n # Alternative Approach: Using find with rg to search within .sol files # Test: Search for pendingOffers() usage using find and rg echo "\nSearching for pendingOffers() usage in Solidity files using find and rg:" find . -type f -name '*.sol' -exec rg 'pendingOffers\(\)' -n {} + # Test: Search for assertions on pendingOffers() results using find and rg echo "\nSearching for assertions on pendingOffers() results in Solidity files using find and rg:" find . -type f -name '*.sol' -exec rg 'assert.*pendingOffers' -n {} +Length of output: 3381
| constructor(uint16 bps, address treasury) MockFactory(bps, treasury) { | ||
| governance = msg.sender; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding parameter validation and review governance initialization.
The constructor implementation is generally correct, but consider the following suggestions:
- Add input validation for the
bpsandtreasuryparameters to ensure they meet any required constraints. - Review if
msg.senderis always the intended governance address. If not, consider passing the governance address as a parameter.
Here's a potential refactor to address these points:
constructor(uint16 bps, address treasury, address _governance) MockFactory(bps, treasury) {
require(bps > 0 && bps <= 10000, "Invalid bps value");
require(treasury != address(0), "Invalid treasury address");
require(_governance != address(0), "Invalid governance address");
governance = _governance;
}This refactor adds basic validation and allows for explicit setting of the governance address.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/interfaces/term/ITermAuctionOfferLocker.sol (1)
37-50: Ensure consistency in struct definitions and commentsThe new
TermAuctionRevealedOfferstruct is a good addition for representing revealed offers. However, there are a few inconsistencies to address:
The comment for
offerPriceRevealedmentions 9 decimal places, which is consistent withTermAuctionOfferSubmissionbut inconsistent withTermAuctionOffer(18 decimal places). Please clarify the correct number of decimal places and update comments accordingly across all structs.The comment for
purchaseTokenuses "lent" instead of "purchase", which might be inconsistent terminology. Consider using consistent terminology across all structs.Consider applying these changes:
/// @dev TermAuctionRevealedOffer represents a revealed offer to offeror an amount of money for a specific interest rate struct TermAuctionRevealedOffer { /// @dev Unique identifier for this bid bytes32 id; /// @dev The address of the offeror address offeror; - /// @dev The offered price as a percentage of the initial loaned amount vs amount returned at maturity. This stores 9 decimal places + /// @dev The offered price as a percentage of the initial loaned amount vs amount returned at maturity. This stores 18 decimal places uint256 offerPriceRevealed; /// @dev The maximum amount of purchase tokens offered uint256 amount; - /// @dev The address of the lent ERC20 token + /// @dev The address of the purchase ERC20 token address purchaseToken; }Also, please ensure that the number of decimal places for
offerPriceRevealedis consistent across all related structs and functions in the interface.src/test/mocks/MockTermAuctionOfferLocker.sol (1)
104-107: Summary ofunlockOfferPartialfunction reviewThe addition of the
unlockOfferPartialfunction enhances the functionality of theMockTermAuctionOfferLockercontract by allowing partial unlocking of offers. However, there are several areas for improvement:
- Implementation logic: The current implementation deletes the entire offer regardless of the amount, which may not be the intended behavior for partial unlocking.
- Input validation: There's a lack of checks for offer existence, offeror validity, and sufficient locked amount.
- Consistency: The error handling and validation patterns should be aligned with other functions in the contract.
These issues have been addressed in the previous comments with specific suggestions for improvement. Implementing these changes will significantly enhance the robustness and reliability of the
unlockOfferPartialfunction.As this is a mock contract for testing purposes, ensure that the behavior of
unlockOfferPartialaccurately reflects the expected behavior of the actual contract it's mocking. This is crucial for maintaining the validity of tests that will use this mock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/interfaces/term/ITermAuctionOfferLocker.sol (1 hunks)
- src/test/TestUSDCOffers.t.sol (9 hunks)
- src/test/TestUSDCSubmitOffer.t.sol (2 hunks)
- src/test/mocks/MockTermAuction.sol (2 hunks)
- src/test/mocks/MockTermAuctionOfferLocker.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/TestUSDCOffers.t.sol
- src/test/TestUSDCSubmitOffer.t.sol
🔇 Additional comments (4)
src/interfaces/term/ITermAuctionOfferLocker.sol (1)
37-50: Overall assessment: New struct enhances the interfaceThe addition of the
TermAuctionRevealedOfferstruct is a positive change that enhances theITermAuctionOfferLockerinterface. It provides a clear and concise representation of revealed offers, which is likely to improve code readability and maintainability.The struct is well-designed and aligns with the existing structures in the interface. With the suggested minor improvements for consistency, this change will contribute to a more robust and clear interface definition.
src/test/mocks/MockTermAuctionOfferLocker.sol (2)
104-107: 🛠️ Refactor suggestionAlign error handling and validation with existing functions
While the new
unlockOfferPartialfunction fits well with the existing code structure, it's important to ensure consistency in error handling and validation across the contract.Consider the following recommendations:
- Review other functions in the contract, particularly
lockOffersandunlockOffers, to identify common validation patterns.- Implement similar error handling mechanisms, such as using
requirestatements with descriptive error messages.- Ensure that events are emitted consistently for significant state changes, if applicable in this mock contract.
Example:
function unlockOfferPartial(bytes32 offerId, address offeror, uint256 amount) external { require(lockedOffers[offerId].id != bytes32(0), "TermAuctionOfferLocker: Offer does not exist"); require(lockedOffers[offerId].offeror == offeror, "TermAuctionOfferLocker: Invalid offeror"); require(lockedOffers[offerId].amount >= amount, "TermAuctionOfferLocker: Insufficient locked amount"); lockedOffers[offerId].amount -= amount; if (lockedOffers[offerId].amount == 0) { delete lockedOffers[offerId]; } repoLocker.releasePurchaseTokens(offeror, amount); // Consider emitting an event if it aligns with the contract's pattern // emit OfferPartiallyUnlocked(offerId, offeror, amount); }Let's verify the error handling patterns in other functions:
#!/bin/bash # Description: Check error handling patterns in other functions rg --type solidity 'require\(' src/test/mocks/MockTermAuctionOfferLocker.sol
104-107:⚠️ Potential issueImprove
unlockOfferPartialfunction implementationThe current implementation of
unlockOfferPartialhas several issues that need to be addressed:
- The function deletes the entire offer from
lockedOffers, regardless of theamountparameter. This could lead to inconsistencies if partial unlocking is intended.- There's no check if the
offerIdexists inlockedOffers.- There's no validation of the
amountparameter against the locked offer amount.Consider implementing the following improvements:
function unlockOfferPartial(bytes32 offerId, address offeror, uint256 amount) external { - delete lockedOffers[offerId]; + require(lockedOffers[offerId].id == offerId, "Offer does not exist"); + require(lockedOffers[offerId].offeror == offeror, "Invalid offeror"); + require(lockedOffers[offerId].amount >= amount, "Insufficient locked amount"); + + lockedOffers[offerId].amount -= amount; + + if (lockedOffers[offerId].amount == 0) { + delete lockedOffers[offerId]; + } repoLocker.releasePurchaseTokens(offeror, amount); }These changes will:
- Ensure the offer exists before processing.
- Validate the offeror to prevent unauthorized unlocking.
- Check if the requested amount is available for unlocking.
- Only partially reduce the locked amount, deleting the offer only if the remaining amount is zero.
To ensure consistency with other parts of the contract, let's verify the implementation of
unlockOffers:src/test/mocks/MockTermAuction.sol (1)
49-51: Ensure all references toauctionCanceledare updatedThe function
auctionCanceledhas been renamed toauctionCancelForWithdrawal. Please verify that all references to the old function nameauctionCanceledhave been updated throughout the codebase to avoid any inconsistencies or broken references.Run the following script to check for any remaining references to
auctionCanceled:Also applies to: 53-68
✅ Verification successful
All references to
auctionCanceledhave been successfully updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the old function name `auctionCanceled` in Solidity files. # Test: Expect no results. If occurrences are found, they need to be updated. rg --type solidity 'auctionCanceled'Length of output: 69
Script:
#!/bin/bash # Description: Search for occurrences of the old function name `auctionCanceled` in Solidity (.sol) files. # Test: Expect no results. If occurrences are found, they need to be updated. rg 'auctionCanceled' --glob '*.sol'Length of output: 35
| function auctionCancelForWithdrawal() external { | ||
| auctionCancelledForWithdrawal = true; | ||
| } |
There was a problem hiding this comment.
Add access control to auctionCancelForWithdrawal
The auctionCancelForWithdrawal function is marked as external and can be called by any address. If this function is intended to be restricted to authorized entities (e.g., contract owner or administrator), consider adding appropriate access control modifiers to prevent unauthorized access.
| function auctionCancel(bytes32[] calldata offerIds) external { | ||
|
|
||
| uint256 i = 0; | ||
| // Return revealed offer funds. | ||
| for (i = 0; i < offerIds.length; ++i) { | ||
| ITermAuctionOfferLocker.TermAuctionOffer memory offer = MockTermAuctionOfferLocker(termAuctionOfferLocker).lockedOffer(offerIds[i]); | ||
|
|
||
| MockTermAuctionOfferLocker(termAuctionOfferLocker).unlockOfferPartial( | ||
| offer.id, | ||
| offer.offeror, | ||
| offer.amount | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Add access control to auctionCancel
The auctionCancel function is declared as external and allows cancellation of offers by accepting an array of offerIds. To prevent unauthorized cancellations, consider adding access control modifiers if this function should be restricted to specific roles.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/test/TestUSDCIntegration.t.sol (1)
195-224: Add assertions to verify successful deposit after unpausingAfter unpausing deposits, the test performs a deposit but does not assert that the deposit was successful. Consider adding assertions to verify that the user's balance has increased accordingly.
You can add the following assertions:
vm.stopPrank(); + // Verify the user's share balance has increased + uint256 userShareBalance = IERC4626(address(termStrategy)).balanceOf(testUser); + assertEq(userShareBalance, expectedShareBalance); + + // Verify the total assets have increased + uint256 totalAssets = IERC4626(address(termStrategy)).totalAssets(); + assertEq(totalAssets, expectedTotalAssets); }Replace
expectedShareBalanceandexpectedTotalAssetswith the expected values after the deposit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/test/TestUSDCIntegration.t.sol (2 hunks)
- src/test/TestUSDCSellRepoToken.t.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/TestUSDCSellRepoToken.t.sol
🔇 Additional comments (2)
src/test/TestUSDCIntegration.t.sol (2)
11-12: Appropriate addition ofIERC4626importThe import of
IERC4626is correctly added to support its usage in the test functions.
179-193: Test correctly verifies repo token blacklisting functionalityThe test ensures that only management can set the repo token blacklist and that a non-management user cannot sell a blacklisted token, as expected.
| function testRemovingMaturedTokensWithRedemptionAttempt() public { | ||
| address testUser = vm.addr(0x11111); | ||
| mockUSDC.mint(testUser, 1e18); | ||
| repoToken1Month.mint(testUser, 1000e18); | ||
|
|
||
| vm.startPrank(testUser); | ||
| mockUSDC.approve(address(mockYearnVault), type(uint256).max); | ||
| mockYearnVault.deposit(1e18, testUser); | ||
| repoToken1Month.approve(address(strategy), type(uint256).max); | ||
| termStrategy.sellRepoToken(address(repoToken1Month), 1e6); | ||
| vm.stopPrank(); | ||
|
|
||
| address[] memory holdings = termStrategy.repoTokenHoldings(); | ||
| assertEq(holdings.length, 1); | ||
|
|
||
|
|
||
| vm.warp(block.timestamp + 5 weeks); | ||
| termStrategy.auctionClosed(); | ||
|
|
||
| holdings = termStrategy.repoTokenHoldings(); | ||
| assertEq(holdings.length, 0); | ||
| assertEq(repoToken1Month.balanceOf(address(strategy)), 0); | ||
| } |
There was a problem hiding this comment.
Add redemption attempt to match the test function name
The test function testRemovingMaturedTokensWithRedemptionAttempt does not include an attempt to redeem the matured tokens, despite the implication in its name. Consider adding logic to attempt redemption and verify the outcome to fully cover the intended scenario.
Apply this diff to include the redemption attempt:
assertEq(holdings.length, 0);
+ // Attempt to redeem the matured tokens
+ uint256 redeemedAmount = termStrategy.redeemMaturedTokens();
+ // Verify the redeemed amount is as expected
+ assertEq(redeemedAmount, expectedRedeemedAmount);
+ // Ensure the repo token balance is zero after redemption
+ assertEq(repoToken1Month.balanceOf(address(strategy)), 0);
}Replace expectedRedeemedAmount with the expected amount after redemption.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
src/test/TestUSDCOffers.t.sol (9)
94-95: Good addition of repo token holding value check after editing offerThe new assertions for checking the repo token holding value after editing an offer are a valuable addition to the test. They ensure that the internal state of the contract is correctly updated after offer modifications.
Consider adding a comment explaining the expected behavior to improve readability:
// Verify that the repo token holding value is updated to the new offer amount uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 4e6);
114-115: Valuable addition of repo token holding value check after deleting offersThe new assertions for checking that the repo token holding value is zero after deleting offers are a great addition to the test. They ensure that the internal state of the contract is correctly updated after offer deletions.
To improve clarity, consider adding a brief comment:
// Verify that the repo token holding value is zero after deleting all offers uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0);
156-158: Good addition of repo token holding value check after successful full auctionThe new assertions for checking the repo token holding value after a successful full auction are a valuable addition to the test. They ensure that the internal state of the contract is correctly updated after the auction is completed.
For consistency with other similar checks, consider removing the blank line:
uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6);
171-173: Valuable addition of repo token holding value check after auction closureThe new assertions for checking the repo token holding value after calling
auctionClosed()are a great addition to the test. They ensure that the internal state of the contract remains consistent after closing the auction.To improve clarity, consider adding a brief comment and removing the blank line:
// Verify that the repo token holding value remains unchanged after closing the auction repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6);
203-210: Excellent addition of repo token holding value checks for partial auction successThe new assertions for checking the repo token holding value before and after a partially successful auction are a valuable addition to the test. They ensure that the internal state of the contract is correctly updated for partial auction completions.
To improve clarity and consistency, consider the following changes:
- Add comments to explain the expected values.
- Remove blank lines for consistency with other similar checks.
// Verify initial repo token holding value before auction uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); repoToken1WeekAuction.auctionSuccess(offerIds, fillAmounts, repoTokenAmounts); // Verify updated repo token holding value after partial auction success repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0.5e6);
221-223: Good addition of repo token holding value check after closing partial auctionThe new assertions for checking the repo token holding value after calling
auctionClosed()in a partially successful auction scenario are a valuable addition to the test. They ensure that the internal state of the contract remains consistent after closing a partially successful auction.To improve clarity and consistency, consider the following changes:
- Add a comment to explain the expected value.
- Remove the blank line for consistency with other similar checks.
// Verify that the repo token holding value remains unchanged after closing the partial auction repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0.5e6);
238-259: Excellent improvements to auction cancellation for withdrawal testThe renaming of the function to
testAuctionCancelForWithdrawaland the addition of repo token holding value checks throughout the auction cancellation process are significant improvements. These changes enhance the test coverage and ensure that the internal state of the contract is consistent with the expected behavior during auction cancellation for withdrawal.To further improve clarity and readability, consider the following suggestions:
- Add comments to explain the expected values at each stage.
- Remove unnecessary blank lines for consistency.
function testAuctionCancelForWithdrawal() public { bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); // Verify initial repo token holding value after submitting offer uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); repoToken1WeekAuction.auctionCancelForWithdrawal(); // Verify repo token holding value remains unchanged after auction cancellation repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); // Complete the auction cancellation process termStrategy.auctionClosed(); // Verify repo token holding value is reset to zero after completing auction cancellation repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); bytes32[] memory offers = termStrategy.pendingOffers(); assertEq(offers.length, 0); }
261-280: Excellent addition of testAuctionCancel functionThe new
testAuctionCancelfunction is a valuable addition to the test suite, enhancing the overall coverage by specifically testing the auction cancellation scenario. The included assertions for repo token holding values at various stages of the cancellation process ensure that the internal state of the contract remains consistent with the expected behavior.To improve clarity and consistency, consider the following suggestions:
- Add comments to explain the expected values at each stage.
- Remove unnecessary blank lines for consistency with other test functions.
function testAuctionCancel() public { bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); // Verify initial repo token holding value after submitting offer uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); bytes32[] memory offerIds = new bytes32[](1); offerIds[0] = offerId1; repoToken1WeekAuction.auctionCancel(offerIds); // Verify repo token holding value is reset to zero after auction cancellation repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); // Complete the auction cancellation process termStrategy.auctionClosed(); // Verify repo token holding value remains zero after completing auction cancellation repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); bytes32[] memory offers = termStrategy.pendingOffers(); assertEq(offers.length, 0); }
Line range hint
1-353: Overall excellent improvements to the test suiteThe changes made to this test file significantly enhance the coverage and accuracy of the auction offer tests. The additions of repo token holding value checks at various stages of the auction process provide a more comprehensive validation of the contract's behavior.
To further improve the consistency across the entire file, consider applying the following suggestion to all test functions:
- Add brief comments before each assertion or group of related assertions to explain the expected behavior being tested.
- Maintain a consistent style for blank lines between assertions and function calls.
These minor enhancements will improve the readability and maintainability of the test suite, making it easier for other developers to understand and modify the tests in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/test/TestUSDCOffers.t.sol (9 hunks)
🔇 Additional comments (2)
src/test/TestUSDCOffers.t.sol (2)
74-76: Excellent addition of repo token holding value assertionsThe new assertions for checking the repo token holding value after submitting an offer are a valuable addition to the test. They ensure that the internal state of the contract is correctly updated, enhancing the overall test coverage and reliability.
297-298: Correct order of offer ID assertionsThe reversal of the order of assertions for checking offer IDs is a crucial correction. This change ensures that the first offer ID corresponds to
offerId1and the second toofferId2, which aligns with the expected behavior of the contract where offers are typically stored in the order they are submitted.This correction improves the accuracy of the test and helps prevent potential false negatives that could occur if the order of offer storage changed in the future.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
src/test/TestUSDCOffers.t.sol (7)
76-81: Enhanced offer submission verificationThe new assertions for
liquidReserveRatioandrepoTokenHoldingValueimprove the test coverage by verifying the strategy's state after offer submission. This ensures that the offer submission process correctly updates the strategy's internal state.Consider adding a comment explaining the expected
liquidReserveRatiovalue (0.99e18) for better readability and maintainability.
83-91: New test for liquid reserve ratio constraintExcellent addition of a test case that verifies the strategy's behavior when attempting to submit an offer that would violate the required reserve ratio. This test enhances the overall robustness of the test suite.
Consider adding an assertion to verify the initial state of
liquidReserveRatiobefore attempting to submit the offer. This would provide a clearer picture of the state transition.
101-116: Improved offer editing testThe enhancements to the
testEditOfferfunction significantly improve the test coverage by verifying the strategy's state before and after editing an offer. The use ofofferId1instead ofidHash1ensures that the correct offer is being edited.Consider adding a comment explaining the expected values for
liquidReserveRatiobefore and after editing the offer, to improve readability and maintainability.
177-180: Improved auction success verificationThe new assertions for
liquidReserveRatioandrepoTokenHoldingValueenhance the test coverage by verifying the strategy's state at different stages of a successful auction. This ensures that the auction process correctly updates the strategy's internal state.Consider adding comments explaining the expected values and the stages they represent (e.g., "After auction success" and "After auction closed") to improve readability.
Also applies to: 193-196
214-214: Enhanced partial auction success verificationThe new assertions for
liquidReserveRatioandrepoTokenHoldingValuesignificantly improve the test coverage by verifying the strategy's state at different stages of a partially successful auction. This ensures that the auction process correctly handles partial fills and updates the strategy's internal state accordingly.Consider adding comments to clearly delineate the different stages of the auction process (e.g., "Before auction", "After partial fill", "After auction closed") to improve readability and maintainability.
Also applies to: 228-237, 248-250
265-286: Improved auction cancellation for withdrawal testThe renaming of this function and the additional assertions significantly enhance the test coverage for auction cancellations due to withdrawals. The new checks for
repoTokenHoldingValueat different stages ensure that the cancellation process correctly updates the strategy's internal state.Consider adding a check for
liquidReserveRatioafter the auction is closed to ensure it returns to the expected value (likely 1e18) after cancellation.
Line range hint
288-311: New test for general auction cancellationExcellent addition of a test case for general auction cancellation. This new function enhances the test suite by covering a distinct cancellation scenario and verifying the correct updates to
repoTokenHoldingValue.Consider adding assertions for
liquidReserveRatioat each stage of the cancellation process to ensure it's correctly updated throughout the process.src/test/mocks/MockTermController.sol (1)
19-20: Provide a function to mark a term as deployedCurrently, there is a function to mark a contract as not deployed but no function to mark it as deployed. This could be necessary for resetting the status or correcting errors.
Consider adding a corresponding function:
function markTermDeployed(address contractAddress) external onlyOwner { notTermDeployedContracts[contractAddress] = false; }src/test/TestUSDCIntegration.t.sol (1)
253-256: Specify expected revert reason for clarityIn the test where management sets an invalid discount rate adapter, consider specifying the expected revert reason in
vm.expectRevert()to ensure the test explicitly checks for the correct failure scenario.Modify the test to include the expected revert reason:
vm.prank(management); -vm.expectRevert(); +vm.expectRevert("InvalidDiscountRateAdapter"); termStrategy.setDiscountRateAdapter(address(invalid));Replace
"InvalidDiscountRateAdapter"with the actual revert reason thrown by thesetDiscountRateAdapterfunction when an invalid adapter is provided. This improves the test's specificity and helps catch unintended pass conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/test/TestUSDCIntegration.t.sol (2 hunks)
- src/test/TestUSDCOffers.t.sol (13 hunks)
- src/test/TestUSDCSellRepoToken.t.sol (4 hunks)
- src/test/mocks/MockTermController.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/TestUSDCSellRepoToken.t.sol
🧰 Additional context used
🔇 Additional comments (8)
src/test/TestUSDCOffers.t.sol (5)
47-48: Improved initial state verificationThe added assertion for
liquidReserveRatioenhances the test setup by ensuring the strategy starts with the expected 100% liquid reserve ratio. This check is crucial for validating the initial state before any operations are performed.
134-137: Enhanced offer deletion verificationThe new assertions for
repoTokenHoldingValueandliquidReserveRatioimprove the test coverage by verifying the strategy's state after offer deletion. This ensures that the offer deletion process correctly updates the strategy's internal state.
324-325: Corrected order of offer ID assertionsThe swapped order of assertions for
offers[0]andoffers[1]improves the accuracy of the test by correctly matching the order of offer submissions. This change ensures that the first offer ID corresponds toofferId1and the second toofferId2.
360-360: Consistent use of offer IDs in edit testsThe change from
idHash1toofferId1in bothtestEditOfferTotalGreaterThanCurrentLiquidityandtestEditOfferTotalLessThanCurrentLiquidityfunctions ensures consistency with thetestEditOfferfunction. This modification guarantees that the correct offer is being edited in all test cases involving offer editing.Also applies to: 371-371
Line range hint
1-374: Overall improvement in test coverage and accuracyThe changes made to this test file significantly enhance the robustness and comprehensiveness of the test suite for the TestUSDCSubmitOffer contract. Key improvements include:
- Addition of new test cases for auction cancellation scenarios.
- Enhanced verification of the strategy's state (liquidReserveRatio and repoTokenHoldingValue) at various stages of the auction process.
- Improved consistency in offer ID usage across different test functions.
- More accurate assertions for multiple offer scenarios.
These modifications collectively contribute to a more thorough validation of the contract's behavior under various conditions, which is crucial for ensuring the reliability and correctness of the USDC offer submission and auction processes.
src/test/TestUSDCIntegration.t.sol (3)
183-193: Test function implementation is correctThe
testSimulateTransactionWithInvalidTokenfunction correctly verifies that attempting to simulate a transaction with an invalid repo token results in a revert with the expected error.
196-210: Test properly validates repo token blacklistingThe
testRepoTokenBlacklistfunction effectively ensures that:
- Only management can set the blacklist status of a repo token.
- Non-management users cannot blacklist a repo token and receive the appropriate revert message.
- Attempts to sell a blacklisted repo token by a user result in a revert with the expected error.
212-241: Test correctly verifies pause and unpause functionalityThe
testPausesfunction accurately tests that:
- Only management can pause and unpause deposits.
- Non-management users receive the correct revert messages when attempting to pause or unpause.
- Deposits are prevented when paused, with the expected revert error.
- Deposits can be made successfully after unpausing.
| @@ -10,9 +10,14 @@ struct TermAuctionResults { | |||
|
|
|||
| contract MockTermController is ITermController { | |||
| mapping(bytes32 => TermAuctionResults) internal auctionResults; | |||
| mapping(address => bool) internal notTermDeployedContracts; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using double negatives in variable names for clarity
The variable notTermDeployedContracts uses a double negative, which can make the code harder to read and understand. Consider renaming it to nonDeployedContracts or refactoring the logic to use a positive term like deployedContracts.
To improve clarity, you could rename the mapping and adjust the associated functions:
-mapping(address => bool) internal notTermDeployedContracts;
+mapping(address => bool) internal termDeployedContracts;Then, update the functions accordingly.
Committable suggestion was skipped due to low confidence.
| return !notTermDeployedContracts[contractAddress]; | ||
| } |
There was a problem hiding this comment.
Ensure correct logic in isTermDeployed function
The isTermDeployed function returns the negation of notTermDeployedContracts[contractAddress]. This can be confusing due to the double negative. If a contract address is not in the mapping, it defaults to false, and !false evaluates to true, indicating the term is deployed. This relies on the default mapping behavior.
Consider refactoring to use positive logic for better readability:
-return !notTermDeployedContracts[contractAddress];
+return termDeployedContracts[contractAddress];Ensure that the termDeployedContracts mapping is properly managed elsewhere in the contract.
Committable suggestion was skipped due to low confidence.
| mapping(address => bool) internal notTermDeployedContracts; | ||
|
|
||
| function isTermDeployed(address contractAddress) external view returns (bool) { | ||
| return true; | ||
| return !notTermDeployedContracts[contractAddress]; | ||
| } | ||
|
|
||
| function markNotTermDeployed(address contractAddress) external { | ||
| notTermDeployedContracts[contractAddress] = true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize storage by using mapping default values
Since the default boolean value in a mapping is false, you might not need to store false explicitly. You can remove entries from the mapping when a contract is deployed.
Adjust the functions to set and unset the mapping entries:
function markNotTermDeployed(address contractAddress) external onlyOwner {
notTermDeployedContracts[contractAddress] = true;
}
+function markTermDeployed(address contractAddress) external onlyOwner {
+ delete notTermDeployedContracts[contractAddress];
+}This way, isTermDeployed will return true when the address is not present in notTermDeployedContracts.
Additionally, update isTermDeployed if necessary to reflect this logic.
Committable suggestion was skipped due to low confidence.
| function markNotTermDeployed(address contractAddress) external { | ||
| notTermDeployedContracts[contractAddress] = true; |
There was a problem hiding this comment.
Add access control to markNotTermDeployed function
The markNotTermDeployed function is external and can be called by any address. This could lead to unauthorized entities altering the deployment status of contracts, which may affect the system's integrity.
Consider restricting access to this function using access control modifiers. For example, if you're using OpenZeppelin's Ownable contract:
+import "@openzeppelin/contracts/access/Ownable.sol";
+
-contract MockTermController is ITermController {
+contract MockTermController is ITermController, Ownable {
// ...
function markNotTermDeployed(address contractAddress) external
+ onlyOwner
{
notTermDeployedContracts[contractAddress] = true;
}This ensures that only the contract owner can mark terms as not deployed.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/test/TestUSDCOffers.t.sol (1)
412-415: Adding Assertions After Editing Offer to a Lower AmountIn
testEditOfferTotalLessThanCurrentLiquidity, after reducing the offer amount to50e6, it's beneficial to add assertions to verify the updated state.Include the following assertions to confirm the strategy's state:
_submitOffer(offerId1, 50e6); +assertEq(termStrategy.totalLiquidBalance(), 50e6); +uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); +assertEq(repoTokenHoldingValue, 50e6); +assertEq(termStrategy.totalAssetValue(), initialState.totalLiquidBalance);These checks ensure that the liquid balance and repo token holdings are updated correctly after decreasing the offer amount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/test/TestUSDCOffers.t.sol (16 hunks)
🧰 Additional context used
🔇 Additional comments (19)
src/test/TestUSDCOffers.t.sol (19)
10-11: ImportingRepoTokenListfor Custom Error HandlingThe added import statement
import {RepoTokenList} from "../RepoTokenList.sol";is necessary for accessing theInvalidRepoTokencustom error used in the tests.
20-21: Adding Test Instances for Extended Maturity PeriodThe introduction of
repoToken100WeekandrepoToken100WeekAuctionallows testing scenarios with a 100-week maturity period, enhancing the coverage for long-term repo tokens.
33-36: Correct Instantiation ofrepoToken100WeekThe
MockTermRepoTokenfor the 100-week term is properly instantiated with appropriate parameters, ensuring accurate testing for longer maturities.
42-42: Initialization of Auction for 100-Week Repo TokenCreating
repoToken100WeekAuctionprovides the necessary setup to test auction functionalities for the long-term repo token.
55-56: Verifying Initial Liquid Reserve RatioThe assertion
assertEq(termStrategy.liquidReserveRatio(), 1e18);confirms that the initial liquid reserve ratio is correctly set to 1 (scaled by 1e18), which is a good practice to ensure the strategy starts in the expected state.
85-89: Validating State After Submitting an OfferThe added assertions effectively check that after submitting an offer:
- The liquid reserve ratio decreases to
0.99e18.- The repo token holding value reflects the offer amount of
1e6.These validations ensure the strategy's state updates correctly upon offer submission.
91-99: Testing Submission Below Required Reserve RatioThe
testSubmitOfferBelowLiquidReserveRatiofunction correctly tests that submitting an offer which would reduce the liquid reserve ratio below the required threshold (0.5e18) results in a revert withBalanceBelowRequiredReserveRatioerror.
101-108: Ensuring Revert on Submitting to an Invalid AuctionThe
testSubmitOfferToInvalidAuctionfunction appropriately verifies that submitting an offer to an auction marked as not deployed triggers a revert with theInvalidTermAuctionerror, improving the robustness of the auction validation logic.
110-117: Ensuring Revert on Submitting with an Invalid Repo TokenThe
testSubmitOfferToAuctionWithInvalidRepoTokenfunction correctly checks that submitting an offer with a repo token marked as invalid results in a revert with theInvalidRepoTokenerror.
119-125: Testing Insufficient Liquidity HandlingThe
testSubmitOfferWithoutEnoughLiquidityfunction ensures that attempting to submit an offer exceeding the available liquid balance (100e6vs.101e6) results in a revert with theInsufficientLiquidBalanceerror, which correctly enforces liquidity constraints.
127-132: Testing Time to Maturity Threshold EnforcementThe
testSubmitOfferWithExcessiveWeightedTimeMaturityfunction properly tests that submitting an offer for a repo token with a time to maturity exceeding the threshold (100 weeksvs. a threshold of3 weeks) causes a revert with theTimeToMaturityAboveThresholderror, ensuring maturity constraints are respected.
142-147: Validating Strategy State Before Editing an OfferThe assertions before editing the offer confirm that:
- The total liquid balance is reduced by the initial offer amount.
- The repo token holding value matches the initial offer amount.
- The total asset value equals the sum of the liquid balance and the repo token holding.
- The liquid reserve ratio is updated to
0.99e18.These validations ensure the strategy's state is correctly established before modifying the offer.
175-178: Ensuring Correct State After Deleting an OfferThe assertions confirm that after deleting the offer:
- The total liquid balance returns to the initial state.
- The total asset value matches the total liquid balance.
- The repo token holding value is zero.
- The liquid reserve ratio resets to
1e18.These checks verify that offer deletion correctly restores the strategy's state.
218-221: Validating Full Auction Success HandlingAfter a full fill in
testCompleteAuctionSuccessFull:
- The liquid reserve ratio remains at
0.99e18.- The repo token holding value equals the filled amount.
These assertions confirm that a successful auction updates the strategy's state as expected.
234-237: Checking State After Closing the AuctionCalling
termStrategy.auctionClosed();and verifying:
- The liquid reserve ratio remains unchanged.
- The repo token holding value is accurate.
These steps ensure that closing the auction finalizes the strategy's state correctly.
Line range hint
255-278: Testing Partial Auction Success HandlingThe
testCompleteAuctionSuccessPartialfunction accurately tests a scenario where only half of the offer is filled. The assertions confirm that:
- The liquid reserve ratio adjusts to
0.995e18.- The repo token holding value reflects the partial fill.
- The total asset value remains consistent.
This test ensures partial fills are handled correctly by the strategy.
306-327: Inconsistency in Repo Token Holding Value After Auction CancellationAfter calling
repoToken1WeekAuction.auctionCancelForWithdrawal();, the repo token holding value remains at1e6untiltermStrategy.auctionClosed();is called, after which it drops to zero.Please verify whether the repo token holding value should update immediately after auction cancellation. It may be necessary for the strategy to reflect the cancellation without waiting for
auctionClosed();to be called.
329-348: Validating Behavior intestAuctionCancelThe test confirms that cancelling an offer updates the repo token holding value to zero both before and after calling
auctionClosed();, which correctly reflects the offer cancellation in the strategy's state.
365-366: Correcting Assertion Order intestMultipleOffersEnsuring that
offers[0]corresponds toofferId1andoffers[1]toofferId2aligns the assertions with the order of offers submitted, improving the accuracy of the test.
| repoToken1WeekAuction, address(repoToken1Week), offerId1, bytes32("test price"), offerAmount | ||
| ); | ||
|
|
||
| uint256 offerDiff = offerAmount - 1e6; | ||
|
|
||
| assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - offerDiff); | ||
| // test: totalAssetValue = total liquid balance + pending offer amount | ||
| assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance() + offerDiff); | ||
| assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - offerAmount); | ||
| repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); | ||
| assertEq(repoTokenHoldingValue, 4e6); | ||
| assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance() + offerAmount); | ||
| assertEq(termStrategy.liquidReserveRatio(), 0.96e18); |
There was a problem hiding this comment.
Correcting Offer Editing Logic
Updating the submitAuctionOffer call to use offerId1 ensures that the existing offer is edited rather than creating a new one. The subsequent assertions validate that:
- The total liquid balance reflects the new offer amount.
- The repo token holding value is updated to the new offer amount.
- The liquid reserve ratio adjusts accordingly.
This change fixes potential issues with offer identification during editing.
| _submitOffer(offerId1, 100e6); | ||
|
|
||
| assertEq(termStrategy.totalLiquidBalance(), 0); | ||
| } |
There was a problem hiding this comment.
Missing Revert Expectation for Insufficient Liquidity
In testEditOfferTotalGreaterThanCurrentLiquidity, when editing the offer to 100e6, the total liquid balance is reduced to zero without handling insufficient liquidity.
Consider adding an expectation for a revert due to insufficient liquidity:
vm.prank(management);
+vm.expectRevert(abi.encodeWithSelector(Strategy.InsufficientLiquidBalance.selector, 50e6, 100e6));
bytes32[] memory offerIds = termStrategy.submitAuctionOffer(
repoToken1WeekAuction, address(repoToken1Week), offerId1, bytes32("test price"), 100e6
);This ensures the test correctly handles scenarios where liquidity is not sufficient to support the increased offer amount.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (22)
src/test/TestUSDCOffers.t.sol (14)
20-21: New instances enhance test coverage.The addition of
repoToken100WeekandrepoToken100WeekAuctionallows for testing scenarios with longer maturity periods, improving the overall test coverage.Consider adding a brief comment explaining the purpose of these new instances, e.g.:
// 100-week instances for testing longer maturity periods MockTermRepoToken internal repoToken100Week; MockTermAuction internal repoToken100WeekAuction;
33-36: Initialization of new repo token instances looks good.The changes are consistent with the addition of the new 100-week instances, and the parameters for both initializations are correct.
Consider aligning the closing parentheses for better readability:
repoToken1Week = new MockTermRepoToken( bytes32("test repo token 1"), address(mockUSDC), address(mockCollateral), 1e18, 1 weeks ); repoToken100Week = new MockTermRepoToken( bytes32("test repo token 100"), address(mockUSDC), address(mockCollateral), 1e18, 100 weeks );
55-56: Good addition of initial state assertion.The new assertion for
liquidReserveRatiohelps verify the initial state before running tests, which is a good practice.Consider adding a brief comment to explain the expected value:
// Verify initial liquidReserveRatio is 100% assertEq(termStrategy.liquidReserveRatio(), 1e18);
84-89: Improved test coverage with new assertions.The addition of assertions for
liquidReserveRatioandrepoTokenHoldingValueprovides more comprehensive checks for the offer submission process.Consider adding brief comments to explain the expected values:
// Verify liquidReserveRatio is 99% after submitting offer assertEq(termStrategy.liquidReserveRatio(), 0.99e18); uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); // Verify repoTokenHoldingValue matches the submitted offer amount assertEq(repoTokenHoldingValue, 1e6);
91-99: Good addition of edge case test.The new
testSubmitOfferBelowLiquidReserveRatiofunction is an important test for verifying the system's behavior when attempting to submit an offer that would violate the required reserve ratio.Consider adding a brief comment explaining the test scenario and extracting the offer amount to a named constant for better readability:
function testSubmitOfferBelowLiquidReserveRatio() public { vm.startPrank(management); termStrategy.setRequiredReserveRatio(0.5e18); // Attempt to submit an offer that would bring the reserve ratio below the required 50% uint256 offerAmount = 51e6; // 51% of the total balance vm.expectRevert(abi.encodeWithSelector(Strategy.BalanceBelowRequiredReserveRatio.selector)); termStrategy.submitAuctionOffer( repoToken1WeekAuction, address(repoToken1Week), bytes32("offer id hash 1"), bytes32("test price"), offerAmount ); }
101-108: Good addition of invalid input test.The new
testSubmitOfferToInvalidAuctionfunction is an important test for verifying the system's behavior when attempting to submit an offer to an invalid auction.Consider adding a brief comment explaining the test scenario:
function testSubmitOfferToInvalidAuction() public { vm.startPrank(management); // Mark the auction as not deployed to simulate an invalid auction termController.markNotTermDeployed(address(repoToken1WeekAuction)); // Attempt to submit an offer to the invalid auction vm.expectRevert(abi.encodeWithSelector(Strategy.InvalidTermAuction.selector, address(repoToken1WeekAuction))); termStrategy.submitAuctionOffer( repoToken1WeekAuction, address(repoToken1Week), bytes32("offer id hash 1"), bytes32("test price"), 1e6 ); }
110-117: Good addition of invalid input test.The new
testSubmitOfferToAuctionWithInvalidRepoTokenfunction is an important test for verifying the system's behavior when attempting to submit an offer with an invalid repo token.Consider adding a brief comment explaining the test scenario:
function testSubmitOfferToAuctionWithInvalidRepoToken() public { vm.startPrank(management); // Mark the repo token as not deployed to simulate an invalid repo token termController.markNotTermDeployed(address(repoToken1Week)); // Attempt to submit an offer with the invalid repo token vm.expectRevert(abi.encodeWithSelector(RepoTokenList.InvalidRepoToken.selector, address(repoToken1Week))); termStrategy.submitAuctionOffer( repoToken1WeekAuction, address(repoToken1Week), bytes32("offer id hash 1"), bytes32("test price"), 1e6 ); }
119-125: Good addition of edge case test.The new
testSubmitOfferWithoutEnoughLiquidityfunction is an important test for verifying the system's behavior when attempting to submit an offer without sufficient liquidity.Consider adding a brief comment explaining the test scenario and extracting the offer amount to a named constant for better readability:
function testSubmitOfferWithoutEnoughLiquidity() public { vm.prank(management); // Attempt to submit an offer for more than the available liquidity uint256 offerAmount = 101e6; // Total liquidity is 100e6 vm.expectRevert(abi.encodeWithSelector(Strategy.InsufficientLiquidBalance.selector, 100e6, offerAmount)); termStrategy.submitAuctionOffer( repoToken1WeekAuction, address(repoToken1Week), bytes32("offer id hash 1"), bytes32("test price"), offerAmount ); }
127-132: Good addition of time to maturity threshold test.The new
testSubmitOfferWithExcessiveWeightedTimeMaturityfunction is an important test for verifying the system's behavior when attempting to submit an offer that exceeds the time to maturity threshold.Consider adding a brief comment explaining the test scenario:
function testSubmitOfferWithExcessiveWeightedTimeMaturity() public { vm.prank(management); // Attempt to submit an offer using the 100-week repo token, which should exceed the time to maturity threshold vm.expectRevert(abi.encodeWithSelector(Strategy.TimeToMaturityAboveThreshold.selector)); termStrategy.submitAuctionOffer( repoToken100WeekAuction, address(repoToken100Week), bytes32("offer id hash 1"), bytes32("test price"), 10e6 ); }
142-157: Improved test coverage with new assertions and correct offer ID usage.The additions of assertions for
liquidReserveRatioandrepoTokenHoldingValueprovide more comprehensive checks for the offer editing process. The use ofofferId1instead ofidHash1is correct and consistent with previous recommendations.Consider adding brief comments to explain the expected values and the offer editing process:
// Check initial state after first offer submission assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - 1e6); uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance() + 1e6); assertEq(termStrategy.liquidReserveRatio(), 0.99e18); // Edit the offer vm.prank(management); bytes32[] memory offerIds = termStrategy.submitAuctionOffer( repoToken1WeekAuction, address(repoToken1Week), offerId1, bytes32("test price"), offerAmount ); // Check state after offer edit assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - offerAmount); repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 4e6); assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance() + offerAmount); assertEq(termStrategy.liquidReserveRatio(), 0.96e18);
175-178: Improved test coverage with new assertions.The additions of assertions for
repoTokenHoldingValueandliquidReserveRatioprovide more comprehensive checks for the offer deletion process.Consider adding a brief comment to explain the expected state after offer deletion:
// Check state after offer deletion uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); assertEq(termStrategy.liquidReserveRatio(), 1e18); // Should be back to 100%
180-193: Good addition of invalid input test.The new
testDeleteOffersFromInvalidAuctionfunction is an important test for verifying the system's behavior when attempting to delete offers from an invalid auction.Consider adding brief comments to explain the test steps:
function testDeleteOffersFromInvalidAuction() public { bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); bytes32[] memory offerIds = new bytes32[](1); offerIds[0] = offerId1; // Ensure only management can delete offers vm.expectRevert("!management"); termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds); vm.startPrank(management); // Mark the auction as not deployed to simulate an invalid auction termController.markNotTermDeployed(address(repoToken1WeekAuction)); // Attempt to delete offers from the invalid auction vm.expectRevert(abi.encodeWithSelector(Strategy.InvalidTermAuction.selector, address(repoToken1WeekAuction))); termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds); }
321-342: Improved test coverage for auction cancellation process.The renaming of the function to
testAuctionCancelForWithdrawaland the addition of new assertions provide a more comprehensive test for the auction cancellation process, particularly for withdrawal scenarios.Consider adding brief comments to explain the expected state at each stage:
function testAuctionCancelForWithdrawal() public { bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); // Check initial repo token holding value uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); // Simulate auction cancellation for withdrawal repoToken1WeekAuction.auctionCancelForWithdrawal(); // Check repo token holding value after cancellation (should remain unchanged) repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); // Complete the auction process termStrategy.auctionClosed(); // Check final repo token holding value (should be zero after auction closed) repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); // Ensure no pending offers remain bytes32[] memory offers = termStrategy.pendingOffers(); assertEq(offers.length, 0); }
344-366: Good addition of general auction cancellation test.The new
testAuctionCancelfunction provides coverage for general auction cancellation scenarios, complementing thetestAuctionCancelForWithdrawalfunction.Consider adding brief comments to explain the expected state at each stage:
function testAuctionCancel() public { bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); // Check initial repo token holding value uint256 repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 1e6); bytes32[] memory offerIds = new bytes32[](1); offerIds[0] = offerId1; // Simulate general auction cancellation repoToken1WeekAuction.auctionCancel(offerIds); // Check repo token holding value after cancellation (should be zero) repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); // Complete the auction process termStrategy.auctionClosed(); // Check final repo token holding value (should remain zero) repoTokenHoldingValue = termStrategy.getRepoTokenHoldingValue(address(repoToken1Week)); assertEq(repoTokenHoldingValue, 0); // Ensure no pending offers remain bytes32[] memory offers = termStrategy.pendingOffers(); assertEq(offers.length, 0); }src/test/TestUSDCSellRepoToken.t.sol (5)
115-140: Great addition of test for selling invalid repo token!This new test case is crucial for verifying that the contract correctly handles attempts to sell invalid repo tokens. It effectively sets up the scenario and checks for the expected revert behavior.
To further strengthen this test, consider adding an additional assertion to verify the state remains unchanged after the failed transaction attempt.
142-166: Excellent test case for insufficient liquid balance scenario!This new test function effectively verifies that the contract correctly handles attempts to sell repo tokens when there's insufficient liquid balance. It's a crucial edge case to test.
Consider adding an assertion to check the contract's state (e.g., liquid balance) after the failed transaction to ensure it remains unchanged.
168-193: Valuable test case for reserve ratio enforcement!This new test function effectively verifies that the contract correctly enforces the required reserve ratio when selling repo tokens. It's an important check to ensure the contract maintains the necessary liquidity.
To enhance this test, consider adding a successful sell scenario just above the reserve ratio limit to ensure the check is not overly restrictive.
675-680: Great addition to complete the pausing test!These new lines effectively verify that operations can resume after unpausing the strategy. This is crucial for ensuring the contract can be properly controlled in emergency situations.
Consider adding an assertion to verify the success of the deposit operation after unpausing, to make the test even more robust.
Line range hint
1-680: Consider modularizing large test functions for improved readability.The additions to this test file significantly enhance the coverage and robustness of the contract testing, which is excellent. However, some test functions have become quite long and complex. Consider breaking down larger test functions into smaller, more focused tests or utilizing helper functions for common setup steps. This would improve readability and maintainability of the test suite.
src/test/TestUSDCIntegration.t.sol (3)
233-235: Adjust the tolerance inassertApproxEqto match unitsWhen asserting approximate equality for time values in seconds, using a tolerance of
1second might be unnecessarily strict. Consider increasing the tolerance to a more appropriate value that accounts for potential minor discrepancies in time calculations.Apply this diff to adjust the tolerance:
-assertApproxEq(simulatedWeightedMaturity, 1.25 weeks, 1); +assertApproxEq(simulatedWeightedMaturity, 1.25 weeks, 10);Here, the tolerance is increased to
10seconds, which should still provide precise validation without being overly strict.
260-263: Remove unnecessaryvm.stopPrank()callsAfter calling
vm.expectRevert()and invoking functions astestUser, there is no need to callvm.stopPrank()immediately since the prank context ends automatically. Removing redundantvm.stopPrank()calls can simplify the test code.Apply this diff to remove unnecessary
vm.stopPrank():vm.prank(testUser); vm.expectRevert("!management"); termStrategy.pauseDeposit(); -vm.stopPrank(); vm.prank(testUser); vm.expectRevert("!management"); termStrategy.unpauseDeposit(); -vm.stopPrank();
271-275: Addapprovewithin the samevm.prankcontextIn the test function
testPauses, themockUSDC.approveand theIERC4626.depositcalls are both made bytestUser. Combining them within the samevm.prankcontext can enhance readability and consistency.Apply this diff to combine the calls:
-vm.prank(testUser); mockUSDC.approve(address(termStrategy), 1e6); -vm.prank(testUser); +vm.prank(testUser); +mockUSDC.approve(address(termStrategy), 1e6); vm.expectRevert(abi.encodeWithSelector(Strategy.DepositPaused.selector)); IERC4626(address(termStrategy)).deposit(1e6, testUser);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/test/TestUSDCIntegration.t.sol (4 hunks)
- src/test/TestUSDCOffers.t.sol (16 hunks)
- src/test/TestUSDCSellRepoToken.t.sol (4 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/test/TestUSDCOffers.t.sol (3)
10-11: Import addition looks good.The addition of the RepoTokenList import is appropriate for the context of this test file, likely supporting new functionality or error handling related to repo tokens.
42-42: New auction initialization is correct.The addition of
repoToken100WeekAuctioninitialization is consistent with the new 100-week instances and follows the same pattern asrepoToken1WeekAuction.
Line range hint
1-431: Comprehensive improvements to test coverage and scenarios.The changes in this file significantly enhance the test suite for the USDC offers functionality. Key improvements include:
- Addition of new test cases for various edge cases and error conditions (e.g., submitting offers below liquid reserve ratio, to invalid auctions, with invalid repo tokens, and without enough liquidity).
- Enhanced existing tests with additional assertions, particularly for
liquidReserveRatioandrepoTokenHoldingValue.- New tests for auction cancellation scenarios, both for withdrawal and general cases.
- Improved clarity in offer editing tests by using the correct offer ID.
These changes collectively strengthen the validation of the system's behavior under various conditions, which is crucial for ensuring the robustness and reliability of the smart contract.
src/test/TestUSDCSellRepoToken.t.sol (2)
92-95: Excellent addition to verify repo token holding value!This new assertion strengthens the test by ensuring that the holding value of the repo token after the sale matches the expected proceeds. It's a great practice to add such checks for financial calculations.
639-641: Good addition of edge case test!This new test case verifies that the
getRepoTokenConcentrationRatiofunction correctly handles invalid input (zero address). It's an excellent practice to include such edge cases in your tests to ensure robust error handling.src/test/TestUSDCIntegration.t.sol (2)
305-306: Ensure the adapter address is correctly setThe assertion at the end of
testSetDiscountRateAdapterconfirms that thediscountRateAdapteraddress is correctly set. This is good practice to verify the state changes after the function execution.
44-46: Verify the usage ofblock.timestamp - 1inrepoTokenMaturedinitializationIn the initialization of
repoTokenMatured, the redemption timestamp is set toblock.timestamp - 1, which means the token is already matured upon creation. Ensure that this is intentional and won't cause issues in the test scenarios, especially regarding underflow risks or unexpected behavior.Run the following script to check for potential underflow issues:
✅ Verification successful
block.timestamp - 1 Usage in Test Files Verified
The usage of
block.timestamp - 1in the initialization ofrepoTokenMaturedis present in the following test files:
src/test/TestUSDCIntegration.t.sol:45src/test/TestUSDCSellRepoToken.t.sol:43These instances are intended for testing scenarios to simulate token maturity upon creation and do not pose any underflow risks or unexpected behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for underflow in `repoTokenMatured` initialization # Test: Search for any arithmetic operations involving `block.timestamp - 1` # Expect: No instances where underflow can occur rg --type solidity --no-heading --line-number 'block\.timestamp\s*-\s*1'Length of output: 107
Script:
#!/bin/bash # Description: Check for underflow in `repoTokenMatured` initialization without specifying file type # Search for any arithmetic operations involving `block.timestamp - 1` in Solidity files fd --extension sol --type f | xargs rg --no-heading --line-number 'block\.timestamp\s*-\s*1'Length of output: 396
| vm.expectRevert(); | ||
| termStrategy.setDiscountRateAdapter(address(invalid)); |
There was a problem hiding this comment.
Specify expected revert reason in vm.expectRevert()
In the test testSetDiscountRateAdapter, the vm.expectRevert() call before setting an invalid adapter address does not specify the expected revert reason or selector. For more precise testing, it's recommended to include the expected error message or error selector.
Apply this diff to specify the expected revert reason:
vm.prank(management);
-vm.expectRevert();
+vm.expectRevert("InvalidTermAdapter(address)");
termStrategy.setDiscountRateAdapter(address(invalid));Replace "InvalidTermAdapter(address)" with the actual revert reason emitted by the setDiscountRateAdapter function when provided with an invalid address.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/test/TestUSDCIntegration.t.sol (1)
353-354: Clean up unused instances in testsThe instance
invalidofTermDiscountRateAdapteris created with an invalid address and isn't cleaned up or used after the test. While this might not have a significant impact, cleaning up can prevent unintended side effects in other tests.Consider adding cleanup code if necessary or ensuring that the instance does not affect other tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/interfaces/term/ITermAuctionOfferLocker.sol (1 hunks)
- src/test/TestUSDCIntegration.t.sol (5 hunks)
- src/test/TestUSDCOffers.t.sol (15 hunks)
- src/test/mocks/MockTermAuctionOfferLocker.sol (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interfaces/term/ITermAuctionOfferLocker.sol
- src/test/mocks/MockTermAuctionOfferLocker.sol
🧰 Additional context used
🔇 Additional comments (11)
src/test/TestUSDCOffers.t.sol (11)
19-20: Good addition for comprehensive testingThe introduction of
repoToken100WeekandrepoToken100WeekAuctionallows for testing scenarios with longer maturity periods (100 weeks). This enhances the test coverage by including a wider range of time frames.
32-35: Improved initialization and consistencyThe code now consistently initializes both
repoToken1WeekandrepoToken100Weekwith improved readability. This change aligns with the addition of the 100-week test scenarios and maintains a consistent style.
41-41: LGTM!The initialization of
repoToken100WeekAuctionis correctly implemented and necessary for the new test scenarios.
54-55: Important initial state verificationThe addition of this assertion for
liquidReserveRatiois crucial. It verifies that the system starts in the expected state (fully liquid), which is important for the validity of subsequent tests.
83-88: Enhanced test coverage for offer submissionThe new assertions for
liquidReserveRatioandrepoTokenHoldingValuesignificantly improve the test coverage. They ensure that submitting an offer correctly affects these crucial values, providing a more comprehensive verification of the offer submission process.
90-124: Excellent addition of error handling testsThe new test functions significantly enhance the robustness of the test suite by covering important edge cases and error scenarios:
testSubmitOfferBelowLiquidReserveRatio: Ensures offers respect the required reserve ratio.testSubmitOfferToInvalidAuction: Verifies handling of invalid auction submissions.testSubmitOfferToAuctionWithInvalidRepoToken: Checks for proper validation of repo tokens.testSubmitOfferWithoutEnoughLiquidity: Confirms that offers are rejected when there's insufficient liquidity.These tests are crucial for verifying the system's behavior under various constraints and invalid inputs, contributing to a more reliable and secure implementation.
126-131: Critical test for maturity risk managementThe
testSubmitOfferWithExcessiveWeightedTimeMaturityfunction is a valuable addition to the test suite. It verifies that the system correctly rejects offers with maturity periods exceeding the threshold, which is crucial for effective risk management in term finance systems. This test helps ensure that the strategy adheres to its defined time-to-maturity limits.
151-166: Improved offer editing test with better state verificationThe modifications to
testEditOfferfunction enhance its effectiveness:
- New assertions for
liquidReserveRatioandrepoTokenHoldingValuebefore and after editing the offer provide better visibility into state transitions.- The use of
offerId1instead ofidHash1when editing the offer is more accurate, as it ensures we're modifying an existing offer rather than potentially creating a new one.These changes result in a more comprehensive and accurate test for the offer editing functionality.
184-202: Enhanced test coverage for offer deletion scenariosThe modifications and additions in this section significantly improve the test coverage for offer deletion:
- In
testDeleteOffers, new assertions forrepoTokenHoldingValueandliquidReserveRatioensure that the system state is correctly updated after deleting offers.- The new
testDeleteOffersFromInvalidAuctionfunction verifies that the system correctly handles attempts to delete offers from an invalid auction.These changes provide more comprehensive verification of the offer deletion process and its error handling, contributing to a more robust test suite.
Line range hint
242-261: Comprehensive improvements in auction-related test coverageThis set of changes significantly enhances the test suite's coverage of auction-related scenarios:
- New assertions for
liquidReserveRatioandrepoTokenHoldingValueacross multiple test functions provide more thorough state verification.- The renaming and modification of
testAuctionCancelForWithdrawal(previouslytestCompleteAuctionCanceled) more accurately reflects its purpose and includes additional state checks.- The new
testAuctionCancelfunction adds coverage for a general auction cancellation scenario.These improvements ensure a more comprehensive verification of the system's behavior in various auction-related situations, including successful completions, partial fills, and cancellations. This contributes to a more robust and reliable implementation of the auction mechanisms.
Also applies to: 279-301, 313-315, 330-371
389-390: Improved accuracy in multiple offer and offer editing testsThese modifications enhance the precision of several tests:
- In
testMultipleOffers, the reordering of assertions ensures that the correct order of offer IDs is verified.- In
testEditOfferTotalGreaterThanCurrentLiquidityandtestEditOfferTotalLessThanCurrentLiquidity, the use ofofferId1instead ofidHash1ensures that existing offers are being edited rather than potentially creating new ones.These changes contribute to more accurate and reliable testing of multiple offer scenarios and offer editing functionality.
Also applies to: 425-425, 436-436
| _submitOffer(bytes32("offer id hash 4"), 1e6, repoToken1YearAuction, repoToken1Year); | ||
|
|
||
| _submitOffer(bytes32("offer id hash 1"), 1e6, repoToken1WeekAuction, repoToken1Week); | ||
|
|
||
| bytes32 offerId2 = _submitOffer(bytes32("offer id hash 2"), 1e6, repoToken1MonthAuction, repoToken1Month); | ||
| _submitOffer(bytes32("offer id hash 2"), 1e6, repoToken1MonthAuction, repoToken1Month); | ||
|
|
||
| bytes32[] memory offers = termStrategy.pendingOffers(); | ||
|
|
||
| assertEq(offers.length, 2); | ||
| bool isSorted = true; | ||
|
|
||
| bytes32 offer1; | ||
| bytes32 offer2; | ||
| address termAuction1; | ||
| address termAuction2; | ||
|
|
||
| for (uint256 i = 0; i < offers.length - 1; i++) { | ||
| bytes32 offerSlot1 = keccak256(abi.encode(offers[i], 7)); | ||
| bytes32 offerSlot2 = keccak256(abi.encode(offers[i+1], 7)); | ||
| offer1 = vm.load(address(termStrategy), offerSlot1); | ||
| offer2 = vm.load(address(termStrategy), offerSlot2); | ||
| termAuction1 = address(uint160(uint256(offer1) >> 64)); | ||
| termAuction2 = address(uint160(uint256(offer2) >> 64)); | ||
|
|
||
| if (termAuction1 > termAuction2) { | ||
| isSorted=false; | ||
| break; | ||
| } | ||
| } | ||
| assertTrue(isSorted); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid directly accessing storage with vm.load
Directly accessing storage using vm.load can make tests brittle and dependent on internal storage layouts, which may change. This approach can lead to maintenance challenges.
Consider refactoring the test to use public getters or events to retrieve necessary information. This makes the test more robust and less sensitive to internal changes.
| assertApproxEq(simulatedWeightedMaturity, 1.25 weeks, 1); | ||
| assertEq(simulatedRepoTokenConcentrationRatio, 0.25e18); | ||
| assertEq(simulatedLiquidityRatio, 0.5e18); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use approximate equality for precision-sensitive assertions
When dealing with calculations involving decimal precision, it's safer to use assertApproxEq instead of assertEq to account for minor discrepancies due to rounding.
Apply this diff to use approximate assertions:
-assertEq(simulatedRepoTokenConcentrationRatio, 0.25e18);
-assertEq(simulatedLiquidityRatio, 0.5e18);
+assertApproxEq(simulatedRepoTokenConcentrationRatio, 0.25e18, 1e15);
+assertApproxEq(simulatedLiquidityRatio, 0.5e18, 1e15);This allows a tolerance in the assertion, preventing false negatives due to insignificant differences.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertApproxEq(simulatedWeightedMaturity, 1.25 weeks, 1); | |
| assertEq(simulatedRepoTokenConcentrationRatio, 0.25e18); | |
| assertEq(simulatedLiquidityRatio, 0.5e18); | |
| assertApproxEq(simulatedWeightedMaturity, 1.25 weeks, 1); | |
| assertApproxEq(simulatedRepoTokenConcentrationRatio, 0.25e18, 1e15); | |
| assertApproxEq(simulatedLiquidityRatio, 0.5e18, 1e15); |
| bytes32("test repo token 3"), address(mockUSDC), address(mockCollateral), 1e18, block.timestamp - 1 | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a clearer timestamp for matured tokens
Setting the maturity to block.timestamp - 1 can be unclear and might lead to edge case issues. Consider using a fixed past timestamp or subtracting a longer duration for better clarity.
Apply this diff to improve readability:
-repoTokenMatured = new MockTermRepoToken(
- bytes32("test repo token 3"), address(mockUSDC), address(mockCollateral), 1e18, block.timestamp - 1
+repoTokenMatured = new MockTermRepoToken(
+ bytes32("test repo token 3"), address(mockUSDC), address(mockCollateral), 1e18, block.timestamp - 1 days
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytes32("test repo token 3"), address(mockUSDC), address(mockCollateral), 1e18, block.timestamp - 1 | |
| ); | |
| bytes32("test repo token 3"), address(mockUSDC), address(mockCollateral), 1e18, block.timestamp - 1 days | |
| ); |
| vm.mockCall(repoToken1WeekAuction.termAuctionOfferLocker(), abi.encodeWithSelector(MockTermAuctionOfferLocker.unlockOffers.selector, offerIds), abi.encodeWithSelector(MockTermAuctionOfferLocker.OfferUnlockingFailed.selector)); | ||
| termStrategy.sellRepoToken(address(repoToken1Month), 1e6); |
There was a problem hiding this comment.
💡 Codebase verification
Mock behavior for offer unlocking failure is not properly handled
The test does not handle the OfferUnlockingFailed scenario as intended. There are no assertions or expectations verifying how the test responds to the mocked failure. Please ensure that the failure scenario is appropriately tested by handling the OfferUnlockingFailed selector.
🔗 Analysis chain
Verify the mock behavior for offer unlocking failure
Ensure that the mocked call to unlockOffers correctly simulates the failure scenario you intend to test. Confirm that the OfferUnlockingFailed selector is appropriate and that the test accurately captures the failure handling logic.
Run the following script to confirm the behavior:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the mocked `unlockOffers` function behaves as expected.
# Test: Search for `unlockOffers` usage and ensure it's properly mocked.
rg "vm\.mockCall.*unlockOffers\.selector"
Length of output: 313
Script:
#!/bin/bash
# Description: Verify how the test handles the OfferUnlockingFailed mock.
# Test: Search for handling of OfferUnlockingFailed.selector in the test file.
rg "OfferUnlockingFailed.selector" src/test/TestUSDCIntegration.t.sol
Length of output: 304
Script:
#!/bin/bash
# Description: Check if the test expects a revert or handles the OfferUnlockingFailed failure.
# Test: Search for usage of OfferUnlockingFailed.selector in expectation or assertion contexts.
rg "OfferUnlockingFailed.selector.*(revert|expectRevert|assert|require)" src/test/TestUSDCIntegration.t.sol
Length of output: 109
Summary by CodeRabbit
New Features
Bug Fixes
Chores
MockFactorywithMockFeesFactoryand removing unnecessary complexity.