[Proposed] ACP-99: Implement using inheritance #667
Conversation
| function initializeValidatorSet( | ||
| ConversionData calldata conversionData, | ||
| uint32 messageIndex | ||
| ) external; |
There was a problem hiding this comment.
Should this necessarily be part of the interface? In theory, one should be able to deploy a contract with the initial validator set hard-coded, right?
The fact that the security interface doesn't have a matching method to this one, but it does for all others, leads me to believe that this isn't necessarily part of the interface (despite the likelihood of its existence in most instances).
| @@ -25,6 +21,8 @@ import {ContextUpgradeable} from | |||
| "@openzeppelin/[email protected]/utils/ContextUpgradeable.sol"; | |||
| import {Initializable} from | |||
| "@openzeppelin/[email protected]/proxy/utils/Initializable.sol"; | |||
| import {IACP99SecurityModule} from "./interfaces/IACP99SecurityModule.sol"; | |||
There was a problem hiding this comment.
Is this used anywhere? And I also notice that this isn't IACP99ValidatorManager. Might make sense to just add a comment saying why (I'm guessing because it's not worth the adding all the events that aren't specified in the interface).
| PChainOwner disableOwner; | ||
| } | ||
|
|
||
| interface IACP99ValidatorManager { |
There was a problem hiding this comment.
Might be worth adding a comment saying that we should also include the events from IValidatorManager?
| } | ||
|
|
||
| function handleCompleteValidatorRegistration(bytes32 validationID) external { | ||
| // No-op |
There was a problem hiding this comment.
Can you add an explanation as to why this is a no-op?
| if (force) { | ||
| return; | ||
| } | ||
|
|
||
| if (!success) { | ||
| revert ValidatorIneligibleForRewards(validationID); | ||
| } |
There was a problem hiding this comment.
... I know, I know, this isn't a functional change, but I can't help myself, 😉.
| if (force) { | |
| return; | |
| } | |
| if (!success) { | |
| revert ValidatorIneligibleForRewards(validationID); | |
| } | |
| if (!force && !success) { | |
| revert ValidatorIneligibleForRewards(validationID); | |
| } |
There was a problem hiding this comment.
Am I correct in saying that there aren't really any changes here, just the calling convention?
| function handleInitializeValidatorRegistration(bytes32 validationID, uint64 weight, bytes calldata args) external { | ||
| } | ||
|
|
||
| function handleCompleteValidatorRegistration(bytes32 validationID) external { | ||
| // No-op | ||
| } | ||
|
|
||
| function handleInitializeEndValidation(bytes32 validationID, bytes calldata args) external { | ||
| // No-op | ||
| } | ||
|
|
||
| function handleCompleteEndValidation(bytes32 validationID) external { | ||
| // No-op | ||
| } | ||
|
|
||
| function handleInitializeValidatorWeightChange(bytes32 validationID, uint64 weight, bytes calldata args) external { | ||
| // No-op | ||
| } | ||
|
|
||
| function handleCompleteValidatorWeightChange(bytes32 validationID, bytes calldata args) external { | ||
| // No-op |
There was a problem hiding this comment.
Wait... shouldn't POA still be able to cycle validators? Maybe I just don't have the full picture
There was a problem hiding this comment.
Yeah, these handlers should likely implement checks against the owner address/key. Keep in mind this PR is meant to demonstrate the architectural changes needed to implement ACP-99, so missing features are expected
cam-schultz
changed the title
Why this should be merged
Experimental implementation of ACP-99 using inheritance. The concrete contracts are extended to implement the new interfaces
IACP99SecurityModuleandIACP99ValidatorManager. Note that this does not implement ACP-99 exactly - this is meant to demonstrate the required refactoring needed to comply with the ACP.This is not functional as-is, and is meant to demonstrate the necessary refactoring to implement ACP-99 using inheritance
How this works
classDiagram class IACP99SecurityModule { handleInitializeValidatorRegistration() handleCompleteValidatorRegistration() handleInitializeEndValidation() handleCompleteEndValidation() handleInitializeValidatorWeightChange() handleCompleteValidatorWeightChange() } <<interface>> IACP99SecurityModule class IACP99ValidatorManager { initializeValidatorSet() initializeValidatorRegistration() completeValidatorRegistration() initializeEndValidation() completeEndValidation() initializeValidatorWeightChange() completeValidatorWeightChange() } class PoSValidatorManager <<abstract>> PoSValidatorManager class ERC20TokenStakingManager class NativeTokenStakingManager class PoAValidatorManager class ACP99ValidatorManager class ValidatorManager <<abstract>> ValidatorManager ValidatorManager <|-- ACP99ValidatorManager IACP99ValidatorManager <|-- ACP99ValidatorManager IACP99SecurityModule <|-- PoSValidatorManager ACP99ValidatorManager <|-- PoSValidatorManager PoSValidatorManager <|-- ERC20TokenStakingManager PoSValidatorManager <|-- NativeTokenStakingManager IACP99SecurityModule <|-- PoAValidatorManager ACP99ValidatorManager <|-- PoAValidatorManagerHow this was tested
How is this documented
Open Question
How to handle
payablemethods for native token staking?