feat: OwnerProxy

[maximebrugel]

In order to complete the ownership architecture, we need the OwnerProxy contract in charge of executing scripts for the Timelock (run transactions atomically).

To show how the contract works, I added two scripts :

• AddOperator.sol, to add an operator (import, add and rebuild cache)
• UpdateFees, to update the entry and exit fees.

We can also use a single OwnerProxy, but it must use upgradeToAndCall to call the NestedFactory owner functions :

[Yashiru]

Suggested change

	it("Cant update fees if wrong exit fees is zero", async () => {
	it("Cant update fees if exit fees are zero", async () => {

[Yashiru]

Suggested change

	it("Cant update fees if wrong entry fees is zero", async () => {
	it("Cant update fees if entry fees are zero", async () => {

[Yashiru]

Add a test that should revert if the calldata NestedFactory address is zero

[Yashiru]

In case the address of the NestedFactory is not correct, wouldn't it be cheaper in gas to make a require to check it first?

[maximebrugel]

Not really, but we must check the operator and factory address before

[adrien-supizet]

(bytes4 selector must be included) should be a /// @dev imo

[adrien-supizet]

Could be great to also have the RemoveOperator ready to prevent making mistakes if we ever need to do it urgently.

[obatirou]

Suggested change

	it("Cant remove operator if nested factory address is zero", async () => {
	it("Cannot remove operator if nested factory address is zero", async () => {

or

Suggested change

	it("Cant remove operator if nested factory address is zero", async () => {
	it("Can't remove operator if nested factory address is zero", async () => {

[Yashiru]

Suggested change

	uint256 operatorLength = operators.length;
	uint256 operatorLength = operators.length;
	require(operatorLength != 0, "DAO-SCRIPT: INVALID_OPERATOR_LEN");

[Yashiru]

Maybe store the new IOperatorResolver.Operator directly in the operatorsToImport array instead of storing it in a memory cached variable and then store it in the array, can save some gas.

Suggested change

	operatorsToImport[i] = operator;
	operatorsToImport[i] = IOperatorResolver.Operator(
	deployedAddress,
	operators[i].selector
	);;

[Yashiru]

I am not very familiar with the MixinOperatorResolver, I wonder if this line is absolutely mandatory?

[maximebrugel]

Not mandatory, but good to have

[adrien-supizet]

Change code errors to sting < 32 bytes. I know it's not important here, but it will be raised in audits later.

[adrien-supizet]

It's a bit confusing:

• method name is singular
• one bytecode as param

but:

• operators param as an array - plural

Consider renaming operators

[maximebrugel]

In the OperatorResolver, an Operator is an address (implementation) and a bytes4 (selector), linked to a bytes32 (name).

The bytecode param will be deployed and as a result, the implementation (address).
So, the operators is the array with name and selector, it is really an "operator" list to add.

I don't know how we could rename operators. Any idea ?

[obatirou]

I would remove tuple from the tupleOperator name, it is explicit by looking at the type, no ?
Maybe name the struct operatorFunction and the array operatorFunctions ?

[adrien-supizet]

I understand how it works, then the method name is inaccurate, and should be plural.

Either the operator is the contract, or it's the address + selector + bytes. It just needs to be consistent across the protocol

[maximebrugel]

@adrien-supizet I misunderstood, I'm updating to DeployAddOperators

[adrien-supizet]

importOperators It does not look like we're removing an operator here. Am I missing something?

[adrien-supizet]

I'm missing something here, I'll take a look at NestedFactory's removeOperator method before finishing the review

[adrien-supizet]

Ok, I think the OperatorResolver should have had a removeOperators method, but this works too.