From d78ae17d576ea9619202932110484145a5850ae7 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sun, 12 Nov 2023 16:29:01 +0000 Subject: [PATCH 01/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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 3b104209553f43f3301416ca930db79e610b3776 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 24 Jan 2024 13:47:56 +0100 Subject: [PATCH 09/11] Update Arrays.test.js --- 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 7db0f478130..fa5d0f1f644 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -4,7 +4,7 @@ const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { randomArray, generators } = require('../helpers/random'); -// See https://en.cppreference.com/w/cpp/algorithm/ranges/lower_bound +// See https://en.cppreference.com/w/cpp/algorithm/lower_bound const lowerBound = (array, value) => { const i = array.findIndex(element => value <= element); return i == -1 ? array.length : i; From 39a9f7e350f3efa0d5550467dc44aef7b76b8d87 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 29 Jan 2024 13:01:09 -0600 Subject: [PATCH 10/11] Apply PR suggestions --- contracts/utils/Arrays.sol | 51 +++++++++++++------------------------- test/utils/Arrays.test.js | 7 ++++-- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index f03b46d4801..ac0758e4e1f 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -18,11 +18,12 @@ library Arrays { * 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. + * NOTE: The `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` + * IMPORTANT: Deprecated. This implementation behaves as {lowerBound} but lacks + * support for repeated elements in the array. The {lowerBound} function should + * be used instead. */ function findUpperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -53,14 +54,12 @@ library Arrays { } /** - * @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, + * @dev Searches an `array` sorted in ascending order and returns the first + * index that contains a value greater or equal than `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). * - * See https://en.cppreference.com/w/cpp/algorithm/ranges/lower_bound + * See C++'s https://en.cppreference.com/w/cpp/algorithm/lower_bound[lower_bound] */ function lowerBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -89,14 +88,12 @@ library Arrays { } /** - * @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). + * @dev Searches an `array` sorted in ascending order and returns the first + * index that contains a value strictly greater than `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 + * See C++'s https://en.cppreference.com/w/cpp/algorithm/upper_bound[upper_bound] */ function upperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -125,14 +122,7 @@ library Arrays { } /** - * @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 + * @dev Same as {lowerBound}, but with an array in memory. */ function lowerBoundMemory(uint256[] memory array, uint256 element) internal pure returns (uint256) { uint256 low = 0; @@ -161,14 +151,7 @@ library Arrays { } /** - * @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 + * @dev Same as {upperBound}, but with an array in memory. */ function upperBoundMemory(uint256[] memory array, uint256 element) internal pure returns (uint256) { uint256 low = 0; diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index fa5d0f1f644..dc35e49447f 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -85,8 +85,11 @@ describe('Arrays', function () { 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)); + if (hasDuplicates(array)) { + expect(await this.mock.findUpperBound(input)).to.be.equal(upperBound(array, input) - 1); + } else { + expect(await this.mock.findUpperBound(input)).to.be.equal(lowerBound(array, input)); + } }); it('lowerBound', async function () { From 7c9f21b7d710fdce563c7562d381c03368cb1baa Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 29 Jan 2024 13:04:35 -0600 Subject: [PATCH 11/11] Lint --- 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 ac0758e4e1f..67c63a7e7b9 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -23,7 +23,7 @@ library Arrays { * * IMPORTANT: Deprecated. This implementation behaves as {lowerBound} but lacks * support for repeated elements in the array. The {lowerBound} function should - * be used instead. + * be used instead. */ function findUpperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -59,7 +59,7 @@ library Arrays { * exists (i.e. all values in the array are strictly less than `element`), the array * length is returned. Time complexity O(log n). * - * See C++'s https://en.cppreference.com/w/cpp/algorithm/lower_bound[lower_bound] + * See C++'s https://en.cppreference.com/w/cpp/algorithm/lower_bound[lower_bound]. */ function lowerBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0; @@ -93,7 +93,7 @@ library Arrays { * exists (i.e. all values in the array are strictly less than `element`), the array * length is returned. Time complexity O(log n). * - * See C++'s https://en.cppreference.com/w/cpp/algorithm/upper_bound[upper_bound] + * See C++'s https://en.cppreference.com/w/cpp/algorithm/upper_bound[upper_bound]. */ function upperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { uint256 low = 0;