Repo redemption haircut #14
Conversation
WalkthroughThe changes introduce a new variable Changes
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Strategy.sol (5 hunks)
Additional comments not posted (4)
src/Strategy.sol (4)
68-68: LGTM!The new
repoRedemptionHaircutmapping is correctly declared to store haircut values for repo tokens.
133-140: LGTM!The new
setRepoRedemptionHaircutfunction is correctly implemented to allow management to set the haircut value for a specific repo token. It follows the required access control using theonlyManagementmodifier.
366-373: LGTM!The changes correctly incorporate the haircut value into the calculation of the repo token amount in base asset precision. The default value of
1e18is used when no haircut is set, ensuring consistent behavior. The haircut is applied as expected during the redemption value calculation.
1041-1046: LGTM!The changes correctly incorporate the haircut value into the calculation of the repo token amount in base asset precision, consistent with the previous code segment. The default value of
1e18is used when no haircut is set, ensuring consistent behavior. The haircut is applied as expected during the redemption value calculation.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/RepoTokenList.sol (2 hunks)
- src/Strategy.sol (7 hunks)
Additional comments not posted (7)
src/RepoTokenList.sol (1)
190-191: Verify the parameter usage in the codebase.Ensure that all calls to
getPresentValueprovide the new parameterrepoRedemptionHaircutMantissa.Run the following script to verify the parameter usage:
Verification successful
Parameter usage verified successfully.
All calls to the
getPresentValuefunction insrc/RepoTokenList.solcorrectly include the new parameterrepoRedemptionHaircutMantissa. No issues were found with the parameter usage in the codebase.
- Verified in
src/Strategy.solwith correct arguments.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `getPresentValue` provide the new parameter. # Test: Search for the function usage. Expect: Only occurrences with 4 arguments. rg --type solidity -A 5 $'getPresentValue'Length of output: 74
Script:
#!/bin/bash # Description: Verify all calls to `getPresentValue` provide the new parameter. # Test: Search for the function usage. Expect: Only occurrences with 4 arguments. rg -A 5 'getPresentValue'Length of output: 1971
src/Strategy.sol (6)
68-68: LGTM!The code changes are approved.
133-140: LGTM!The code changes are approved.
366-373: LGTM!The code changes are approved.
416-421: LGTM!The code changes are approved.
443-450: LGTM!The code changes are approved.
1045-1050: LGTM!The code changes are approved.
src/RepoTokenList.sol
Outdated
| (ITermRepoToken(current).redemptionValue() * repoRedemptionHaircutMantissa * repoTokenBalance * purchaseTokenPrecision) / | ||
| (repoTokenPrecision * RepoTokenUtils.RATE_PRECISION * 1e18); |
There was a problem hiding this comment.
LGTM!
The calculation looks good and aligns with the AI-generated summary.
Refactor suggestion: Use constants for precision values.
Consider defining constants for the precision values to improve readability and maintainability.
Apply this diff to define and use constants:
+uint256 private constant REPO_TOKEN_UTILS_RATE_PRECISION = 1e18;
+uint256 private constant PRECISION_FACTOR = 1e18;
uint256 repoTokenBalanceInBaseAssetPrecision =
- (ITermRepoToken(current).redemptionValue() * repoRedemptionHaircutMantissa * repoTokenBalance * purchaseTokenPrecision) /
- (repoTokenPrecision * RepoTokenUtils.RATE_PRECISION * 1e18);
+ (ITermRepoToken(current).redemptionValue() * repoRedemptionHaircutMantissa * repoTokenBalance * purchaseTokenPrecision) /
+ (repoTokenPrecision * REPO_TOKEN_UTILS_RATE_PRECISION * PRECISION_FACTOR);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/RepoTokenList.sol (1 hunks)
Additional comments not posted (5)
src/RepoTokenList.sol (5)
190-191: LGTM!The function signature change aligns with the AI-generated summary and appears to be intentional.
196-202: LGTM!The changes to the internal logic align with the AI-generated summary and enhance the clarity and efficiency of the code.
213-245: LGTM!The new
_getTotalPresentValuefunction aligns with the AI-generated summary and promotes code reuse and maintainability. The incorporation of therepoRedemptionHaircutMantissaparameter in the calculation reflects the changes made to thegetPresentValuefunction.
234-236: Refactor suggestion: Use constants for precision values.The past review comment suggesting the use of constants for precision values is still valid and applicable to the updated calculation.
241-242: LGTM!The function call to
RepoTokenUtils.calculatePresentValueappears to be correct and consistent with the changes made to the calculation ofrepoTokenBalanceInBaseAssetPrecision.
There was a problem hiding this comment.
@aazhou1 this is not correct, we need to do this in the discountRateAdapter
- we (term) needs to be the one that sets haircut by keeper
- we are not the Manager here
- this is useable across all vaults
==> centralize shortfall haircut keeper method to the discountRateAdapater
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/Strategy.sol (6 hunks)
- src/TermDiscountRateAdapter.sol (2 hunks)
- src/interfaces/term/ITermDiscountRateAdapter.sol (1 hunks)
- src/test/utils/Setup.sol (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Strategy.sol
Additional comments not posted (7)
src/interfaces/term/ITermDiscountRateAdapter.sol (1)
6-6: Existing functiongetDiscountRateremains unchanged and correctly implemented.The function
getDiscountRateis appropriately declared with the correct modifiers and parameter names, and remains unchanged in this pull request.src/TermDiscountRateAdapter.sol (5)
7-7: Approved: Import of AccessControlUpgradeable.The import statement for
AccessControlUpgradeableis correct and necessary for the added functionality.
14-14: Approved: Contract inheritance change.The contract now correctly inherits from both
ITermDiscountRateAdapterandAccessControlUpgradeable, aligning with the PR's objectives to introduce access control.
16-16: Approved: Definition of DEVOPS_ROLE.The constant
DEVOPS_ROLEis defined using a keccak256 hash, which is a standard practice for defining roles in Solidity contracts using the AccessControl library.
19-19: Approved: New mapping for repoRedemptionHaircut.The introduction of the
repoRedemptionHaircutmapping is well-implemented. It allows the storage of haircut values associated with specific repo tokens, which is crucial for the new functionality.
26-28: Review: Constructor updates.The constructor now accepts an additional parameter
devopsWallet_and correctly grants theDEVOPS_ROLEto this address. Ensure that the address provided during deployment is correct and that the role assignment aligns with your access control policies.Verification successful
Constructor usage verified.
The deployment script correctly uses the updated constructor signature for
TermDiscountRateAdapter, including thedevopsWallet_parameter. Ensure that thedevopsWalletaddress provided aligns with your access control policies.
- Constructor usage:
discountRateAdapter = new TermDiscountRateAdapter(address(termController), devopsWallet);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of the `devopsWallet_` parameter in deployment scripts. # Test: Search for the constructor usage in deployment scripts. Expect: Correct usage of the new constructor signature. rg --type solidity -A 5 $'new TermDiscountRateAdapter'Length of output: 88
Script:
#!/bin/bash # Find all Solidity files and search for the constructor usage of TermDiscountRateAdapter fd --extension sol --exec rg 'new TermDiscountRateAdapter' {}Length of output: 163
src/test/utils/Setup.sol (1)
98-98: Verify the updated constructor usage ofTermDiscountRateAdapter.The change to include
devopsWalletin the constructor ofTermDiscountRateAdaptersuggests an enhancement in functionality. It is crucial to:
- Ensure that the
TermDiscountRateAdaptercontract has been appropriately updated to handle this new parameter.- Verify that
devopsWalletis being used correctly within theTermDiscountRateAdapter.- Assess the impact of this change on other parts of the system, especially where
TermDiscountRateAdapteris utilized.Run the following script to verify the constructor usage and the handling of
devopsWalletwithin theTermDiscountRateAdapter:Verification successful
Constructor update for
TermDiscountRateAdapterverified successfully.The constructor of
TermDiscountRateAdapterhas been updated to include thedevopsWalletparameter, and it is used to grant a role within the contract. This change is consistent with the instantiation inSetup.soland appears to be implemented correctly.
- The constructor in
src/TermDiscountRateAdapter.solincludesdevopsWallet_and uses it appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor usage and handling of `devopsWallet` in `TermDiscountRateAdapter`. # Test: Search for the constructor definition and usage. Expect: Constructor should include `devopsWallet`. ast-grep --lang solidity --pattern $'contract TermDiscountRateAdapter { constructor($_, $_) { $$$ } }'Length of output: 228
Script:
#!/bin/bash # Use ripgrep to search for the constructor definition of TermDiscountRateAdapter in Solidity files. rg 'constructor\(' --glob '*.sol' -A 5Length of output: 4637
| @@ -2,5 +2,6 @@ | |||
| pragma solidity ^0.8.18; | |||
|
|
|||
| interface ITermDiscountRateAdapter { | |||
| function repoRedemptionHaircut(address) external view returns (uint256); | |||
There was a problem hiding this comment.
Specify parameter name for clarity in repoRedemptionHaircut.
The function repoRedemptionHaircut lacks a parameter name, which could lead to confusion or errors when implementing the interface. It is recommended to specify a parameter name to improve readability and maintainability.
Consider modifying the function declaration as follows:
-function repoRedemptionHaircut(address) external view returns (uint256);
+function repoRedemptionHaircut(address repoAddress) external view returns (uint256);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.
| function repoRedemptionHaircut(address) external view returns (uint256); | |
| function repoRedemptionHaircut(address repoAddress) external view returns (uint256); |
| function setRepoRedemptionHaircut(address repoToken, uint256 haircut) external onlyRole(DEVOPS_ROLE) { | ||
| repoRedemptionHaircut[repoToken] = haircut; | ||
| } |
There was a problem hiding this comment.
Review: New function setRepoRedemptionHaircut.
The function setRepoRedemptionHaircut is correctly implemented with role-based access control using onlyRole(DEVOPS_ROLE). This ensures that only authorized users can modify the haircut values. However, consider adding input validation for the haircut parameter to ensure it falls within expected ranges, especially since it represents financial data.
Consider adding input validation for the haircut parameter:
require(haircut <= 100 * 10**18, "Haircut value is too high"); // Assuming 100% is the maximum valid haircut
Summary by CodeRabbit
New Features
Improvements
Access Control