Move Self-Destruct Check to Deploy Cache

[fdarian]

The current implementation requires calling upgradeTo after deploying the beacon to set the implementation and make the beacon usable. However, it adds more complexity when the beacon is owned by multisig/governance which needs to be set on every deployment.

One way to ease the flow is to set the implementation in constructor like OZ's UpgradeableBeacon, but it will revert since there is selfdestruct check. Therefore, I suggest to move the check into deployCache() since the impact of having no self-destruct is only applied when the implementation has already been cloned to the cache.

I think this could be a better solution and probably could skip the check for empty implementation check since calling the function to an unknown contract will revert. Need everyone's opinion on this, feel free to assess the impact.

ps: there is a trade-off and more work for this. First, using an implementation that isn't exposing self-destruct is still allowed, but I think it has no great impact before deployCache is called. Second, we need to pre-compute the beacon address for implementation deployment.

[frangio]

Thanks for the suggestion!

I consider this repo to be a proof of concept and not necessarily production ready, so I'd rather keep the code as a clean demonstration than increase the complexity to cater for a use case that isn't well supported. My initial impression is that these changes aren't worth the trade off.

but it will revert since there is selfdestruct check

Are you sure about this? The only issue I see with adding upgradeTo in the constructor is that it requires the implementation to be deployed beforehand with knowledge of the beacon address, and this usually makes deployments more complicated as there is a kind of circular dependency.

I'm not sure I understand the problem to begin with. Can you describe the flow with a multisig and the current code?

[fdarian]

@frangio

but it will revert since there is selfdestruct check

I have tried this and fails at .selfDestructIfCache() error signature check, which is caused by calling beacon.cache() that hasn't exist yet since the call is in the constructor (ref).

The problem with the current implementation is the owner, which normally would be a multisig or even a governance contract, need to call upgradeTo() after deployment. That would be time-consuming for all singers to approve. Although there is a workaround by setting the owner to the deployer at first and transferring the ownership back after upgradeTo() is called, that requires 3 transactions.

using an implementation that isn't exposing self-destruct is still allowed, but I think it has no great impact before deployCache is called.

Another point that I'd like to point out is when to check the self-destruct compatibility, I see this as a limitation to set the implementation in the constructor. The anti-pattern can be solved by instantiating the implementation in the constructor.