MAX_POOLS refactor #17
Conversation
There was a problem hiding this comment.
This looks great!
Left some notes as I'm concerned an arbitrary _limit + _offset could lead to an overflow if we don't have checks.
Also, could you please add a test for the call to tokens(2000, 1000, 0x0000000000000000000000000000000000000000, [])
This seems to fail for the same reason we have this PR.
contracts/LpSugar.vy
Outdated
| for pindex in range(0, MAX_POOLS): | ||
| if pindex >= pools_count or len(pools) >= _limit + _offset: | ||
| for pindex in range(0, MAX_ITERATIONS): | ||
| if pindex >= pools_count or visited >= _limit + _offset: |
There was a problem hiding this comment.
We'll need to add here also the len(pools) >= MAX_POOLS otherwise we risk overflowing again since the MAX_ITERATIONS is larger than MAX_POOLS.
| @@ -324,7 +326,7 @@ def forSwaps(_limit: uint256, _offset: uint256) -> DynArray[SwapLp, MAX_POOLS]: | |||
| nfpm: address = self._fetch_nfpm(factory.address) | |||
| pools_count: uint256 = factory.allPoolsLength() | |||
|
|
|||
| for pindex in range(0, MAX_POOLS): | |||
| for pindex in range(0, MAX_ITERATIONS): | |||
There was a problem hiding this comment.
We'll need a check to break for len(pools) >= MAX_POOLS now that there might be more iterations.
contracts/LpSugar.vy
Outdated
| @@ -394,7 +396,7 @@ def tokens(_limit: uint256, _offset: uint256, _account: address, \ | |||
| col.append(self._token(_addresses[index], _account)) | |||
| seen.append(_addresses[index]) | |||
|
|
|||
| for index in range(_offset, _offset + MAX_TOKENS): | |||
| for index in range(0, MAX_TOKENS): | |||
There was a problem hiding this comment.
Thinking we'll need to change this to MAX_POOLS instead of MAX_TOKENS, to keep the iterations bounded to the size of the pools.
Changes the output of the internal
_pools()function to be the fixed size ofMAX_POOLS. The output of_pools()no longer pads the front of the array with placeholder data.MAX_ITERATIONSis a new higher constant that determines the number of pools that can be looped through.MAX_POOLSshouldn't be increased going forward