Consider new API for deploy

[adrianmcli]

Current deployIfDifferent function accepts quite a few arguments and it can be confusing to new users of this plugin. Here is the source of the function I had to look at in order to understand what the function calls actually mean.

source: https://github.com/wighawag/buidler-deploy/blob/177e8c1ac598180c5b827adcbec274a65d3bd4ca/src/utils/eth.js#L225-L233

The current example usage in the README is the following (reformatted with Prettier):

const deployResult = await deployIfDifferent(
  ["data"],
  "GenericMetaTxProcessor",
  { from: deployer, gas: 4000000 },
  "GenericMetaTxProcessor"
);

const deployResult = await deployIfDifferent(
  "data",
  "Token",
  { from: deployer },
  "Token"
);

const deployResult = await deployIfDifferent(
  "data",
  "ERC721BidSale",
  { from: deployer },
  "ERC721BidSale",
  Token.address,
  1,
  3600
);

Using Named Parameters

At first glance, this code doesn't really tell you what is going on (unless you are already very familiar with the API). I suggest we used named parameters as it is a lot more explicit and actually quite flexible if you wish to add or remove features later on.

For example (along with my suggestion from #6):

const deployResult = await deploy({
  name: "GenericMetaTxProcessor",
  contractName: "GenericMetaTxProcessor",
  fieldsToCompare: ["data"],
  options: { from: deployer, gas: 4000000 }
});

const deployResult = await deploy({
  name: "Token",
  contractName: "Token",
  fieldsToCompare: "data",
  options: { from: deployer }
});

const deployResult = await deploy({
  name: "ERC721BidSale",
  contractName: "ERC721BidSale",
  fieldsToCompare: "data",
  options: { from: deployer },
  args: [Token.address, 1, 3600]
});

Further Refinement

A further refinement could be to:

1. Assume a default value for fieldsToCompare to be data.
2. Assume contractName to be the same as name (can specify if it's not the case)
3. In-line the options object

This would lead to:

const deployResult = await deploy({
  name: "GenericMetaTxProcessor",
  from: deployer,
  gas: 4000000,
});

const deployResult = await deploy({
  name: "Token",
  from: deployer,
});

const deployResult = await deploy({
  name: "ERC721BidSale",
  args: [Token.address, 1, 3600],
  from: deployer,
});

Conclusion

This is just to spark some discussion, I hope this has been useful. I think simplifying the API for end-users is an important step of getting adoption.

[wighawag]

Hi @adrianmcli thanks so much for the detailed feedback. I agree completely, this make a better api. Going to implement these. I am going to rethink the other api too.

[adrianmcli]

From #6: Will consider renaming deployIfDifferent to deploy as part of this issue.

[wighawag]

I had a deeper look and I am gonna go with a slightly different signature :

deploy(name: string, options: DeployOptions, ...args: unknown[]): Promise<DeployResult>;

where DeployOptions is

    from: string;
    contractName?: string,    
    fieldsToCompare?: string | string[],
    linkedData?: any;

This is to keep the name as the first param for all deployment related function and to keep args as variadic.

This is almost like before except for the contractName btu I think it is quite self explanatory. what do you think ?

[adrianmcli]

👍 on having the name parameter first.

Having options last is a very common pattern, so I am worried about having it first. But having options last can also be confusing because every contract can have different number of params. Both can be problematic (see below).

I want to suggest a third alternative where the args are part of the options object. Let's see which one "feels" best in practice:

1. Args first

const deployResult = await deploy("ERC721BidSale", Token.address, 1, 3600, {
  from: deployer,
});

At first glance this looks fine, but when run through a formatter, it can look pretty ugly and unreadable because the contract name blends in with the args:

const deployResult = await deploy(
  "ERC721BidSale",
  Token.address,
  1,
  3600,
  { from: deployer }
);

2. Options first

const deployResult = await deploy(
  "ERC721BidSale",
  { from: deployer },
  Token.address,
  1,
  3600
);

But if you don't have any options object to pass, you will have to pass an empty object literal which can look really strange and requires extra work on the user:

const deployResult = await deploy("ERC721BidSale", {}, Token.address, 1, 3600);

3. In-line args in options (my suggestion)

So far, I think this is the most readable.

const deployResult = await deploy("ERC721BidSale", {
  args: [Token.address, 1, 3600],
  from: deployer,
});

The benefit of this approach is that even when run through a formatter with large number of params, you can still clearly see which are the args:

const deployResult = await deploy("ERC721BidSale", {
  args: [
    Token.address,
    12345,
    360000,
    someOtherVar,
    andAnotherVar,
    andEvenOneMoreVar,
  ],
  from: deployer,
});

@wighawag thoughts?

[wighawag]

Yes, I considered adding options last but do not like that. and prefer option first

regarding 2. Note that const deployResult = await deploy("ERC721BidSale", {}, Token.address, 1, 3600); is not valid as you always need to specify the address that will deploy, so the empty {} will not happen

Now I am not sure we should consider formatter to drive our API design.

Plus this is quite easy to read too:

const deployResult = await deploy(
   "ERC721BidSale",
    {
      from: deployer,
    },
    Token.address,
    12345,
    360000,
    someOtherVar,
    andAnotherVar,
    andEvenOneMoreVar,
});

Technically we could support both. like we can detect if args is supplied as part of the options and if not fallback on the variadic argument. Like this we have both but not sure if this a good idea ?

[adrianmcli]

🤔🤔🤔 You make a good point, and you're right it doesn't look too bad. I guess I just enjoy the args as part of options because it's more explicit and feels cleaner to me. I am also very wary of variadic arguments because it introduces uncertainty.

I want to have the choice of both, but I also agree it might be weird to support two ways. I'm not sure what the best way forward is. Maybe we should ask @alcuadrado and other people on the Buidler Telegram group to take a quick poll?

[wighawag]

closing it as it has now been implemented in v0.4.0