Refactors Consensus::enterCommittee #161
Conversation
|
@pgev Is it available for review? |
|
@abhayks1 I've rebased and fixed failing tests. It's again ready for review. |
| * - and, less or equal than validator's end height | ||
| * - was not slashed | ||
| */ | ||
| function isValidator(address _account, uint256 _metablockHeight) |
There was a problem hiding this comment.
A lot of code between isActiveValidator and isValidator is common. We can refactor the common code to a private method with parameters account, metablockHeight. isActiveValidator can pass openKernelHeight as metablockHeight
What do you think?
| * - greater or equal than validator's begin height | ||
| * - and, less or equal than validator's end height | ||
| * - was not slashed | ||
| */ |
There was a problem hiding this comment.
We should add documentation for function arguments and and return values as per dev guidelines.
Similar comment for isActiveValidator method.
| CoreI _core, | ||
| bytes32 _committeeMetachainId, | ||
| bytes32 _validatorMetachainId, | ||
| uint256 _validatorMetablockHeight, | ||
| address _validator, |
There was a problem hiding this comment.
I feel _validator argument should come before _validatorMetachainId, _validatorMetablockHeight.
A validator is entering into committee. _validatorMetachainId and _validatorMetablockHeight seems like they are the properties of a validator. The _validator argument significance is strong enough to be prefixed before _validatorMetachainId and _validatorMetablockHeight
@pgev @deepesh-kn what do you think? It's an interface change.
| Metablock storage currentMetablock = metablockchains[_metachainId][currentHeight]; | ||
| uint256 formationHeight = committeeMetablock.roundBlockNumber; | ||
| uint256 checkValidatorForHeight = 0; | ||
| if (validatorMetablock.roundBlockNumber >= formationHeight) { |
There was a problem hiding this comment.
I think the logic of evaluating checkValidatorForHeight can be refactored to private/internal method 🤔.
enterCommittee method has grown significantly.
|
Closing the PR for now. Will add into #147 as a note for later reference if needed. |
No description provided.