delete three offers bugs #64
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related issues
Possibly related PRs
Poem
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: 2
🧹 Outside diff range and nitpick comments (1)
src/TermAuctionList.sol (1)
Line range hint
150-211: Suggestions for improving theremoveCompletedfunctionWhile the recent change fixes a critical issue, there are several areas where the
removeCompletedfunction could be further improved:
Error Handling: Consider adding try-catch blocks around external calls to
offer.offerLocker.lockedOffer(current).amountandoffer.termAuction.auctionCompleted()to handle potential reverts gracefully.Gas Optimization: The function performs multiple storage reads and writes. Consider caching frequently accessed storage variables in memory to reduce gas costs.
Reentrancy Protection: The function modifies state after making external calls. While it doesn't appear to be vulnerable to reentrancy in its current form, it's a good practice to implement a reentrancy guard or follow the checks-effects-interactions pattern more strictly.
Complexity: The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions to improve readability and maintainability.
Event Emission: Consider emitting events when offers are removed or when significant state changes occur. This can aid in off-chain tracking and debugging.
Would you like me to provide a code snippet demonstrating these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/TermAuctionList.sol (1 hunks)
- src/test/TestUSDCOffers.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/test/TestUSDCOffers.t.sol (1)
Line range hint
1-391: Overall assessment of changes in TestUSDCOffers.t.solThe additions to this file, including the new test function
testDeleteThreeOffers()and the constantsTHREESIXTY_DAYCOUNT_SECONDSandRATE_PRECISION, contribute to improved test coverage and calculation precision. These changes align well with the PR objective of addressing "delete three offers bugs".However, there are opportunities to further enhance the test coverage and improve code organization:
- The
testDeleteThreeOffers()function could be expanded to cover more scenarios and edge cases.- The placement and visibility of the new constants could be optimized for better readability and encapsulation.
Consider implementing the suggested improvements to make the tests more robust and the code more maintainable. These enhancements will contribute to a more comprehensive test suite and clearer code structure.
src/TermAuctionList.sol (1)
209-210: Approve: Critical fix for offer processingThis change is crucial for the correct functioning of the
removeCompletedfunction. By settingcurrent = prev;after removing a node, it ensures that no offers are skipped during the removal process.Before this fix, if a node was removed, the next iteration would skip the node that was originally after the removed node. This could lead to some offers not being properly processed or removed.
With this change, the function will correctly process all offers in the list, maintaining the integrity of the offer removal process.
| function testDeleteThreeOffers() public { | ||
| bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6); | ||
| bytes32 offerId2 = _submitOffer(bytes32("offer id hash 2"), 1e6); | ||
| bytes32 offerId3 = _submitOffer(bytes32("offer id hash 3"), 1e6); | ||
|
|
||
| bytes32[] memory offerIds = new bytes32[](3); | ||
| offerIds[0] = offerId1; | ||
| offerIds[1] = offerId2; | ||
| offerIds[2] = offerId3; | ||
|
|
||
| vm.expectRevert("!management"); | ||
| termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds); | ||
|
|
||
| vm.prank(management); | ||
| termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds); | ||
|
|
||
| bytes32[] memory offers = termStrategy.pendingOffers(); | ||
|
|
||
| assertEq(offers.length, 0); | ||
|
|
||
| assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance); | ||
| assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and assertions in testDeleteThreeOffers()
The test function effectively covers the basic scenario of deleting three offers. However, consider the following improvements to make it more robust:
- Add assertions to verify the state after submitting the offers and before deletion.
- Test partial deletion of offers (e.g., delete only two out of three offers).
- Use a more specific error message in the revert check, such as "Caller is not the management".
- Add a test case for deleting non-existent offers.
Here's an example of how you could enhance the test:
function testDeleteThreeOffers() public {
bytes32 offerId1 = _submitOffer(bytes32("offer id hash 1"), 1e6);
bytes32 offerId2 = _submitOffer(bytes32("offer id hash 2"), 1e6);
bytes32 offerId3 = _submitOffer(bytes32("offer id hash 3"), 1e6);
// Assert state after submitting offers
assertEq(termStrategy.pendingOffers().length, 3);
assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - 3e6);
bytes32[] memory offerIds = new bytes32[](3);
offerIds[0] = offerId1;
offerIds[1] = offerId2;
offerIds[2] = offerId3;
vm.expectRevert("Caller is not the management");
termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds);
// Test partial deletion
bytes32[] memory partialOfferIds = new bytes32[](2);
partialOfferIds[0] = offerId1;
partialOfferIds[1] = offerId2;
vm.prank(management);
termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), partialOfferIds);
assertEq(termStrategy.pendingOffers().length, 1);
assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance - 1e6);
// Delete remaining offer
vm.prank(management);
termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), new bytes32[](1));
bytes32[] memory offers = termStrategy.pendingOffers();
assertEq(offers.length, 0);
assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance);
assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance());
// Test deleting non-existent offers
vm.prank(management);
termStrategy.deleteAuctionOffers(address(repoToken1WeekAuction), offerIds);
// Assert that no changes occur when deleting non-existent offers
assertEq(termStrategy.totalLiquidBalance(), initialState.totalLiquidBalance);
assertEq(termStrategy.totalAssetValue(), termStrategy.totalLiquidBalance());
}These enhancements will provide more comprehensive coverage and help catch potential edge cases.
|
|
||
| uint256 public constant THREESIXTY_DAYCOUNT_SECONDS = 360 days; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve constant declarations and placement
The newly added constants THREESIXTY_DAYCOUNT_SECONDS and RATE_PRECISION are well-named and their values are appropriate for their intended use. However, consider the following improvements:
-
Change the visibility to
internalinstead ofpublicas these are test constants:uint256 internal constant THREESIXTY_DAYCOUNT_SECONDS = 360 days; uint256 internal constant RATE_PRECISION = 1e18;
-
Move these constants to the top of the contract, just after the contract declaration, to improve readability and follow common conventions:
contract TestUSDCSubmitOffer is Setup { uint256 internal constant THREESIXTY_DAYCOUNT_SECONDS = 360 days; uint256 internal constant RATE_PRECISION = 1e18; // ... rest of the contract code ... }
-
Add a comment explaining the use of 360 days instead of 365 days for
THREESIXTY_DAYCOUNT_SECONDS:// Using 360 days for financial year calculations (30/360 day count convention) uint256 internal constant THREESIXTY_DAYCOUNT_SECONDS = 360 days;
These changes will improve the overall structure and clarity of the contract.
| @@ -206,6 +206,8 @@ library TermAuctionList { | |||
| listData.nodes[prev].next = next; | |||
| delete listData.nodes[current]; | |||
| delete listData.offers[current]; | |||
| current = prev; | |||
|
|
|||
There was a problem hiding this comment.
I suggest the following optimization to avoid useless assignments:
if (removeNode) {
// Update the list to remove the current node
delete listData.nodes[current];
delete listData.offers[current];
if (current == listData.head) {
listData.head = next;
} else {
listData.nodes[prev].next = next;
current = prev;
}
}
Summary by CodeRabbit
New Features
Tests