fix: better DeployMethod

[Thunkar]

Refactor to avoid multiple footguns with DeployMethod. The idea is that all address-affecting parameters have to be "locked in" in the constructor, so any subsequent invocation yields a deterministic address. Any other path throws and exception.

The "convenient" flow of just saying await Contract.deploy(wallet, ...args).send({ from: acc }) is still supported, but generating an incompatible DeployMethod instance (for example, using a different deployer) is not allowed anymore. Furthermore, it is not possible to set the salt on send to avoid the same issue, and the object has to be regenerated. Read migration notes for more details

[nchamo]

It's a really good change, but I feel what it would be a good opportunity to use obj params for constructor args. Feels off to have different versions do different things

Would also like @mverzilli 's opinion on it

[nchamo]

It feels weird that in other cases we are doing params at the end:

const deploy = MyContract.deploy(wallet, ...args, { publicKeys });

But here, we are doing params at the start

await cd.deploy({ salt }, ...ctorArgs).send({ from: alice });

I know they are different things, but it felt weird when I saw this:

const { receipt: txReceipt } = await deployer
        .deploy({ salt: new Fr(BigInt(1)) }, ownerAddress, 1)
        .send({ from: ownerAddress });

Maybe could add the constructor args as part of the object?

const { receipt: txReceipt } = await deployer
        .deploy({ ctorArgs: [ownerAddress, 1], salt: new Fr(BigInt(1)) })
        .send({ from: ownerAddress });

Update:
Now, that I think about it, I think this could also look clearer if we used object args:

const deployMethod = StatefulTestContract.deploy(wallet, defaultAccountAddress, 0, {
        salt: Fr.random(),
        deployer: defaultAccountAddress,
      });

vs

const deployMethod = StatefulTestContract.deploy({
  wallet, 
  ctorArgs: [defaultAccountAddress, 0],
  salt: Fr.random(),
  deployer: defaultAccountAddress,
});

I do love object params 😅 But I really feel like it helps understand what each param actually is in this case

[Thunkar]

TBH I didn't pay much attention to the ContractDeployer since it's on its way out, unified so now looks like:

const deployMethod = StatefulTestContract.deploy(wallet, defaultAccountAddress, 0, {
        salt: Fr.random(),
        deployer: defaultAccountAddress,
      });

const { receipt: txReceipt } = await deployer
        .deploy([ownerAddress, 1], { salt: new Fr(BigInt(1)) })
        .send({ from: ownerAddress });

I quite like keeping the args separate because the options object is often omitted. But it's true the order should be preserved, done!

[mverzilli]

I agree, and was about to make the same observation, but I guess here the intention is to reduce api breakage (you can always add params and things more or less keep working).

[mverzilli]

Unpacking deploy from send makes me realize having deploy be a verb is a bit misleading, maybe deployment would be more apt (a bit offtopic maybe)

[Thunkar]

That's actually a great idea, but I'd do it on a different PR (or even version, this change is bigger, but the other can just be a massive search and replace)

[mverzilli]

maybe not practical, feel free to dismiss: if we set a deployer we can make deploy's type different, such that this line becomes a compile time error.

[Thunkar]

mmm, but how would we detect deployer: alice and from: alice? Is that even possible using types? It's the one case we want to allow (to generate deterministic addresses, but send the tx at a later time)

[mverzilli]

the constructor is getting logically dense in particular as regards deployers. may be a future enhancement idea could be to provide some factory/builder methods?

[mverzilli]

this is starting to look a lot like it wants to be a DeployerLockedDeployMethod subclass

[mverzilli]

another symptom that maybe we want to refactor into three different types: UnlockedDeployMethod, SpecificDeployerDeployMethod, UniversalDeployerDeployMethod (names suck ack)

[Thunkar]

Exploring this for a next PR, I like the idea!

[mverzilli]

Looks good. I do think DeployMethod could use some refactoring. Not sure what the best way is as I don't have a feel for the code. Maybe it could be split into specific classes to handle different deployer cases, or maybe we keep a single DeployMethod type but it holds a new Deployer union type or class.

[AztecBot]

❌ Failed to cherry-pick to v4-next due to conflicts. (🤖) View backport run.