Feat: add sGhoSteward #6
Conversation
| address public executor = vm.addr(0x0001); | ||
| address public ghoCommittee = vm.addr(0x0002); | ||
|
|
||
| bytes32 public constant YIELD_MANAGER_ROLE = 'YIELD_MANAGER'; |
There was a problem hiding this comment.
Nbd, but why is yield manager role defined differently from the ones below? EDIT: Oh perhaps because it's to distinguish it's a role on sGho rather than Steward contract
src/contracts/misc/sGhoSteward.md
Outdated
| - Supports updating with each rate parameter *individually* or *multiple parameters simultaneously*. | ||
| - Integrates **Role-Based Access Control** from OpenZeppelin, enabling secure assignment, modification, and revocation of roles. | ||
| - Provides functions to view the current configuration and pre-calculate the `targetRate` for any given configuration. | ||
| - Enforces a hard cap of `50%` for the computed `targetRate`, regardless of configuration input. |
There was a problem hiding this comment.
To me, this sentence is a bit misleading, because it kind of sounds like the contract does not revert (and perhaps instead just uses 50% target rate even if it would calculate higher based on input config), when indeed it does revert based on the configuration input. It's mentioned below and indeed the contract does revert when this exceeds 50%. Perhaps can clarify here too that the contract reverts if configuration input would put target rate over 50%.
tests/misc/sGhoSteward.t.sol
Outdated
| fixedRate: 300 // 3% | ||
| }); | ||
|
|
||
| vm.expectRevert(); |
There was a problem hiding this comment.
Perhaps explicitly verify that the revert reason is because of the role?
tests/misc/sGhoSteward.t.sol
Outdated
| assertEq(sGho.targetRate(), 400); | ||
| } | ||
|
|
||
| function test_setRateConfigNotAllRoles() public { |
There was a problem hiding this comment.
Might be cleaner and more straightforward imo to break this up into multiple tests, showing that each role can set each different config param, and reverts for exactly the reason of not having the particular role after revoking it.
There was a problem hiding this comment.
Test splitted into 3 different.
|
|
||
| uint16 public constant AMPLIFICATION_DENOMINATOR = 100_00; | ||
|
|
||
| bytes32 public constant AMPLIFICATION_MANAGER_ROLE = keccak256('AMPLIFICATION_MANAGER_ROLE'); |
There was a problem hiding this comment.
Should we add the @inheritdoc for all of these?
src/contracts/misc/sGhoSteward.sol
Outdated
| /// @notice Current rate parameters | ||
| RateConfig public rateConfig; | ||
|
|
||
| IsGHO public immutable sGHO; |
There was a problem hiding this comment.
In the other contracts in the repo these are prefixed and then params on functions don't have prefix/suffix from what i've seen
There was a problem hiding this comment.
Fixed accoarding to other contracts in this repo.
src/contracts/misc/sGhoSteward.sol
Outdated
| rateConfig_.fixedRate; | ||
|
|
||
| if (targetRate > MAX_RATE) { | ||
| revert TooBigRate(); |
There was a problem hiding this comment.
I'd change to RateTooBig()
4 participants
Adding code for
sGhoStewardthat can updatesupplyCapandtargetRateinsGho.For this to work, this steward should be granted with
YIELD_MANAGER_ROLE.For convenience, the
targetRateis divided into three components and a special role is allocated for updating each variable.