Feat surplus invariant fixed #786
Conversation
WalkthroughThe recent updates involve enhancing the logic for surplus checks in a smart contract and improving a test suite for better coverage and debugging capabilities. Specifically, the surplus check logic now considers an additional condition, and the testing framework sees refinements in variable assignments and the introduction of new logging and testing functionalities focused on liquidation scenarios. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
|
1 similar comment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (1 hunks)
- packages/contracts/foundry_test/EchidnaToFoundry.t.sol (4 hunks)
Additional comments: 5
packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (1)
- 240-243: The conditional surplus checks after liquidation are based on
_icrToLiq <= cdpManager.MCR(). This condition ensures that surplus checks are only performed when the ICR (Initial Collateral Ratio) to liquidate is less than or equal to the MCR (Minimum Collateral Ratio). It's crucial to verify that this logic aligns with the intended behavior for handling surplus in liquidation scenarios. If the condition is too restrictive or not aligned with the protocol's risk parameters, it may need adjustment.packages/contracts/foundry_test/EchidnaToFoundry.t.sol (4)
- 28-30: The conditional assignment for
senderbased onmsg.sendermodulo 3 is a clever way to simulate different users in tests. However, ensure that this randomness aligns with the intended test scenarios and doesn't introduce unintended flakiness.- 37-37: Changing the assignment of
actorin thesetUpfunction to useaddress(USER1)directly simplifies the setup process. This change assumesUSER1is a predefined constant representing a user address, which should be validated for correctness in the context of the test environment.- 229-232: Adding logging statements for price-related variables in the
_logStakesfunction enhances the visibility of test execution, which is beneficial for debugging. Ensure that these logs provide sufficient context and are used consistently across all relevant test functions.- 1678-1690: The addition of a new test function
test_debugTheLiquidationfor liquidation scenarios is a valuable enhancement to the testing framework. It's important to ensure that this test covers a wide range of liquidation conditions and accurately reflects the contract's behavior in these scenarios. Consider adding more detailed comments or documentation within the test function to explain the specific scenarios being tested and the expected outcomes.
| if (_icrToLiq <= cdpManager.MCR()) { | ||
| gte(vars.collSurplusPoolBefore + 4, vars.collSurplusPoolAfter, "SURPLUS-CHECK-1"); | ||
| gte(vars.userSurplusBefore + 4, vars.userSurplusAfter, "SURPLUS-CHECK-2"); | ||
| } |
There was a problem hiding this comment.
The logic for surplus checks after liquidation (SURPLUS-CHECK-1 and SURPLUS-CHECK-2) uses a fixed value of 4 to compare the before and after states of vars.collSurplusPool and vars.userSurplus. This approach assumes a maximum surplus cap of 4 wei, which might not be flexible or accurate for all scenarios. Consider if this fixed value aligns with the intended logic and whether it should be configurable or calculated based on dynamic parameters to ensure it accurately reflects the surplus cap under different conditions.
The use of gte function calls within the surplus check logic (SURPLUS-CHECK-1 and SURPLUS-CHECK-2) implies a comparison that expects the after state to be greater than or equal to the before state plus a fixed value. This logic should be carefully reviewed to ensure it accurately represents the expected state changes following a liquidation event. If the logic does not correctly account for all possible outcomes or if the fixed value does not accurately represent the surplus cap, it may lead to incorrect assessments of the protocol's financial health post-liquidation.
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (2 hunks)
- packages/contracts/foundry_test/EchidnaToFoundry.t.sol (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol
Additional comments: 4
packages/contracts/foundry_test/EchidnaToFoundry.t.sol (4)
- 28-31: The conditional assignment for
senderbased onmsg.sendermodulo 3 is a clever way to diversify the sender address for testing purposes. However, ensure that this logic aligns with the intended test scenarios and that it doesn't introduce any unintended side effects in the test outcomes.- 37-37: Changing the assignment of
actorin thesetUpfunction to useaddress(USER1)directly is a straightforward modification. This change simplifies the code by removing an unnecessary indirection through theactorsarray. Just ensure thatUSER1is properly defined and accessible within this context.- 224-240: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [229-260]
Adding logging statements for price-related variables in the
_logStakesfunction enhances the visibility of test execution, which is beneficial for debugging and understanding the test flow. It's good practice to include such logging in test environments to aid in diagnosing issues. Ensure that these logs are only enabled during testing to avoid any performance impact in production code.
- 1678-1693: The addition of a new test function
test_debugTheLiquidationfor liquidation scenarios is a valuable enhancement to the testing framework. It appears to simulate various operations leading up to and including liquidation, which is crucial for ensuring the robustness of the contract under test. Make sure that the test covers a wide range of scenarios and correctly asserts the expected outcomes to fully validate the liquidation logic.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor