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

Fix "id to index" bug #109

Merged
merged 10 commits into from
Jun 24, 2018
Merged

Fix "id to index" bug #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a1f5153
Fix incorrect description
lknix Jun 24, 2018
9c92ae9
Fix wording
lknix Jun 24, 2018
264310a
Assert should come after a check if tokenId is valid
lknix Jun 24, 2018
8af7ad4
Set idToIndex when token is minted and put to the global array
lknix Jun 24, 2018
317c2c0
Update unit tests
lknix Jun 24, 2018
26ca313
Add asserts to for tokens and idToIndex
lknix Jun 24, 2018
e9f11a6
Update package-lock.json
lknix Jun 24, 2018
95c84a4
Remove redundant code
lknix Jun 24, 2018
b0f3562
Unit test typo fix
lknix Jun 24, 2018
9984f29
Add assert comments
lknix Jun 24, 2018
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
10 changes: 7 additions & 3 deletions contracts/tokens/NFTokenEnumerable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract NFTokenEnumerable is
uint256[] internal tokens;

/**
* @dev Mapping from owner address to a list of owned NFT IDs.
* @dev Mapping from token ID its index in global tokens array.
*/
mapping(uint256 => uint256) internal idToIndex;

Expand Down Expand Up @@ -56,6 +56,7 @@ contract NFTokenEnumerable is
{
super._mint(_to, _tokenId);
tokens.push(_tokenId);
idToIndex[_tokenId] = tokens.length.sub(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

push function returns new array length, so I tend to do:

uint length = tokens.push(_tokenId);
idToIndex[_tokenId] = length - 1;

Don't know if that's more gas efficient tho.

}

/**
Expand All @@ -72,15 +73,16 @@ contract NFTokenEnumerable is
)
internal
{
assert(tokens.length > 0);
super._burn(_owner, _tokenId);
assert(tokens.length > 0);

uint256 tokenIndex = idToIndex[_tokenId];
// Sanity check. This could be removed in the future.
assert(tokens[tokenIndex] == _tokenId);
uint256 lastTokenIndex = tokens.length.sub(1);
uint256 lastToken = tokens[lastTokenIndex];

tokens[tokenIndex] = lastToken;
tokens[lastTokenIndex] = 0;

tokens.length--;
idToIndex[_tokenId] = 0;
Expand Down Expand Up @@ -155,6 +157,8 @@ contract NFTokenEnumerable is
returns (uint256)
{
require(_index < tokens.length);
// Sanity check. This could be removed in the future.
assert(idToIndex[tokens[_index]] == _index);
return tokens[_index];
}

Expand Down
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 23 additions & 10 deletions test/tokens/NFTokenEnumerable.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,19 @@ contract('NFTokenEnumerableMock', (accounts) => {
await nftoken.mint(accounts[1], id2);
await nftoken.mint(accounts[2], id3);

const tokenId = await nftoken.tokenByIndex(1);
let tokenId = await nftoken.tokenByIndex(0);
assert.equal(tokenId, id1);

tokenId = await nftoken.tokenByIndex(1);
assert.equal(tokenId, id2);

tokenId = await nftoken.tokenByIndex(2);
assert.equal(tokenId, id3);
});

it('throws when trying to get token by unexistant index', async () => {
it('throws when trying to get token by non-existing index', async () => {
await nftoken.mint(accounts[1], id1);
await assertRevert(nftoken.tokenByIndex(1));
await assertRevert(nftoken.tokenByIndex(10));
});

it('returns the correct token of owner by index', async () => {
Expand All @@ -64,29 +70,36 @@ contract('NFTokenEnumerableMock', (accounts) => {
assert.equal(tokenId, id2);
});

it('throws when trying to get token of owner by unexistant index', async () => {
it('throws when trying to get token of owner by non-existing index', async () => {
await nftoken.mint(accounts[1], id1);
await nftoken.mint(accounts[2], id3);

await assertRevert(nftoken.tokenOfOwnerByIndex(accounts[1], 1));
await assertRevert(nftoken.tokenOfOwnerByIndex(accounts[1], 4));
});

it('corectly burns a NFT', async () => {
await nftoken.mint(accounts[1], id1);
await nftoken.mint(accounts[1], id2);
await nftoken.mint(accounts[1], id3);
const { logs } = await nftoken.burn(accounts[1], id2);
const transferEvent = logs.find(e => e.event === 'Transfer');
assert.notEqual(transferEvent, undefined);

const balance = await nftoken.balanceOf(accounts[1]);
assert.equal(balance, 0);
assert.equal(balance, 2);

await assertRevert(nftoken.ownerOf(id2));

const totalSupply = await nftoken.totalSupply();
assert.equal(totalSupply, 0);
assert.equal(totalSupply, 2);

await assertRevert(nftoken.tokenByIndex(0));
await assertRevert(nftoken.tokenOfOwnerByIndex(accounts[1], 0));
});
await assertRevert(nftoken.tokenByIndex(2));
await assertRevert(nftoken.tokenOfOwnerByIndex(accounts[1], 2));

let tokenId = await nftoken.tokenByIndex(0);
assert.equal(tokenId, id1);

tokenId = await nftoken.tokenByIndex(1);
assert.equal(tokenId, id3);
});
});