add governor role for access to setters for term strategy parameters #75
Conversation
WalkthroughThe pull request introduces modifications to several files, primarily focusing on the integration of a new input variable, Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (4)
script/Strategy.s.sol (1)
40-41: Fix typo in log message & LGTM on constructor changes.The constructor parameter addition is correct, but there's a typo in the log message after this block ("strateghy" instead of "strategy").
- console.log("deployed strateghy contract to"); + console.log("deployed strategy contract to");src/test/utils/Setup.sol (1)
122-123: Consider improving parameter formatting for better readability.While the parameter additions are correct, the formatting could be improved to maintain consistent indentation with other parameters.
Consider this formatting:
new Strategy( address(asset), "Tokenized Strategy", address(mockYearnVault), address(discountRateAdapter), - address(termVaultEventEmitter), - governor + address(termVaultEventEmitter), + governor )src/test/TestUSDCOffers.t.sol (1)
Line range hint
1-444: Consider adding additional test cases for comprehensive coverageWhile the current test coverage is good, consider adding these test cases to ensure robust validation of the governor role:
- Attempt to call setter functions with non-governor address
- Test parameter boundary conditions with governor role
- Verify parameter persistence after setting with governor role
Would you like me to help generate these additional test cases?
src/Strategy.sol (1)
Line range hint
467-467: Inconsistent use of access control modifiersThe functions
submitAuctionOfferanddeleteAuctionOffersstill useonlyManagementfor access control, whereas other functions have transitioned toonlyRole(GOVERNOR_ROLE). For consistency and to centralize permission management, consider replacingonlyManagementwithonlyRole(GOVERNOR_ROLE).Apply the following diff to update access control:
-function submitAuctionOffer( +function submitAuctionOffer( ITermAuction termAuction, address repoToken, bytes32 idHash, bytes32 offerPriceHash, uint256 purchaseTokenAmount -) +) external whenNotPaused nonReentrant notBlacklisted(repoToken) - onlyManagement + onlyRole(GOVERNOR_ROLE) returns (bytes32[] memory offerIds) { require(purchaseTokenAmount > 0, "Purchase token amount must be greater than zero"); // Function body... } -function deleteAuctionOffers( +function deleteAuctionOffers( address termAuction, bytes32[] calldata offerIds -) +) external - onlyManagement + onlyRole(GOVERNOR_ROLE) { // Function body... }Also applies to: 595-595
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/deploy-sepolia-strategy.yaml (1 hunks)
- script/Strategy.s.sol (2 hunks)
- src/Strategy.sol (14 hunks)
- src/test/TestUSDCIntegration.t.sol (6 hunks)
- src/test/TestUSDCOffers.t.sol (3 hunks)
- src/test/TestUSDCSellRepoToken.t.sol (11 hunks)
- src/test/TestUSDCSubmitOffer.t.sol (2 hunks)
- src/test/utils/ExtendedTest.sol (1 hunks)
- src/test/utils/Setup.sol (4 hunks)
🔇 Additional comments (38)
.github/workflows/deploy-sepolia-strategy.yaml (1)
67-67: LGTM! Verify environment configuration.The addition of
GOVERNOR_ROLE_ADDRESSfollows the established pattern for role-based access control and is appropriately configured using repository variables.Let's verify the environment configuration:
✅ Verification successful
✓ GOVERNOR_ROLE configuration is properly integrated
The GOVERNOR_ROLE is correctly implemented and consistently used across the codebase:
- Defined as a constant in
src/Strategy.sol- Used with
onlyRolemodifier for all governor-restricted functions- Properly granted during contract initialization
- Referenced in deployment script
Strategy.s.solusing the environment variable- Workflow variable aligns with the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that GOVERNOR_ROLE_ADDRESS is properly configured in the Sepolia environment # and follows the same pattern as other role addresses. # Test: Check if the variable is referenced consistently across deployment-related files rg -A 2 "GOVERNOR_ROLE|governor" --type yaml --type solLength of output: 85
Script:
#!/bin/bash # Let's try a different approach to verify the GOVERNOR_ROLE usage: # 1. Check workflow files for GOVERNOR_ROLE rg "GOVERNOR_ROLE" .github/workflows/ # 2. Find all Solidity files that might contain governor role references fd "\.sol$" # 3. Check content of found Solidity files for governor-related code rg "governor|GOVERNOR" --type-add 'sol:*.sol' -t solLength of output: 5362
src/test/utils/ExtendedTest.sol (1)
86-86: LGTM: New overload of assertApproxEq with error message.The addition of a custom error message parameter enhances the test failure output without changing the core functionality.
script/Strategy.s.sol (2)
Line range hint
48-73: Verify permission requirements for test mode configuration.The test mode configuration calls several setter functions that might now require governor role permissions. Ensure that these calls will work with the new access control system.
Let's check the Strategy contract's permission requirements:
#!/bin/bash # Check permission requirements for setter functions in Strategy contract ast-grep --pattern 'function set$$$($$$) $$$onlyRole(GOVERNOR_ROLE)$$$'
25-25: LGTM! Verify environment variable setup in CI/CD.The governor role address is properly retrieved using
vm.envAddress.Let's verify the environment variable is properly set in the deployment workflow:
✅ Verification successful
Environment variable GOVERNOR_ROLE_ADDRESS is properly configured
The GOVERNOR_ROLE_ADDRESS is correctly set up in the deployment workflow:
- It's defined as
${{ vars.GOVERNOR_ROLE_ADDRESS }}in the strategy deployment workflow- It's used as a required parameter in the Strategy constructor
- The variable is stored in GitHub repository variables (vars) which is the appropriate place for non-sensitive configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if GOVERNOR_ROLE_ADDRESS is properly configured in deployment workflows rg -l "GOVERNOR_ROLE_ADDRESS:" .github/workflows/Length of output: 73
Script:
#!/bin/bash # Let's check for workflow files and their content fd -e yml -e yaml . .github/workflows/ -x cat {} # Also check if there are any environment files that might contain this variable fd -g '*.env*' . -x cat {} # Check the deployment script itself to understand the context better cat script/Strategy.s.solLength of output: 10323
src/test/utils/Setup.sol (3)
44-44: LGTM: Governor role declaration follows established patterns.The new governor role is properly declared and initialized, maintaining consistency with other role address declarations in the test setup.
108-108: LGTM: Governor address properly labeled for test traces.The governor address is appropriately labeled, maintaining consistency with other role labels and improving test trace readability.
141-142: Verify access control consistency with Strategy contract.The change from
managementtogovernorfor setting the term controller aligns with the PR objective. However, we should verify this matches the Strategy contract's access control implementation.✅ Verification successful
Access control verification successful - setTermController requires GOVERNOR_ROLE
The change from
managementtogovernorin the test setup is consistent with the Strategy contract's implementation. ThesetTermControllerfunction in Strategy.sol explicitly requiresGOVERNOR_ROLEthrough theonlyRolemodifier, confirming that only accounts with governor privileges can set the term controller.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Strategy contract uses GOVERNOR_ROLE for setTermController # Test: Search for setTermController function definition ast-grep --pattern 'function setTermController($$$) { $$$ }' # Test: Search for GOVERNOR_ROLE usage in access control rg -A 2 'GOVERNOR_ROLE'Length of output: 3200
src/test/TestUSDCSubmitOffer.t.sol (2)
Line range hint
36-42: LGTM! Clear separation of roles implemented.The changes correctly implement role-based access control by having the governor set critical strategy parameters during setup, while maintaining management's operational role for offer submissions.
96-97: LGTM! Verify role-based access control implementation.The change correctly uses the governor role for setting concentration limits while preserving management's ability to submit offers.
Let's verify the complete role-based access control implementation:
✅ Verification successful
Role-based access control implementation verified correctly
The verification confirms proper role separation:
- Governor role is consistently used for all parameter setters (setTermController, setDiscountRateAdapter, setTimeToMaturityThreshold, etc.) and admin functions (pause/unpause)
- Management role is properly restricted to operational functions (submitAuctionOffer, deleteAuctionOffers)
- Access control is enforced through onlyRole(GOVERNOR_ROLE) and onlyManagement modifiers
- No instances found where management can access governor functions or vice versa
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent role-based access control implementation # Expected: governor for parameter setters, management for operations # Check for any remaining management calls to setter functions rg -A 2 "management.*set[A-Z]" # Check for any governor calls to operational functions rg -A 2 "governor.*(submit|delete|auction)" # Check for proper role separation in tests ast-grep --pattern 'function test$_() { $$$ vm.prank(management); $$$ }'Length of output: 159
Script:
#!/bin/bash # Let's try a more comprehensive search for role-based patterns # Search for prank calls with governor role rg -A 2 "prank.*governor" # Search for prank calls with management role rg -A 2 "prank.*management" # Search for setters to understand access control rg -A 2 "function set[A-Z][a-zA-Z]*\(" # Search for onlyGovernor modifier usage rg -A 2 "onlyGovernor" # Search for onlyManagement modifier usage rg -A 2 "onlyManagement"Length of output: 14991
src/test/TestUSDCIntegration.t.sol (5)
Line range hint
64-70: LGTM: Proper governor role implementation for parameter settersThe change correctly implements the governor role for setting strategy parameters, with proper access control wrapping.
251-254: LGTM: Consistent governor role implementationThe changes maintain consistency with the governor role pattern established in the setup.
Line range hint
323-343: LGTM: Comprehensive pause functionality testingThe test cases properly verify both authorized (governor) and unauthorized access for pause/unpause functionality.
357-366: LGTM: Well-structured discount rate adapter testsThe test cases thoroughly verify the discount rate adapter functionality with proper authorization checks and error handling.
306-312: Verify the expected revert message behaviorThe test removes the specific revert message (
"!management"). While this might be correct if using OpenZeppelin's AccessControl, it would be beneficial to verify that the actual revert message matches the expected AccessControl behavior.✅ Verification successful
Removal of specific revert message is correct
The verification confirms that the contract uses OpenZeppelin's AccessControl with
GOVERNOR_ROLEand theonlyRolemodifier. The removal of the specific revert message"!management"is correct because:
- The function
setRepoTokenBlacklistis protected byonlyRole(GOVERNOR_ROLE)- AccessControl's
onlyRolemodifier handles the revert message automatically- The test correctly verifies both unauthorized and authorized access scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AccessControl usage in Strategy contract ast-grep --pattern 'import {$_} from "@openzeppelin/contracts/access/AccessControl.sol"'Length of output: 90
Script:
#!/bin/bash # Let's try a broader search for AccessControl imports and inheritance rg -l "AccessControl" # Also search for role-based access patterns ast-grep --pattern 'bytes32 public constant $_ = keccak256' # Check the actual implementation of setRepoTokenBlacklist ast-grep --pattern 'function setRepoTokenBlacklist($$$) { $$$ }'Length of output: 263
Script:
#!/bin/bash # Let's examine the Strategy.sol file for the implementation details rg -A 5 "setRepoTokenBlacklist" src/Strategy.sol # Check for role definitions in Strategy.sol rg "MANAGEMENT_ROLE|DEFAULT_ADMIN_ROLE" src/Strategy.sol # Look at the actual contract definition to see access control inheritance rg -A 3 "contract Strategy" src/Strategy.solLength of output: 686
Script:
#!/bin/bash # Let's check for GOVERNOR_ROLE definition rg "GOVERNOR_ROLE" src/Strategy.sol # And check if there are any specific revert messages in BaseStrategy rg -A 3 "contract BaseStrategy" src/BaseStrategy.sol # Look for any custom errors related to governance rg "error.*Governor" src/*.solLength of output: 1099
src/test/TestUSDCOffers.t.sol (3)
Line range hint
43-49: LGTM: Proper initialization of strategy parameters with governor roleThe setup correctly implements the new access control mechanism by using the governor role to set strategy parameters.
91-98: LGTM: Clear separation of governor and management rolesThe test case correctly demonstrates the separation of responsibilities:
- Governor role for parameter configuration
- Management role for operational tasks
137-144: LGTM: Consistent implementation of role-based access controlThe test case maintains consistency with other tests in implementing the role-based access control pattern and proper error validation.
src/test/TestUSDCSellRepoToken.t.sol (6)
Line range hint
48-52: LGTM: Proper governor role usage in setUp.The setup correctly uses the governor role to initialize strategy parameters.
80-83: LGTM: Proper governor role usage in test setup.The test correctly uses the governor role to set collateral token parameters and maturity threshold.
469-480: LGTM: Proper governor role usage in validation tests.The test correctly uses the governor role to set parameters for repo token validation tests.
493-494: LGTM: Proper governor role usage in maturity threshold test.The test correctly uses the governor role to set the time to maturity threshold.
630-635: LGTM: Proper governor role usage in concentration limit test.The test correctly verifies that:
- Non-governor accounts cannot modify the concentration limit
- Governor can successfully set the concentration limit
Line range hint
658-676: LGTM: Proper governor role usage in pause functionality test.The test correctly verifies that:
- Non-governor accounts cannot pause/unpause the strategy
- Governor can successfully pause and unpause the strategy
- Paused state properly prevents deposits and repo token sales
src/Strategy.sol (15)
19-19: Importing AccessControl: Proper inclusion for role managementThe addition of
AccessControlimport allows the contract to leverage role-based access control features, which is appropriate for managing permissions.
37-37: Inheriting AccessControl: Correct integration of access controlThe contract now inherits from
AccessControl, enabling role-based permission management throughout the contract.
51-51: Defining GOVERNOR_ROLE constant: Appropriate role setupDefining
GOVERNOR_ROLEas a constant usingkeccak256ensures a unique identifier for the role, which is a best practice.
87-89: Updated access control to GOVERNOR_ROLE inpauseDepositThe
pauseDepositfunction now correctly usesonlyRole(GOVERNOR_ROLE)for access control, ensuring only authorized users can invoke it.
95-97: Updated access control to GOVERNOR_ROLE inunpauseDepositThe
unpauseDepositfunction now correctly usesonlyRole(GOVERNOR_ROLE), maintaining consistent access control.
103-105: Updated access control to GOVERNOR_ROLE inpauseStrategyThe
pauseStrategyfunction now usesonlyRole(GOVERNOR_ROLE), aligning with the role-based access control system.
112-114: Updated access control to GOVERNOR_ROLE inunpauseStrategyThe
unpauseStrategyfunction correctly usesonlyRole(GOVERNOR_ROLE)for access control.
Line range hint
124-130: Updated access control insetTermControllerThe
setTermControllerfunction now requiresonlyRole(GOVERNOR_ROLE), ensuring only the governor can change the term controller.
Line range hint
142-149: Updated access control insetDiscountRateAdapterAccess control is updated to
onlyRole(GOVERNOR_ROLE)insetDiscountRateAdapter, which is appropriate.
Line range hint
158-164: Updated access control insetTimeToMaturityThresholdThe function now requires
onlyRole(GOVERNOR_ROLE), maintaining consistent access control.
Line range hint
173-179: Updated access control insetRequiredReserveRatioUsing
onlyRole(GOVERNOR_ROLE)in this function ensures only the governor can modify the required reserve ratio.
Line range hint
187-193: Updated access control insetRepoTokenConcentrationLimitThe function now correctly uses
onlyRole(GOVERNOR_ROLE)for access control.
Line range hint
201-208: Updated access control insetDiscountRateMarkupRequiring
onlyRole(GOVERNOR_ROLE)in this function ensures that only authorized roles can change the discount rate markup.
216-222: Updated access control insetCollateralTokenParamsThe function now uses
onlyRole(GOVERNOR_ROLE)for access control, which is appropriate.
224-226: Added functionsetRepoTokenBlacklistwith GOVERNOR_ROLE access controlThe new function
setRepoTokenBlacklistallows blacklisting of repo tokens and is appropriately protected byonlyRole(GOVERNOR_ROLE).
| function expectRoleRevert(bytes32 role) internal { | ||
| string memory revertMessage = string(abi.encodePacked( | ||
| "AccessControl: account ", | ||
| address(this), | ||
| " is missing role ", | ||
| role | ||
| )); | ||
| vm.expectRevert(bytes(revertMessage)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the caller address configurable.
The expectRoleRevert function assumes that address(this) is always the caller address in the revert message. This might not be flexible enough for testing scenarios where the unauthorized caller is a different address.
Consider this implementation:
- function expectRoleRevert(bytes32 role) internal {
+ function expectRoleRevert(bytes32 role, address account) internal {
string memory revertMessage = string(abi.encodePacked(
"AccessControl: account ",
- address(this),
+ account,
" is missing role ",
role
));
vm.expectRevert(bytes(revertMessage));
}
+
+ // Overload for backward compatibility and convenience when this contract is the caller
+ function expectRoleRevert(bytes32 role) internal {
+ expectRoleRevert(role, address(this));
+ }📝 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 expectRoleRevert(bytes32 role) internal { | |
| string memory revertMessage = string(abi.encodePacked( | |
| "AccessControl: account ", | |
| address(this), | |
| " is missing role ", | |
| role | |
| )); | |
| vm.expectRevert(bytes(revertMessage)); | |
| } | |
| function expectRoleRevert(bytes32 role, address account) internal { | |
| string memory revertMessage = string(abi.encodePacked( | |
| "AccessControl: account ", | |
| account, | |
| " is missing role ", | |
| role | |
| )); | |
| vm.expectRevert(bytes(revertMessage)); | |
| } | |
| // Overload for backward compatibility and convenience when this contract is the caller | |
| function expectRoleRevert(bytes32 role) internal { | |
| expectRoleRevert(role, address(this)); | |
| } |
| vm.expectRevert(); | ||
| termStrategy.setTermController(address(newController)); | ||
|
|
||
| vm.expectRevert(); | ||
| vm.prank(management); | ||
| vm.prank(governor); | ||
| termStrategy.setTermController(address(0)); | ||
|
|
||
| address currentController = address(termStrategy.currTermController()); | ||
| vm.prank(management); | ||
| vm.prank(governor); | ||
| termStrategy.setTermController(address(newController)); | ||
| assertEq(address(termStrategy.currTermController()), address(newController)); | ||
| assertEq(address(termStrategy.prevTermController()), currentController); | ||
|
|
||
| vm.expectRevert("!management"); | ||
| vm.expectRevert(); | ||
| termStrategy.setTimeToMaturityThreshold(12345); | ||
|
|
||
| vm.prank(management); | ||
| vm.prank(governor); | ||
| termStrategy.setTimeToMaturityThreshold(12345); | ||
| assertEq(termStrategy.timeToMaturityThreshold(), 12345); | ||
|
|
||
| vm.expectRevert("!management"); | ||
| vm.expectRevert(); | ||
| termStrategy.setRequiredReserveRatio(12345); | ||
|
|
||
| vm.prank(management); | ||
| vm.prank(governor); | ||
| termStrategy.setRequiredReserveRatio(12345); | ||
| assertEq(termStrategy.requiredReserveRatio(), 12345); | ||
|
|
||
| vm.expectRevert("!management"); | ||
| vm.expectRevert(); | ||
| termStrategy.setDiscountRateMarkup(12345); | ||
|
|
||
| vm.prank(management); | ||
| vm.prank(governor); | ||
| termStrategy.setDiscountRateMarkup(12345); | ||
| assertEq(termStrategy.discountRateMarkup(), 12345); | ||
|
|
||
| vm.expectRevert("!management"); | ||
| vm.expectRevert(); | ||
| termStrategy.setCollateralTokenParams(address(mockCollateral), 12345); | ||
|
|
||
| vm.prank(management); | ||
| vm.prank(governor); |
There was a problem hiding this comment.
💡 Codebase verification
Events are properly emitted for parameter changes, but boundary tests are missing.
The code shows that all parameter changes emit events through TERM_VAULT_EVENT_EMITTER:
emitTermControllerUpdatedfor controller changesemitTimeToMaturityThresholdUpdatedfor maturity thresholdemitRequiredReserveRatioUpdatedfor reserve ratioemitDiscountRateMarkupUpdatedfor discount rate markupemitMinCollateralRatioUpdatedfor collateral params
However, the tests should be extended to verify:
- Maximum values for ratios (e.g., reserve ratio > 1e18)
- Zero values for thresholds and markups
- Negative impact scenarios (e.g., very low reserve ratio)
🔗 Analysis chain
Verify access control test coverage.
The governance parameter tests correctly verify that:
- Non-governor accounts cannot modify parameters (lines 409, 422, 429, 436, 443)
- Governor can successfully modify parameters
- Zero address validation for critical parameters (line 413)
However, consider adding tests for:
- Parameter boundary values
- Event emissions for parameter changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any event emission tests for parameter changes
rg -A 2 "emit.*\b(TimeToMaturityThreshold|RequiredReserveRatio|DiscountRateMarkup|CollateralTokenParams)\b"
Length of output: 109
Script:
#!/bin/bash
# Check for event declarations in the contract
rg -A 2 "event.*\b(TimeToMaturityThreshold|RequiredReserveRatio|DiscountRateMarkup|CollateralTokenParams)\b"
# Check the contract implementation for event emissions
ast-grep --pattern 'emit $name($$$);'
# Get the contract interface/implementation
fd -e sol -x cat {} | grep -A 5 "function set.*\b(TimeToMaturityThreshold|RequiredReserveRatio|DiscountRateMarkup|CollateralTokenParams)\b"
Length of output: 294
Script:
#!/bin/bash
# Let's first find the contract file that contains these setter functions
fd -e sol | xargs grep -l "setTimeToMaturityThreshold\|setRequiredReserveRatio\|setDiscountRateMarkup\|setCollateralTokenParams"
# Then get the full content of the contract to analyze events and function implementations
fd "TermStrategy.sol" -t f -x cat {}
Length of output: 345
Script:
#!/bin/bash
# Let's check the Strategy.sol file since it appears to be the main contract
cat src/Strategy.sol
Length of output: 52813
| @@ -1111,6 +1116,8 @@ contract Strategy is BaseStrategy, Pausable, ReentrancyGuard { | |||
| requiredReserveRatio = 0.2e18; | |||
| discountRateMarkup = 0.005e18; | |||
| repoTokenConcentrationLimit = 0.1e18; | |||
|
|
|||
| _grantRole(GOVERNOR_ROLE, _governorAddress); | |||
There was a problem hiding this comment.
Missing assignment of DEFAULT_ADMIN_ROLE
Consider assigning the DEFAULT_ADMIN_ROLE to the governor address to enable future role management. Without an account holding DEFAULT_ADMIN_ROLE, it won't be possible to grant or revoke roles after deployment.
Apply this diff to the constructor to set the DEFAULT_ADMIN_ROLE:
constructor(
address _asset,
string memory _name,
address _yearnVault,
address _discountRateAdapter,
address _eventEmitter,
address _governorAddress
) BaseStrategy(_asset, _name) {
YEARN_VAULT = IERC4626(_yearnVault);
TERM_VAULT_EVENT_EMITTER = ITermVaultEvents(_eventEmitter);
PURCHASE_TOKEN_PRECISION = 10 ** ERC20(asset).decimals();
discountRateAdapter = ITermDiscountRateAdapter(_discountRateAdapter);
IERC20(_asset).safeApprove(_yearnVault, type(uint256).max);
timeToMaturityThreshold = 45 days;
requiredReserveRatio = 0.2e18;
discountRateMarkup = 0.005e18;
repoTokenConcentrationLimit = 0.1e18;
+ _grantRole(DEFAULT_ADMIN_ROLE, _governorAddress);
_grantRole(GOVERNOR_ROLE, _governorAddress);
}📝 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.
| _grantRole(GOVERNOR_ROLE, _governorAddress); | |
| constructor( | |
| address _asset, | |
| string memory _name, | |
| address _yearnVault, | |
| address _discountRateAdapter, | |
| address _eventEmitter, | |
| address _governorAddress | |
| ) BaseStrategy(_asset, _name) { | |
| YEARN_VAULT = IERC4626(_yearnVault); | |
| TERM_VAULT_EVENT_EMITTER = ITermVaultEvents(_eventEmitter); | |
| PURCHASE_TOKEN_PRECISION = 10 ** ERC20(asset).decimals(); | |
| discountRateAdapter = ITermDiscountRateAdapter(_discountRateAdapter); | |
| IERC20(_asset).safeApprove(_yearnVault, type(uint256).max); | |
| timeToMaturityThreshold = 45 days; | |
| requiredReserveRatio = 0.2e18; | |
| discountRateMarkup = 0.005e18; | |
| repoTokenConcentrationLimit = 0.1e18; | |
| _grantRole(DEFAULT_ADMIN_ROLE, _governorAddress); | |
| _grantRole(GOVERNOR_ROLE, _governorAddress); | |
| } |
Summary by CodeRabbit
New Features
GOVERNOR_ROLE_ADDRESSfor enhanced deployment capabilities on the Sepolia network.Strategycontract, improving security and management flexibility.Bug Fixes
Documentation
Chores
managementwithgovernor.