Add ability to create clones with initial value in Clones.sol

[k06a]

I don't know why this was not supported initially, we faces issue trying to use Clones but was also required to pass non-zero value...

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 528ebdc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

Hello @k06a

When creating a clone, there is no "constructor" function that can process that value is use it to set anything. So far, I considered that if a clone needs to receive value, it would be when its being initialized:

address instance = Clones.clone(implementation);
ISomeContract(instance).initialize{ value: value}(...args);

That would only work if the initializer is payable by design, reducing the risk of sending value by mistake.

Do you have a usecase where there is no such initialization that can carry the value?

[k06a]

@Amxx we do not need any initializer actually. It seems separate call would cost 6800+ gas, while passing value with create/create2 if free of charge.

[Amxx]

Yes, if you don't have an initializer call, then passing the value at creation is definitely cheaper.
And you are right, a call to a hot address with some value and no data is 6800. I did not expect it to be that much.

[Amxx]

This requires a changelog entry, and some unit test.

One thing we may want to address is what error to trigger if the balance is not sufficient. There may be a custom error in Address.sol that we can reuse here.

I'll work on that later today

[Amxx]

@ernestognw

In utils/Address.sol

error AddressInsufficientBalance(address account); // error is always thrown with address(this)

In utils/Create2.sol

error Create2InsufficientBalance(uint256 balance, uint256 needed);

Now, we could add a

error ClonesInsufficientBalance(...);

But it really feels like these should all be the same. Is that the moment we create a StdError library that includes a generic InsufficientBalance error ???

[Amxx]

Was not able to push to this branch, so I pushed my changes to https://github.com/Amxx/openzeppelin-contracts/tree/feature/clones-value

[k06a]

@Amxx added you to the repo to let you pushing easily, strange it is not allowed by GitHub as usually happens on PR.

[ernestognw]

But it really feels like these should all be the same. Is that the moment we create a StdError library that includes a generic InsufficientBalance error ???

Last time we discussed about an errors library we concluded the only candidates were:

error FailedCall();
error OutOfBounds(); // Now replaced with Panic `0x32` using the new `Panic.sol` library
error InvalidSignature();
error ExpiredSignature();

I would discard InvalidSignature and ExpiredSignature because I don't think they're general enough (perhaps a separate discussion). Regarding the FailedCall, it now makes more sense because it's used in a couple of other places.

I'm also fine with FailedDeployment, but I think InsufficientETHBalance would be better than InsufficientBalance since the error is intended for ETH (or native currency) balances.

The main reason I wasn't a fan of an errors library is that the more general the error, the less contract-specific information it provides, so I would generally recommend against using them if there's a more specific alternative. This means I'd rather have an InsufficientBalance(address token, ...) for tokens (and maybe use address(0) as ether's address) but that would be too much and rather unnecessary for the ETH case.

Let's keep the library, but I have a couple of questions @Amxx:

• Does an errors library imply backwards compatibility guarantees? (Note the proposed name is "Std Errors") We've agreed on not keeping compatibility for errors, but I'd like to make clear expectations for developers (e.g. upgrading to a version that changed the name of the error won't compile, is that expected?)
• Since I'd generally recommend for using more specific errors, I wonder what's the benefit of recommending users to throw errors from this library?

Now on the main topic of the PR: Yes, it makes sense to accept non zero values. Thanks for bringing it up @k06a

[ernestognw]

I think it'd be more generic to just call it FailedCall. However, I'm realizing that the main information this error provides is that it was a subcall that failed.

I'd like to give a second thought to FailedInnerCall. Not sure if Inner is the best. Would it make more sense to call it FailedSubCall?

[Amxx]

FailedCall, FailedInnerCall and FailedSubCall are all good to me. My favorite is FailedCall, because simplicity ... but all are good honestly.

[Amxx]

I also like the symetry of FailedCall <> FailedDeployment

[Amxx]

Note that this error is also emitted when the call that fails is actually a static call or a delegate call

[ernestognw]

I also like the symetry of FailedCall <> FailedDeployment

Let's do FailedCall, I think it covers the static and delegate call as well.

[k06a]

@ernestognw you are allowed to write to the repo now

[ernestognw]

Now that this accepts value, a factory may get locked if it doesn't accept ETH. Shall we add a note? @Amxx

[ernestognw]

Actually, this enables a couple of attack vectors:

• Can the factory be griefed (e.g. steal eth from it)?
• Can the factory stop receiving ETH to fund its operation?
• Can the factory be partially DoS'd if all payable entrypoints are restricted to a single entity?

I start to feel we didn't do this in the past because it's easy to fuck it up. We can still have the function, I just want to make sure we document the risks correctly.

[Amxx]

IMO, that is factory design, which is beyond the scope of this library. Most factory will not use value. Those that do need to take care of that.

IMO it's like Address library (that can do call with value) and TrustedForwarder (that can trigger that call). We don't have any warning in Address about the value issue.

[ernestognw]

Right, indeed, we don't have a warning in Address nor ERC2771Forwarder, but the main difference is the use case:

• ERC2771Forwarder's balance should be always zero since every call forwarded goes along with the value and everything else is refunded
• Address is more generic and it's fine if the caller doesn't have enough balance

Although Clones is very similar to Address, it's more likely users will need to consider designing their factory in such a way that always has enough balance. Even though that's factory design, I think it makes sense to make a simple recommendation:

NOTE: Using a non-zero value at creation will require the contract using this function (e.g. a factory)
to always have enough balance for new deployments. Consider exposing this function under a payable
method.

[Amxx]

That warning is fine

[Amxx]

Though, 99% or devs won't ever use this library, and out of the 1% remining, 99% will not use value.

That leaves us with 0.01%, which is basically only @k06a. Unless he decides to not test/audit its code, I think nobody will ever need that warning :)

