ACP 99 reference implementation #700
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This reverts commit 2804658.
| _initiateValidatorRemoval(validationID); | ||
| Validator memory validator = getValidator(validationID); |
There was a problem hiding this comment.
The order of these calls matters, since _initiateValidatorRemoval modifies the Validator. I removed the Validator return value from the ACP99Manager interface since it was not strictly necessary, and can otherwise be accessed like it is here. If the internal ACP99Manager functions do return a Validator, it increases the ERC20TokenStakingManager contract size by ~900 bytes, surpassing the size limit.
I settled on adding a @dev comment to the ValidatorManager implementation of these methods to warn developers that this function mutates accessible state.
There was a problem hiding this comment.
You should probably add a little blurb inline here.
// order matters for the next two lines, see...Then reference an explanation somewhere? Ideally in a github permalink or something else that is guaranteed not to change. That is unless you can make the comment succinct, haha (I can't).
There was a problem hiding this comment.
I agree that a blurb should be added. I think it would be difficult for someone to discover the ordering constraint here without one.
There was a problem hiding this comment.
I added a comment above the call to getValidator here. I also added a @dev natspec comment to all of the methods in ValidatorManager that touch the internal validator state.
cam-schultz
requested review from
iansuvak,
richardpringle,
geoff-vball,
bernard-avalabs and
michaelkaplan13
| _initiateValidatorRemoval(validationID); | ||
| Validator memory validator = getValidator(validationID); |
There was a problem hiding this comment.
You should probably add a little blurb inline here.
// order matters for the next two lines, see...Then reference an explanation somewhere? Ideally in a github permalink or something else that is guaranteed not to change. That is unless you can make the comment succinct, haha (I can't).
| _initiateValidatorRemoval(validationID); | ||
| Validator memory validator = getValidator(validationID); |
There was a problem hiding this comment.
I agree that a blurb should be added. I think it would be difficult for someone to discover the ordering constraint here without one.
| _initiateValidatorRemoval(validationID); | ||
|
|
||
| // The validator must be fetched after the removal has been initiated, since the above call modifies | ||
| // the validator's state. |
| function _fixedNodeID(bytes memory nodeID) private pure returns (bytes20) { | ||
| bytes20 fixedID; | ||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| fixedID := mload(add(nodeID, 32)) | ||
| } | ||
| return fixedID; | ||
| } |
tests/utils/validator_manager.go
Outdated
Why this should be merged
Implements ACP-99 as described in avalanche-foundation/ACPs#170
How this works
Adds
ACP99Managerto the class hierarchy,.ValidatorManagerimplementsACP99Manager.How this was tested
CI, fixed breaking tests
How is this documented
Updates README to point to ACP-99
Future work
The following items are candidates to be included in this PR. They may also be deferred to later changes.
IValidatorManageraltogether, asACP99Managerdoes the same job of defining the core validator manager functionalityinitialize*methods toinitiate*to match ACP-99.initializeis a misnomer, since nothing is initialized as part of these function calls. Rather, theyinitiatethe process of registering/modifying a validator.