From c1d093d87a2896aab259366d435f8a946e41f94e Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Thu, 29 Feb 2024 21:34:52 +0100 Subject: [PATCH 1/4] Add Clones lib ability to support non-zero value --- contracts/proxy/Clones.sol | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 92ed339a756..f46d390d7b7 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -26,6 +26,15 @@ library Clones { * This function uses the create opcode, which should never revert. */ function clone(address implementation) internal returns (address instance) { + return clone(implementation, 0); + } + + /** + * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. + * + * This function uses the create opcode, which should never revert. + */ + function clone(address implementation, uint256 value) internal returns (address instance) { /// @solidity memory-safe-assembly assembly { // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes @@ -33,7 +42,7 @@ library Clones { mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) // Packs the remaining 17 bytes of `implementation` with the bytecode after the address. mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3)) - instance := create(0, 0x09, 0x37) + instance := create(value, 0x09, 0x37) } if (instance == address(0)) { revert ERC1167FailedCreateClone(); @@ -48,6 +57,17 @@ library Clones { * the clones cannot be deployed twice at the same address. */ function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) { + return cloneDeterministic(implementation, salt, 0); + } + + /** + * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. + * + * This function uses the create2 opcode and a `salt` to deterministically deploy + * the clone. Using the same `implementation` and `salt` multiple time will revert, since + * the clones cannot be deployed twice at the same address. + */ + function cloneDeterministic(address implementation, bytes32 salt, uint256 value) internal returns (address instance) { /// @solidity memory-safe-assembly assembly { // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes @@ -55,7 +75,7 @@ library Clones { mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) // Packs the remaining 17 bytes of `implementation` with the bytecode after the address. mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3)) - instance := create2(0, 0x09, 0x37, salt) + instance := create2(value, 0x09, 0x37, salt) } if (instance == address(0)) { revert ERC1167FailedCreateClone(); From 1052cb1ba84f66c56605e19ba587b154f3aae5cd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Mar 2024 16:24:28 +0100 Subject: [PATCH 2/4] add Clones testing with value & create utils/Errors.sol --- .changeset/chilled-walls-develop.md | 5 ++++ .changeset/strong-singers-talk.md | 5 ++++ contracts/metatx/ERC2771Forwarder.sol | 5 ++-- contracts/proxy/Clones.sol | 17 +++++++------ contracts/utils/Address.sol | 30 ++++++++-------------- contracts/utils/Create2.sol | 18 ++++---------- contracts/utils/Errors.sol | 23 +++++++++++++++++ test/proxy/Clones.behaviour.js | 36 +++++++++++++++++++++------ test/proxy/Clones.test.js | 34 +++++++++++++++++-------- test/utils/Address.test.js | 20 +++++++-------- test/utils/Create2.test.js | 4 +-- 11 files changed, 125 insertions(+), 72 deletions(-) create mode 100644 .changeset/chilled-walls-develop.md create mode 100644 .changeset/strong-singers-talk.md create mode 100644 contracts/utils/Errors.sol diff --git a/.changeset/chilled-walls-develop.md b/.changeset/chilled-walls-develop.md new file mode 100644 index 00000000000..4108feb612d --- /dev/null +++ b/.changeset/chilled-walls-develop.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Clones`: Add version of `clone` and `cloneDeterministic` that support sending value at creation. diff --git a/.changeset/strong-singers-talk.md b/.changeset/strong-singers-talk.md new file mode 100644 index 00000000000..f905e008364 --- /dev/null +++ b/.changeset/strong-singers-talk.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Errors`: New library of standard custom errors. diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 221dabb627c..659a8783583 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -6,8 +6,9 @@ pragma solidity ^0.8.20; import {ERC2771Context} from "./ERC2771Context.sol"; import {ECDSA} from "../utils/cryptography/ECDSA.sol"; import {EIP712} from "../utils/cryptography/EIP712.sol"; -import {Nonces} from "../utils/Nonces.sol"; import {Address} from "../utils/Address.sol"; +import {Errors} from "../utils/Errors.sol"; +import {Nonces} from "../utils/Nonces.sol"; /** * @dev A forwarder compatible with ERC-2771 contracts. See {ERC2771Context}. @@ -132,7 +133,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } if (!_execute(request, true)) { - revert Address.FailedInnerCall(); + revert Errors.FailedInnerCall(); } } diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index f46d390d7b7..458aa785204 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; +import {Errors} from '../utils/Errors.sol'; + /** * @dev https://eips.ethereum.org/EIPS/eip-1167[ERC-1167] is a standard for * deploying minimal proxy contracts, also known as "clones". @@ -15,11 +17,6 @@ pragma solidity ^0.8.20; * deterministic method. */ library Clones { - /** - * @dev A clone instance deployment failed. - */ - error ERC1167FailedCreateClone(); - /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. * @@ -35,6 +32,9 @@ library Clones { * This function uses the create opcode, which should never revert. */ function clone(address implementation, uint256 value) internal returns (address instance) { + if (value > 0 && address(this).balance < value) { + revert Errors.InsufficientBalance(address(this).balance, value); + } /// @solidity memory-safe-assembly assembly { // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes @@ -45,7 +45,7 @@ library Clones { instance := create(value, 0x09, 0x37) } if (instance == address(0)) { - revert ERC1167FailedCreateClone(); + revert Errors.FailedDeployment(); } } @@ -68,6 +68,9 @@ library Clones { * the clones cannot be deployed twice at the same address. */ function cloneDeterministic(address implementation, bytes32 salt, uint256 value) internal returns (address instance) { + if (value > 0 && address(this).balance < value) { + revert Errors.InsufficientBalance(address(this).balance, value); + } /// @solidity memory-safe-assembly assembly { // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes @@ -78,7 +81,7 @@ library Clones { instance := create2(value, 0x09, 0x37, salt) } if (instance == address(0)) { - revert ERC1167FailedCreateClone(); + revert Errors.FailedDeployment(); } } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index b7e3059529a..720d13b1c39 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -3,25 +3,17 @@ pragma solidity ^0.8.20; +import {Errors} from './Errors.sol'; + /** * @dev Collection of functions related to the address type */ library Address { - /** - * @dev The ETH balance of the account is not enough to perform the operation. - */ - error AddressInsufficientBalance(address account); - /** * @dev There's no code at `target` (it is not a contract). */ error AddressEmptyCode(address target); - /** - * @dev A call to an address target failed. The target may have reverted. - */ - error FailedInnerCall(); - /** * @dev Replacement for Solidity's `transfer`: sends `amount` wei to * `recipient`, forwarding all available gas and reverting on errors. @@ -40,12 +32,12 @@ library Address { */ function sendValue(address payable recipient, uint256 amount) internal { if (address(this).balance < amount) { - revert AddressInsufficientBalance(address(this)); + revert Errors.InsufficientBalance(address(this).balance, amount); } (bool success, ) = recipient.call{value: amount}(""); if (!success) { - revert FailedInnerCall(); + revert Errors.FailedInnerCall(); } } @@ -57,7 +49,7 @@ library Address { * If `target` reverts with a revert reason or custom error, it is bubbled * up by this function (like regular Solidity function calls). However, if * the call reverted with no returned reason, this function reverts with a - * {FailedInnerCall} error. + * {Errors.FailedInnerCall} error. * * Returns the raw returned data. To convert to the expected return value, * use https://solidity.readthedocs.io/en/latest/units-and-global-variables.html?highlight=abi.decode#abi-encoding-and-decoding-functions[`abi.decode`]. @@ -82,7 +74,7 @@ library Address { */ function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) { if (address(this).balance < value) { - revert AddressInsufficientBalance(address(this)); + revert Errors.InsufficientBalance(address(this).balance, value); } (bool success, bytes memory returndata) = target.call{value: value}(data); return verifyCallResultFromTarget(target, success, returndata); @@ -108,8 +100,8 @@ library Address { /** * @dev Tool to verify that a low level call to smart-contract was successful, and reverts if the target - * was not a contract or bubbling up the revert reason (falling back to {FailedInnerCall}) in case of an - * unsuccessful call. + * was not a contract or bubbling up the revert reason (falling back to {Errors.FailedInnerCall}) in case + * of an unsuccessful call. */ function verifyCallResultFromTarget( address target, @@ -130,7 +122,7 @@ library Address { /** * @dev Tool to verify that a low level call was successful, and reverts if it wasn't, either by bubbling the - * revert reason or with a default {FailedInnerCall} error. + * revert reason or with a default {Errors.FailedInnerCall} error. */ function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) { if (!success) { @@ -141,7 +133,7 @@ library Address { } /** - * @dev Reverts with returndata if present. Otherwise reverts with {FailedInnerCall}. + * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedInnerCall}. */ function _revert(bytes memory returndata) private pure { // Look for revert reason and bubble it up if present @@ -153,7 +145,7 @@ library Address { revert(add(32, returndata), returndata_size) } } else { - revert FailedInnerCall(); + revert Errors.FailedInnerCall(); } } } diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index ad1cd5f4d54..3cfed135d5f 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; +import {Errors} from './Errors.sol'; + /** * @dev Helper to make usage of the `CREATE2` EVM opcode easier and safer. * `CREATE2` can be used to compute in advance the address where a smart @@ -13,21 +15,11 @@ pragma solidity ^0.8.20; * information. */ library Create2 { - /** - * @dev Not enough balance for performing a CREATE2 deploy. - */ - error Create2InsufficientBalance(uint256 balance, uint256 needed); - /** * @dev There's no code to deploy. */ error Create2EmptyBytecode(); - /** - * @dev The deployment failed. - */ - error Create2FailedDeployment(); - /** * @dev Deploys a contract using `CREATE2`. The address where the contract * will be deployed can be known in advance via {computeAddress}. @@ -43,8 +35,8 @@ library Create2 { * - if `amount` is non-zero, `bytecode` must have a `payable` constructor. */ function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address addr) { - if (address(this).balance < amount) { - revert Create2InsufficientBalance(address(this).balance, amount); + if (amount > 0 && address(this).balance < amount) { + revert Errors.InsufficientBalance(address(this).balance, amount); } if (bytecode.length == 0) { revert Create2EmptyBytecode(); @@ -54,7 +46,7 @@ library Create2 { addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt) } if (addr == address(0)) { - revert Create2FailedDeployment(); + revert Errors.FailedDeployment(); } } diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol new file mode 100644 index 00000000000..ecb83ab0b6a --- /dev/null +++ b/contracts/utils/Errors.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +/** + * @dev Collection of standard custom error that are used in multiple contracts + */ +library Errors { + /** + * @dev The ETH balance of the account is not enough to perform the operation. + */ + error InsufficientBalance(uint256 balance, uint256 needed); + + /** + * @dev A call to an address target failed. The target may have reverted. + */ + error FailedInnerCall(); + + /** + * @dev The deployment failed. + */ + error FailedDeployment(); +} \ No newline at end of file diff --git a/test/proxy/Clones.behaviour.js b/test/proxy/Clones.behaviour.js index 861fae8a2af..bd63e051f02 100644 --- a/test/proxy/Clones.behaviour.js +++ b/test/proxy/Clones.behaviour.js @@ -13,6 +13,26 @@ module.exports = function shouldBehaveLikeClone() { }); }; + describe('construct with value', function () { + const value = 10n; + + it ('factory has enough balance', async function () { + await this.deployer.sendTransaction({ to: this.factory, value }); + + const instance = await this.createClone({ deployValue: value }); + await expect(instance.deploymentTransaction()) + .to.changeEtherBalances([ this.factory, instance ], [ -value, value ]); + + expect(await ethers.provider.getBalance(instance)).to.equal(value); + }); + + it ('factory does not have enough balance', async function () { + await expect(this.createClone({ deployValue: value })) + .to.be.revertedWithCustomError(this.factory, 'InsufficientBalance') + .withArgs(0n, value); + }); + }) + describe('initialization without parameters', function () { describe('non payable', function () { const expectedInitializedValue = 10n; @@ -23,7 +43,7 @@ module.exports = function shouldBehaveLikeClone() { describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData); + this.proxy = await this.createClone({ initData: this.initializeData }); }); assertProxyInitialization({ @@ -36,7 +56,7 @@ module.exports = function shouldBehaveLikeClone() { const value = 10n ** 6n; it('reverts', async function () { - await expect(this.createClone(this.initializeData, { value })).to.be.reverted; + await expect(this.createClone({ initData: this.initializeData, initValue: value })).to.be.reverted; }); }); }); @@ -50,7 +70,7 @@ module.exports = function shouldBehaveLikeClone() { describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData); + this.proxy = await this.createClone({ initData: this.initializeData }); }); assertProxyInitialization({ @@ -63,7 +83,7 @@ module.exports = function shouldBehaveLikeClone() { const value = 10n ** 6n; beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData, { value }); + this.proxy = await this.createClone({ initData: this.initializeData, initValue: value }); }); assertProxyInitialization({ @@ -86,7 +106,7 @@ module.exports = function shouldBehaveLikeClone() { describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData); + this.proxy = await this.createClone({ initData: this.initializeData }); }); assertProxyInitialization({ @@ -99,7 +119,7 @@ module.exports = function shouldBehaveLikeClone() { const value = 10n ** 6n; it('reverts', async function () { - await expect(this.createClone(this.initializeData, { value })).to.be.reverted; + await expect(this.createClone({ initData: this.initializeData, initValue: value })).to.be.reverted; }); }); }); @@ -115,7 +135,7 @@ module.exports = function shouldBehaveLikeClone() { describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData); + this.proxy = await this.createClone({ initData: this.initializeData }); }); assertProxyInitialization({ @@ -128,7 +148,7 @@ module.exports = function shouldBehaveLikeClone() { const value = 10n ** 6n; beforeEach('creating proxy', async function () { - this.proxy = await this.createClone(this.initializeData, { value }); + this.proxy = await this.createClone({ initData: this.initializeData, initValue: value }); }); assertProxyInitialization({ diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 626b1e56467..7447a354b6e 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -10,21 +10,33 @@ async function fixture() { const factory = await ethers.deployContract('$Clones'); const implementation = await ethers.deployContract('DummyImplementation'); - const newClone = async (initData, opts = {}) => { + const newClone = async (opts = {}) => { const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); - await factory.$clone(implementation); - await deployer.sendTransaction({ to: clone, value: opts.value ?? 0n, data: initData ?? '0x' }); - return clone; + const tx = await ( + opts.deployValue + ? factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) + : factory.$clone(implementation) + ); + if (opts.initData || opts.initValue) { + await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); + } + return Object.assign(clone, { deploymentTransaction: () => tx }); }; - const newCloneDeterministic = async (initData, opts = {}) => { + const newCloneDeterministic = async (opts = {}) => { const salt = opts.salt ?? ethers.randomBytes(32); const clone = await factory.$cloneDeterministic .staticCall(implementation, salt) .then(address => implementation.attach(address)); - await factory.$cloneDeterministic(implementation, salt); - await deployer.sendTransaction({ to: clone, value: opts.value ?? 0n, data: initData ?? '0x' }); - return clone; + const tx = await ( + opts.deployValue + ? factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) + : factory.$cloneDeterministic(implementation, salt) + ); + if (opts.initData || opts.initValue) { + await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); + } + return Object.assign(clone, { deploymentTransaction: () => tx }); }; return { deployer, factory, implementation, newClone, newCloneDeterministic }; @@ -56,13 +68,13 @@ describe('Clones', function () { // deploy once await expect(this.factory.$cloneDeterministic(this.implementation, salt)).to.emit( this.factory, - 'return$cloneDeterministic', + 'return$cloneDeterministic_address_bytes32', ); // deploy twice await expect(this.factory.$cloneDeterministic(this.implementation, salt)).to.be.revertedWithCustomError( this.factory, - 'ERC1167FailedCreateClone', + 'FailedDeployment', ); }); @@ -80,7 +92,7 @@ describe('Clones', function () { expect(predicted).to.equal(expected); await expect(this.factory.$cloneDeterministic(this.implementation, salt)) - .to.emit(this.factory, 'return$cloneDeterministic') + .to.emit(this.factory, 'return$cloneDeterministic_address_bytes32') .withArgs(predicted); }); }); diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index bdfd64bf94c..efab3de461a 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -23,13 +23,13 @@ describe('Address', function () { describe('sendValue', function () { describe('when sender contract has no funds', function () { it('sends 0 wei', async function () { - await expect(this.mock.$sendValue(this.other, 0)).to.changeEtherBalance(this.recipient, 0); + await expect(this.mock.$sendValue(this.other, 0n)).to.changeEtherBalance(this.recipient, 0n); }); it('reverts when sending non-zero amounts', async function () { - await expect(this.mock.$sendValue(this.other, 1)) - .to.be.revertedWithCustomError(this.mock, 'AddressInsufficientBalance') - .withArgs(this.mock); + await expect(this.mock.$sendValue(this.other, 1n)) + .to.be.revertedWithCustomError(this.mock, 'InsufficientBalance') + .withArgs(0n, 1n); }); }); @@ -42,7 +42,7 @@ describe('Address', function () { describe('with EOA recipient', function () { it('sends 0 wei', async function () { - await expect(this.mock.$sendValue(this.recipient, 0)).to.changeEtherBalance(this.recipient, 0); + await expect(this.mock.$sendValue(this.recipient, 0n)).to.changeEtherBalance(this.recipient, 0n); }); it('sends non-zero amounts', async function () { @@ -59,8 +59,8 @@ describe('Address', function () { it('reverts when sending more than the balance', async function () { await expect(this.mock.$sendValue(this.recipient, funds + 1n)) - .to.be.revertedWithCustomError(this.mock, 'AddressInsufficientBalance') - .withArgs(this.mock); + .to.be.revertedWithCustomError(this.mock, 'InsufficientBalance') + .withArgs(funds, funds + 1n); }); }); @@ -155,7 +155,7 @@ describe('Address', function () { it('calls the requested function', async function () { const call = this.target.interface.encodeFunctionData('mockFunction'); - await expect(this.mock.$functionCallWithValue(this.target, call, 0)) + await expect(this.mock.$functionCallWithValue(this.target, call, 0n)) .to.emit(this.target, 'MockFunctionCalled') .to.emit(this.mock, 'return$functionCallWithValue') .withArgs(coder.encode(['string'], ['0x1234'])); @@ -169,8 +169,8 @@ describe('Address', function () { const call = this.target.interface.encodeFunctionData('mockFunction'); await expect(this.mock.$functionCallWithValue(this.target, call, value)) - .to.be.revertedWithCustomError(this.mock, 'AddressInsufficientBalance') - .withArgs(this.mock); + .to.be.revertedWithCustomError(this.mock, 'InsufficientBalance') + .withArgs(0n, value); }); it('calls the requested function with existing value', async function () { diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 86d00e13197..aba4deeb002 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -114,7 +114,7 @@ describe('Create2', function () { await expect(this.factory.$deploy(0n, saltHex, this.constructorByteCode)).to.be.revertedWithCustomError( this.factory, - 'Create2FailedDeployment', + 'FailedDeployment', ); }); @@ -127,7 +127,7 @@ describe('Create2', function () { it('fails deploying a contract if factory contract does not have sufficient balance', async function () { await expect(this.factory.$deploy(1n, saltHex, this.constructorByteCode)) - .to.be.revertedWithCustomError(this.factory, 'Create2InsufficientBalance') + .to.be.revertedWithCustomError(this.factory, 'InsufficientBalance') .withArgs(0n, 1n); }); }); From 6aab4880c1c2771ae41b6d33c814b1723ee3d681 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Mar 2024 16:41:27 +0100 Subject: [PATCH 3/4] fix lint --- contracts/proxy/Clones.sol | 8 ++++++-- contracts/utils/Address.sol | 2 +- contracts/utils/Create2.sol | 2 +- contracts/utils/Errors.sol | 2 +- test/proxy/Clones.behaviour.js | 9 ++++----- test/proxy/Clones.test.js | 12 ++++-------- 6 files changed, 17 insertions(+), 18 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 458aa785204..0cbe1abd646 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.20; -import {Errors} from '../utils/Errors.sol'; +import {Errors} from "../utils/Errors.sol"; /** * @dev https://eips.ethereum.org/EIPS/eip-1167[ERC-1167] is a standard for @@ -67,7 +67,11 @@ library Clones { * the clone. Using the same `implementation` and `salt` multiple time will revert, since * the clones cannot be deployed twice at the same address. */ - function cloneDeterministic(address implementation, bytes32 salt, uint256 value) internal returns (address instance) { + function cloneDeterministic( + address implementation, + bytes32 salt, + uint256 value + ) internal returns (address instance) { if (value > 0 && address(this).balance < value) { revert Errors.InsufficientBalance(address(this).balance, value); } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index 720d13b1c39..9e6d68f25a9 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.20; -import {Errors} from './Errors.sol'; +import {Errors} from "./Errors.sol"; /** * @dev Collection of functions related to the address type diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 3cfed135d5f..91ef7254ac9 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.20; -import {Errors} from './Errors.sol'; +import {Errors} from "./Errors.sol"; /** * @dev Helper to make usage of the `CREATE2` EVM opcode easier and safer. diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index ecb83ab0b6a..c46459fb881 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -20,4 +20,4 @@ library Errors { * @dev The deployment failed. */ error FailedDeployment(); -} \ No newline at end of file +} diff --git a/test/proxy/Clones.behaviour.js b/test/proxy/Clones.behaviour.js index bd63e051f02..dcc62066533 100644 --- a/test/proxy/Clones.behaviour.js +++ b/test/proxy/Clones.behaviour.js @@ -16,22 +16,21 @@ module.exports = function shouldBehaveLikeClone() { describe('construct with value', function () { const value = 10n; - it ('factory has enough balance', async function () { + it('factory has enough balance', async function () { await this.deployer.sendTransaction({ to: this.factory, value }); const instance = await this.createClone({ deployValue: value }); - await expect(instance.deploymentTransaction()) - .to.changeEtherBalances([ this.factory, instance ], [ -value, value ]); + await expect(instance.deploymentTransaction()).to.changeEtherBalances([this.factory, instance], [-value, value]); expect(await ethers.provider.getBalance(instance)).to.equal(value); }); - it ('factory does not have enough balance', async function () { + it('factory does not have enough balance', async function () { await expect(this.createClone({ deployValue: value })) .to.be.revertedWithCustomError(this.factory, 'InsufficientBalance') .withArgs(0n, value); }); - }) + }); describe('initialization without parameters', function () { describe('non payable', function () { diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 7447a354b6e..70220fbf7a0 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -12,11 +12,9 @@ async function fixture() { const newClone = async (opts = {}) => { const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); - const tx = await ( - opts.deployValue + const tx = await (opts.deployValue ? factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) - : factory.$clone(implementation) - ); + : factory.$clone(implementation)); if (opts.initData || opts.initValue) { await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); } @@ -28,11 +26,9 @@ async function fixture() { const clone = await factory.$cloneDeterministic .staticCall(implementation, salt) .then(address => implementation.attach(address)); - const tx = await ( - opts.deployValue + const tx = await (opts.deployValue ? factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) - : factory.$cloneDeterministic(implementation, salt) - ); + : factory.$cloneDeterministic(implementation, salt)); if (opts.initData || opts.initValue) { await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); } From 9618bcdf24385b27c531a359212ee33437c548b2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Mar 2024 17:23:51 +0100 Subject: [PATCH 4/4] simplify --- contracts/proxy/Clones.sol | 4 ++-- contracts/utils/Create2.sol | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 0cbe1abd646..e5c88791dc8 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -32,7 +32,7 @@ library Clones { * This function uses the create opcode, which should never revert. */ function clone(address implementation, uint256 value) internal returns (address instance) { - if (value > 0 && address(this).balance < value) { + if (address(this).balance < value) { revert Errors.InsufficientBalance(address(this).balance, value); } /// @solidity memory-safe-assembly @@ -72,7 +72,7 @@ library Clones { bytes32 salt, uint256 value ) internal returns (address instance) { - if (value > 0 && address(this).balance < value) { + if (address(this).balance < value) { revert Errors.InsufficientBalance(address(this).balance, value); } /// @solidity memory-safe-assembly diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 91ef7254ac9..b98253bf3ad 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -35,7 +35,7 @@ library Create2 { * - if `amount` is non-zero, `bytecode` must have a `payable` constructor. */ function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address addr) { - if (amount > 0 && address(this).balance < amount) { + if (address(this).balance < amount) { revert Errors.InsufficientBalance(address(this).balance, amount); } if (bytecode.length == 0) {