[ernestognw]

Yeah probably true 😂, but ideally we would get more experienced developers like @k06a in the long run.

Think that with the Account Abstraction trend, I'd expect more factories to deploy accounts, and therefore more developers requiring initial value.

[ernestognw]

Coverage is failing but it's fine imo, tests are missing a condition we've never been able to trigger.

[Amxx]

but I think InsufficientETHBalance would be better than InsufficientBalance since the error is intended for ETH (or native currency) balances.

To be, balance is by default the native currency. So I understand InsufficientBalance as "not enough native currency".
On mainnet, that is Eth, but on other network, that may be a different currency.
Lets say you are on Gnosis chain, where native currency is (wrapped) dai ... and where you can have bridged Eth. Would InsufficientETHBalance be accurate ? I don't think so. Same applies to Matic (and probably many other EVM compatible networkd).

So for the sake of simplicity, I think that the default should be InsufficientBalance. Then token standards like ERC-20 can add their own domain.

[ernestognw]

To be, balance is by default the native currency. So I understand InsufficientBalance as "not enough native currency".
On mainnet, that is Eth, but on other network, that may be a different currency.

Fair, I wasn't considering this. Then let's keep InsufficientBalance.

Then token standards like ERC-20 can add their own domain.

Correct. This is also why I'm not a fan of the errors library, we use them because we want to generalize enough, but I feel developers starting a new project will define their own specializations.

[ernestognw]

Last thing:

Although we don't guarantee backwards compatibility on errors. Anyone upgrading from 5.0 to 5.1 will stop finding Address.FailedInnerCall. I think we should document this in the changelog.

Breaking changes

Users of upgradeable contracts depending on Address.FailedInnerCall, change it to Errors.FailedCall.

Not sure if there are more errors we've rewritten though

[Amxx]

Not sure if there are more errors we've rewritten though

Before this PR there was 2 different versions of InsufficientBalance that were unified:

• AddressInsufficientBalance(address)
• Create2InsufficientBalance(uint256,uint256)

We also had ERC1167FailedCreateClone and Create2FailedDeployment that were both replace by FailedDeployment

There are many changes, and we did clearly say that this is not something where we guarantee backward compatibility. Instead of listing all the changes I would either add nothing, or add a small note that some error names were modified, without going into the details

[ernestognw]

I'm worried about those same users who came to us asking how to replace Counters.sol, probably worrying too much

It'll be fine mentioning the error renaming generally, but I'd rather have a clear simple instruction (i.e. replace x for y). Just asked @frangio's opinion and seems he agrees with a simple list of replacements in the Changelog. Would like to hear his input (if any).

The only caveat I see with this approach is that we'll need to update the Changelog manually each time and it's kinda easy to forget if we're relaxing backwards compatibility requirements for custom errors

[ernestognw]

I added the list of error replacements to the Changelog. The reasoning is that upgrades need to be as smooth as possible, and even if the developers are skilled enough, it takes time to decide and evaluate a change when there's no straight-forward recommendation.

Tried to be as succinct as possible.

[ernestognw]

LGTM 🚢

[k06a]

@Amxx do you have any estimate when this functionality could be release as 5.0.3 or most likely 5.1.0?

[Amxx]

Patches (5.0.3) are only for security updates. We don't plan on doing one, and even if we did, this would not get into it.

5.1.0 may take some time to be released :/