Add Arrays library with unit tests (#1209) #1375
Conversation
* prepared due to snapshot token requirements
* add library with method to find upper bound
* add unit test for basic and edge cases
jbogacz
force-pushed
the
fix/Add-Arrays-library-#1209
branch
from
c420dd1 to
0e8358a
Compare
contracts/utils/Arrays.sol
Outdated
| } | ||
| } | ||
|
|
||
| // At this point at `low` is the exclusive upper bound. We will return the inclusive upper bound. |
contracts/utils/Arrays.sol
Outdated
| library Arrays { | ||
|
|
||
| /** | ||
| * @dev Find upper bound for searching element. Utilize binary search function with O(log(n)) cost. |
There was a problem hiding this comment.
We should improve this documentation: in particular, it is critical to document that element is the value we're searching for, that array is sorted in ascending order, and the return value is an index.
We should also better explain what the function does: given said sorted array, it returns the smallest array index such that the value at that index is larger or equal to the received element, or array.length if no such index exists (i.e. all values inside the array are smaller that the target). It also returns 0 for an empty array, though we may want to revert in that scenario.
test/utils/Arrays.test.js
Outdated
| }); | ||
|
|
||
| function range (start, end) { | ||
| return Array(end - start + 1).fill().map((_, idx) => start + idx); |
There was a problem hiding this comment.
This implementation might not match people's expectations, since lodash's range doesn't include end. Could we also add a short comment describing what it does?
test/utils/Arrays.test.js
Outdated
| (await this.arrays.findUpperBound(139)).should.be.bignumber.equal(11); | ||
| }); | ||
|
|
||
| it('should return `index 0` for the first element', async function () { |
There was a problem hiding this comment.
I'd change these to just 'should return 0 for the first element'
test/utils/Arrays.test.js
Outdated
| }); | ||
|
|
||
| it('should return `last index + 1` for the element over the upper boundary', async function () { | ||
| (await this.arrays.findUpperBound(512)).should.be.bignumber.equal(129); |
There was a problem hiding this comment.
The fact that the array is 129 elements long, and the first element is 128, may be a bit confusing. Consider using e.g. a shorter array.
test/utils/Arrays.test.js
Outdated
| this.arrays = await ArraysImpl.new(WITH_GAP_ARRAY); | ||
| }); | ||
|
|
||
| it('should return `last index + 1` of first range for the element from the gap', async function () { |
There was a problem hiding this comment.
I really like what you're testing for here, but fear it may be hard to follow, due to the arrays being generated by range, and there being so many magic numbers. What if we simply made a short test array? e.g. const gapArray = [0, 1, 2, 3,4, 8, 9, 10, 11], and then test for 7.
There was a problem hiding this comment.
We may also want to use a shorter range in all tests, actually, to make it easier for the reader to understand what's going on.
test/utils/Arrays.test.js
Outdated
| .should(); | ||
|
|
||
| contract('Arrays', function () { | ||
| context('Odd number of elements', function () { |
There was a problem hiding this comment.
Love the testing for odd and even elements! We should also add a test for an empty array (and decide what to do in that scenario).
Simplify Arrays.test.js to use short arrays as test date
nventuro
added
feature
* Explaned why uint256 mid variable calculated as Math.average is safe to use as index of array.
|
Nice, just want to thanks again for your help here 😄 I will go for #1331 now. |
…1375) * Add Arrays library with unit tests (OpenZeppelin#1209) * prepared due to snapshot token requirements * add library with method to find upper bound * add unit test for basic and edge cases * Imporove documentation for Arrays library Simplify Arrays.test.js to use short arrays as test date * Added comment for uint256 mid variable. * Explaned why uint256 mid variable calculated as Math.average is safe to use as index of array. (cherry picked from commit f7e53d9)
Blocks PR #1331
🚀 Description
Arrays library which is the requirement for ERC20 token with snapshots. Blocks PR #1331.
npm run lint:fix).