Pitch: Make InitAddresses the voting and payout strategy addresses, not a factory

[0xZakk]

Background:

We (somewhat recently?) updated the InitAddress struct to be addresses for factory contracts for voting and payout strategies:

struct InitAddress {
    IVotingStrategyFactory votingStrategyFactory; // Deployed voting strategy factory contract
    IPayoutStrategyFactory payoutStrategyFactory; // Deployed payout strategy factory contract
}

Source

This means, that when someone creates a round, they pass in factory contracts for each strategy (as opposed to the contracts themselves) and initialize() (1) calls create() on the factory contracts and then (2) calls init on the contract that comes back.

    votingStrategyFactory = _initAddress.votingStrategyFactory;
    payoutStrategyFactory = _initAddress.payoutStrategyFactory;

    // ...

    // deploy voting contract
    votingStrategy = votingStrategyFactory.create();

    // Invoke init on voting contract
    IVotingStrategy(votingStrategy).init();

    // deploy payout contract
    payoutStrategy = payable(payoutStrategyFactory.create());

    // Invoke init on payout contract
    IPayoutStrategy(payoutStrategy).init();

Source

Context:

I think I missed what motivated this change and just want to flag, I think we'd prefer to have any factory logic be outside of the protocol and accept addresses for the strategies directly. This forces everyone to use a factory, which is extra code to write and extra gas to spend (both in deploying the factory and in calling create() here rather than just being able to call init().

Most strategies probably wont need a factory anyway. And our current implementation does create a way to pass arguments to create() so the utility of having a factory is somewhat negated.

Suggested Implementation:

I think we should change the struct back to holding the strategy addresses and not assume that a factory is being passed in.

struct InitAddress {
    IVotingStrategy votingStrategy; // Deployed voting strategy contract
    IPayoutStrategy payoutStrategy; // Deployed payout strategy contract
}

Then update initialize() to

    // store voting strategy contract
    IVotingStrategy(votingStrategy) = _initAddress.votingStrategy;

    // Invoke init on voting strategy
   votingStrategy.init();

    // store payout strategy contract
    IPayoutStrategy(payoutStrategy) = payable(_initAddress.payoutStrategy);

    // Invoke init on payout strategy
   payoutStrategy.init();

[0xKurt]

I think the motivation was simply being able to deploy a whole round with all contracts in one call.
I see your point, but i also think about a way to have the best of both.

The factories basically just create proxies. What do you think about adding the clone logic into the initialize function.
so every deployed strategy contract would become re-usable, no factory needed and you would be able to deploy all contracts in one call, and no need of deploying a factory contract.

bytes32 salt = keccak256(abi.encodePacked(msg.sender, nonce));
votingStrategy = IVoteStrategy(ClonesUpgradeable.cloneDeterministic(votingContract, salt));

votingStrategy.init();