Consider converting ValidatorManager to an external library

[cam-schultz]

Context and scope
The PoSValidatorManager implementation contracts (ERC20TokenStakingManager and NativeTokenStakingManager) are currently flirting with the 24kB contract size limit. This severly limits our ability to continue to develop features. The first strategy the EF lists to work around this is to separate contracts. In the case of ValidatorManager and PoSValidatorManager, this is difficult because each carries its own state.

Discussion and alternatives
The contracts Foo and Bar on this branch demonstrate how to convert a stateful contract to an external library. The general steps to do so are:

• Define a struct in the library contract defining the state that it will modify
• Add a instance of this struct to the calling contract's state
• Extend this struct with the library contract's methods

Library methods that modify the caller's state can then be invoked from the calling contract.

Another note: currently, there are entry point functions throughout the inheritance hierarchy, including in ValidatorManager. We'll need to move these entry point functions to PoSValidatorManager/PoAValidatorManager.

Open questions

[richardpringle]

This looks good and I think the abstraction lines are super clear (and clean). I think the open question would be upgrades, but it should be relatively straight forward if you're adding fields to the end, right?

I think you could fairly easily upgrade the library before upgrading the calling contract without breaking anything (as long as you aren't removing functionality).

[cam-schultz]

it should be relatively straight forward if you're adding fields to the end

That's my understanding. It becomes tricky if the caller's state and the library's "state" both need to be upgraded, but in theory it should be possible to do something like this:

// Defined in the library
struct Storage {
    uint256 varBar;
+   uint256 varBar2;
}

// Defined and accessed in the caller
struct FooStorage {
    uint256 varFoo;
    Bar.Storage barStorage;
+   uint256 varFoo2;
}

I think you could fairly easily upgrade the library before upgrading the calling contract without breaking anything (as long as you aren't removing functionality).

Probably simpler to not upgrade the library at all and instead point to a new instance with the updated logic/storage spec from the caller

[cam-schultz]

Another interesting consideration is how to upgrade to this external library pattern from a set of stateful contracts. For example, if we move forward with this library approach, and somebody has already deployed the ValidatorManager contracts as they currently are, how can we migrate ValidatorManager's state to the caller?

One approach is to define an initializer in the new implementation contract that migrates the library's former state to its own storage. This is possible because all the storage involved is in the proxy's address space. It's a bit cumbersome since you have to add getters for the migrated-from contract's storage to the new implementation that are only used on initialization. Once the state is migrated, these can be removed in a future upgrade.

I POC'd this here. The v1 contracts consist of Foo and Bar, with Foo extending Bar. The v2 contracts consist of FooV2 and BarLibrary, which implements the pattern in the issue description. State migration is done in FooV2.initialize. This e2e test demonstrates the upgrade + state migration in action.

[michaelkaplan13]

The contracts Foo and Bar on this branch demonstrate how to convert a stateful contract to an external library. The general steps to do so are:
Define a struct in the library contract defining the state that it will modify
Add a instance of this struct to the calling contract's state
Extend this struct with the library contract's methods

I was thinking it would look something more like:

struct FooStorage {
    uint256 _val;
}

library FooModifier {
    function setVal(FooStorage storage $, uint256 newValue) external {
        $._val = newValue;
    }
}

contract Foo {
    bytes32 public contract FOO_STORAGE_SLOT = 0x...;

    function _getFooStorage() private pure returns (FooStorage storage $)
    {
        ....
    }

    function doSomething() public {
        FooStorage storage $ = _getFooStorage();
        ...
        FooModifier.setVal($, 42);
    }
}

[cam-schultz]

I like that approach. It maintains state isolation to each individual contract, and offloads the bulk of the contract logic to stateless libraries. The only difference I'd make would be move all the contract logic to the library, and have each method in the class simply fetch the state and pass it to the library.

We could apply this pattern up and down the inheritance hierarchy without changing any parent-child relationships.

So we'd have something like

classDiagram

class ValidatorManager
class PoSValidatorManager
class ValidatorManagerLibrary
class PoSValidatorManagerLibrary

ValidatorManager <|-- PoSValidatorManager
ValidatorManagerLibrary <.. ValidatorManager
PoSValidatorManagerLibrary <.. PoSValidatorManager

Loading

Where the dotted lines represent calls into the respective libraries.

This would also elide the need for the state migration I described above, since stateful entities persist on upgrades.

[cam-schultz]

Closing in favor of #662, which is more general. I've linked the comment describing the external library forwarding pattern in that issue.