[Proposed] ACP-99: Implement using composition - ValidatorManager as single entry point

[cam-schultz]

Why this should be merged

Experimental implementation of ACP-99 using composition. Concrete contracts implement either IACP99SecurityModule or IACP99ValidatorManager.

Notably, the contract sizes decrease significantly. ERC20TokenStakingManager goes from right at the 24kB size limit down to 14.6kB.

This is not functional as-is, and is meant to demonstrate the necessary refactoring to implement ACP-99 using composition

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


ACP99ValidatorManager *-- IACP99SecurityModule


ValidatorManager <|-- ACP99ValidatorManager
IACP99ValidatorManager <|-- ACP99ValidatorManager

IACP99SecurityModule <|-- PoSValidatorManager
PoSValidatorManager <|-- ERC20TokenStakingManager
PoSValidatorManager <|-- NativeTokenStakingManager
IACP99SecurityModule <|-- PoAValidatorManager

Loading

This implementation deviates slightly from the ACP-99 spec as written in that the ValidatorManager is the only contract that the end user interacts with. To achieve this, the handle* method signatures must be generic enough to cover all possible SecurityModule use cases, notably PoA vs PoS. This results in a greater-than-minimal IACP99SecurityModule interface, where some implementations won't use certain arguments (for example PoA applications won't need payable methods, but native token staking applications will).

How this was tested

How is this documented

[richardpringle]

Notably, the contract sizes decrease significantly. ERC20TokenStakingManager goes from right at the 24kB size limit down to 14.6kB.

I'm curious to know how much this affects gas costs, that would be one of the major advantages of keeping everything in a single contract, right?

[richardpringle]

😜

Suggested change

	// TODO: calling this should be restricted to...who?
	// TODO: calling this should be restricted to...whom?

[cam-schultz]

I'm curious to know how much this affects gas costs, that would be one of the major advantages of keeping everything in a single contract, right?

We won't know until we get unit tests working, but in theory the only added cost should be the external call to the security module. If we're not transferring ETH, and assuming the address is "warm" (it should be - every interaction will call the same security module), then gas is on the order of hundreds https://github.com/wolflo/evm-opcodes/blob/main/gas.md#aa-1-call

[richardpringle]

What's the key difference here, that the security module is a property of the validator-manager instead of the validator-manager implementing the security module interface?

[richardpringle]

Should the security module itself control its own upgrades? If not, should there be a SecurityUpdateModule?

[richardpringle]

I definitely need to go through the ACP again, but I would expect the security module to do some form of validation on the inputs before calling the _initializeValidatorRegistration. Or does it not matter since the security module will just revert anyway?

[cam-schultz]

It's a little tricky to reverse the order in our current implementation since the security module needs the validationID, which is provided by the manager contract. It will revert if the security module's checks fail, regardless of the ordering. We should definitely keep in mind patterns such as checks-effects-interactions if/when we decide to move forward on this implementation.