Centralize reward management in PoSValidatorManager #643
Conversation
richardpringle
force-pushed
the
consolidate-reward-updates
branch
from
005ec21 to
7cb0f4a
Compare
richardpringle
requested review from
iansuvak,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
richardpringle
force-pushed
the
consolidate-reward-updates
branch
from
7cb0f4a to
7e3c8ff
Compare
| @@ -760,4 +744,41 @@ abstract contract PoSValidatorManager is | |||
| PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | |||
| return $._posValidatorInfo[validationID].owner != address(0); | |||
| } | |||
|
|
|||
| function _withdrawValidationRewards(address rewardRecipient, bytes32 validationID) internal { | |||
| PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | |||
There was a problem hiding this comment.
Does it make more sense to pass this in?
There was a problem hiding this comment.
We currently retrieve the contract storage in every function that needs it, mainly under the assumption that the Solidity compiler is smart enough to cache subsequent reads of the same value. However, I'm not seeing anything in the optimizer steps that would directly translate. We should do a gas cost analysis of this pattern versus passing the storage object around (not necessary for this PR - let's stick to the existing pattern here)
| @@ -760,4 +744,41 @@ abstract contract PoSValidatorManager is | |||
| PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | |||
| return $._posValidatorInfo[validationID].owner != address(0); | |||
| } | |||
|
|
|||
| function _withdrawValidationRewards(address rewardRecipient, bytes32 validationID) internal { | |||
| PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | |||
There was a problem hiding this comment.
We currently retrieve the contract storage in every function that needs it, mainly under the assumption that the Solidity compiler is smart enough to cache subsequent reads of the same value. However, I'm not seeing anything in the optimizer steps that would directly translate. We should do a gas cost analysis of this pattern versus passing the storage object around (not necessary for this PR - let's stick to the existing pattern here)
| uint256 delegationRewards = 0; | ||
| uint256 validatorFees = 0; |
There was a problem hiding this comment.
OpenZeppelin called out in their recent audit that not initializing variables with their default value is more gas efficient.
| uint256 delegationRewards = 0; | |
| uint256 validatorFees = 0; | |
| uint256 delegationRewards; | |
| uint256 validatorFees; |
There was a problem hiding this comment.
That's insane that this doesn't compile down to the same bytecode... will fix!
richardpringle
force-pushed
the
consolidate-reward-updates
branch
2 times, most recently
from
45a5f32 to
498650f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
richardpringle
force-pushed
the
consolidate-reward-updates
branch
from
498650f to
dee4e3d
Compare
Why this should be merged
If any of the reward logic changes, the change happens in one place. For the delegator, the logic only existed in one place before, but this keeps the pattern consistent and improves the readability of
_completeEndDelegationfunction.How this works
Same as before, I just moved code
How this was tested
Existing tests
How is this documented
inline docs on new functions