From f328ec2b6298b92990d353415989ff76debb1991 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 22 Feb 2023 23:52:12 +0100 Subject: [PATCH 01/17] perf: replace prev bucket by highest bucket --- src/LogarithmicBuckets.sol | 11 +++++------ test/TestLogarithmicBuckets.t.sol | 18 +++++++++--------- test/mocks/LogarithmicBucketsMock.sol | 6 ++---- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index dcb51f73..9d9f1b1c 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -69,7 +69,7 @@ library LogarithmicBuckets { if (next != 0) return _buckets.buckets[next].getHead(); - uint256 prev = prevBucket(lowerMask, bucketsMask); + uint256 prev = highestBucket(bucketsMask); return _buckets.buckets[prev].getHead(); } @@ -128,7 +128,7 @@ library LogarithmicBuckets { } } - /// @notice Returns the following bucket which contains greater values. + /// @notice Returns the following non-empty bucket. /// @dev The bucket returned is the lowest that is in `bucketsMask` and not in `lowerMask`. function nextBucket(uint256 lowerMask, uint256 bucketsMask) internal @@ -141,10 +141,9 @@ library LogarithmicBuckets { } } - /// @notice Returns the preceding bucket which contains smaller values. - /// @dev The bucket returned is the highest that is in both `bucketsMask` and `lowerMask`. - function prevBucket(uint256 lowerMask, uint256 bucketsMask) internal pure returns (uint256) { - uint256 lowerBucketsMask = setLowerBits(lowerMask & bucketsMask); + /// @notice Returns the highest non-empty bucket. + function highestBucket(uint256 bucketsMask) internal pure returns (uint256) { + uint256 lowerBucketsMask = setLowerBits(bucketsMask); return lowerBucketsMask ^ (lowerBucketsMask >> 1); } } diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index 747b82b2..04545f7c 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -151,18 +151,18 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } } - function testProvePrevBucket(uint256 _value) public { + function testProveHighestBucket(uint256 _value) public { uint256 curr = LogarithmicBuckets.computeBucket(_value); - uint256 prev = prevBucketValue(_value); + uint256 highest = highestBucketValue(); uint256 bucketsMask = buckets.bucketsMask; - // check that `prev` is a non-empty bucket that is lower than or equal to `curr`; or zero - assertTrue(prev == 0 || isPowerOfTwo(prev)); - assertTrue(prev <= curr); - assertTrue(prev == 0 || bucketsMask & prev != 0); + // check that `highest` is a non-empty bucket that is lower than or equal to `curr`; or zero + assertTrue(highest == 0 || isPowerOfTwo(highest)); + assertTrue(highest <= curr); + assertTrue(highest == 0 || bucketsMask & highest != 0); unchecked { - // check that `prev` is the highest one among such lower non-empty buckets, if exist - // note: this also checks that all the lower buckets are empty when `prev` == 0 - for (uint256 i = curr; i > prev; i >>= 1) { + // check that `highest` is the highest one among such lower non-empty buckets, if exist + // note: this also checks that all the lower buckets are empty when `highest` == 0 + for (uint256 i = curr; i > highest; i >>= 1) { assertTrue(bucketsMask & i == 0); } } diff --git a/test/mocks/LogarithmicBucketsMock.sol b/test/mocks/LogarithmicBucketsMock.sol index 1d73999f..922e7e80 100644 --- a/test/mocks/LogarithmicBucketsMock.sol +++ b/test/mocks/LogarithmicBucketsMock.sol @@ -54,9 +54,7 @@ contract LogarithmicBucketsMock { return LogarithmicBuckets.nextBucket(lowerMask, bucketsMask); } - function prevBucketValue(uint256 _value) internal view returns (uint256) { - uint256 bucketsMask = buckets.bucketsMask; - uint256 lowerMask = LogarithmicBuckets.setLowerBits(_value); - return LogarithmicBuckets.prevBucket(lowerMask, bucketsMask); + function highestBucketValue() internal view returns (uint256) { + return LogarithmicBuckets.highestBucket(buckets.bucketsMask); } } From 6ed3f031c6ad09c167a281fff1f84ae60fa812db Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 23 Feb 2023 09:36:15 +0100 Subject: [PATCH 02/17] test: fix highest bucket test --- test/TestLogarithmicBuckets.t.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index 04545f7c..c8ec919d 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -138,8 +138,9 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { uint256 curr = LogarithmicBuckets.computeBucket(_value); uint256 next = nextBucketValue(_value); uint256 bucketsMask = buckets.bucketsMask; - // check that `next` is a strictly higer non-empty bucket, or zero + // check that `next` is a power of two or zero. assertTrue(next == 0 || isPowerOfTwo(next)); + // check that `next` is a strictly higher non-empty bucket, or zero assertTrue(next == 0 || next > curr); assertTrue(next == 0 || bucketsMask & next != 0); unchecked { @@ -151,18 +152,17 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } } - function testProveHighestBucket(uint256 _value) public { - uint256 curr = LogarithmicBuckets.computeBucket(_value); + function testProveHighestBucket() public { uint256 highest = highestBucketValue(); uint256 bucketsMask = buckets.bucketsMask; - // check that `highest` is a non-empty bucket that is lower than or equal to `curr`; or zero + // check that `highest` is a power of two or zero. assertTrue(highest == 0 || isPowerOfTwo(highest)); - assertTrue(highest <= curr); + // check that `highest` is a non-empty bucket or zero. assertTrue(highest == 0 || bucketsMask & highest != 0); unchecked { - // check that `highest` is the highest one among such lower non-empty buckets, if exist + // check that `highest` is the highest non-empty bucket, if exists. // note: this also checks that all the lower buckets are empty when `highest` == 0 - for (uint256 i = curr; i > highest; i >>= 1) { + for (uint256 i = 1 >> 256; i > highest; i >>= 1) { assertTrue(bucketsMask & i == 0); } } From 98df6c637b613e130ba4b6346279d5922c93edc2 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 23 Feb 2023 09:45:49 +0100 Subject: [PATCH 03/17] test: add highest prev equivalence test --- test/TestLogarithmicBuckets.t.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index c8ec919d..3a3f8697 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -167,4 +167,15 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } } } + + function testProveHighestPrevEquivalence(uint256 _value) public { + uint256 curr = LogarithmicBuckets.computeBucket(_value); + uint256 next = nextBucketValue(_value); + + if (next == 0) { + uint256 highest = highestBucketValue(); + // check that in this case, `highest` is smaller than `curr`. + assertTrue(highest <= curr); + } + } } From bb39532092dc4506ffb70b384123015d8b597f5d Mon Sep 17 00:00:00 2001 From: MathisGD <74971347+MathisGD@users.noreply.github.com> Date: Thu, 23 Feb 2023 10:25:04 +0100 Subject: [PATCH 04/17] docs: improve comments Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Signed-off-by: MathisGD <74971347+MathisGD@users.noreply.github.com> --- test/TestLogarithmicBuckets.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index 3a3f8697..cfd773bc 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -138,9 +138,9 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { uint256 curr = LogarithmicBuckets.computeBucket(_value); uint256 next = nextBucketValue(_value); uint256 bucketsMask = buckets.bucketsMask; - // check that `next` is a power of two or zero. + // Check that `next` is a power of two or zero. assertTrue(next == 0 || isPowerOfTwo(next)); - // check that `next` is a strictly higher non-empty bucket, or zero + // Check that `next` is a strictly higher non-empty bucket, or zero. assertTrue(next == 0 || next > curr); assertTrue(next == 0 || bucketsMask & next != 0); unchecked { From bd8cf2c9465384ace35ea74380a2beaa485b7b54 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Mon, 27 Feb 2023 12:06:56 +0100 Subject: [PATCH 05/17] refactor: merge computeBucket and highestBucket --- src/LogarithmicBuckets.sol | 25 +++++++++++----------- test/TestLogarithmicBuckets.t.sol | 6 +++--- test/TestLogarithmicBucketsInvariant.t.sol | 2 +- test/mocks/LogarithmicBucketsMock.sol | 2 +- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 9d9f1b1c..66b05d59 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -39,17 +39,20 @@ library LogarithmicBuckets { if (value == 0) { if (_newValue == 0) revert ZeroValue(); - _insert(_buckets, _id, computeBucket(_newValue), _head); + // `highestSetBit` is used to computed the bucket associated with `_newValue`. + _insert(_buckets, _id, highestSetBit(_newValue), _head); return; } - uint256 currentBucket = computeBucket(value); + // `highestSetBit` is used to computed the bucket associated with `value`. + uint256 currentBucket = highestSetBit(value); if (_newValue == 0) { _remove(_buckets, _id, currentBucket); return; } - uint256 newBucket = computeBucket(_newValue); + // `highestSetBit` is used to computed the bucket associated with `_newValue`. + uint256 newBucket = highestSetBit(_newValue); if (newBucket != currentBucket) { _remove(_buckets, _id, currentBucket); _insert(_buckets, _id, newBucket, _head); @@ -69,7 +72,9 @@ library LogarithmicBuckets { if (next != 0) return _buckets.buckets[next].getHead(); - uint256 prev = highestBucket(bucketsMask); + // `highestSetBit` is used to compute the highest non-empty bucket. + // Knowing that `next` == 0, it is also the highest previous non-empty bucket. + uint256 prev = highestSetBit(bucketsMask); return _buckets.buckets[prev].getHead(); } @@ -107,8 +112,10 @@ library LogarithmicBuckets { /// PURE HELPERS /// - /// @notice Returns the bucket in which the given value would fall. - function computeBucket(uint256 _value) internal pure returns (uint256) { + /// @notice Returns the highest set bit. + /// @dev Used to compute the bucket associated to a given `value`. + /// @dev Used to compute the highest non empty bucket given the `bucketsMask`. + function highestSetBit(uint256 _value) internal pure returns (uint256) { uint256 lowerMask = setLowerBits(_value); return lowerMask ^ (lowerMask >> 1); } @@ -140,10 +147,4 @@ library LogarithmicBuckets { bucket := and(higherBucketsMask, add(not(higherBucketsMask), 1)) } } - - /// @notice Returns the highest non-empty bucket. - function highestBucket(uint256 bucketsMask) internal pure returns (uint256) { - uint256 lowerBucketsMask = setLowerBits(bucketsMask); - return lowerBucketsMask ^ (lowerBucketsMask >> 1); - } } diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index 3a3f8697..b549e1e9 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -125,7 +125,7 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } function testProveComputeBucket(uint256 _value) public { - uint256 bucket = LogarithmicBuckets.computeBucket(_value); + uint256 bucket = LogarithmicBuckets.highestSetBit(_value); unchecked { // cross-check that bucket == 2^{floor(log_2 value)}, or 0 if value == 0 assertTrue(bucket == 0 || isPowerOfTwo(bucket)); @@ -135,7 +135,7 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } function testProveNextBucket(uint256 _value) public { - uint256 curr = LogarithmicBuckets.computeBucket(_value); + uint256 curr = LogarithmicBuckets.highestSetBit(_value); uint256 next = nextBucketValue(_value); uint256 bucketsMask = buckets.bucketsMask; // check that `next` is a power of two or zero. @@ -169,7 +169,7 @@ contract TestProveLogarithmicBuckets is LogarithmicBucketsMock, Test { } function testProveHighestPrevEquivalence(uint256 _value) public { - uint256 curr = LogarithmicBuckets.computeBucket(_value); + uint256 curr = LogarithmicBuckets.highestSetBit(_value); uint256 next = nextBucketValue(_value); if (next == 0) { diff --git a/test/TestLogarithmicBucketsInvariant.t.sol b/test/TestLogarithmicBucketsInvariant.t.sol index 392621cd..b3c78933 100644 --- a/test/TestLogarithmicBucketsInvariant.t.sol +++ b/test/TestLogarithmicBucketsInvariant.t.sol @@ -75,7 +75,7 @@ contract TestLogarithmicBucketsSeenInvariant is Test, Random { address user = buckets.seen(i); uint256 value = buckets.getValueOf(user); if (value != 0) { - uint256 bucket = LogarithmicBuckets.computeBucket(value); + uint256 bucket = LogarithmicBuckets.highestSetBit(value); address next = buckets.getNext(bucket, user); address prev = buckets.getPrev(bucket, user); assertEq(buckets.getNext(bucket, prev), user); diff --git a/test/mocks/LogarithmicBucketsMock.sol b/test/mocks/LogarithmicBucketsMock.sol index 922e7e80..23051084 100644 --- a/test/mocks/LogarithmicBucketsMock.sol +++ b/test/mocks/LogarithmicBucketsMock.sol @@ -55,6 +55,6 @@ contract LogarithmicBucketsMock { } function highestBucketValue() internal view returns (uint256) { - return LogarithmicBuckets.highestBucket(buckets.bucketsMask); + return LogarithmicBuckets.highestSetBit(buckets.bucketsMask); } } From 996bf004e70b11e59f4e69713f01aa6278bc82bd Mon Sep 17 00:00:00 2001 From: MathisGD Date: Mon, 27 Feb 2023 12:12:57 +0100 Subject: [PATCH 06/17] refactor: factorize lowerMask in nextBucket --- src/LogarithmicBuckets.sol | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 66b05d59..22975f74 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -66,16 +66,13 @@ library LogarithmicBuckets { function getMatch(Buckets storage _buckets, uint256 _value) internal view returns (address) { uint256 bucketsMask = _buckets.bucketsMask; if (bucketsMask == 0) return address(0); - uint256 lowerMask = setLowerBits(_value); - - uint256 next = nextBucket(lowerMask, bucketsMask); + uint256 next = nextBucket(_value, bucketsMask); if (next != 0) return _buckets.buckets[next].getHead(); // `highestSetBit` is used to compute the highest non-empty bucket. // Knowing that `next` == 0, it is also the highest previous non-empty bucket. uint256 prev = highestSetBit(bucketsMask); - return _buckets.buckets[prev].getHead(); } @@ -137,11 +134,8 @@ library LogarithmicBuckets { /// @notice Returns the following non-empty bucket. /// @dev The bucket returned is the lowest that is in `bucketsMask` and not in `lowerMask`. - function nextBucket(uint256 lowerMask, uint256 bucketsMask) - internal - pure - returns (uint256 bucket) - { + function nextBucket(uint256 value, uint256 bucketsMask) internal pure returns (uint256 bucket) { + uint256 lowerMask = setLowerBits(value); assembly { let higherBucketsMask := and(not(lowerMask), bucketsMask) bucket := and(higherBucketsMask, add(not(higherBucketsMask), 1)) From 96beaf9ee544d5e13371d9ecaad23df8d0fc05dd Mon Sep 17 00:00:00 2001 From: MathisGD Date: Mon, 27 Feb 2023 13:02:31 +0100 Subject: [PATCH 07/17] docs: fix typo --- src/LogarithmicBuckets.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 22975f74..fe6a6d10 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -39,19 +39,19 @@ library LogarithmicBuckets { if (value == 0) { if (_newValue == 0) revert ZeroValue(); - // `highestSetBit` is used to computed the bucket associated with `_newValue`. + // `highestSetBit` is used to compute the bucket associated with `_newValue`. _insert(_buckets, _id, highestSetBit(_newValue), _head); return; } - // `highestSetBit` is used to computed the bucket associated with `value`. + // `highestSetBit` is used to compute the bucket associated with `value`. uint256 currentBucket = highestSetBit(value); if (_newValue == 0) { _remove(_buckets, _id, currentBucket); return; } - // `highestSetBit` is used to computed the bucket associated with `_newValue`. + // `highestSetBit` is used to compute the bucket associated with `_newValue`. uint256 newBucket = highestSetBit(_newValue); if (newBucket != currentBucket) { _remove(_buckets, _id, currentBucket); From 0354b28c386868aeff44cdad3c193ae683de0048 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Mon, 27 Feb 2023 12:20:16 +0100 Subject: [PATCH 08/17] style: remove args underscore --- src/LogarithmicBuckets.sol | 104 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index fe6a6d10..3a1fb006 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -22,89 +22,89 @@ library LogarithmicBuckets { /// INTERNAL /// - /// @notice Updates an account in the `_buckets`. - /// @param _buckets The buckets to update. - /// @param _id The address of the account. - /// @param _newValue The new value of the account. - /// @param _head Indicates whether to insert the new values at the head or at the tail of the buckets list. + /// @notice Updates an account in the `buckets`. + /// @param buckets The buckets to update. + /// @param id The address of the account. + /// @param newValue The new value of the account. + /// @param head Indicates whether to insert the new values at the head or at the tail of the buckets list. function update( - Buckets storage _buckets, - address _id, - uint256 _newValue, - bool _head + Buckets storage buckets, + address id, + uint256 newValue, + bool head ) internal { - if (_id == address(0)) revert ZeroAddress(); - uint256 value = _buckets.valueOf[_id]; - _buckets.valueOf[_id] = _newValue; + if (id == address(0)) revert ZeroAddress(); + uint256 value = buckets.valueOf[id]; + buckets.valueOf[id] = newValue; if (value == 0) { - if (_newValue == 0) revert ZeroValue(); - // `highestSetBit` is used to compute the bucket associated with `_newValue`. - _insert(_buckets, _id, highestSetBit(_newValue), _head); + if (newValue == 0) revert ZeroValue(); + // `highestSetBit` is used to compute the bucket associated with `newValue`. + insert(buckets, id, highestSetBit(newValue), head); return; } // `highestSetBit` is used to compute the bucket associated with `value`. uint256 currentBucket = highestSetBit(value); - if (_newValue == 0) { - _remove(_buckets, _id, currentBucket); + if (newValue == 0) { + remove(buckets, id, currentBucket); return; } - // `highestSetBit` is used to compute the bucket associated with `_newValue`. - uint256 newBucket = highestSetBit(_newValue); + // `highestSetBit` is used to compute the bucket associated with `newValue`. + uint256 newBucket = highestSetBit(newValue); if (newBucket != currentBucket) { - _remove(_buckets, _id, currentBucket); - _insert(_buckets, _id, newBucket, _head); + remove(buckets, id, currentBucket); + insert(buckets, id, newBucket, head); } } - /// @notice Returns the address in `_buckets` that is a candidate for matching the value `_value`. - /// @param _buckets The buckets to get the head. - /// @param _value The value to match. + /// @notice Returns the address in `buckets` that is a candidate for matching the value `value`. + /// @param buckets The buckets to get the head. + /// @param value The value to match. /// @return The address of the head. - function getMatch(Buckets storage _buckets, uint256 _value) internal view returns (address) { - uint256 bucketsMask = _buckets.bucketsMask; + function getMatch(Buckets storage buckets, uint256 value) internal view returns (address) { + uint256 bucketsMask = buckets.bucketsMask; if (bucketsMask == 0) return address(0); - uint256 next = nextBucket(_value, bucketsMask); - if (next != 0) return _buckets.buckets[next].getHead(); + uint256 next = nextBucket(value, bucketsMask); + if (next != 0) return buckets.buckets[next].getHead(); // `highestSetBit` is used to compute the highest non-empty bucket. // Knowing that `next` == 0, it is also the highest previous non-empty bucket. uint256 prev = highestSetBit(bucketsMask); - return _buckets.buckets[prev].getHead(); + return buckets.buckets[prev].getHead(); } /// PRIVATE /// - /// @notice Removes an account in the `_buckets`. + /// @notice Removes an account in the `buckets`. /// @dev Does not update the value. - /// @param _buckets The buckets to modify. - /// @param _id The address of the account to remove. - /// @param _bucket The mask of the bucket where to remove. - function _remove( - Buckets storage _buckets, - address _id, - uint256 _bucket + /// @param buckets The buckets to modify. + /// @param id The address of the account to remove. + /// @param bucket The mask of the bucket where to remove. + function remove( + Buckets storage buckets, + address id, + uint256 bucket ) private { - if (_buckets.buckets[_bucket].remove(_id)) _buckets.bucketsMask &= ~_bucket; + if (buckets.buckets[bucket].remove(id)) buckets.bucketsMask &= ~bucket; } - /// @notice Inserts an account in the `_buckets`. - /// @dev Expects that `_id` != 0. + /// @notice Inserts an account in the `buckets`. + /// @dev Expects that `id` != 0. /// @dev Does not update the value. - /// @param _buckets The buckets to modify. - /// @param _id The address of the account to update. - /// @param _bucket The mask of the bucket where to insert. - /// @param _head Whether to insert at the head or at the tail of the list. - function _insert( - Buckets storage _buckets, - address _id, - uint256 _bucket, - bool _head + /// @param buckets The buckets to modify. + /// @param id The address of the account to update. + /// @param bucket The mask of the bucket where to insert. + /// @param head Whether to insert at the head or at the tail of the list. + function insert( + Buckets storage buckets, + address id, + uint256 bucket, + bool head ) private { - if (_buckets.buckets[_bucket].insert(_id, _head)) _buckets.bucketsMask |= _bucket; + if (buckets.buckets[bucket].insert(id, head)) buckets.bucketsMask |= bucket; } /// PURE HELPERS /// @@ -112,8 +112,8 @@ library LogarithmicBuckets { /// @notice Returns the highest set bit. /// @dev Used to compute the bucket associated to a given `value`. /// @dev Used to compute the highest non empty bucket given the `bucketsMask`. - function highestSetBit(uint256 _value) internal pure returns (uint256) { - uint256 lowerMask = setLowerBits(_value); + function highestSetBit(uint256 value) internal pure returns (uint256) { + uint256 lowerMask = setLowerBits(value); return lowerMask ^ (lowerMask >> 1); } From 5b727643c036fd3b33e7c01f409bf9cba4c3d619 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 1 Mar 2023 11:20:27 +0100 Subject: [PATCH 09/17] style: revert private functions naming --- src/LogarithmicBuckets.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 3a1fb006..1e764943 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -40,22 +40,22 @@ library LogarithmicBuckets { if (value == 0) { if (newValue == 0) revert ZeroValue(); // `highestSetBit` is used to compute the bucket associated with `newValue`. - insert(buckets, id, highestSetBit(newValue), head); + _insert(buckets, id, highestSetBit(newValue), head); return; } // `highestSetBit` is used to compute the bucket associated with `value`. uint256 currentBucket = highestSetBit(value); if (newValue == 0) { - remove(buckets, id, currentBucket); + _remove(buckets, id, currentBucket); return; } // `highestSetBit` is used to compute the bucket associated with `newValue`. uint256 newBucket = highestSetBit(newValue); if (newBucket != currentBucket) { - remove(buckets, id, currentBucket); - insert(buckets, id, newBucket, head); + _remove(buckets, id, currentBucket); + _insert(buckets, id, newBucket, head); } } @@ -83,7 +83,7 @@ library LogarithmicBuckets { /// @param buckets The buckets to modify. /// @param id The address of the account to remove. /// @param bucket The mask of the bucket where to remove. - function remove( + function _remove( Buckets storage buckets, address id, uint256 bucket @@ -98,7 +98,7 @@ library LogarithmicBuckets { /// @param id The address of the account to update. /// @param bucket The mask of the bucket where to insert. /// @param head Whether to insert at the head or at the tail of the list. - function insert( + function _insert( Buckets storage buckets, address id, uint256 bucket, From 38415d36e815f8ff9dcf8c5af79ac68c9d5760b6 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 1 Mar 2023 11:24:41 +0100 Subject: [PATCH 10/17] style: harmonize BucketDLL --- src/BucketDLL.sol | 92 +++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/BucketDLL.sol b/src/BucketDLL.sol index 911d495b..c1b2d174 100644 --- a/src/BucketDLL.sol +++ b/src/BucketDLL.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.0; library BucketDLL { - /// STRUCTS /// + /* STRUCTS */ struct Account { address prev; @@ -13,78 +13,78 @@ library BucketDLL { mapping(address => Account) accounts; } - /// INTERNAL /// + /* INTERNAL */ - /// @notice Returns the address at the head of the `_list`. - /// @param _list The list from which to get the head. + /// @notice Returns the address at the head of the `list`. + /// @param list The list from which to get the head. /// @return The address of the head. - function getHead(List storage _list) internal view returns (address) { - return _list.accounts[address(0)].next; + function getHead(List storage list) internal view returns (address) { + return list.accounts[address(0)].next; } - /// @notice Returns the address at the tail of the `_list`. - /// @param _list The list from which to get the tail. + /// @notice Returns the address at the tail of the `list`. + /// @param list The list from which to get the tail. /// @return The address of the tail. - function getTail(List storage _list) internal view returns (address) { - return _list.accounts[address(0)].prev; + function getTail(List storage list) internal view returns (address) { + return list.accounts[address(0)].prev; } - /// @notice Returns the next id address from the current `_id`. - /// @param _list The list to search in. - /// @param _id The address of the current account. + /// @notice Returns the next id address from the current `id`. + /// @param list The list to search in. + /// @param id The address of the current account. /// @return The address of the next account. - function getNext(List storage _list, address _id) internal view returns (address) { - return _list.accounts[_id].next; + function getNext(List storage list, address id) internal view returns (address) { + return list.accounts[id].next; } - /// @notice Returns the previous id address from the current `_id`. - /// @param _list The list to search in. - /// @param _id The address of the current account. + /// @notice Returns the previous id address from the current `id`. + /// @param list The list to search in. + /// @param id The address of the current account. /// @return The address of the previous account. - function getPrev(List storage _list, address _id) internal view returns (address) { - return _list.accounts[_id].prev; + function getPrev(List storage list, address id) internal view returns (address) { + return list.accounts[id].prev; } - /// @notice Removes an account of the `_list`. - /// @dev This function should not be called with `_id` equal to address 0. - /// @param _list The list to search in. - /// @param _id The address of the account. + /// @notice Removes an account of the `list`. + /// @dev This function should not be called with `id` equal to address 0. + /// @param list The list to search in. + /// @param id The address of the account. /// @return Whether the bucket is empty after removal. - function remove(List storage _list, address _id) internal returns (bool) { - Account memory account = _list.accounts[_id]; + function remove(List storage list, address id) internal returns (bool) { + Account memory account = list.accounts[id]; address prev = account.prev; address next = account.next; - _list.accounts[prev].next = next; - _list.accounts[next].prev = prev; + list.accounts[prev].next = next; + list.accounts[next].prev = prev; - delete _list.accounts[_id]; + delete list.accounts[id]; return (prev == address(0) && next == address(0)); } - /// @notice Inserts an account in the `_list`. - /// @dev This function should not be called with `_id` equal to address 0. - /// @param _list The list to search in. - /// @param _id The address of the account. - /// @param _head Tells whether to insert at the head or at the tail of the list. + /// @notice Inserts an account in the `list`. + /// @dev This function should not be called with `id` equal to address 0. + /// @param list The list to search in. + /// @param id The address of the account. + /// @param head Tells whether to insert at the head or at the tail of the list. /// @return Whether the bucket was empty before insertion. function insert( - List storage _list, - address _id, - bool _head + List storage list, + address id, + bool head ) internal returns (bool) { - if (_head) { - address head = _list.accounts[address(0)].next; - _list.accounts[address(0)].next = _id; - _list.accounts[head].prev = _id; - _list.accounts[_id].next = head; + if (head) { + address head = list.accounts[address(0)].next; + list.accounts[address(0)].next = id; + list.accounts[head].prev = id; + list.accounts[id].next = head; return head == address(0); } else { - address tail = _list.accounts[address(0)].prev; - _list.accounts[address(0)].prev = _id; - _list.accounts[tail].next = _id; - _list.accounts[_id].prev = tail; + address tail = list.accounts[address(0)].prev; + list.accounts[address(0)].prev = id; + list.accounts[tail].next = id; + list.accounts[id].prev = tail; return tail == address(0); } } From 041c3daa365f3a5b0674351c52f12c042ec9b4d6 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 1 Mar 2023 11:25:51 +0100 Subject: [PATCH 11/17] style: harmonize headers comments --- src/LogarithmicBuckets.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 1e764943..d35e8429 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -12,7 +12,7 @@ library LogarithmicBuckets { uint256 bucketsMask; } - /// ERRORS /// + /* ERRORS */ /// @notice Thrown when the address is zero at insertion. error ZeroAddress(); @@ -20,7 +20,7 @@ library LogarithmicBuckets { /// @notice Thrown when 0 value is inserted. error ZeroValue(); - /// INTERNAL /// + /* INTERNAL */ /// @notice Updates an account in the `buckets`. /// @param buckets The buckets to update. @@ -76,7 +76,7 @@ library LogarithmicBuckets { return buckets.buckets[prev].getHead(); } - /// PRIVATE /// + /* PRIVATE */ /// @notice Removes an account in the `buckets`. /// @dev Does not update the value. @@ -107,7 +107,7 @@ library LogarithmicBuckets { if (buckets.buckets[bucket].insert(id, head)) buckets.bucketsMask |= bucket; } - /// PURE HELPERS /// + /* PURE HELPERS */ /// @notice Returns the highest set bit. /// @dev Used to compute the bucket associated to a given `value`. From 1b9ce7f07f6a97aabfa63c54062847f291315fbf Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 1 Mar 2023 11:40:46 +0100 Subject: [PATCH 12/17] style: rename head --- src/BucketDLL.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/BucketDLL.sol b/src/BucketDLL.sol index c1b2d174..9a6f0718 100644 --- a/src/BucketDLL.sol +++ b/src/BucketDLL.sol @@ -67,14 +67,14 @@ library BucketDLL { /// @dev This function should not be called with `id` equal to address 0. /// @param list The list to search in. /// @param id The address of the account. - /// @param head Tells whether to insert at the head or at the tail of the list. + /// @param atHead Tells whether to insert at the head or at the tail of the list. /// @return Whether the bucket was empty before insertion. function insert( List storage list, address id, - bool head + bool atHead ) internal returns (bool) { - if (head) { + if (atHead) { address head = list.accounts[address(0)].next; list.accounts[address(0)].next = id; list.accounts[head].prev = id; From bebf71d508e550201b2f1b2df2585fdac9c8e2a6 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 2 Mar 2023 10:19:28 +0100 Subject: [PATCH 13/17] docs: improve natspec and comments --- src/BucketDLL.sol | 4 ++++ src/LogarithmicBuckets.sol | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/BucketDLL.sol b/src/BucketDLL.sol index 9a6f0718..2b07a678 100644 --- a/src/BucketDLL.sol +++ b/src/BucketDLL.sol @@ -1,6 +1,10 @@ // SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.0; +/// @title BucketDLL +/// @author Morpho Labs +/// @custom:contact security@morpho.xyz +/// @notice The doubly linked list used in logarithmic buckets. library BucketDLL { /* STRUCTS */ diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index d35e8429..fe8d606d 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -3,6 +3,10 @@ pragma solidity ^0.8.0; import "./BucketDLL.sol"; +/// @title LogarithmicBuckets +/// @author Morpho Labs +/// @custom:contact security@morpho.xyz +/// @notice The logarithmic buckets data-structure. library LogarithmicBuckets { using BucketDLL for BucketDLL.List; @@ -132,7 +136,7 @@ library LogarithmicBuckets { } } - /// @notice Returns the following non-empty bucket. + /// @notice Returns the lowest non-empty bucket containing bigger values. /// @dev The bucket returned is the lowest that is in `bucketsMask` and not in `lowerMask`. function nextBucket(uint256 value, uint256 bucketsMask) internal pure returns (uint256 bucket) { uint256 lowerMask = setLowerBits(value); From 4a9cd16789e949a7f0113e88bd03fbc16e0c6207 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 2 Mar 2023 11:00:10 +0100 Subject: [PATCH 14/17] docs: document further constraints in bucketDLL --- src/BucketDLL.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/BucketDLL.sol b/src/BucketDLL.sol index 2b07a678..8404de69 100644 --- a/src/BucketDLL.sol +++ b/src/BucketDLL.sol @@ -51,6 +51,7 @@ library BucketDLL { /// @notice Removes an account of the `list`. /// @dev This function should not be called with `id` equal to address 0. + /// @dev This function should not be called with an `_id` that is not in the list. /// @param list The list to search in. /// @param id The address of the account. /// @return Whether the bucket is empty after removal. @@ -69,6 +70,7 @@ library BucketDLL { /// @notice Inserts an account in the `list`. /// @dev This function should not be called with `id` equal to address 0. + /// @dev This function should not be called with an `_id` that is already in the list. /// @param list The list to search in. /// @param id The address of the account. /// @param atHead Tells whether to insert at the head or at the tail of the list. From 1cf04d424fc7daef5b5165102fd5aee17beb7c95 Mon Sep 17 00:00:00 2001 From: MathisGD <74971347+MathisGD@users.noreply.github.com> Date: Thu, 2 Mar 2023 11:04:07 +0100 Subject: [PATCH 15/17] docs: minor improvement Co-authored-by: Romain Milon Signed-off-by: MathisGD <74971347+MathisGD@users.noreply.github.com> --- src/LogarithmicBuckets.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index fe8d606d..4be06d22 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -136,7 +136,7 @@ library LogarithmicBuckets { } } - /// @notice Returns the lowest non-empty bucket containing bigger values. + /// @notice Returns the lowest non-empty bucket containing larger values. /// @dev The bucket returned is the lowest that is in `bucketsMask` and not in `lowerMask`. function nextBucket(uint256 value, uint256 bucketsMask) internal pure returns (uint256 bucket) { uint256 lowerMask = setLowerBits(value); From 7366e39d7288d729fde40faa141f47baf331535f Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Fri, 3 Mar 2023 15:46:46 +0100 Subject: [PATCH 16/17] refactor: keep only the getNext getter in BucketDLL --- src/BucketDLL.sol | 25 +----------- src/LogarithmicBuckets.sol | 4 +- test/TestBucketDLL.t.sol | 4 +- test/TestLogarithmicBuckets.t.sol | 2 +- test/TestLogarithmicBucketsInvariant.t.sol | 3 +- test/mocks/BucketDLLMock.sol | 44 ++++++++++++++++++++++ test/mocks/LogarithmicBucketsMock.sol | 3 +- 7 files changed, 55 insertions(+), 30 deletions(-) create mode 100644 test/mocks/BucketDLLMock.sol diff --git a/src/BucketDLL.sol b/src/BucketDLL.sol index 8404de69..42154136 100644 --- a/src/BucketDLL.sol +++ b/src/BucketDLL.sol @@ -19,21 +19,8 @@ library BucketDLL { /* INTERNAL */ - /// @notice Returns the address at the head of the `list`. - /// @param list The list from which to get the head. - /// @return The address of the head. - function getHead(List storage list) internal view returns (address) { - return list.accounts[address(0)].next; - } - - /// @notice Returns the address at the tail of the `list`. - /// @param list The list from which to get the tail. - /// @return The address of the tail. - function getTail(List storage list) internal view returns (address) { - return list.accounts[address(0)].prev; - } - /// @notice Returns the next id address from the current `id`. + /// @dev Pass the address 0 to get the head of the list. /// @param list The list to search in. /// @param id The address of the current account. /// @return The address of the next account. @@ -41,14 +28,6 @@ library BucketDLL { return list.accounts[id].next; } - /// @notice Returns the previous id address from the current `id`. - /// @param list The list to search in. - /// @param id The address of the current account. - /// @return The address of the previous account. - function getPrev(List storage list, address id) internal view returns (address) { - return list.accounts[id].prev; - } - /// @notice Removes an account of the `list`. /// @dev This function should not be called with `id` equal to address 0. /// @dev This function should not be called with an `_id` that is not in the list. @@ -70,7 +49,7 @@ library BucketDLL { /// @notice Inserts an account in the `list`. /// @dev This function should not be called with `id` equal to address 0. - /// @dev This function should not be called with an `_id` that is already in the list. + /// @dev This function should not be called with an `id` that is already in the list. /// @param list The list to search in. /// @param id The address of the account. /// @param atHead Tells whether to insert at the head or at the tail of the list. diff --git a/src/LogarithmicBuckets.sol b/src/LogarithmicBuckets.sol index 4be06d22..248c7a07 100644 --- a/src/LogarithmicBuckets.sol +++ b/src/LogarithmicBuckets.sol @@ -72,12 +72,12 @@ library LogarithmicBuckets { if (bucketsMask == 0) return address(0); uint256 next = nextBucket(value, bucketsMask); - if (next != 0) return buckets.buckets[next].getHead(); + if (next != 0) return buckets.buckets[next].getNext(address(0)); // `highestSetBit` is used to compute the highest non-empty bucket. // Knowing that `next` == 0, it is also the highest previous non-empty bucket. uint256 prev = highestSetBit(bucketsMask); - return buckets.buckets[prev].getHead(); + return buckets.buckets[prev].getNext(address(0)); } /* PRIVATE */ diff --git a/test/TestBucketDLL.t.sol b/test/TestBucketDLL.t.sol index ce1496ca..5019fd9e 100644 --- a/test/TestBucketDLL.t.sol +++ b/test/TestBucketDLL.t.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; -import "src/BucketDLL.sol"; +import "./mocks/BucketDLLMock.sol"; contract TestBucketDLL is Test { - using BucketDLL for BucketDLL.List; + using BucketDLLMock for BucketDLL.List; uint256 internal numberOfAccounts = 50; address[] public accounts; diff --git a/test/TestLogarithmicBuckets.t.sol b/test/TestLogarithmicBuckets.t.sol index a2c12027..2d9c5038 100644 --- a/test/TestLogarithmicBuckets.t.sol +++ b/test/TestLogarithmicBuckets.t.sol @@ -5,7 +5,7 @@ import "forge-std/Test.sol"; import "./mocks/LogarithmicBucketsMock.sol"; contract TestLogarithmicBuckets is LogarithmicBucketsMock, Test { - using BucketDLL for BucketDLL.List; + using BucketDLLMock for BucketDLL.List; using LogarithmicBuckets for LogarithmicBuckets.Buckets; uint256 public accountsLength = 50; diff --git a/test/TestLogarithmicBucketsInvariant.t.sol b/test/TestLogarithmicBucketsInvariant.t.sol index b3c78933..6d0193c8 100644 --- a/test/TestLogarithmicBucketsInvariant.t.sol +++ b/test/TestLogarithmicBucketsInvariant.t.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; import "./helpers/Random.sol"; import "./mocks/LogarithmicBucketsMock.sol"; +import "./mocks/BucketDLLMock.sol"; import "forge-std/Test.sol"; contract TestLogarithmicBucketsInvariant is Test, Random { @@ -31,7 +32,7 @@ contract TestLogarithmicBucketsInvariant is Test, Random { } contract LogarithmicBucketsSeenMock is LogarithmicBucketsMock { - using BucketDLL for BucketDLL.List; + using BucketDLLMock for BucketDLL.List; using LogarithmicBuckets for LogarithmicBuckets.Buckets; address[] public seen; diff --git a/test/mocks/BucketDLLMock.sol b/test/mocks/BucketDLLMock.sol new file mode 100644 index 00000000..33c8dddc --- /dev/null +++ b/test/mocks/BucketDLLMock.sol @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity ^0.8.0; + +import "src/BucketDLL.sol"; + +library BucketDLLMock { + function remove(BucketDLL.List storage _list, address _id) internal returns (bool) { + return BucketDLL.remove(_list, _id); + } + + function insert( + BucketDLL.List storage _list, + address _id, + bool _head + ) internal returns (bool) { + return BucketDLL.insert(_list, _id, _head); + } + + function getNext(BucketDLL.List storage _list, address _id) internal view returns (address) { + return BucketDLL.getNext(_list, _id); + } + + /// @notice Returns the address at the head of the `_list`. + /// @param _list The list from which to get the head. + /// @return The address of the head. + function getHead(BucketDLL.List storage _list) internal view returns (address) { + return _list.accounts[address(0)].next; + } + + /// @notice Returns the address at the tail of the `_list`. + /// @param _list The list from which to get the tail. + /// @return The address of the tail. + function getTail(BucketDLL.List storage _list) internal view returns (address) { + return _list.accounts[address(0)].prev; + } + + /// @notice Returns the previous id address from the current `_id`. + /// @param _list The list to search in. + /// @param _id The address of the current account. + /// @return The address of the previous account. + function getPrev(BucketDLL.List storage _list, address _id) internal view returns (address) { + return _list.accounts[_id].prev; + } +} diff --git a/test/mocks/LogarithmicBucketsMock.sol b/test/mocks/LogarithmicBucketsMock.sol index 23051084..e6793091 100644 --- a/test/mocks/LogarithmicBucketsMock.sol +++ b/test/mocks/LogarithmicBucketsMock.sol @@ -2,9 +2,10 @@ pragma solidity ^0.8.0; import "src/LogarithmicBuckets.sol"; +import "./BucketDLLMock.sol"; contract LogarithmicBucketsMock { - using BucketDLL for BucketDLL.List; + using BucketDLLMock for BucketDLL.List; using LogarithmicBuckets for LogarithmicBuckets.Buckets; LogarithmicBuckets.Buckets public buckets; From bf66f98b309390cd58d7c10289c0b820083d5861 Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Sat, 4 Mar 2023 10:26:40 +0100 Subject: [PATCH 17/17] refactor: remove natspec in mock bucket dll --- test/mocks/BucketDLLMock.sol | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/test/mocks/BucketDLLMock.sol b/test/mocks/BucketDLLMock.sol index 33c8dddc..50857642 100644 --- a/test/mocks/BucketDLLMock.sol +++ b/test/mocks/BucketDLLMock.sol @@ -20,25 +20,15 @@ library BucketDLLMock { return BucketDLL.getNext(_list, _id); } - /// @notice Returns the address at the head of the `_list`. - /// @param _list The list from which to get the head. - /// @return The address of the head. function getHead(BucketDLL.List storage _list) internal view returns (address) { return _list.accounts[address(0)].next; } - /// @notice Returns the address at the tail of the `_list`. - /// @param _list The list from which to get the tail. - /// @return The address of the tail. - function getTail(BucketDLL.List storage _list) internal view returns (address) { - return _list.accounts[address(0)].prev; - } - - /// @notice Returns the previous id address from the current `_id`. - /// @param _list The list to search in. - /// @param _id The address of the current account. - /// @return The address of the previous account. function getPrev(BucketDLL.List storage _list, address _id) internal view returns (address) { return _list.accounts[_id].prev; } + + function getTail(BucketDLL.List storage _list) internal view returns (address) { + return _list.accounts[address(0)].prev; + } }