-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Add ERC20 token with snapshoting mechanism (#1209) #1331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @jbogacz! Here's some initial comments. I'll get back to you on the tests tomorrow.
@frangio thanks for nice code review. I will apply your comments. |
Require reason message removed Inline documentation in NatSpec added Snapshot added to _mint and _burn method
61e76c4
to
1302e6a
Compare
Hey, @frangio I have followed your comments and hope this time it looks better. |
* release candidate v2.0.0-rc.1 * fix linter error (cherry picked from commit c12a1c6) * Roles now emit events in construction and when renouncing.
* Add the missing test for ERC721Holder * fix lint * Move the holder test to a separate file
* Removed mintingFinished from ERC20Mintable. * Removed MintingFinished from ERC721Mintable. * Removed MintingFinished event.
* Improved bounty tests. * Fixed linter errors. * Addressed review comments.
* Add BigNumber support to expectEvent/inLogs (#1026) * switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js * Refactor expectEvent.inLogs function to use simple value number check * Introduced should.be.bignumber method to compare BigNumber values * Destructure transaction object to extract logs field
* fix: refactor sign.js and related tests * fix: remove unused dep * fix: update package.json correctly * Add missing tests to ECRecovery * fix lint * Reorganize the tests * Reuse signature * fix static errors * Apply suggestions by @frangion and @nventuro * Remove only * More suggestions * Remove unnecessary max-len * remove only
* Add BigNumber support to expectEvent/inLogs (#1026) * switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js * Refactor expectEvent.inLogs function to use simple value number check * Introduced should.be.bignumber method to compare BigNumber values * Use expectEvent to test logs (#1232) * Removed trailing space
* add test for msg.data too short * fix test to hit that branch * Update SignatureBouncer.test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there @jbogacz! This is in quite a good place, except that we're missing tests for the Arrays
library. How would you feel about opening a new PR with just that library (and its tests), and then continuing this one on top? That way feedback can be more focused, and we don't have to be thinking about multiple things at once :)
Yeah, @nventuro you are absolutely right! I will go for |
Great! I'll put this PR on hold then until that one is merged. |
Definitely agree with @nventuro! Let's work on The fixes to my previous review look good! |
function burn(address account, uint256 amount) public { | ||
_burn(account, amount); | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* @return An uint256 representing current snapshot id. | ||
*/ | ||
function snapshot() external returns (uint256) { | ||
currentSnapshotId += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not utilize SafeMath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you are right!
function transfer(address to, uint256 value) public returns (bool) { | ||
updateSnapshot(msg.sender); | ||
updateSnapshot(to); | ||
return super.transfer(to, value); |
There was a problem hiding this comment.
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.
{ | ||
updateSnapshot(from); | ||
updateSnapshot(to); | ||
return super.transferFrom(from, to, value); |
There was a problem hiding this comment.
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.
*/ | ||
function _mint(address account, uint256 amount) internal { | ||
updateSnapshot(account); | ||
super._mint(account, amount); |
There was a problem hiding this comment.
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
*/ | ||
function _burn(address account, uint256 amount) internal { | ||
updateSnapshot(account); | ||
super._burn(account, amount); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
* @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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments below I've included inline per file.
Noted, this PR was submitted awhile back and OZ's repo has been updated since then.
No tracking of totalSupply() history
Does not follow OZ's fail early standard
Not using SafeMath
Requires an external actor to trigger a snapshot (a discussion should be had about this)
Require reason message removed Inline documentation in NatSpec added Snapshot added to _mint and _burn method
….com/jbogacz/openzeppelin-solidity into fix/ERC20-token-with-snapshots-#1209
@mswezey23 your comments sounds reasonable but with my coding I based on @frangio instructions and @nventuro comments so before any change it's kind to ask guys what do they think about it. @frangio @nventuro could you put your comments here ? Anyway @mswezey23 implemented #1398 which pretty much covers snapshoting for particular block number and keep total supply as well. I did some research due devident payment and voting power and it follows these requirements so I'm fine to close this my PR and track work on SnapshotToken @mswezey23's PR #1398. |
Hey everyone - just checking in to see if there has been any updates in regards to this and the PR: Thanks! |
* @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 { |
There was a problem hiding this comment.
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.
event Snapshot(uint256 id); | ||
|
||
/** | ||
* @dev Increments current snapshot. Emites Snapshot event. |
There was a problem hiding this comment.
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".
Hey @jbogacz, sorry for taking so long to review this! We intend to include snapshots in the upcoming release, which is scheduled for next week. Sadly it looks like the diff got too large, due to commits, etc. on |
The equivalent #1617 was just merged. |
🚀 Description
This pull request is based on changes introduced with frangio:feature-snapshot-token. It adds snapshoting mechanism to regular ERC20 token.
Fixes #1209
npm run lint:fix
).