Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unbounded loop in size() implementation #21

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Remove unbounded loop in size() implementation #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ada5a1
Remove unbounded loop in size() implementation
fleupold Apr 7, 2020
dbdd9c6
eth gas reporter
fleupold Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 2 additions & 22 deletions contracts/libraries/IterableAppendOnlySet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ library IterableAppendOnlySet {
struct Data {
mapping(address => address) nextMap;
address last;
uint96 size; // width is chosen to align struct size to full words
}

function insert(Data storage self, address value) public returns (bool) {
Expand All @@ -13,6 +14,7 @@ library IterableAppendOnlySet {
}
self.nextMap[self.last] = value;
self.last = value;
self.size += 1;
return true;
}

Expand All @@ -31,26 +33,4 @@ library IterableAppendOnlySet {
require(value != self.last, "Trying to get next of last element");
return self.nextMap[value];
}

function size(Data storage self) public view returns (uint256) {
if (self.last == address(0)) {
return 0;
}
uint256 count = 1;
address current = first(self);
while (current != self.last) {
current = next(self, current);
count++;
}
return count;
}

function atIndex(Data storage self, uint256 index) public view returns (address) {
require(index < size(self), "requested index too large");
address res = first(self);
for (uint256 i = 0; i < index; i++) {
res = next(self, res);
}
return res;
}
}
8 changes: 2 additions & 6 deletions contracts/libraries/wrappers/IterableAppendOnlySetWrapper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,11 @@ contract IterableAppendOnlySetWrapper {
return data.last;
}

function size() public view returns (uint256) {
return data.size();
function size() public view returns (uint96) {
return data.size;
}

function next(address value) public view returns (address) {
return data.next(value);
}

function atIndex(uint256 index) public view returns (address) {
Copy link
Contributor

@bh2smith bh2smith Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah. There is no need to keep it if it isn't used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this a but more, wouldn't we avoid a lot of mess if size was just stored in the contract?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return data.atIndex(index);
}
}
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"eslint": "^5.12.0",
"eslint-plugin-no-only-tests": "^2.3.0",
"eslint-plugin-react": "^7.12.3",
"eth-gas-reporter": "^0.2.16",
"solhint": "^1.5.0",
"solidity-coverage": "^0.6.7",
"solium": "^1.2.4",
Expand Down
53 changes: 25 additions & 28 deletions test/libraries/iterable_append_only_set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
const IterableSetLib = artifacts.require(".libraries/IterableAppendOnlySet.sol")
const IterableSet = artifacts.require(".libraries/wrappers/IterableAppendOnlySetWrapper.sol")
const IterableSet = artifacts.require(
".libraries/wrappers/IterableAppendOnlySetWrapper.sol"
)

const truffleAssert = require("truffle-assertions")

Expand All @@ -19,7 +21,7 @@ async function getSetContent(set) {
return result
}

contract("IterableSet", function (accounts) {
contract("IterableSet", function(accounts) {
beforeEach(async () => {
const lib = await IterableSetLib.new()
await IterableSet.link(IterableSetLib, lib.address)
Expand All @@ -29,10 +31,18 @@ contract("IterableSet", function (accounts) {
const set = await IterableSet.new()
assert.deepEqual(await getSetContent(set), [])

assert.equal(await set.contains(accounts[0]), false, "The element should not be there")
assert.equal(
await set.contains(accounts[0]),
false,
"The element should not be there"
)
await set.insert(accounts[0])

assert.equal(await set.contains(accounts[0]), true, "The element should be there")
assert.equal(
await set.contains(accounts[0]),
true,
"The element should be there"
)
assert.deepEqual(await getSetContent(set), accounts.slice(0, 1))
})

Expand All @@ -54,11 +64,19 @@ contract("IterableSet", function (accounts) {
it("should insert the same value only once", async () => {
const set = await IterableSet.new()

assert.equal(await set.insert.call(accounts[0]), true, "First insert should insert")
assert.equal(
await set.insert.call(accounts[0]),
true,
"First insert should insert"
)
await set.insert(accounts[0])
assert.deepEqual(await getSetContent(set), accounts.slice(0, 1))

assert.equal(await set.insert.call(accounts[0]), false, "Second insert should do nothing")
assert.equal(
await set.insert.call(accounts[0]),
false,
"Second insert should do nothing"
)
await set.insert(accounts[0])
assert.deepEqual(await getSetContent(set), accounts.slice(0, 1))
})
Expand Down Expand Up @@ -114,25 +132,4 @@ contract("IterableSet", function (accounts) {
await set.insert(accounts[0])
await truffleAssert.reverts(set.next(accounts[0]))
})

it("fetches elements at index (by insertion order)", async () => {
const set = await IterableSet.new()

await set.insert(accounts[0])
await set.insert(accounts[1])

assert.equal(await set.atIndex(0), accounts[0])
assert.equal(await set.atIndex(1), accounts[1])
})
it("fails to fetches at index when requested index is too large", async () => {
const set = await IterableSet.new()

// Nothing in the set yet!
await truffleAssert.reverts(set.atIndex(0))

await set.insert(accounts[0])
await truffleAssert.reverts(set.atIndex(1))
})


})
})
Loading