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

ERC20 Snapshot Impl #1398

Closed
wants to merge 0 commits into from
Closed

ERC20 Snapshot Impl #1398

wants to merge 0 commits into from

Conversation

mswezey23
Copy link
Contributor

@mswezey23 mswezey23 commented Oct 8, 2018

Fixes #1209

Introduces a working ERC20Snapshot token that is modular, per OZ standard. ERC20Snapshot is compatible with mint, burn, and burnFrom.

This implementation will fail early. Meaning if the transfer fails normal ERC20 methods, no extra gas was wasted in attempts to write to storage to create the snapshot.

Another feature is that it tracks totalSupplyAt, which is very much needed for proper dividend calculations.

API wise, it adds the following two functions:

balanceOfAt(address owner, uint blockNumber)
totalSupplyAt(uint blockNumber) 

✅- reviewed the OpenZeppelin Contributor Guidelines
✅- added tests where applicable to test new functionality,
✅- made sure that your contracts are well-documented, and
✅- run the JS/Solidity linters and fixed any issues (npm run lint:fix).

@adamdossa
Copy link
Contributor

Something to consider is whether the optimal implementation is to keep all historical balances, or allow the token owner to set checkpoints and only keep historical balances (and total supply) at those checkpoints. The latter is obviously more gas efficient than this approach (which looks similar to MiniMe), although the downside is that you have to manually set checkpoints (in our implementation this is somewhat mitigated as there is a callback on every transfer / mint / burn type operation which can be used to set a checkpoint if needed).

For an implementation which allows explicit checkpoints to be set, you can see:
https://github.com/PolymathNetwork/polymath-core/blob/development-1.5.0/contracts/tokens/SecurityToken.sol

@mswezey23
Copy link
Contributor Author

@adamdossa Thanks for your reply!

I've also had similar thoughts, for gas efficiency. To simply put it, turn off/on the snapshot feature for gas savings when it's not required. The tricky part is to define when it's not required. Some features have an interval period, so that's trivial of when a snapshot is to occur. But if a random proposal pops up, that wasn't expected, and a recent snapshot hasn't occurred yet...

End of the day - it's going to come down to client's requirements and what rules/regulations they must abide by. You could even implement a frequency of every X amount of time. Say every 30 days a snapshot will occur on the next transfer/transferFrom method from either account that triggers that time check.

If the high frequency of transactions is an issue per a particular account(s), they should look into payment channels to optimize gas savings further.

I'd like to see this snapshot feature further enhanced (by myself or anyone) that allows these features to be inherited and configured.
First step is to get OpenZeppelin to merge this PR ;)

Off topic - your dividend payment article over the minime token was super useful 👍

@frangio frangio added this to the v2.1 milestone Nov 28, 2018
@frangio frangio modified the milestones: v2.1, v2.2 Dec 11, 2018
@nventuro nventuro self-assigned this Jan 15, 2019
@nventuro nventuro added feature New contracts, functions, or helpers. contracts Smart contract code. labels Jan 15, 2019
@nventuro
Copy link
Contributor

Hey @mswezey23, 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 master. Could you git rebase so that we get a cleaner changeset? Thanks!

@mswezey23
Copy link
Contributor Author

Hey @nventuro, will do 👍

@mswezey23 mswezey23 closed this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERC20 token with snapshots
4 participants