Return the address when deploying a mastercopy

[asgeir-s]

Also, add a computeTargetAddress function that can be used to get the address of an already deployed mastercopy

[cristovaoth]

Before I submit my review, a quick clarification @manboy-eth

File mastercopyDeployer, line 109, there's the assumption that a targetAddress AddressZero would correspond to an already deployed contract. However, the call in line 94 (singletonFactory.deploy) internally uses create2, and this is specified to throw on address already occupied or address non zero nonce. See ethereum/EIPs#684

IT feels like the zero address assumption for already deployed is wrong. Thoughts?

[asgeir-s]

Before I submit my review, a quick clarification @manboy-eth

File mastercopyDeployer, line 109, there's the assumption that a targetAddress AddressZero would correspond to an already deployed contract. However, the call in line 94 (singletonFactory.deploy) internally uses create2, and this is specified to throw on address already occupied or address non zero nonce. See ethereum/EIPs#684

IT feels like the zero address assumption for already deployed is wrong. Thoughts?

It seems to be working as expected. Also, reading about it here says, that it fails without reverting:

Deployment can fail due to:

A contract already exists at the destination address.
...
Note that these failures only affect the return value and do not cause the calling context to revert (unlike the error cases below).
From https://www.evm.codes/#f5?fork=merge

Perhaps [ethereum/EIPs#684] was closed without being implemented?

[cristovaoth]

Before I submit my review, a quick clarification @manboy-eth
File mastercopyDeployer, line 109, there's the assumption that a targetAddress AddressZero would correspond to an already deployed contract. However, the call in line 94 (singletonFactory.deploy) internally uses create2, and this is specified to throw on address already occupied or address non zero nonce. See ethereum/EIPs#684
IT feels like the zero address assumption for already deployed is wrong. Thoughts?

It seems to be working as expected. Also, reading about it here says, that it fails without reverting:

Deployment can fail due to:
A contract already exists at the destination address.
...
Note that these failures only affect the return value and do not cause the calling context to revert (unlike the error cases below).
From https://www.evm.codes/#f5?fork=merge

Perhaps [ethereum/EIPs#684] was closed without being implemented?

Very interesting. Ok makes sense. Lets proceed assuming that the function does not revert and returns Zero if already deployed

[cristovaoth]

Looks good, thank you for taking the time to improve the functions!

Left a bunch of low impact (mostly stylistic) comments

[cristovaoth]

I did not know this assert variation.

Usually I import from "assert"

And then simply do assert(a === b)

[cristovaoth]

Looks good, let's merge