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).
| (bytes32 messageValidationID, uint64 nonce) = | ||
| completeValidatorWeightUpdate(messageIndex); | ||
| if (validationID != messageValidationID) { | ||
| revert InvalidValidationID(delegator.validationID); |
There was a problem hiding this comment.
I think we should probably output both the validation ID of the message, as well as the validation ID of the delegator here. (I realize this is the old behaviour, so feel free to punt this)
There was a problem hiding this comment.
I added a new error type UnexpectedValidationID for cases when the provided ID does not match the expected one. The unary InvalidaValidationID is still used in the resend methods for cases where the provided validation ID doesn't correspond to a registered validation.
| _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; | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
| @@ -171,25 +181,27 @@ | |||
| if ($._registeredValidators[initialValidator.nodeID] != bytes32(0)) { | |||
| revert NodeAlreadyRegistered(initialValidator.nodeID); | |||
| } | |||
| if (initialValidator.nodeID.length != NODE_ID_LENGTH) { | |||
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: solidity.performance.array-length-outside-loop.array-length-outside-loop Note
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.