From d78ae17d576ea9619202932110484145a5850ae7 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sun, 12 Nov 2023 16:29:01 +0000 Subject: [PATCH 01/26] Migrate 'arrays' --- test/helpers/random.js | 7 ++++ test/utils/Arrays.test.js | 80 ++++++++++++++++++++++++--------------- 2 files changed, 57 insertions(+), 30 deletions(-) create mode 100644 test/helpers/random.js diff --git a/test/helpers/random.js b/test/helpers/random.js new file mode 100644 index 00000000000..43477760443 --- /dev/null +++ b/test/helpers/random.js @@ -0,0 +1,7 @@ +const { ethers } = require("hardhat"); + +function randomHex(length) { + return `0x${Buffer.from(ethers.randomBytes(length)).toString('hex')}` +} + +module.exports = { randomHex } \ No newline at end of file diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index d939d59bdf2..38dec907b6e 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -1,88 +1,106 @@ -require('@openzeppelin/test-helpers'); - +const { ethers } = require('hardhat'); const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { randomHex } = require('../helpers/random'); + +const AddressArraysMock = 'AddressArraysMock'; +const Bytes32ArraysMock = 'Bytes32ArraysMock'; +const Uint256ArraysMock = 'Uint256ArraysMock'; -const AddressArraysMock = artifacts.require('AddressArraysMock'); -const Bytes32ArraysMock = artifacts.require('Bytes32ArraysMock'); -const Uint256ArraysMock = artifacts.require('Uint256ArraysMock'); +async function fixture () {} -contract('Arrays', function () { +describe('Arrays', function () { describe('findUpperBound', function () { - context('Even number of elements', function () { + describe('Even number of elements', function () { const EVEN_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20]; + const fixture = async () => { + const arrays = await ethers.deployContract(Uint256ArraysMock, [EVEN_ELEMENTS_ARRAY]); + return { arrays }; + } + beforeEach(async function () { - this.arrays = await Uint256ArraysMock.new(EVEN_ELEMENTS_ARRAY); + Object.assign(this, await loadFixture(fixture)); }); it('returns correct index for the basic case', async function () { - expect(await this.arrays.findUpperBound(16)).to.be.bignumber.equal('5'); + expect(await this.arrays.findUpperBound(16)).to.be.equal('5'); }); it('returns 0 for the first element', async function () { - expect(await this.arrays.findUpperBound(11)).to.be.bignumber.equal('0'); + expect(await this.arrays.findUpperBound(11)).to.be.equal('0'); }); it('returns index of the last element', async function () { - expect(await this.arrays.findUpperBound(20)).to.be.bignumber.equal('9'); + expect(await this.arrays.findUpperBound(20)).to.be.equal('9'); }); it('returns first index after last element if searched value is over the upper boundary', async function () { - expect(await this.arrays.findUpperBound(32)).to.be.bignumber.equal('10'); + expect(await this.arrays.findUpperBound(32)).to.be.equal('10'); }); it('returns 0 for the element under the lower boundary', async function () { - expect(await this.arrays.findUpperBound(2)).to.be.bignumber.equal('0'); + expect(await this.arrays.findUpperBound(2)).to.be.equal('0'); }); }); - context('Odd number of elements', function () { + describe('Odd number of elements', function () { const ODD_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21]; + const fixture = async () => { + const arrays = await ethers.deployContract(Uint256ArraysMock, [ODD_ELEMENTS_ARRAY]); + return { arrays }; + } + beforeEach(async function () { - this.arrays = await Uint256ArraysMock.new(ODD_ELEMENTS_ARRAY); + Object.assign(this, await loadFixture(fixture)); }); it('returns correct index for the basic case', async function () { - expect(await this.arrays.findUpperBound(16)).to.be.bignumber.equal('5'); + expect(await this.arrays.findUpperBound(16)).to.be.equal('5'); }); it('returns 0 for the first element', async function () { - expect(await this.arrays.findUpperBound(11)).to.be.bignumber.equal('0'); + expect(await this.arrays.findUpperBound(11)).to.be.equal('0'); }); it('returns index of the last element', async function () { - expect(await this.arrays.findUpperBound(21)).to.be.bignumber.equal('10'); + expect(await this.arrays.findUpperBound(21)).to.be.equal('10'); }); it('returns first index after last element if searched value is over the upper boundary', async function () { - expect(await this.arrays.findUpperBound(32)).to.be.bignumber.equal('11'); + expect(await this.arrays.findUpperBound(32)).to.be.equal('11'); }); it('returns 0 for the element under the lower boundary', async function () { - expect(await this.arrays.findUpperBound(2)).to.be.bignumber.equal('0'); + expect(await this.arrays.findUpperBound(2)).to.be.equal('0'); }); }); - context('Array with gap', function () { + describe('Array with gap', function () { const WITH_GAP_ARRAY = [11, 12, 13, 14, 15, 20, 21, 22, 23, 24]; + const fixture = async () => { + const arrays = await ethers.deployContract(Uint256ArraysMock, [WITH_GAP_ARRAY]); + return { arrays }; + } + beforeEach(async function () { - this.arrays = await Uint256ArraysMock.new(WITH_GAP_ARRAY); + Object.assign(this, await loadFixture(fixture)); }); it('returns index of first element in next filled range', async function () { - expect(await this.arrays.findUpperBound(17)).to.be.bignumber.equal('5'); + expect(await this.arrays.findUpperBound(17)).to.be.equal('5'); }); }); context('Empty array', function () { beforeEach(async function () { - this.arrays = await Uint256ArraysMock.new([]); + this.arrays = await ethers.deployContract(Uint256ArraysMock, [[]]); }); it('always returns 0 for empty array', async function () { - expect(await this.arrays.findUpperBound(10)).to.be.bignumber.equal('0'); + expect(await this.arrays.findUpperBound(10)).to.be.equal('0'); }); }); }); @@ -94,28 +112,30 @@ contract('Arrays', function () { artifact: AddressArraysMock, elements: Array(10) .fill() - .map(() => web3.utils.randomHex(20)), + .map(() => randomHex(20)) }, { type: 'bytes32', artifact: Bytes32ArraysMock, elements: Array(10) .fill() - .map(() => web3.utils.randomHex(32)), + .map(() => randomHex(32)), }, { type: 'uint256', artifact: Uint256ArraysMock, elements: Array(10) .fill() - .map(() => web3.utils.randomHex(32)), + .map(() => randomHex(32)), }, ]) { it(type, async function () { - const contract = await artifact.new(elements); + const contract = await ethers.deployContract(artifact, [elements]); for (const i in elements) { - expect(await contract.unsafeAccess(i)).to.be.bignumber.equal(elements[i]); + expect(await contract.unsafeAccess(i)).to.be.equal( + type == 'address' ? ethers.getAddress(elements[i]) : elements[i] + ) } }); } From 52b3bd8d336f02b5c3ba89436089385ecbc091e3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 17 Nov 2023 16:43:27 +0100 Subject: [PATCH 02/26] fix findUpperBound and add findLowerBound --- contracts/mocks/ArraysMock.sol | 4 + contracts/utils/Arrays.sol | 35 ++- test/helpers/random.js | 7 - test/utils/Arrays.test.js | 282 ++++++++++++++----------- test/utils/structs/Checkpoints.test.js | 19 +- 5 files changed, 200 insertions(+), 147 deletions(-) delete mode 100644 test/helpers/random.js diff --git a/contracts/mocks/ArraysMock.sol b/contracts/mocks/ArraysMock.sol index a00def29cb6..182698656b5 100644 --- a/contracts/mocks/ArraysMock.sol +++ b/contracts/mocks/ArraysMock.sol @@ -17,6 +17,10 @@ contract Uint256ArraysMock { return _array.findUpperBound(element); } + function findLowerBound(uint256 element) external view returns (uint256) { + return _array.findLowerBound(element); + } + function unsafeAccess(uint256 pos) external view returns (uint256) { return _array.unsafeAccess(pos).value; } diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index aaab3ce592b..4ea76eed79d 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -34,18 +34,37 @@ library Arrays { // Note that mid will always be strictly less than high (i.e. it will be a valid array index) // because Math.average rounds towards zero (it does integer division with truncation). - if (unsafeAccess(array, mid).value > element) { - high = mid; + if (unsafeAccess(array, mid).value < element) { + // this cannot overflow because mid < high + unchecked { + low = mid + 1; + } } else { - low = mid + 1; + high = mid; } } - // At this point `low` is the exclusive upper bound. We will return the inclusive upper bound. - if (low > 0 && unsafeAccess(array, low - 1).value == element) { - return low - 1; - } else { - return low; + return low; + } + + /** + * @dev Searches a sorted `array` and returns the last index that contains + * a value smaller or equal to `element`. If no such index exists (i.e. all + * values in the array are strictly greater than `element`), the array length is + * returned. Time complexity O(log n). + * + * `array` is expected to be sorted in ascending order, and to contain no + * repeated elements. + */ + function findLowerBound(uint256[] storage array, uint256 element) internal view returns (uint256) { + // following math cannot overflow + unchecked { + uint256 length = array.length; + if (element == type(uint256).max) { + return length == 0 ? 0 : length - 1; + } + uint256 upperBoundForNext = findUpperBound(array, element + 1); + return upperBoundForNext == 0 ? length : upperBoundForNext - 1; } } diff --git a/test/helpers/random.js b/test/helpers/random.js deleted file mode 100644 index 43477760443..00000000000 --- a/test/helpers/random.js +++ /dev/null @@ -1,7 +0,0 @@ -const { ethers } = require("hardhat"); - -function randomHex(length) { - return `0x${Buffer.from(ethers.randomBytes(length)).toString('hex')}` -} - -module.exports = { randomHex } \ No newline at end of file diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 38dec907b6e..47374ec1a06 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -1,141 +1,181 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const { randomHex } = require('../helpers/random'); -const AddressArraysMock = 'AddressArraysMock'; -const Bytes32ArraysMock = 'Bytes32ArraysMock'; -const Uint256ArraysMock = 'Uint256ArraysMock'; +const findUpperBound = (array, value) => { + const i = array.findIndex(x => x >= value); + return i == -1 ? array.length : i; +}; -async function fixture () {} +const findLowerBound = (array, value) => { + const i = array.findLastIndex(x => x <= value); + return i == -1 ? array.length : i; +}; describe('Arrays', function () { describe('findUpperBound', function () { - describe('Even number of elements', function () { - const EVEN_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20]; - - const fixture = async () => { - const arrays = await ethers.deployContract(Uint256ArraysMock, [EVEN_ELEMENTS_ARRAY]); - return { arrays }; - } - - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); - - it('returns correct index for the basic case', async function () { - expect(await this.arrays.findUpperBound(16)).to.be.equal('5'); - }); - - it('returns 0 for the first element', async function () { - expect(await this.arrays.findUpperBound(11)).to.be.equal('0'); - }); - - it('returns index of the last element', async function () { - expect(await this.arrays.findUpperBound(20)).to.be.equal('9'); - }); - - it('returns first index after last element if searched value is over the upper boundary', async function () { - expect(await this.arrays.findUpperBound(32)).to.be.equal('10'); - }); - - it('returns 0 for the element under the lower boundary', async function () { - expect(await this.arrays.findUpperBound(2)).to.be.equal('0'); - }); - }); - - describe('Odd number of elements', function () { - const ODD_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21]; - - const fixture = async () => { - const arrays = await ethers.deployContract(Uint256ArraysMock, [ODD_ELEMENTS_ARRAY]); - return { arrays }; - } - - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); - - it('returns correct index for the basic case', async function () { - expect(await this.arrays.findUpperBound(16)).to.be.equal('5'); - }); - - it('returns 0 for the first element', async function () { - expect(await this.arrays.findUpperBound(11)).to.be.equal('0'); - }); - - it('returns index of the last element', async function () { - expect(await this.arrays.findUpperBound(21)).to.be.equal('10'); - }); - - it('returns first index after last element if searched value is over the upper boundary', async function () { - expect(await this.arrays.findUpperBound(32)).to.be.equal('11'); - }); - - it('returns 0 for the element under the lower boundary', async function () { - expect(await this.arrays.findUpperBound(2)).to.be.equal('0'); - }); - }); - - describe('Array with gap', function () { - const WITH_GAP_ARRAY = [11, 12, 13, 14, 15, 20, 21, 22, 23, 24]; - - const fixture = async () => { - const arrays = await ethers.deployContract(Uint256ArraysMock, [WITH_GAP_ARRAY]); - return { arrays }; - } - - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); - - it('returns index of first element in next filled range', async function () { - expect(await this.arrays.findUpperBound(17)).to.be.equal('5'); - }); - }); - - context('Empty array', function () { - beforeEach(async function () { - this.arrays = await ethers.deployContract(Uint256ArraysMock, [[]]); + for (const [title, { array, tests }] of Object.entries({ + 'Even number of elements': { + array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n], + tests: { + 'returns correct index for the basic case': 16n, + 'returns 0 for the first element': 11n, + 'returns index of the last element': 20n, + 'returns first index after last element if searched value is over the upper boundary': 32n, + 'returns 0 for the element under the lower boundary': 2n, + }, + }, + 'Odd number of elements': { + array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n, 21n], + tests: { + 'returns correct index for the basic case': 16n, + 'returns 0 for the first element': 11n, + 'returns index of the last element': 20n, + 'returns first index after last element if searched value is over the upper boundary': 32n, + 'returns 0 for the element under the lower boundary': 2n, + }, + }, + 'Array with gap': { + array: [11n, 12n, 13n, 14n, 15n, 20n, 21n, 22n, 23n, 24n], + tests: { + 'returns index of first element in next filled range': 17n, + }, + }, + 'Array with duplicated elements': { + array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], + tests: { + 'returns index of first occurence': 10n, + }, + }, + 'Array with duplicated first element': { + array: [10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], + tests: { + 'returns index of first occurence': 10n, + }, + }, + 'Array with duplicated last element': { + array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n], + tests: { + 'returns index of first occurence': 10n, + }, + }, + 'Empty array': { + array: [], + tests: { + 'always returns 0 for empty array': 10n, + }, + }, + })) { + describe(title, function () { + const fixture = async () => { + return { mock: await ethers.deployContract('Uint256ArraysMock', [array]) }; + }; + + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + for (const [name, input] of Object.entries(tests)) { + it(name, async function () { + expect(await this.mock.findUpperBound(input)).to.be.equal(findUpperBound(array, input)); + }); + } }); + } + }); - it('always returns 0 for empty array', async function () { - expect(await this.arrays.findUpperBound(10)).to.be.equal('0'); + describe('findLowerBound', function () { + for (const [title, { array, tests }] of Object.entries({ + 'Even number of elements': { + array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n], + tests: { + 'returns correct index for the basic case': 16n, + 'returns 0 for the first element': 11n, + 'returns index of the last element': 20n, + 'returns index of last element if searched value is over the upper boundary': 32n, + 'returns length for the element under the lower boundary': 2n, + }, + }, + 'Odd number of elements': { + array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n, 21n], + tests: { + 'returns correct index for the basic case': 16n, + 'returns 0 for the first element': 11n, + 'returns index of the last element': 20n, + 'returns index of last element if searched value is over the upper boundary': 32n, + 'returns length for the element under the lower boundary': 2n, + }, + }, + 'Array with gap': { + array: [11n, 12n, 13n, 14n, 15n, 20n, 21n, 22n, 23n, 24n], + tests: { + 'returns index of first element in next filled range': 17n, + }, + }, + 'Array with duplicated elements': { + array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], + tests: { + 'returns index of last occurence': 10n, + }, + }, + 'Array with duplicated first element': { + array: [10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], + tests: { + 'returns index of last occurence': 10n, + }, + }, + 'Array with duplicated last element': { + array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n], + tests: { + 'returns index of last occurence': 10n, + }, + }, + 'Empty array': { + array: [], + tests: { + 'always returns 0 for empty array': 10n, + }, + }, + })) { + describe(title, function () { + const fixture = async () => { + return { mock: await ethers.deployContract('Uint256ArraysMock', [array]) }; + }; + + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + for (const [name, input] of Object.entries(tests)) { + it(name, async function () { + expect(await this.mock.findLowerBound(input)).to.be.equal(findLowerBound(array, input)); + }); + } }); - }); + } }); describe('unsafeAccess', function () { - for (const { type, artifact, elements } of [ - { - type: 'address', - artifact: AddressArraysMock, - elements: Array(10) - .fill() - .map(() => randomHex(20)) - }, - { - type: 'bytes32', - artifact: Bytes32ArraysMock, - elements: Array(10) - .fill() - .map(() => randomHex(32)), - }, - { - type: 'uint256', - artifact: Uint256ArraysMock, - elements: Array(10) - .fill() - .map(() => randomHex(32)), - }, - ]) { - it(type, async function () { + for (const [name, { artifact, generator }] of Object.entries({ + address: { + artifact: 'AddressArraysMock', + generator: () => ethers.getAddress(ethers.hexlify(ethers.randomBytes(20))), + }, + bytes32: { + artifact: 'Bytes32ArraysMock', + generator: () => ethers.hexlify(ethers.randomBytes(32)), + }, + uint256: { + artifact: 'Uint256ArraysMock', + generator: () => ethers.toBigInt(ethers.randomBytes(32)), + }, + })) { + it(name, async function () { + const elements = Array(10).fill().map(generator); const contract = await ethers.deployContract(artifact, [elements]); for (const i in elements) { - expect(await contract.unsafeAccess(i)).to.be.equal( - type == 'address' ? ethers.getAddress(elements[i]) : elements[i] - ) + expect(await contract.unsafeAccess(i)).to.be.equal(elements[i]); } }); } diff --git a/test/utils/structs/Checkpoints.test.js b/test/utils/structs/Checkpoints.test.js index 936ac565af6..cf547aaece9 100644 --- a/test/utils/structs/Checkpoints.test.js +++ b/test/utils/structs/Checkpoints.test.js @@ -11,9 +11,6 @@ const $Checkpoints = artifacts.require('$Checkpoints'); // The library name may be 'Checkpoints' or 'CheckpointsUpgradeable' const libraryName = $Checkpoints._json.contractName.replace(/^\$/, ''); -const first = array => (array.length ? array[0] : undefined); -const last = array => (array.length ? array[array.length - 1] : undefined); - contract('Checkpoints', function () { beforeEach(async function () { this.mock = await $Checkpoints.new(); @@ -85,17 +82,17 @@ contract('Checkpoints', function () { }); it('returns latest value', async function () { - expect(await this.methods.latest()).to.be.bignumber.equal(last(this.checkpoints).value); + expect(await this.methods.latest()).to.be.bignumber.equal(this.checkpoints.at(-1).value); const ckpt = await this.methods.latestCheckpoint(); expect(ckpt[0]).to.be.equal(true); - expect(ckpt[1]).to.be.bignumber.equal(last(this.checkpoints).key); - expect(ckpt[2]).to.be.bignumber.equal(last(this.checkpoints).value); + expect(ckpt[1]).to.be.bignumber.equal(this.checkpoints.at(-1).key); + expect(ckpt[2]).to.be.bignumber.equal(this.checkpoints.at(-1).value); }); it('cannot push values in the past', async function () { await expectRevertCustomError( - this.methods.push(last(this.checkpoints).key - 1, '0'), + this.methods.push(this.checkpoints.at(-1).key - 1, '0'), 'CheckpointUnorderedInsertion', [], ); @@ -108,7 +105,7 @@ contract('Checkpoints', function () { expect(await this.methods.length()).to.be.bignumber.equal(this.checkpoints.length.toString()); // update last key - await this.methods.push(last(this.checkpoints).key, newValue); + await this.methods.push(this.checkpoints.at(-1).key, newValue); expect(await this.methods.latest()).to.be.bignumber.equal(newValue); // check that length did not change @@ -117,7 +114,7 @@ contract('Checkpoints', function () { it('lower lookup', async function () { for (let i = 0; i < 14; ++i) { - const value = first(this.checkpoints.filter(x => i <= x.key))?.value || '0'; + const value = this.checkpoints.find(x => i <= x.key)?.value || '0'; expect(await this.methods.lowerLookup(i)).to.be.bignumber.equal(value); } @@ -125,7 +122,7 @@ contract('Checkpoints', function () { it('upper lookup & upperLookupRecent', async function () { for (let i = 0; i < 14; ++i) { - const value = last(this.checkpoints.filter(x => i >= x.key))?.value || '0'; + const value = this.checkpoints.findLast(x => i >= x.key)?.value || '0'; expect(await this.methods.upperLookup(i)).to.be.bignumber.equal(value); expect(await this.methods.upperLookupRecent(i)).to.be.bignumber.equal(value); @@ -147,7 +144,7 @@ contract('Checkpoints', function () { } for (let i = 0; i < 25; ++i) { - const value = last(allCheckpoints.filter(x => i >= x.key))?.value || '0'; + const value = allCheckpoints.findLast(x => i >= x.key)?.value || '0'; expect(await this.methods.upperLookup(i)).to.be.bignumber.equal(value); expect(await this.methods.upperLookupRecent(i)).to.be.bignumber.equal(value); } From 9a1411bda117f35cbea507d56f7e60ed6e469363 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 20 Nov 2023 17:12:18 +0100 Subject: [PATCH 03/26] add memory variants --- contracts/mocks/ArraysMock.sol | 20 ++++- contracts/utils/Arrays.sol | 154 ++++++++++++++++++++++++++++++--- test/utils/Arrays.test.js | 127 ++++++++------------------- 3 files changed, 194 insertions(+), 107 deletions(-) diff --git a/contracts/mocks/ArraysMock.sol b/contracts/mocks/ArraysMock.sol index 182698656b5..a2fbb6dea63 100644 --- a/contracts/mocks/ArraysMock.sol +++ b/contracts/mocks/ArraysMock.sol @@ -13,12 +13,24 @@ contract Uint256ArraysMock { _array = array; } - function findUpperBound(uint256 element) external view returns (uint256) { - return _array.findUpperBound(element); + function findUpperBound(uint256 value) external view returns (uint256) { + return _array.findUpperBound(value); } - function findLowerBound(uint256 element) external view returns (uint256) { - return _array.findLowerBound(element); + function lowerBound(uint256 value) external view returns (uint256) { + return _array.lowerBound(value); + } + + function upperBound(uint256 value) external view returns (uint256) { + return _array.upperBound(value); + } + + function lowerBoundMemory(uint256[] memory array, uint256 value) external pure returns (uint256) { + return array.lowerBoundMemory(value); + } + + function upperBoundMemory(uint256[] memory array, uint256 value) external pure returns (uint256) { + return array.upperBoundMemory(value); } function unsafeAccess(uint256 pos) external view returns (uint256) { diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 4ea76eed79d..f03b46d4801 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -20,6 +20,9 @@ library Arrays { * * `array` is expected to be sorted in ascending order, and to contain no * repeated elements. + * + * Deprecated in favor of `lowerBound` and `findUpperBound`. Note that this actually implements a lower bound + * search, and should be replaced with `lowerBound` */ function findUpperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -29,6 +32,44 @@ library Arrays { return 0; } + while (low < high) { + uint256 mid = Math.average(low, high); + + // Note that mid will always be strictly less than high (i.e. it will be a valid array index) + // because Math.average rounds towards zero (it does integer division with truncation). + if (unsafeAccess(array, mid).value > element) { + high = mid; + } else { + low = mid + 1; + } + } + + // At this point `low` is the exclusive upper bound. We will return the inclusive upper bound. + if (low > 0 && unsafeAccess(array, low - 1).value == element) { + return low - 1; + } else { + return low; + } + } + + /** + * @dev Searches a sorted `array` and returns the first index that contains + * a value greater or equal to `element`. If no such index exists (i.e. all + * values in the array are strictly less than `element`), the array length is + * returned. Time complexity O(log n). + * + * `array` is expected to be sorted in ascending order, + * + * See https://en.cppreference.com/w/cpp/algorithm/ranges/lower_bound + */ + function lowerBound(uint256[] storage array, uint256 element) internal view returns (uint256) { + uint256 low = 0; + uint256 high = array.length; + + if (high == 0) { + return 0; + } + while (low < high) { uint256 mid = Math.average(low, high); @@ -48,24 +89,111 @@ library Arrays { } /** - * @dev Searches a sorted `array` and returns the last index that contains - * a value smaller or equal to `element`. If no such index exists (i.e. all - * values in the array are strictly greater than `element`), the array length is + * @dev Searches a sorted `array` and returns the first index that contains + * a value strictly greater to `element`. If no such index exists (i.e. all + * values in the array are strictly less than `element`), the array length is * returned. Time complexity O(log n). * - * `array` is expected to be sorted in ascending order, and to contain no - * repeated elements. + * `array` is expected to be sorted in ascending order, + * + * See https://en.cppreference.com/w/cpp/algorithm/upper_bound */ - function findLowerBound(uint256[] storage array, uint256 element) internal view returns (uint256) { - // following math cannot overflow - unchecked { - uint256 length = array.length; - if (element == type(uint256).max) { - return length == 0 ? 0 : length - 1; + function upperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { + uint256 low = 0; + uint256 high = array.length; + + if (high == 0) { + return 0; + } + + while (low < high) { + uint256 mid = Math.average(low, high); + + // Note that mid will always be strictly less than high (i.e. it will be a valid array index) + // because Math.average rounds towards zero (it does integer division with truncation). + if (unsafeAccess(array, mid).value > element) { + high = mid; + } else { + // this cannot overflow because mid < high + unchecked { + low = mid + 1; + } } - uint256 upperBoundForNext = findUpperBound(array, element + 1); - return upperBoundForNext == 0 ? length : upperBoundForNext - 1; } + + return low; + } + + /** + * @dev Searches a sorted `array` and returns the first index that contains + * a value greater or equal to `element`. If no such index exists (i.e. all + * values in the array are strictly less than `element`), the array length is + * returned. Time complexity O(log n). + * + * `array` is expected to be sorted in ascending order, + * + * See https://en.cppreference.com/w/cpp/algorithm/ranges/lower_bound + */ + function lowerBoundMemory(uint256[] memory array, uint256 element) internal pure returns (uint256) { + uint256 low = 0; + uint256 high = array.length; + + if (high == 0) { + return 0; + } + + while (low < high) { + uint256 mid = Math.average(low, high); + + // Note that mid will always be strictly less than high (i.e. it will be a valid array index) + // because Math.average rounds towards zero (it does integer division with truncation). + if (unsafeMemoryAccess(array, mid) < element) { + // this cannot overflow because mid < high + unchecked { + low = mid + 1; + } + } else { + high = mid; + } + } + + return low; + } + + /** + * @dev Searches a sorted `array` and returns the first index that contains + * a value strictly greater to `element`. If no such index exists (i.e. all + * values in the array are strictly less than `element`), the array length is + * returned. Time complexity O(log n). + * + * `array` is expected to be sorted in ascending order, + * + * See https://en.cppreference.com/w/cpp/algorithm/upper_bound + */ + function upperBoundMemory(uint256[] memory array, uint256 element) internal pure returns (uint256) { + uint256 low = 0; + uint256 high = array.length; + + if (high == 0) { + return 0; + } + + while (low < high) { + uint256 mid = Math.average(low, high); + + // Note that mid will always be strictly less than high (i.e. it will be a valid array index) + // because Math.average rounds towards zero (it does integer division with truncation). + if (unsafeMemoryAccess(array, mid) > element) { + high = mid; + } else { + // this cannot overflow because mid < high + unchecked { + low = mid + 1; + } + } + } + + return low; } /** diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 47374ec1a06..75be9926b77 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -2,61 +2,65 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const findUpperBound = (array, value) => { - const i = array.findIndex(x => x >= value); +// See https://en.cppreference.com/w/cpp/algorithm/ranges/lower_bound +const lowerBound = (array, value) => { + const i = array.findIndex(element => value <= element); return i == -1 ? array.length : i; }; -const findLowerBound = (array, value) => { - const i = array.findLastIndex(x => x <= value); +// See https://en.cppreference.com/w/cpp/algorithm/upper_bound +const upperBound = (array, value) => { + const i = array.findIndex(element => value < element); return i == -1 ? array.length : i; }; +const hasDuplicates = array => array.some((v, i) => array.indexOf(v) != i); + describe('Arrays', function () { - describe('findUpperBound', function () { + describe('search', function () { for (const [title, { array, tests }] of Object.entries({ 'Even number of elements': { array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n], tests: { - 'returns correct index for the basic case': 16n, - 'returns 0 for the first element': 11n, - 'returns index of the last element': 20n, - 'returns first index after last element if searched value is over the upper boundary': 32n, - 'returns 0 for the element under the lower boundary': 2n, + 'basic case': 16n, + 'first element': 11n, + 'last element': 20n, + 'searched value is over the upper boundary': 32n, + 'searched value is under the lower boundary': 2n, }, }, 'Odd number of elements': { array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n, 21n], tests: { - 'returns correct index for the basic case': 16n, - 'returns 0 for the first element': 11n, - 'returns index of the last element': 20n, - 'returns first index after last element if searched value is over the upper boundary': 32n, - 'returns 0 for the element under the lower boundary': 2n, + 'basic case': 16n, + 'first element': 11n, + 'last element': 21n, + 'searched value is over the upper boundary': 32n, + 'searched value is under the lower boundary': 2n, }, }, 'Array with gap': { array: [11n, 12n, 13n, 14n, 15n, 20n, 21n, 22n, 23n, 24n], tests: { - 'returns index of first element in next filled range': 17n, + 'search value in gap': 17n, }, }, 'Array with duplicated elements': { array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], tests: { - 'returns index of first occurence': 10n, + 'search value is duplicated': 10n, }, }, 'Array with duplicated first element': { array: [10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], tests: { - 'returns index of first occurence': 10n, + 'search value is duplicated first element': 10n, }, }, 'Array with duplicated last element': { array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n], tests: { - 'returns index of first occurence': 10n, + 'search value is duplicated last element': 10n, }, }, 'Empty array': { @@ -76,79 +80,22 @@ describe('Arrays', function () { }); for (const [name, input] of Object.entries(tests)) { - it(name, async function () { - expect(await this.mock.findUpperBound(input)).to.be.equal(findUpperBound(array, input)); - }); - } - }); - } - }); + describe(name, function () { + it('[deprecated] findUpperBound', async function () { + // findUpperBound does not support duplicated + if (hasDuplicates(array)) this.skip(); + expect(await this.mock.findUpperBound(input)).to.be.equal(lowerBound(array, input)); + }); - describe('findLowerBound', function () { - for (const [title, { array, tests }] of Object.entries({ - 'Even number of elements': { - array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n], - tests: { - 'returns correct index for the basic case': 16n, - 'returns 0 for the first element': 11n, - 'returns index of the last element': 20n, - 'returns index of last element if searched value is over the upper boundary': 32n, - 'returns length for the element under the lower boundary': 2n, - }, - }, - 'Odd number of elements': { - array: [11n, 12n, 13n, 14n, 15n, 16n, 17n, 18n, 19n, 20n, 21n], - tests: { - 'returns correct index for the basic case': 16n, - 'returns 0 for the first element': 11n, - 'returns index of the last element': 20n, - 'returns index of last element if searched value is over the upper boundary': 32n, - 'returns length for the element under the lower boundary': 2n, - }, - }, - 'Array with gap': { - array: [11n, 12n, 13n, 14n, 15n, 20n, 21n, 22n, 23n, 24n], - tests: { - 'returns index of first element in next filled range': 17n, - }, - }, - 'Array with duplicated elements': { - array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], - tests: { - 'returns index of last occurence': 10n, - }, - }, - 'Array with duplicated first element': { - array: [10n, 10n, 10n, 10n, 10n, 10n, 10n, 20n], - tests: { - 'returns index of last occurence': 10n, - }, - }, - 'Array with duplicated last element': { - array: [0n, 10n, 10n, 10n, 10n, 10n, 10n, 10n], - tests: { - 'returns index of last occurence': 10n, - }, - }, - 'Empty array': { - array: [], - tests: { - 'always returns 0 for empty array': 10n, - }, - }, - })) { - describe(title, function () { - const fixture = async () => { - return { mock: await ethers.deployContract('Uint256ArraysMock', [array]) }; - }; - - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); + it('lowerBound', async function () { + expect(await this.mock.lowerBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.mock.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); + }); - for (const [name, input] of Object.entries(tests)) { - it(name, async function () { - expect(await this.mock.findLowerBound(input)).to.be.equal(findLowerBound(array, input)); + it('upperBound', async function () { + expect(await this.mock.upperBound(input)).to.be.equal(upperBound(array, input)); + expect(await this.mock.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); + }); }); } }); From a6ec616d379223814dc602b67bef2f73811ccdc1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Jan 2024 10:53:08 +0100 Subject: [PATCH 04/26] fix merge --- test/utils/Arrays.test.js | 2 +- test/utils/structs/Checkpoints.test.js | 53 ++------------------------ 2 files changed, 4 insertions(+), 51 deletions(-) diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 35173edb08f..7db0f478130 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -104,7 +104,7 @@ describe('Arrays', function () { } }); - describe.only('unsafeAccess', function () { + describe('unsafeAccess', function () { for (const [title, { artifact, elements }] of Object.entries({ address: { artifact: 'AddressArraysMock', elements: randomArray(generators.address, 10) }, bytes32: { artifact: 'Bytes32ArraysMock', elements: randomArray(generators.bytes32, 10) }, diff --git a/test/utils/structs/Checkpoints.test.js b/test/utils/structs/Checkpoints.test.js index 47ddb16649b..6df4b31fef3 100644 --- a/test/utils/structs/Checkpoints.test.js +++ b/test/utils/structs/Checkpoints.test.js @@ -4,22 +4,7 @@ const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { VALUE_SIZES } = require('../../../scripts/generate/templates/Checkpoints.opts'); -<<<<<<< HEAD -const $Checkpoints = artifacts.require('$Checkpoints'); - -// The library name may be 'Checkpoints' or 'CheckpointsUpgradeable' -const libraryName = $Checkpoints._json.contractName.replace(/^\$/, ''); - -contract('Checkpoints', function () { - beforeEach(async function () { - this.mock = await $Checkpoints.new(); - }); - -======= -const last = array => (array.length ? array[array.length - 1] : undefined); - describe('Checkpoints', function () { ->>>>>>> master for (const length of VALUE_SIZES) { describe(`Trace${length}`, function () { const fixture = async () => { @@ -92,28 +77,14 @@ describe('Checkpoints', function () { }); it('returns latest value', async function () { -<<<<<<< HEAD - expect(await this.methods.latest()).to.be.bignumber.equal(this.checkpoints.at(-1).value); - - const ckpt = await this.methods.latestCheckpoint(); - expect(ckpt[0]).to.be.equal(true); - expect(ckpt[1]).to.be.bignumber.equal(this.checkpoints.at(-1).key); - expect(ckpt[2]).to.be.bignumber.equal(this.checkpoints.at(-1).value); - }); - - it('cannot push values in the past', async function () { - await expectRevertCustomError( - this.methods.push(this.checkpoints.at(-1).key - 1, '0'), -======= const latest = this.checkpoints.at(-1); expect(await this.methods.latest()).to.equal(latest.value); - expect(await this.methods.latestCheckpoint()).to.have.ordered.members([true, latest.key, latest.value]); + expect(await this.methods.latestCheckpoint()).to.deep.equal([ true, latest.key, latest.value ]); }); it('cannot push values in the past', async function () { await expect(this.methods.push(this.checkpoints.at(-1).key - 1n, 0n)).to.be.revertedWithCustomError( this.mock, ->>>>>>> master 'CheckpointUnorderedInsertion', ); }); @@ -126,11 +97,7 @@ describe('Checkpoints', function () { // update last key await this.methods.push(this.checkpoints.at(-1).key, newValue); -<<<<<<< HEAD - expect(await this.methods.latest()).to.be.bignumber.equal(newValue); -======= expect(await this.methods.latest()).to.equal(newValue); ->>>>>>> master // check that length did not change expect(await this.methods.length()).to.equal(this.checkpoints.length); @@ -138,11 +105,7 @@ describe('Checkpoints', function () { it('lower lookup', async function () { for (let i = 0; i < 14; ++i) { -<<<<<<< HEAD - const value = this.checkpoints.find(x => i <= x.key)?.value || '0'; -======= const value = this.checkpoints.find(x => i <= x.key)?.value || 0n; ->>>>>>> master expect(await this.methods.lowerLookup(i)).to.equal(value); } @@ -150,11 +113,7 @@ describe('Checkpoints', function () { it('upper lookup & upperLookupRecent', async function () { for (let i = 0; i < 14; ++i) { -<<<<<<< HEAD - const value = this.checkpoints.findLast(x => i >= x.key)?.value || '0'; -======= - const value = last(this.checkpoints.filter(x => i >= x.key))?.value || 0n; ->>>>>>> master + const value = this.checkpoints.findLast(x => i >= x.key)?.value || 0n; expect(await this.methods.upperLookup(i)).to.equal(value); expect(await this.methods.upperLookupRecent(i)).to.equal(value); @@ -176,15 +135,9 @@ describe('Checkpoints', function () { } for (let i = 0; i < 25; ++i) { -<<<<<<< HEAD - const value = allCheckpoints.findLast(x => i >= x.key)?.value || '0'; - expect(await this.methods.upperLookup(i)).to.be.bignumber.equal(value); - expect(await this.methods.upperLookupRecent(i)).to.be.bignumber.equal(value); -======= - const value = last(allCheckpoints.filter(x => i >= x.key))?.value || 0n; + const value = allCheckpoints.findLast(x => i >= x.key)?.value || 0n; expect(await this.methods.upperLookup(i)).to.equal(value); expect(await this.methods.upperLookupRecent(i)).to.equal(value); ->>>>>>> master } }); }); From 23e8db94e39d59dcba76d39373b8d8c284a62b73 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Jan 2024 10:53:42 +0100 Subject: [PATCH 05/26] fix lint --- test/utils/structs/Checkpoints.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/structs/Checkpoints.test.js b/test/utils/structs/Checkpoints.test.js index 6df4b31fef3..fd22544b955 100644 --- a/test/utils/structs/Checkpoints.test.js +++ b/test/utils/structs/Checkpoints.test.js @@ -79,7 +79,7 @@ describe('Checkpoints', function () { it('returns latest value', async function () { const latest = this.checkpoints.at(-1); expect(await this.methods.latest()).to.equal(latest.value); - expect(await this.methods.latestCheckpoint()).to.deep.equal([ true, latest.key, latest.value ]); + expect(await this.methods.latestCheckpoint()).to.deep.equal([true, latest.key, latest.value]); }); it('cannot push values in the past', async function () { From c72591f2c62f9c8db6c37bc32cabb2abd5a61528 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Jan 2024 11:01:15 +0100 Subject: [PATCH 06/26] minimize change --- lib/forge-std | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/forge-std b/lib/forge-std index eb980e1d4f0..c2236853aad 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit eb980e1d4f0e8173ec27da77297ae411840c8ccb +Subproject commit c2236853aadb8e2d9909bbecdc490099519b70a4 From 9162e4289faf27696fc00c0fca3b3aca25f579d0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Jan 2024 11:04:55 +0100 Subject: [PATCH 07/26] add changeset --- .changeset/flat-turtles-repeat.md | 5 +++++ .changeset/thick-pumpkins-report.md | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/flat-turtles-repeat.md create mode 100644 .changeset/thick-pumpkins-report.md diff --git a/.changeset/flat-turtles-repeat.md b/.changeset/flat-turtles-repeat.md new file mode 100644 index 00000000000..6b627201ac9 --- /dev/null +++ b/.changeset/flat-turtles-repeat.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Arrays`: deprecate `findUpperBound` in favor of the new `lowerBound`. diff --git a/.changeset/thick-pumpkins-report.md b/.changeset/thick-pumpkins-report.md new file mode 100644 index 00000000000..c965d2d7770 --- /dev/null +++ b/.changeset/thick-pumpkins-report.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Arrays`: add new functions `lowerBound`, `upperBound`, lowerBoundMemory`and`upperBoundMemory` for lookups in sorted arrays with potential duplicates. From ed1de5bfc1f10651237d028af1fb8ecc20401931 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Jan 2024 11:05:39 +0100 Subject: [PATCH 08/26] Apply suggestions from code review --- .changeset/thick-pumpkins-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/thick-pumpkins-report.md b/.changeset/thick-pumpkins-report.md index c965d2d7770..f17a208950c 100644 --- a/.changeset/thick-pumpkins-report.md +++ b/.changeset/thick-pumpkins-report.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Arrays`: add new functions `lowerBound`, `upperBound`, lowerBoundMemory`and`upperBoundMemory` for lookups in sorted arrays with potential duplicates. +`Arrays`: add new functions `lowerBound`, `upperBound`, `lowerBoundMemory` and `upperBoundMemory` for lookups in sorted arrays with potential duplicates. From 4c1c7f448145814dcc7b0fff258052127f16acae Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 00:11:17 +0100 Subject: [PATCH 09/26] add Arrays.sort --- .changeset/dirty-cobras-smile.md | 5 ++++ contracts/utils/Arrays.sol | 43 ++++++++++++++++++++++++++++++++ test/utils/Arrays.test.js | 40 +++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 .changeset/dirty-cobras-smile.md diff --git a/.changeset/dirty-cobras-smile.md b/.changeset/dirty-cobras-smile.md new file mode 100644 index 00000000000..f03e936b5d2 --- /dev/null +++ b/.changeset/dirty-cobras-smile.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Arrays`: add a `sort` function. diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index f03b46d4801..a466c611739 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -12,6 +12,49 @@ import {Math} from "./math/Math.sol"; library Arrays { using StorageSlot for bytes32; + /** + * @dev Sort an array (in memory) in increassing order. + * + * This function does the sorting "in place", meaning that it overrides the input. The object is returned for + * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. + * + * Note: this function's cost is O(n.log(n)) in average and O(n²) in the worst case, with n the length of the + * array. Using it in view function that are executed through `eth_call` is safe, but one should be very carefull + * when executing this as part of a transaction. Depending on the size of the array, this can be very exepensive + * to run, leading to potential DoS attacks. + */ + function sort(uint256[] memory array) internal pure returns (uint256[] memory) { + _quickSort(array, 0, array.length); + return array; + } + + function _quickSort(uint256[] memory array, uint256 i, uint256 j) private pure { + unchecked { + if (j - i < 2) return; + + uint256 p = i; + for (uint256 k = i + 1; k < j; ++k) { + if (unsafeMemoryAccess(array, i) > unsafeMemoryAccess(array, k)) { + _swap(array, ++p, k); + } + } + _swap(array, i, p); + _quickSort(array, i, p); + _quickSort(array, p + 1, j); + } + } + + function _swap(uint256[] memory arr, uint256 i, uint256 j) private pure { + assembly { + let pos_i := add(add(arr, 0x20), mul(i, 0x20)) + let pos_j := add(add(arr, 0x20), mul(j, 0x20)) + let val_i := mload(pos_i) + let val_j := mload(pos_j) + mstore(pos_i, val_j) + mstore(pos_j, val_i) + } + } + /** * @dev Searches a sorted `array` and returns the first index that contains * a value greater or equal to `element`. If no such index exists (i.e. all diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 7db0f478130..44e35244394 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -16,9 +16,49 @@ const upperBound = (array, value) => { return i == -1 ? array.length : i; }; +// By default, js "sort" cast to string and then sort in alphabetical order. Use this to sort numbers. +const compareNumbers = (a, b) => a > b ? 1 : a < b ? -1 : 0; + const hasDuplicates = array => array.some((v, i) => array.indexOf(v) != i); describe('Arrays', function () { + describe('sort', function () { + const fixture = async () => { + return { mock: await ethers.deployContract('$Arrays') }; + }; + + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + for (const length of [0, 1, 2, 8, 32, 128]) { + it(`sort array of length ${length}`, async function () { + this.elements = randomArray(generators.uint256, length); + this.expected = Array.from(this.elements).sort(compareNumbers); + }); + + if (length > 1) { + it(`sort array of length ${length} (identical elements)`, async function () { + this.elements = Array(length).fill(generators.uint256()); + this.expected = this.elements; + }); + + it(`sort array of length ${length} (already sorted)`, async function () { + this.elements = randomArray(generators.uint256, length).sort(compareNumbers); + this.expected = this.elements; + }); + + it(`sort array of length ${length} (sorted in reverse order)`, async function () { + this.elements = randomArray(generators.uint256, length).sort(compareNumbers).reverse(); + this.expected = Array.from(this.elements).reverse(); + }); + } + } + afterEach(async function () { + expect(await this.mock.$sort(this.elements)).to.deep.equal(this.expected); + }); + }); + describe('search', function () { for (const [title, { array, tests }] of Object.entries({ 'Even number of elements': { From abd07b694fbf6a595ce411dcf43af0b6f8795e47 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 00:14:45 +0100 Subject: [PATCH 10/26] add sort test --- test/utils/Arrays.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 44e35244394..b49b632fea1 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -52,6 +52,13 @@ describe('Arrays', function () { this.elements = randomArray(generators.uint256, length).sort(compareNumbers).reverse(); this.expected = Array.from(this.elements).reverse(); }); + + it(`sort array of length ${length} (almost sorted)`, async function () { + this.elements = randomArray(generators.uint256, length).sort(compareNumbers); + this.expected = Array.from(this.elements); + // rotate (move the last element to the front) for an almost sorted effect + this.elements.unshift(this.elements.pop()); + }); } } afterEach(async function () { From 1e89815909506aaa0feada12bc856d516c2c9e91 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 00:19:54 +0100 Subject: [PATCH 11/26] fix lint --- test/utils/Arrays.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index b49b632fea1..0aea52a205b 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -17,7 +17,7 @@ const upperBound = (array, value) => { }; // By default, js "sort" cast to string and then sort in alphabetical order. Use this to sort numbers. -const compareNumbers = (a, b) => a > b ? 1 : a < b ? -1 : 0; +const compareNumbers = (a, b) => (a > b ? 1 : a < b ? -1 : 0); const hasDuplicates = array => array.some((v, i) => array.indexOf(v) != i); From b73989d61e2d80b8f3ddaeedc99336f49817ce9a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 10:53:53 +0100 Subject: [PATCH 12/26] codespell --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index a466c611739..9aff12aea34 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -19,7 +19,7 @@ library Arrays { * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. * * Note: this function's cost is O(n.log(n)) in average and O(n²) in the worst case, with n the length of the - * array. Using it in view function that are executed through `eth_call` is safe, but one should be very carefull + * array. Using it in view function that are executed through `eth_call` is safe, but one should be very careful * when executing this as part of a transaction. Depending on the size of the array, this can be very exepensive * to run, leading to potential DoS attacks. */ From 79bf3679066f302958f2ef3b422d2e6cdcba5f9e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 13:40:39 +0100 Subject: [PATCH 13/26] add fuzzing tests for Arrays.sort --- scripts/generate/templates/Checkpoints.t.js | 4 ++-- test/utils/Arrays.t.sol | 15 +++++++++++++++ test/utils/Base64.t.sol | 1 - test/utils/math/Math.t.sol | 1 - test/utils/structs/Checkpoints.t.sol | 4 ++-- 5 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 test/utils/Arrays.t.sol diff --git a/scripts/generate/templates/Checkpoints.t.js b/scripts/generate/templates/Checkpoints.t.js index d21beb53e8e..79ab70e5b29 100644 --- a/scripts/generate/templates/Checkpoints.t.js +++ b/scripts/generate/templates/Checkpoints.t.js @@ -7,8 +7,8 @@ const header = `\ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {SafeCast} from "../../../contracts/utils/math/SafeCast.sol"; -import {Checkpoints} from "../../../contracts/utils/structs/Checkpoints.sol"; +import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; +import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol"; `; /* eslint-disable max-len */ diff --git a/test/utils/Arrays.t.sol b/test/utils/Arrays.t.sol new file mode 100644 index 00000000000..c3d147562ce --- /dev/null +++ b/test/utils/Arrays.t.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Arrays} from "@openzeppelin/contracts/utils/Arrays.sol"; + +contract ArraysTest is Test { + function testSort(uint256[] memory values) public { + Arrays.sort(values); + for (uint256 i = 1; i < values.length; ++i) { + assertLe(values[i - 1], values[i]); + } + } +} diff --git a/test/utils/Base64.t.sol b/test/utils/Base64.t.sol index 80e4f49df9c..22c859b4b42 100644 --- a/test/utils/Base64.t.sol +++ b/test/utils/Base64.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; - import {Base64} from "@openzeppelin/contracts/utils/Base64.sol"; /// NOTE: This test requires `ffi` to be enabled. It does not run in the CI diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index a757833796f..b6553932ef4 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; - import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; contract MathTest is Test { diff --git a/test/utils/structs/Checkpoints.t.sol b/test/utils/structs/Checkpoints.t.sol index 7bdbcfddfbc..704d79eee0a 100644 --- a/test/utils/structs/Checkpoints.t.sol +++ b/test/utils/structs/Checkpoints.t.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {SafeCast} from "../../../contracts/utils/math/SafeCast.sol"; -import {Checkpoints} from "../../../contracts/utils/structs/Checkpoints.sol"; +import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; +import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol"; contract CheckpointsTrace224Test is Test { using Checkpoints for Checkpoints.Trace224; From f2d49ef839f1c0a149ba81b49d3f3b99274a712a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 16:37:51 +0100 Subject: [PATCH 14/26] add unsafeMemoryAccess tests --- contracts/utils/Arrays.sol | 15 ++++++-- test/utils/Arrays.test.js | 70 +++++++++++++++++++++++--------------- 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 9aff12aea34..93628af6dfd 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -298,7 +298,7 @@ library Arrays { * * WARNING: Only use if you are certain `pos` is lower than the array length. */ - function unsafeMemoryAccess(uint256[] memory arr, uint256 pos) internal pure returns (uint256 res) { + function unsafeMemoryAccess(address[] memory arr, uint256 pos) internal pure returns (address res) { assembly { res := mload(add(add(arr, 0x20), mul(pos, 0x20))) } @@ -309,7 +309,18 @@ library Arrays { * * WARNING: Only use if you are certain `pos` is lower than the array length. */ - function unsafeMemoryAccess(address[] memory arr, uint256 pos) internal pure returns (address res) { + function unsafeMemoryAccess(bytes32[] memory arr, uint256 pos) internal pure returns (bytes32 res) { + assembly { + res := mload(add(add(arr, 0x20), mul(pos, 0x20))) + } + } + + /** + * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. + * + * WARNING: Only use if you are certain `pos` is lower than the array length. + */ + function unsafeMemoryAccess(uint256[] memory arr, uint256 pos) internal pure returns (uint256 res) { assembly { res := mload(add(add(arr, 0x20), mul(pos, 0x20))) } diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 0aea52a205b..ad520a30be5 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -22,15 +22,15 @@ const compareNumbers = (a, b) => (a > b ? 1 : a < b ? -1 : 0); const hasDuplicates = array => array.some((v, i) => array.indexOf(v) != i); describe('Arrays', function () { - describe('sort', function () { - const fixture = async () => { - return { mock: await ethers.deployContract('$Arrays') }; - }; + const fixture = async () => { + return { mock: await ethers.deployContract('$Arrays') }; + }; - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + describe('sort', function () { for (const length of [0, 1, 2, 8, 32, 128]) { it(`sort array of length ${length}`, async function () { this.elements = randomArray(generators.uint256, length); @@ -121,7 +121,7 @@ describe('Arrays', function () { })) { describe(title, function () { const fixture = async () => { - return { mock: await ethers.deployContract('Uint256ArraysMock', [array]) }; + return { instance: await ethers.deployContract('Uint256ArraysMock', [array]) }; }; beforeEach(async function () { @@ -133,17 +133,17 @@ describe('Arrays', function () { it('[deprecated] findUpperBound', async function () { // findUpperBound does not support duplicated if (hasDuplicates(array)) this.skip(); - expect(await this.mock.findUpperBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.findUpperBound(input)).to.be.equal(lowerBound(array, input)); }); it('lowerBound', async function () { - expect(await this.mock.lowerBound(input)).to.be.equal(lowerBound(array, input)); - expect(await this.mock.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.lowerBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); }); it('upperBound', async function () { - expect(await this.mock.upperBound(input)).to.be.equal(upperBound(array, input)); - expect(await this.mock.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); + expect(await this.instance.upperBound(input)).to.be.equal(upperBound(array, input)); + expect(await this.instance.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); }); }); } @@ -152,28 +152,44 @@ describe('Arrays', function () { }); describe('unsafeAccess', function () { - for (const [title, { artifact, elements }] of Object.entries({ + for (const [type, { artifact, elements }] of Object.entries({ address: { artifact: 'AddressArraysMock', elements: randomArray(generators.address, 10) }, bytes32: { artifact: 'Bytes32ArraysMock', elements: randomArray(generators.bytes32, 10) }, uint256: { artifact: 'Uint256ArraysMock', elements: randomArray(generators.uint256, 10) }, })) { - describe(title, function () { - const fixture = async () => { - return { mock: await ethers.deployContract(artifact, [elements]) }; - }; + describe(type, function () { + describe('storage', function () { + const fixture = async () => { + return { instance: await ethers.deployContract(artifact, [elements]) }; + }; + + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); - beforeEach(async function () { - Object.assign(this, await loadFixture(fixture)); - }); + for (const i in elements) { + it(`unsafeAccess within bounds #${i}`, async function () { + expect(await this.instance.unsafeAccess(i)).to.equal(elements[i]); + }); + } - for (const i in elements) { - it(`unsafeAccess within bounds #${i}`, async function () { - expect(await this.mock.unsafeAccess(i)).to.equal(elements[i]); + it('unsafeAccess outside bounds', async function () { + await expect(this.instance.unsafeAccess(elements.length)).to.not.be.rejected; }); - } + }); - it('unsafeAccess outside bounds', async function () { - await expect(this.mock.unsafeAccess(elements.length)).to.not.be.rejected; + describe('memory', function () { + const fragment = `$unsafeMemoryAccess(${type}[] arr, uint256 pos)`; + + for (const i in elements) { + it(`unsafeMemoryAccess within bounds #${i}`, async function () { + expect(await this.mock[fragment](elements, i)).to.equal(elements[i]); + }); + } + + it('unsafeMemoryAccess outside bounds', async function () { + await expect(this.mock[fragment](elements, elements.length)).to.not.be.rejected; + }); }); }); } From c75de32ed21ce9a7c251475cca320ccf4e23812a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 29 Jan 2024 21:58:13 +0100 Subject: [PATCH 15/26] fix lint --- test/utils/Arrays.test.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index e1c68d703df..528d00f3018 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -133,20 +133,20 @@ describe('Arrays', function () { it('[deprecated] findUpperBound', async function () { // findUpperBound does not support duplicated if (hasDuplicates(array)) { - expect(await this.mock.findUpperBound(input)).to.be.equal(upperBound(array, input) - 1); + expect(await this.instance.findUpperBound(input)).to.be.equal(upperBound(array, input) - 1); } else { - expect(await this.mock.findUpperBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.findUpperBound(input)).to.be.equal(lowerBound(array, input)); } }); it('lowerBound', async function () { - expect(await this.mock.lowerBound(input)).to.be.equal(lowerBound(array, input)); - expect(await this.mock.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.lowerBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); }); it('upperBound', async function () { - expect(await this.mock.upperBound(input)).to.be.equal(upperBound(array, input)); - expect(await this.mock.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); + expect(await this.instance.upperBound(input)).to.be.equal(upperBound(array, input)); + expect(await this.instance.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); }); }); } @@ -155,12 +155,12 @@ describe('Arrays', function () { }); describe('unsafeAccess', function () { - for (const [title, { artifact, elements }] of Object.entries({ + for (const [type, { artifact, elements }] of Object.entries({ address: { artifact: 'AddressArraysMock', elements: randomArray(generators.address, 10) }, bytes32: { artifact: 'Bytes32ArraysMock', elements: randomArray(generators.bytes32, 10) }, uint256: { artifact: 'Uint256ArraysMock', elements: randomArray(generators.uint256, 10) }, })) { - describe(title, function () { + describe(type, function () { describe('storage', function () { const fixture = async () => { return { instance: await ethers.deployContract(artifact, [elements]) }; From f823bee40b6d78f4a3b02a81c766e8b8b9a84173 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jan 2024 11:54:00 +0100 Subject: [PATCH 16/26] lint --- test/utils/Arrays.test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 528d00f3018..ffe5d5a22fe 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -133,20 +133,20 @@ describe('Arrays', function () { it('[deprecated] findUpperBound', async function () { // findUpperBound does not support duplicated if (hasDuplicates(array)) { - expect(await this.instance.findUpperBound(input)).to.be.equal(upperBound(array, input) - 1); + expect(await this.instance.findUpperBound(input)).to.equal(upperBound(array, input) - 1); } else { - expect(await this.instance.findUpperBound(input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.findUpperBound(input)).to.equal(lowerBound(array, input)); } }); it('lowerBound', async function () { - expect(await this.instance.lowerBound(input)).to.be.equal(lowerBound(array, input)); - expect(await this.instance.lowerBoundMemory(array, input)).to.be.equal(lowerBound(array, input)); + expect(await this.instance.lowerBound(input)).to.equal(lowerBound(array, input)); + expect(await this.instance.lowerBoundMemory(array, input)).to.equal(lowerBound(array, input)); }); it('upperBound', async function () { - expect(await this.instance.upperBound(input)).to.be.equal(upperBound(array, input)); - expect(await this.instance.upperBoundMemory(array, input)).to.be.equal(upperBound(array, input)); + expect(await this.instance.upperBound(input)).to.equal(upperBound(array, input)); + expect(await this.instance.upperBoundMemory(array, input)).to.equal(upperBound(array, input)); }); }); } From c90f12b353c942e14a1ad8a9f2c5ef3e088025ae Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Feb 2024 19:01:46 +0100 Subject: [PATCH 17/26] Update contracts/utils/Arrays.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 6f12a1893b8..641a8bf4e5b 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -13,7 +13,7 @@ library Arrays { using StorageSlot for bytes32; /** - * @dev Sort an array (in memory) in increassing order. + * @dev Sort an array (in memory) in increasing order. * * This function does the sorting "in place", meaning that it overrides the input. The object is returned for * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. From 180a96973a65b895f5daea9722d15819cb96fcc0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Feb 2024 19:01:53 +0100 Subject: [PATCH 18/26] Update contracts/utils/Arrays.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 641a8bf4e5b..acee39f7517 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -18,7 +18,7 @@ library Arrays { * This function does the sorting "in place", meaning that it overrides the input. The object is returned for * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. * - * Note: this function's cost is O(n.log(n)) in average and O(n²) in the worst case, with n the length of the + * NOTE: this function's cost is O(n.log(n)) in average and O(n²) in the worst case, with n the length of the * array. Using it in view function that are executed through `eth_call` is safe, but one should be very careful * when executing this as part of a transaction. Depending on the size of the array, this can be very exepensive * to run, leading to potential DoS attacks. From 708972f1cb96946e7cf869bd3a62be2251f16979 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Feb 2024 19:02:27 +0100 Subject: [PATCH 19/26] Apply suggestions from code review --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index acee39f7517..d407cb55814 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -18,7 +18,7 @@ library Arrays { * This function does the sorting "in place", meaning that it overrides the input. The object is returned for * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. * - * NOTE: this function's cost is O(n.log(n)) in average and O(n²) in the worst case, with n the length of the + * NOTE: this function's cost is `O(n · log(n))` in average and `O(n²)` in the worst case, with n the length of the * array. Using it in view function that are executed through `eth_call` is safe, but one should be very careful * when executing this as part of a transaction. Depending on the size of the array, this can be very exepensive * to run, leading to potential DoS attacks. From 3f1f0a5b1fe92daadbc0a880aeefc29c4e450b2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 2 Feb 2024 17:04:07 -0600 Subject: [PATCH 20/26] Update contracts/utils/Arrays.sol Co-authored-by: Hadrien Croubois --- contracts/utils/Arrays.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index d407cb55814..b0291d4f77e 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -19,9 +19,9 @@ library Arrays { * convenience, but that returned value can be discarded safely if the caller has a memory pointer to the array. * * NOTE: this function's cost is `O(n · log(n))` in average and `O(n²)` in the worst case, with n the length of the - * array. Using it in view function that are executed through `eth_call` is safe, but one should be very careful - * when executing this as part of a transaction. Depending on the size of the array, this can be very exepensive - * to run, leading to potential DoS attacks. + * array. Using it in view functions that are executed through `eth_call` is safe, but one should be very careful + * when executing this as part of a transaction. If the array being sorted is too large, the sort operation may + * consume more gas than is available in a block, leading to potential DoS. */ function sort(uint256[] memory array) internal pure returns (uint256[] memory) { _quickSort(array, 0, array.length); From 5a0ad7f9048fe89ce805f8062c48382f0f2fc72f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 2 Feb 2024 18:56:41 -0600 Subject: [PATCH 21/26] Add comments to `_quickSort` --- contracts/utils/Arrays.sol | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index b0291d4f77e..f9e06b39388 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -28,26 +28,43 @@ library Arrays { return array; } + /** + * @dev Performs a quick sort on an array in memory. The array is sorted in increasing order. + * This private implementation assumes that `i <= j` and that `j < array.length`. + */ function _quickSort(uint256[] memory array, uint256 i, uint256 j) private pure { unchecked { + // Can't overflow given `i <= j` if (j - i < 2) return; - uint256 p = i; + // Use first element as pivot + // i = pivot index + uint256 index = i; + for (uint256 k = i + 1; k < j; ++k) { + // pivot > array[k] + // Unsafe access is safe given `j < array.length` and `k < j`. if (unsafeMemoryAccess(array, i) > unsafeMemoryAccess(array, k)) { - _swap(array, ++p, k); + _swap(array, ++index, k); } } - _swap(array, i, p); - _quickSort(array, i, p); - _quickSort(array, p + 1, j); + + // Swap pivot into place + _swap(array, i, index); + + _quickSort(array, i, index); // Sort the left side of the pivot + _quickSort(array, index + 1, j); // Sort the right side of the pivot } } + /** + * @dev Swaps the elements at positions `i` and `j` in the `arr` array. + */ function _swap(uint256[] memory arr, uint256 i, uint256 j) private pure { assembly { - let pos_i := add(add(arr, 0x20), mul(i, 0x20)) - let pos_j := add(add(arr, 0x20), mul(j, 0x20)) + let start := add(arr, 0x20) // Pointer to the first element of the array + let pos_i := add(start, mul(i, 0x20)) + let pos_j := add(start, mul(j, 0x20)) let val_i := mload(pos_i) let val_j := mload(pos_j) mstore(pos_i, val_j) From a8e6f54473124474a1e5424ac1e736de2e83445f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 2 Feb 2024 19:00:13 -0600 Subject: [PATCH 22/26] Lint --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index f9e06b39388..69bd4c03530 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -20,7 +20,7 @@ library Arrays { * * NOTE: this function's cost is `O(n · log(n))` in average and `O(n²)` in the worst case, with n the length of the * array. Using it in view functions that are executed through `eth_call` is safe, but one should be very careful - * when executing this as part of a transaction. If the array being sorted is too large, the sort operation may + * when executing this as part of a transaction. If the array being sorted is too large, the sort operation may * consume more gas than is available in a block, leading to potential DoS. */ function sort(uint256[] memory array) internal pure returns (uint256[] memory) { From 7600291f75d96d5d070572a4677020a5ca70a0e7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Feb 2024 10:20:36 +0100 Subject: [PATCH 23/26] Update contracts/utils/Arrays.sol --- contracts/utils/Arrays.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 69bd4c03530..a785ad23e4f 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -30,7 +30,8 @@ library Arrays { /** * @dev Performs a quick sort on an array in memory. The array is sorted in increasing order. - * This private implementation assumes that `i <= j` and that `j < array.length`. + * Invariant: `i <= j` and `j <= array.length`. This is the case when initially called by {sort} and is preserved + * in subcalls. */ function _quickSort(uint256[] memory array, uint256 i, uint256 j) private pure { unchecked { From 6f163d2f1f0d6612a3b395ce82f89e5561508901 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Feb 2024 10:23:16 +0100 Subject: [PATCH 24/26] Update contracts/utils/Arrays.sol --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index a785ad23e4f..9d9d0a05fc5 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -44,7 +44,7 @@ library Arrays { for (uint256 k = i + 1; k < j; ++k) { // pivot > array[k] - // Unsafe access is safe given `j < array.length` and `k < j`. + // Unsafe access is safe given `k < j <= array.length`. if (unsafeMemoryAccess(array, i) > unsafeMemoryAccess(array, k)) { _swap(array, ++index, k); } From 898306616d89c4b777379baf33b90fc392d83a07 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Feb 2024 10:27:36 +0100 Subject: [PATCH 25/26] cache the pivot and improve doc --- contracts/utils/Arrays.sol | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 9d9d0a05fc5..a08bd80492f 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -30,8 +30,9 @@ library Arrays { /** * @dev Performs a quick sort on an array in memory. The array is sorted in increasing order. - * Invariant: `i <= j` and `j <= array.length`. This is the case when initially called by {sort} and is preserved - * in subcalls. + * + * Invariant: `i <= j <= array.length`. This is the case when initially called by {sort} and is preserved in + * subcalls. */ function _quickSort(uint256[] memory array, uint256 i, uint256 j) private pure { unchecked { @@ -39,13 +40,14 @@ library Arrays { if (j - i < 2) return; // Use first element as pivot - // i = pivot index + uint256 pivot = unsafeMemoryAccess(array, i); + // Position where the pivot should be at the end of the loop uint256 index = i; for (uint256 k = i + 1; k < j; ++k) { - // pivot > array[k] // Unsafe access is safe given `k < j <= array.length`. - if (unsafeMemoryAccess(array, i) > unsafeMemoryAccess(array, k)) { + if (unsafeMemoryAccess(array, k) < pivot) { + // If array[k] is smaller than the pivot, we move it to the index position and increment it. _swap(array, ++index, k); } } From 8704763e258221d88c2c4c3a09031dea313c3908 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Feb 2024 10:45:50 +0100 Subject: [PATCH 26/26] Apply suggestions from code review --- contracts/utils/Arrays.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index a08bd80492f..f1a77f37133 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -47,7 +47,7 @@ library Arrays { for (uint256 k = i + 1; k < j; ++k) { // Unsafe access is safe given `k < j <= array.length`. if (unsafeMemoryAccess(array, k) < pivot) { - // If array[k] is smaller than the pivot, we move it to the index position and increment it. + // If array[k] is smaller than the pivot, we increment the index and move array[k] there. _swap(array, ++index, k); } }