From c1d093d87a2896aab259366d435f8a946e41f94e Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Thu, 29 Feb 2024 21:34:52 +0100 Subject: [PATCH 01/17] 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 02/17] 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 03/17] 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 04/17] 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) { From d0f918697d337ccc5941c9cdf1e4442086da1344 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Mar 2024 18:14:05 +0100 Subject: [PATCH 05/17] fix tests --- test/utils/cryptography/EIP712.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js index 166038b3680..2b6e7fa9787 100644 --- a/test/utils/cryptography/EIP712.test.js +++ b/test/utils/cryptography/EIP712.test.js @@ -58,7 +58,7 @@ describe('EIP712', function () { const clone = await factory .$clone(this.eip712) .then(tx => tx.wait()) - .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone').args.instance) + .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone_address').args.instance) .then(address => ethers.getContractAt('$EIP712Verifier', address)); const expectedDomain = { ...this.domain, verifyingContract: clone.target }; From a41bc51472b2b4b1b1cd3e889bc1502741174944 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Mar 2024 22:00:54 +0100 Subject: [PATCH 06/17] fix upgradeable patch --- README.md | 2 +- scripts/upgradeable/upgradeable.patch | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2f4609f002c..fa7b4e31e55 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ $ npm install @openzeppelin/contracts $ forge install OpenZeppelin/openzeppelin-contracts ``` -Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` +Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` ### Usage diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index 46893d7d26b..458ecd43580 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -59,7 +59,7 @@ index ff596b0c3..000000000 - - diff --git a/README.md b/README.md -index 35083bc6e..05cf4fc27 100644 +index fa7b4e31e..4799b6376 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,9 @@ @@ -89,8 +89,8 @@ index 35083bc6e..05cf4fc27 100644 +$ forge install OpenZeppelin/openzeppelin-contracts-upgradeable ``` --Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` -+Add `@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/` in `remappings.txt.` +-Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` ++Add `@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/` in `remappings.txt.` ### Usage @@ -110,7 +110,7 @@ index 35083bc6e..05cf4fc27 100644 } ``` diff --git a/contracts/package.json b/contracts/package.json -index 6ab89138a..ece834a44 100644 +index 845e8c403..8dc181b91 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,5 +1,5 @@ @@ -118,7 +118,7 @@ index 6ab89138a..ece834a44 100644 - "name": "@openzeppelin/contracts", + "name": "@openzeppelin/contracts-upgradeable", "description": "Secure Smart Contract library for Solidity", - "version": "5.0.1", + "version": "5.0.2", "files": [ @@ -13,7 +13,7 @@ }, @@ -307,7 +307,7 @@ index 77c4c8990..602467f40 100644 } } diff --git a/package.json b/package.json -index ec2c44ced..46eedc98f 100644 +index c4b358e10..96ab2559c 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ @@ -328,7 +328,7 @@ index 304d1386a..a1cd63bee 100644 +@openzeppelin/contracts-upgradeable/=contracts/ +@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/ diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js -index 166038b36..268e0d29d 100644 +index 2b6e7fa97..268e0d29d 100644 --- a/test/utils/cryptography/EIP712.test.js +++ b/test/utils/cryptography/EIP712.test.js @@ -47,27 +47,6 @@ describe('EIP712', function () { @@ -346,7 +346,7 @@ index 166038b36..268e0d29d 100644 - const clone = await factory - .$clone(this.eip712) - .then(tx => tx.wait()) -- .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone').args.instance) +- .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone_address').args.instance) - .then(address => ethers.getContractAt('$EIP712Verifier', address)); - - const expectedDomain = { ...this.domain, verifyingContract: clone.target }; From 4676598cfcbb94a146c8bb16c125a0c40d674170 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 4 Mar 2024 19:05:33 -0600 Subject: [PATCH 07/17] Apply PR suggestions --- .changeset/strong-singers-talk.md | 2 +- contracts/metatx/ERC2771Forwarder.sol | 2 +- contracts/utils/Errors.sol | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.changeset/strong-singers-talk.md b/.changeset/strong-singers-talk.md index f905e008364..7897980cbae 100644 --- a/.changeset/strong-singers-talk.md +++ b/.changeset/strong-singers-talk.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Errors`: New library of standard custom errors. +`Errors`: New library of common custom errors. diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 659a8783583..14e7b32db69 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -6,9 +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}. diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index c46459fb881..e8040563215 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -3,7 +3,10 @@ pragma solidity ^0.8.20; /** - * @dev Collection of standard custom error that are used in multiple contracts + * @dev Collection of common custom errors used in multiple contracts + * + * IMPORTANT: Backwards compatibility is not guaranteed in future versions of the library. + * It is recommended to avoid relying on the error API for critical functionality. */ library Errors { /** From 463ff75ed887f48fb012e03c656109b86b58e8a8 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 4 Mar 2024 19:20:04 -0600 Subject: [PATCH 08/17] Apply PR suggestions --- contracts/proxy/Clones.sol | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index e5c88791dc8..5a5ab15e77c 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -27,9 +27,7 @@ library Clones { } /** - * @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. + * @dev Same as {clone}, but with a `value` parameter to send native currency to the new contract. */ function clone(address implementation, uint256 value) internal returns (address instance) { if (address(this).balance < value) { @@ -61,11 +59,7 @@ library Clones { } /** - * @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. + * @dev Same as {cloneDeterministic}, but with a `value` parameter to send native currency to the new contract. */ function cloneDeterministic( address implementation, From 132bccb71d99b60fffc0248768d862a2bf2eae94 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 4 Mar 2024 19:29:13 -0600 Subject: [PATCH 09/17] Fix docs --- contracts/proxy/Clones.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 5a5ab15e77c..53f40ddd0e7 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -27,7 +27,7 @@ library Clones { } /** - * @dev Same as {clone}, but with a `value` parameter to send native currency to the new contract. + * @dev Same as {xref-Clones-clone-address-}[clone], but with a `value` parameter to send native currency to the new contract. */ function clone(address implementation, uint256 value) internal returns (address instance) { if (address(this).balance < value) { @@ -59,7 +59,7 @@ library Clones { } /** - * @dev Same as {cloneDeterministic}, but with a `value` parameter to send native currency to the new contract. + * @dev Same as {xref-Clones-cloneDeterministic-address-bytes32-}[cloneDeterministic], but with a `value` parameter to send native currency to the new contract. */ function cloneDeterministic( address implementation, From fcc327bf212c5e8328256123e9452ea296fb0f52 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Mar 2024 16:48:42 +0100 Subject: [PATCH 10/17] add warning --- contracts/proxy/Clones.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index e5c88791dc8..ad9a68fede9 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -30,6 +30,9 @@ library Clones { * @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. + * + * 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. */ function clone(address implementation, uint256 value) internal returns (address instance) { if (address(this).balance < value) { @@ -66,6 +69,9 @@ library Clones { * 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. + * + * 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. */ function cloneDeterministic( address implementation, From d2fcabf53ec7671a6709eb41d5e3ca80caf0149d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Mar 2024 16:50:07 +0100 Subject: [PATCH 11/17] =?UTF-8?q?Rename=20FailedInnerCall=20=E2=86=92=20Fa?= =?UTF-8?q?iledCall?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/metatx/ERC2771Forwarder.sol | 2 +- contracts/utils/Address.sol | 12 ++++++------ contracts/utils/Errors.sol | 2 +- test/finance/VestingWallet.behavior.js | 2 +- test/governance/Governor.test.js | 2 +- test/governance/TimelockController.test.js | 10 +++++----- test/metatx/ERC2771Forwarder.test.js | 2 +- test/utils/Address.test.js | 12 ++++++------ 8 files changed, 22 insertions(+), 22 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 659a8783583..5386caf55c3 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -133,7 +133,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } if (!_execute(request, true)) { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index 9e6d68f25a9..53a3c442049 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -37,7 +37,7 @@ library Address { (bool success, ) = recipient.call{value: amount}(""); if (!success) { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } @@ -49,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 - * {Errors.FailedInnerCall} error. + * {Errors.FailedCall} 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`]. @@ -100,7 +100,7 @@ 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 {Errors.FailedInnerCall}) in case + * was not a contract or bubbling up the revert reason (falling back to {Errors.FailedCall}) in case * of an unsuccessful call. */ function verifyCallResultFromTarget( @@ -122,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 {Errors.FailedInnerCall} error. + * revert reason or with a default {Errors.FailedCall} error. */ function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) { if (!success) { @@ -133,7 +133,7 @@ library Address { } /** - * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedInnerCall}. + * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedCall}. */ function _revert(bytes memory returndata) private pure { // Look for revert reason and bubble it up if present @@ -145,7 +145,7 @@ library Address { revert(add(32, returndata), returndata_size) } } else { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } } diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index c46459fb881..86b487c2d2b 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -14,7 +14,7 @@ library Errors { /** * @dev A call to an address target failed. The target may have reverted. */ - error FailedInnerCall(); + error FailedCall(); /** * @dev The deployment failed. diff --git a/test/finance/VestingWallet.behavior.js b/test/finance/VestingWallet.behavior.js index 819a47ff635..b45ffeecfd3 100644 --- a/test/finance/VestingWallet.behavior.js +++ b/test/finance/VestingWallet.behavior.js @@ -12,7 +12,7 @@ async function envSetup(mock, beneficiary, token) { const beneficiaryMock = await ethers.deployContract('EtherReceiverMock'); await beneficiaryMock.setAcceptEther(false); await mock.connect(beneficiary).transferOwnership(beneficiaryMock); - return { args: [], error: [mock, 'FailedInnerCall'] }; + return { args: [], error: [mock, 'FailedCall'] }; }, releasedEvent: 'EtherReleased', args: [], diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index e097ef0eff4..3e48ccfee7f 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -445,7 +445,7 @@ describe('Governor', function () { await this.helper.waitForSnapshot(); await this.helper.connect(this.voter1).vote({ support: VoteType.For }); await this.helper.waitForDeadline(); - await expect(this.helper.execute()).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + await expect(this.helper.execute()).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('if receiver revert with reason', async function () { diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index f7ba96c8215..a62cbd4440b 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -893,7 +893,7 @@ describe('TimelockController', function () { operation.predecessor, operation.salt, ), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); }); }); @@ -1099,7 +1099,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('call throw', async function () { @@ -1146,7 +1146,7 @@ describe('TimelockController', function () { .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt, { gasLimit: '100000', }), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('call payable with eth', async function () { @@ -1199,7 +1199,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); expect(await ethers.provider.getBalance(this.mock)).to.equal(0n); expect(await ethers.provider.getBalance(this.callreceivermock)).to.equal(0n); @@ -1227,7 +1227,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); expect(await ethers.provider.getBalance(this.mock)).to.equal(0n); expect(await ethers.provider.getBalance(this.callreceivermock)).to.equal(0n); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 8a2a09233dc..8653ad708cb 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -122,7 +122,7 @@ describe('ERC2771Forwarder', function () { data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason'), }); - await expect(this.forwarder.execute(request)).to.be.revertedWithCustomError(this.forwarder, 'FailedInnerCall'); + await expect(this.forwarder.execute(request)).to.be.revertedWithCustomError(this.forwarder, 'FailedCall'); }); }); diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index efab3de461a..944489feff7 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -74,7 +74,7 @@ describe('Address', function () { await this.targetEther.setAcceptEther(false); await expect(this.mock.$sendValue(this.targetEther, funds)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -103,7 +103,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); @@ -118,7 +118,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call, { gasLimit: 120_000n })).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); @@ -134,7 +134,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -207,7 +207,7 @@ describe('Address', function () { await expect(this.mock.$functionCallWithValue(this.target, call, value)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -225,7 +225,7 @@ describe('Address', function () { await expect(this.mock.$functionStaticCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); From e8eb6251e0707dc2fff4a7d6eaf9e53593d8c2e4 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 5 Mar 2024 09:50:36 -0600 Subject: [PATCH 12/17] Apply suggestions --- contracts/metatx/ERC2771Forwarder.sol | 2 +- contracts/proxy/Clones.sol | 12 ++++++++++-- contracts/utils/Address.sol | 12 ++++++------ contracts/utils/Errors.sol | 2 +- test/finance/VestingWallet.behavior.js | 2 +- test/governance/Governor.test.js | 2 +- test/governance/TimelockController.test.js | 10 +++++----- test/metatx/ERC2771Forwarder.test.js | 2 +- test/utils/Address.test.js | 12 ++++++------ 9 files changed, 32 insertions(+), 24 deletions(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 14e7b32db69..fe2c9d1d591 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -133,7 +133,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } if (!_execute(request, true)) { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 53f40ddd0e7..d4b8035610b 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -27,7 +27,11 @@ library Clones { } /** - * @dev Same as {xref-Clones-clone-address-}[clone], but with a `value` parameter to send native currency to the new contract. + * @dev Same as {xref-Clones-clone-address-}[clone], but with a `value` parameter to send native currency + * to the new contract. + * + * 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. */ function clone(address implementation, uint256 value) internal returns (address instance) { if (address(this).balance < value) { @@ -59,7 +63,11 @@ library Clones { } /** - * @dev Same as {xref-Clones-cloneDeterministic-address-bytes32-}[cloneDeterministic], but with a `value` parameter to send native currency to the new contract. + * @dev Same as {xref-Clones-cloneDeterministic-address-bytes32-}[cloneDeterministic], but with + * a `value` parameter to send native currency to the new contract. + * + * 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. */ function cloneDeterministic( address implementation, diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index 9e6d68f25a9..53a3c442049 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -37,7 +37,7 @@ library Address { (bool success, ) = recipient.call{value: amount}(""); if (!success) { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } @@ -49,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 - * {Errors.FailedInnerCall} error. + * {Errors.FailedCall} 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`]. @@ -100,7 +100,7 @@ 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 {Errors.FailedInnerCall}) in case + * was not a contract or bubbling up the revert reason (falling back to {Errors.FailedCall}) in case * of an unsuccessful call. */ function verifyCallResultFromTarget( @@ -122,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 {Errors.FailedInnerCall} error. + * revert reason or with a default {Errors.FailedCall} error. */ function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) { if (!success) { @@ -133,7 +133,7 @@ library Address { } /** - * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedInnerCall}. + * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedCall}. */ function _revert(bytes memory returndata) private pure { // Look for revert reason and bubble it up if present @@ -145,7 +145,7 @@ library Address { revert(add(32, returndata), returndata_size) } } else { - revert Errors.FailedInnerCall(); + revert Errors.FailedCall(); } } } diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index e8040563215..024109444f0 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -17,7 +17,7 @@ library Errors { /** * @dev A call to an address target failed. The target may have reverted. */ - error FailedInnerCall(); + error FailedCall(); /** * @dev The deployment failed. diff --git a/test/finance/VestingWallet.behavior.js b/test/finance/VestingWallet.behavior.js index 819a47ff635..b45ffeecfd3 100644 --- a/test/finance/VestingWallet.behavior.js +++ b/test/finance/VestingWallet.behavior.js @@ -12,7 +12,7 @@ async function envSetup(mock, beneficiary, token) { const beneficiaryMock = await ethers.deployContract('EtherReceiverMock'); await beneficiaryMock.setAcceptEther(false); await mock.connect(beneficiary).transferOwnership(beneficiaryMock); - return { args: [], error: [mock, 'FailedInnerCall'] }; + return { args: [], error: [mock, 'FailedCall'] }; }, releasedEvent: 'EtherReleased', args: [], diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index e097ef0eff4..3e48ccfee7f 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -445,7 +445,7 @@ describe('Governor', function () { await this.helper.waitForSnapshot(); await this.helper.connect(this.voter1).vote({ support: VoteType.For }); await this.helper.waitForDeadline(); - await expect(this.helper.execute()).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + await expect(this.helper.execute()).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('if receiver revert with reason', async function () { diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index f7ba96c8215..a62cbd4440b 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -893,7 +893,7 @@ describe('TimelockController', function () { operation.predecessor, operation.salt, ), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); }); }); @@ -1099,7 +1099,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('call throw', async function () { @@ -1146,7 +1146,7 @@ describe('TimelockController', function () { .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt, { gasLimit: '100000', }), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('call payable with eth', async function () { @@ -1199,7 +1199,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); expect(await ethers.provider.getBalance(this.mock)).to.equal(0n); expect(await ethers.provider.getBalance(this.callreceivermock)).to.equal(0n); @@ -1227,7 +1227,7 @@ describe('TimelockController', function () { this.mock .connect(this.executor) .execute(operation.target, operation.value, operation.data, operation.predecessor, operation.salt), - ).to.be.revertedWithCustomError(this.mock, 'FailedInnerCall'); + ).to.be.revertedWithCustomError(this.mock, 'FailedCall'); expect(await ethers.provider.getBalance(this.mock)).to.equal(0n); expect(await ethers.provider.getBalance(this.callreceivermock)).to.equal(0n); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 8a2a09233dc..8653ad708cb 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -122,7 +122,7 @@ describe('ERC2771Forwarder', function () { data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason'), }); - await expect(this.forwarder.execute(request)).to.be.revertedWithCustomError(this.forwarder, 'FailedInnerCall'); + await expect(this.forwarder.execute(request)).to.be.revertedWithCustomError(this.forwarder, 'FailedCall'); }); }); diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index efab3de461a..944489feff7 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -74,7 +74,7 @@ describe('Address', function () { await this.targetEther.setAcceptEther(false); await expect(this.mock.$sendValue(this.targetEther, funds)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -103,7 +103,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); @@ -118,7 +118,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call, { gasLimit: 120_000n })).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); @@ -134,7 +134,7 @@ describe('Address', function () { await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -207,7 +207,7 @@ describe('Address', function () { await expect(this.mock.$functionCallWithValue(this.target, call, value)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); }); @@ -225,7 +225,7 @@ describe('Address', function () { await expect(this.mock.$functionStaticCall(this.target, call)).to.be.revertedWithCustomError( this.mock, - 'FailedInnerCall', + 'FailedCall', ); }); From 27015a6c9c28825dfc06fa9171a57529a2f337b9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Mar 2024 16:51:19 +0100 Subject: [PATCH 13/17] fix lint --- test/utils/Address.test.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index 944489feff7..21775397ab2 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -101,10 +101,7 @@ describe('Address', function () { it('reverts when the called function reverts with no reason', async function () { const call = this.target.interface.encodeFunctionData('mockFunctionRevertsNoReason'); - await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( - this.mock, - 'FailedCall', - ); + await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); it('reverts when the called function reverts, bubbling up the revert reason', async function () { @@ -132,10 +129,7 @@ describe('Address', function () { const interface = new ethers.Interface(['function mockFunctionDoesNotExist()']); const call = interface.encodeFunctionData('mockFunctionDoesNotExist'); - await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( - this.mock, - 'FailedCall', - ); + await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError(this.mock, 'FailedCall'); }); }); From 6942c467c0b8a891fa112cc257d3730dc0e3937d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Mar 2024 16:55:19 +0100 Subject: [PATCH 14/17] re-fix lint --- test/utils/Address.test.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index 5a28cbf53d6..21775397ab2 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -101,14 +101,7 @@ describe('Address', function () { it('reverts when the called function reverts with no reason', async function () { const call = this.target.interface.encodeFunctionData('mockFunctionRevertsNoReason'); -<<<<<<< HEAD await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError(this.mock, 'FailedCall'); -======= - await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( - this.mock, - 'FailedCall', - ); ->>>>>>> e8eb6251e0707dc2fff4a7d6eaf9e53593d8c2e4 }); it('reverts when the called function reverts, bubbling up the revert reason', async function () { @@ -136,14 +129,7 @@ describe('Address', function () { const interface = new ethers.Interface(['function mockFunctionDoesNotExist()']); const call = interface.encodeFunctionData('mockFunctionDoesNotExist'); -<<<<<<< HEAD await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError(this.mock, 'FailedCall'); -======= - await expect(this.mock.$functionCall(this.target, call)).to.be.revertedWithCustomError( - this.mock, - 'FailedCall', - ); ->>>>>>> e8eb6251e0707dc2fff4a7d6eaf9e53593d8c2e4 }); }); From 524ccb6a8953cae7d862457a8f66eab8d53ae8c8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Mar 2024 17:03:02 +0100 Subject: [PATCH 15/17] fix merge conflict --- contracts/proxy/Clones.sol | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index dec90cf0186..d49c0ab1bf2 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -27,19 +27,11 @@ library Clones { } /** -<<<<<<< HEAD - * @dev Same as {xref-Clones-clone-address-}[clone], but with a `value` parameter to send native currency to the - * new contract. - * - * 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. -======= * @dev Same as {xref-Clones-clone-address-}[clone], but with a `value` parameter to send native currency * to the new contract. - * + * * 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. ->>>>>>> e8eb6251e0707dc2fff4a7d6eaf9e53593d8c2e4 */ function clone(address implementation, uint256 value) internal returns (address instance) { if (address(this).balance < value) { @@ -71,19 +63,11 @@ library Clones { } /** -<<<<<<< HEAD - * @dev Same as {xref-Clones-cloneDeterministic-address-bytes32-}[cloneDeterministic], but with a `value` parameter - * to send native currency to the new contract. - * - * 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. -======= * @dev Same as {xref-Clones-cloneDeterministic-address-bytes32-}[cloneDeterministic], but with * a `value` parameter to send native currency to the new contract. * * 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. ->>>>>>> e8eb6251e0707dc2fff4a7d6eaf9e53593d8c2e4 */ function cloneDeterministic( address implementation, From f02beb73605505c1b288a865288c01059d42117c Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 5 Mar 2024 11:08:56 -0600 Subject: [PATCH 16/17] Document error changes --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ffaa1b9bcf..147af99fe0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +### Custom error changes + +This version comes with changes to the custom error identifiers. Contracts previously depending on the following errors should be replaced accordingly: + +- Replace `Address.FailedInnerCall` with `Errors.FailedCall` +- Replace `Address.AddressInsufficientBalance` with `Errors.InsufficientBalance` +- Replace `Clones.Create2InsufficientBalance` with `Errors.InsufficientBalance` +- Replace `Clones.ERC1167FailedCreateClone` with `Errors.FailedDeployment` +- Replace `Clones.Create2FailedDeployment` with `Errors.FailedDeployment` ## 5.0.2 (2024-02-29) From 528ebdcb2f3b4165fb8969459c912bf567dec1c8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Mar 2024 17:24:21 +0100 Subject: [PATCH 17/17] fixing: how could that happen? good thing we have tests! --- contracts/proxy/Clones.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 1cfdc40494c..43532dcab30 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -45,7 +45,7 @@ library Clones { mstore(0x11, implementation) // Packs the first 3 bytes of the `implementation` address with the bytecode before the address. mstore(0x00, or(shr(0x88, implementation), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) - instance := create(0, 0x09, 0x37) + instance := create(value, 0x09, 0x37) } if (instance == address(0)) { revert Errors.FailedDeployment(); @@ -86,7 +86,7 @@ library Clones { mstore(0x11, implementation) // Packs the first 3 bytes of the `implementation` address with the bytecode before the address. mstore(0x00, or(shr(0x88, implementation), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) - instance := create2(0, 0x09, 0x37, salt) + instance := create2(value, 0x09, 0x37, salt) } if (instance == address(0)) { revert Errors.FailedDeployment();