Remove unbounded loop in size() implementation #21
Conversation
| } | ||
|
|
||
| function next(address value) public view returns (address) { | ||
| return data.next(value); | ||
| } | ||
|
|
||
| function atIndex(uint256 index) public view returns (address) { |
There was a problem hiding this comment.
I don’t see how removing this is relevant here. This is a standard implement of getting and element by index from such a data structure and I am under the impression that this is used somewhere.
However, I understand the implementation is subject to the same issue and suppose it would be overall better for someone to fetch the entire set and get their elements as needed off chain.
We will soon find out if/where it was being used.
There was a problem hiding this comment.
We are not currently using it and my worry was that the fact this is using an unbounded loop is hidden behind a layer of abstraction making it easy for people to accidentally use it in code that would break if the set got too big. I can put it back if you feel it's valueable to keep.
There was a problem hiding this comment.
Nah. There is no need to keep it if it isn't used.
There was a problem hiding this comment.
Thinking about this a but more, wouldn't we avoid a lot of mess if size was just stored in the contract?
There was a problem hiding this comment.
Technically we store the size in the contract (since the contract stores the self storage struct). The point for this library is to abstract set operations such as insertion and iteration (and arguably keeping track of size).
In order to deploy the exchange on xDai, we need to use the newest version of the solidity-data-structures package. There was a breaking change in the way that size gets computed (gnosis/solidity-data-structures#21) which requires small adjustments in the smart contract code. ### Test Plan CI
More context: gnosis/dex-contracts#641
size()is currently iterating over all items making it potentially very gas costly. Since this behaviour is internal to the library it is easy for dependent contracts to oversee the consequences.This PR therefore changes the implementation so that every insert will increment the size variable.
Due to the fact that the size variable is aligned (last takes 160 bits leaving 96 in the word), the amount of gas used on insertion barely changes (by measuring test suite with eth-gas-reporter):
I also removed the
atIndexmethod as it is subject to the same flaw and therefore of questionable use anyway.