I want to do something different here, so please leave your thoughts about this review, and more meta, about making the review in this form. And even better, share your own reviews of this contract!
This can't be an audit like the ones we do at Zeppelin because I'm not an independent objective auditor. I'm part of the community, with super strong subjective opinions of what should be part of OpenZeppelin. So my idea is to make it more open, report the issues I find, but also critique the code and make open questions for others to join the discussion. The goal is to add this new feature to a [non-draft] stable OpenZeppelin release, in a way that is safe, transparent, and that satisfies users and contributors.
Back to the contract at hand, this is a nice and clever solution to the problem. It is an unintuitive solution, but the harder parts of the code are accompanied by good comments and it has a good test suite to cover all the corner cases.
Here are my findings (and PITA opinions), in order of appearance:
- There is no ERC for the API of ERC20 snapshots.
- I think the name
ERC20Snapshotis not good. To be fully explicit, a better name would be
ERC20WithOnDemandSnapshots. If we want a shorter name, then I'm not sure what's best. We can brainstorm it.
- There is no link to the MiniMe Token that inspired this contract. [PR#1637]
- The contract's comment is very confusing: "Snapshots store a value at the time a snapshot is taken (and a new snapshot id created), and the corresponding snapshot id." Maybe it could be something like: "When a snapshot is made, the balances and totalSupply at the time of the snapshot are recorded for later access." [PR#1638]
- There is no documentation of how to use this contract. [PR#1639]
- It would be nice to document the extra gas costs for using this instead of a normal ERC20.
- Comments are not following natspec.
Snapshotevent should have a comment that mentions when it is emitted.
- The justification for the struct implementation is very poor: " These could be an array of a Snapshot struct, but that would impede usage of functions that work on an array." It could be a good decision, but this comment makes me doubt it. It seems that the design is made to fit the libraries, instead of implementing the right design and libraries for it. Being super strict (because I like you ), for things like this I would expect a link where the more-straightforward implementation is compared to the chosen one, and the winner has at least some merits.
- Typo: snapshoted -> snapshotted. [PR#1640]
- I think snapshot is not a nice verb. I suggest to rename the
SnapshotMade, and never ever say snapshotted.
- Snapshot ids start at 1. This is weird, and we are more used to 0-indexing so it could lead to off-by-1 errors.
- Digging into the above, I see that findUpperBound returns 0 to signal an error. This is wrong, 0 is a valid element of an array. We need a better way to handle errors. Because solidity's exceptions are not very useful here, we can use the go style of returning a tuple with a boolean to indicate success, a style already used in this contract.
- I thought it would be nice to record the block of the snapshot somewhere, maybe even using it as the id. However, multiple transactions can be in the same block, so there could be multiple snapshots with the same block.
- Anybody can call snapshot. There should be a role for this.
- The snapshot implementation would be nicer if
Counter.incrementreturned the new value. Any reason not to do this?
- balanceOfAt has no comment.
- I dislike
balanceOf, but balanceOfAt is terrible. I suggest
- totalSupplyAt has no comment.
- I suggest
- The contract overrides
burn. It would be better to refactor ERC20 to have functions
_updateTotalSupply, and override those two.
_updateSnapshot has an
else. This code path is taken when either no snapshots have been made and it is ok to do nothing; or when
_lastSnapshotId(snapshots.ids) > currentIdwhich is an impossible state with the current code, confirmed by the comprehensive test suite. There should be a comment explaining that there is no
elseby design, instead of an error condition that's not failing loudly. In particular, special care has to be taken to make sure that this function is the only place that modifies the arrays, to avoid breaking the strict relation of
_lastSnapshotId(snapshots.ids) <= currentId.
- There is no test that checks the return value of