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.

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

Add ERC20 token with snapshoting mechanism (#1209) #1331

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
3b66e70
Add ERC20 token with snapshoting mechanism (#1209)
Sep 18, 2018
1302e6a
Methods visibility corrected
jbogacz Sep 23, 2018
21198bf
Roles now emit events in construction and when renouncing. (#1329)
nventuro Sep 26, 2018
396680b
Add the missing test for ERC721Holder (#1249)
Sep 26, 2018
db2e1d2
Removed assertions from Escrow and SplitPayment. (#1349)
nventuro Sep 26, 2018
5fdeaa8
Removed mintingFinished. (#1351)
nventuro Sep 26, 2018
ae109f6
Improved bounty tests. (#1350)
nventuro Sep 26, 2018
947de54
Add BigNumber support to expectEvent/inLogs (#1026) (#1338)
jbogacz Sep 26, 2018
75c0a59
Add missing tests to ECSDA (#1248)
Sep 26, 2018
536262f
Feature/use expect event in test logs assertions #1232 (#1343)
jbogacz Sep 27, 2018
1a4e534
Add SignatureBouncer test for when msg.data is too short (#1360)
frangio Sep 27, 2018
a688977
Dangling commas are now required. (#1359)
nventuro Sep 29, 2018
fa1dfbd
Fix #1358 Test helper to send funds (#1367)
Aniket-Engg Oct 1, 2018
6ae041b
Fix/#1355 test helper to check balance difference (#1368)
Aniket-Engg Oct 1, 2018
34bc709
Merged latestTime, increaseTime and duration into a time helper. (#1364)
nventuro Oct 2, 2018
269981e
Created test utils directory
nventuro Oct 2, 2018
f4d6f40
Fixed test path.
nventuro Oct 2, 2018
43ebb4f
ERC20 internal transfer method (#1370)
k06a Oct 2, 2018
f3888bb
Removing unrequired `_burn()` override (#1373)
Aniket-Engg Oct 3, 2018
c87433e
Prevents Bounty from being claimed twice (#1374)
Aniket-Engg Oct 3, 2018
5c22880
Update issue templates (#1376)
frangio Oct 4, 2018
ace14d3
Add note about compiling artifacts to releasing steps (#1377)
frangio Oct 4, 2018
fd4de77
Replaces `amount` with `value` for consistency (#1378)
Aniket-Engg Oct 4, 2018
b41b125
`this` is used in tests (#1380)
Aniket-Engg Oct 4, 2018
744f567
Separate ERC721Mintable (#1365)
frangio Oct 4, 2018
308e5e9
Removed unnecessary Secondary inheritance from RefundEscrow. (#1381)
nventuro Oct 4, 2018
b17de01
Removed old, unused mocks. (#1382)
nventuro Oct 6, 2018
41f84f8
Removed selfdestruct from BreakInvariantBounty (#1385)
Aniket-Engg Oct 8, 2018
f7e53d9
Add Arrays library with unit tests (#1209) (#1375)
jbogacz Oct 8, 2018
3acc2b4
Added a constructor to BreakInvariantBounty. (#1395)
nventuro Oct 8, 2018
aa6b6d0
Add ERC20 token with snapshoting mechanism (#1209)
Sep 18, 2018
7f59f86
Methods visibility corrected
jbogacz Sep 23, 2018
360c85e
Use SafeMath.
jbogacz Oct 9, 2018
474ec3d
Merge branch 'fix/ERC20-token-with-snapshots-#1209' of https://github…
jbogacz Oct 9, 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
19 changes: 19 additions & 0 deletions contracts/mocks/ERC20SnapshotMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
pragma solidity ^0.4.24;

import "../token/ERC20/ERC20Snapshot.sol";


contract ERC20SnapshotMock is ERC20Snapshot {

constructor(address initialAccount, uint256 initialBalance) public {
_mint(initialAccount, initialBalance);
}

function mint(address account, uint256 amount) public {
_mint(account, amount);
}

function burn(address account, uint256 amount) public {
_burn(account, amount);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not include _burnFrom from the current v2 RC2 OZ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mock for unit test purpose so I don't know if this even need mint and burn functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

burn and mint functions (and burnFrom) impact the total supply. Which is important to keep history of for specific purposes.

Examples include:
Voting Power
Dividend Share

Which both are dependent on the ratio of held tokens at a specified time. In which knowing the total supply is necessary.

130 changes: 130 additions & 0 deletions contracts/token/ERC20/ERC20Snapshot.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
pragma solidity ^0.4.24;

import "./ERC20.sol";
import "../../utils/Arrays.sol";


/**
* @title ERC20Snapshot token
* @dev An ERC20 token which enables taking snapshots of account balances.
* This can be useful to safely implement voting weighed by balance.
*/
contract ERC20Snapshot is ERC20 {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no tracking of the total supply history of the token if it was mintable or burnable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the reason to keep racking total supply. Could you elaborate ?

Copy link
Contributor

@mswezey23 mswezey23 Oct 10, 2018

Choose a reason for hiding this comment

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

Voting power and dividend payments require knowing the proper ratio of tokens held against the current token supply at a specified time in history.

Choose a reason for hiding this comment

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

Small typo in the comment here, should be "weighted" by balance.


using Arrays for uint256[];

// The 0 id represents no snapshot was taken yet.
uint256 private currentSnapshotId;

mapping (address => uint256[]) private snapshotIds;
mapping (address => uint256[]) private snapshotBalances;

event Snapshot(uint256 id);

/**
* @dev Increments current snapshot. Emites Snapshot event.

Choose a reason for hiding this comment

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

Should be "Emits Snapshot event." instead of "Emites".

* @return An uint256 representing current snapshot id.
*/
function snapshot() external returns (uint256) {
jbogacz marked this conversation as resolved.
Show resolved Hide resolved
currentSnapshotId += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not utilize SafeMath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you are right!

emit Snapshot(currentSnapshotId);
return currentSnapshotId;
}

/**
* @dev Returns account balance for specific snapshot.
* @param account address The address to query the balance of.
* @param snapshotId uint256 The snapshot id for which to query the balance of.
* @return An uint256 representing the amount owned by the passed address for specific snapshot.
*/
function balanceOfAt(
address account,
uint256 snapshotId
)
public
view
returns (uint256)
{
require(snapshotId > 0 && snapshotId <= currentSnapshotId);

uint256 idx = snapshotIds[account].findUpperBound(snapshotId);

if (idx == snapshotIds[account].length) {
return balanceOf(account);
} else {
return snapshotBalances[account][idx];
}
}

/**
* @dev Transfer token for a specified address. It takes balance snapshot for the sender and recipient account
* before transfer is done.
* @param to The address to transfer to.
* @param value The amount to be transferred.
*/
function transfer(address to, uint256 value) public returns (bool) {
updateSnapshot(msg.sender);
updateSnapshot(to);
return super.transfer(to, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

super.transfer() should be called first to follow OZ's standard of failing early.
Also the balanceOf() function call made below after calling updateSnapshot() will not be correct as the state change has not occurred yet.

}

/**
* @dev Transfer tokens from one address to another. It takes balance snapshot of both accounts before
* the transfer is done.
* @param from address The address which you want to send tokens from
* @param to address The address which you want to transfer to
* @param value uint256 the amount of tokens to be transferred
*/
function transferFrom(
address from,
address to,
uint256 value
)
public
returns (bool)
{
updateSnapshot(from);
updateSnapshot(to);
return super.transferFrom(from, to, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, super.transferFrom() should be called first.

}

/**
* @dev Internal function that mints an amount of the token and assigns it to
* an account. This encapsulates the modification of balances such that the
* proper events are emitted. Takes snapshot before tokens are minted.
* @param account The account that will receive the created tokens.
* @param amount The amount that will be created.
*/
function _mint(address account, uint256 amount) internal {
updateSnapshot(account);
super._mint(account, amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

super._mint() should be called first

}

/**
* @dev Internal function that burns an amount of the token of a given
* account. Takes snapshot before tokens are burned.
* @param account The account whose tokens will be burnt.
* @param amount The amount that will be burnt.
*/
function _burn(address account, uint256 amount) internal {
updateSnapshot(account);
super._burn(account, amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

super._burn() should be called first

}

function updateSnapshot(address account) private {
if (lastSnapshotId(account) < currentSnapshotId) {
snapshotIds[account].push(currentSnapshotId);
snapshotBalances[account].push(balanceOf(account));
Copy link
Contributor

Choose a reason for hiding this comment

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

balanceOf(account) will have the previous amount recorded. Perhaps this was a desired effect to have the history always behind and not current due to the nature of having to iterate a new snapshot manually.

A discussion is to be had about having snapshots with an on/off feature or have snapshots that are always active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's desired flow to have history always behind. If you want to have snapshots off then use regular ERC20.

Copy link
Contributor

@mswezey23 mswezey23 Oct 10, 2018

Choose a reason for hiding this comment

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

In your implementation, snapshots are off until another iteration is called upon. It is not an always on feature.

I have to know which snapshot id I want to query, instead of using timestamp or blockNumber, you use a trivial number starting at 1 (0 == no snapshot exists yet). This will make it hard for user adoption as now I have to look up which snapshot id I want in relation to blockNumber/timestamp.

Also, one needs to define which state of the balance do you care about at a specific block number? The balance before the block has been applied to the state or after?

}
}

function lastSnapshotId(address account) private view returns (uint256) {
uint256[] storage snapshots = snapshotIds[account];
if (snapshots.length == 0) {
return 0;
} else {
return snapshots[snapshots.length - 1];
}
}

}
43 changes: 43 additions & 0 deletions contracts/utils/Arrays.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
pragma solidity ^0.4.23;

import "../math/Math.sol";


/**
* @title Arrays
* @dev Utility library of inline array functions
*/
library Arrays {

/**
* @dev Find upper bound for searching element. Utilize binary search function with O(log(n)) cost.
*/
function findUpperBound(
jbogacz marked this conversation as resolved.
Show resolved Hide resolved
uint256[] storage array,
uint256 element
)
internal
view
returns (uint256)
{
uint256 low = 0;
uint256 high = array.length;

while (low < high) {
uint256 mid = Math.average(low, high);

if (array[mid] > element) {
high = mid;
} else {
low = mid + 1;
}
}

// At this point at `low` is the exclusive upper bound. We will return the inclusive upper bound.
if (low > 0 && array[low - 1] == element) {
return low - 1;
} else {
return low;
}
}
}
136 changes: 136 additions & 0 deletions test/token/ERC20/ERC20Snapshot.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
const ERC20Snapshot = artifacts.require('ERC20SnapshotMock');
const { expectThrow } = require('../../helpers/expectThrow');
const { EVMRevert } = require('../../helpers/EVMRevert');
const BigNumber = web3.BigNumber;

require('chai')
.use(require('chai-bignumber')(BigNumber))
.should();

contract('ERC20Snapshot', function ([_, owner, recipient, spender]) {
beforeEach(async function () {
this.token = await ERC20Snapshot.new(owner, 100);
});

context('there is no snapshots yet', function () {
describe('balanceOfAt', function () {
it('rejected for snapshotId parameter equals to 0', async function () {
await expectThrow(
this.token.balanceOfAt(owner, 0),
EVMRevert,
);
});

it('rejected for snapshotId parameter greather than currentSnapshotId', async function () {
await expectThrow(
this.token.balanceOfAt(owner, 1),
EVMRevert,
);
});
});

describe('after transfer', function () {
beforeEach(async function () {
await this.token.transfer(recipient, 10, { from: owner });
});

it('balanceOf returns correct value', async function () {
(await this.token.balanceOf(owner)).should.be.bignumber.equal(90);
(await this.token.balanceOf(recipient)).should.be.bignumber.equal(10);
});
});
});

context('snapshots exist', function () {
beforeEach(async function () {
await this.token.snapshot();
});

describe('accounts do not have snapshots yet', function () {
it('snapshot value of the owner account equals to his balance', async function () {
const balanceOfAt = (await this.token.balanceOfAt(owner, 1));
const balanceOf = (await this.token.balanceOf(owner));
balanceOfAt.should.be.bignumber.equal(balanceOf);
});

it('snapshot value of the recipient account equals to balance', async function () {
const balanceOfAt = (await this.token.balanceOfAt(recipient, 1));
const balanceOf = (await this.token.balanceOf(recipient));
balanceOfAt.should.be.bignumber.equal(balanceOf);
});
});

describe('accounts have already one snapshot', function () {
beforeEach(async function () {
await this.token.transfer(recipient, 10, { from: owner });
});

it('snapshot keeps previous balance', async function () {
(await this.token.balanceOfAt(owner, 1)).should.be.bignumber.equal(100);
(await this.token.balanceOfAt(recipient, 1)).should.be.bignumber.equal(0);
});

it('account has correct balance', async function () {
(await this.token.balanceOf(owner)).should.be.bignumber.equal(90);
(await this.token.balanceOf(recipient)).should.be.bignumber.equal(10);
});
});

describe('snapshot keeps previous balance even for multiple transfers', async function () {
beforeEach(async function () {
await this.token.transfer(recipient, 10, { from: owner });
await this.token.transfer(recipient, 10, { from: owner });
});

it('snapshot has previous balance', async function () {
(await this.token.balanceOfAt(owner, 1)).should.be.bignumber.equal(100);
(await this.token.balanceOfAt(recipient, 1)).should.be.bignumber.equal(0);
});

it('account has correct balance', async function () {
(await this.token.balanceOf(owner)).should.be.bignumber.equal(80);
(await this.token.balanceOf(recipient)).should.be.bignumber.equal(20);
});
});

describe('snapshot keeps correct values for transfer from action', async function () {
beforeEach(async function () {
await this.token.approve(spender, 20, { from: owner });
await this.token.transferFrom(owner, recipient, 20, { from: spender });
});

it('spender and recipient snapshot is stored', async function () {
(await this.token.balanceOfAt(owner, 1)).should.be.bignumber.equal(100);
(await this.token.balanceOfAt(recipient, 1)).should.be.bignumber.equal(0);
});
});

describe('new tokens are minted', function () {
beforeEach(async function () {
await this.token.mint(owner, 50);
});

it('snapshot keeps balance before mint', async function () {
(await this.token.balanceOfAt(owner, 1)).should.be.bignumber.equal(100);
});

it('current balance is greater after mint', async function () {
(await this.token.balanceOf(owner)).should.be.bignumber.equal(150);
});
});

describe('chunk tokens are burned', function () {
beforeEach(async function () {
await this.token.burn(owner, 30);
});

it('snapshot keeps balance before burn', async function () {
(await this.token.balanceOfAt(owner, 1)).should.be.bignumber.equal(100);
});

it('crrent balance is lower after burn', async function () {
(await this.token.balanceOf(owner)).should.be.bignumber.equal(70);
});
});
});
});