ERC20 Snapshot Impl #2

[mswezey23]

Fixes #1209

Note: due to vast changes within OZ's repo, I wiped out my own and re-forked to start clean versus tackling many different rebase patches, etc...

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).

[nventuro]

Hey there @mswezey23! Due to timing constraints I took the liberty of changing your code a bit so that it'd fit better with what we had in mind. I really like the addition of snapshotting the total supply, thanks for the idea! I also wanted to keep id and value inside a Snapshot struct, but sadly that would've prevented us from using Arrays.findUpperBound, which works on plain arrays.

[frangio]

LGTM overall. But please remove .DS_Store files.

[nventuro]

Force-pushed after a rebase to get the newly added Counters library.

[frangio]

LGTM

[mswezey23]

@nventuro Hey thanks for the feedback! Glad you liked the snapshotting of the total supply.

I had a question why you prefer the snapshot be taken before the super._transfer() call?

I've noticed in other implementations (MinimeToken for one) that they, too, took the snapshot before the balance change occurred.

My logic is this:
When one queries block 100. Does one really want to get the balance that was the result of block 99? Or does one want the true balance of the result of block 100?

Granted you've changed to on demand ids now versus blocknumber/timestamp but the argument remains the same.

Thanks :)

[frangio]

@mswezey23 I agree that if snapshots are per-block then taking the snapshot after the changes feels more natural. However, I can see an argument for taking the snapshot before the changes: all queries in the same block can be made to return the same value (the value before the entire block was executed). Otherwise, a transaction early in a block might return value X and another one later in the same block might return value X+n if there is a transfer in between. I can see this causing bugs. I don't know if MiniMe works like this, though.

Since we changed it to on-demand snapshot ids, we now need to store the balances corresponding to the last snapshot before they change in a transfer, because that last snapshot was in fact requested before the transfer.

Does this explain?

[mswezey23]

ll queries in the same block can be made to return the same value (the value before the entire block was executed). Otherwise, a transaction early in a block might return value X and another one later in the same block might return value X+n

Ah! Thank you. I've been looking for a good argument not to do it the way I implemented (minus the extra gas savings).

I will say, IIRC, the way I originally implemented the code the after-balance snapshot would update for every N transactions that occurred in the block since snapshots were done automatically. (Incurring extra gas costs per TX) So that was a none issue feature wise as the query would be accurate no matter how many transactions involved the same account occurring in that block.

        if (
            (checkpoints.length == 0) ||
            (checkpoints[checkpoints.length.sub(1)].fromBlock < block.number)
        ) {
            checkpoints.push(Snapshot(uint128(block.number), uint128(value)));
        } else {
            checkpoints[checkpoints.length.sub(1)].value = uint128(value);
        }

But since you're proceeding with an on-demand snapshot that only takes 1 snapshot for each account (much more gas friendly); you're forced to take the balance beforehand otherwise, as you already pointed out, any transactions afterwards involving the same account(s) included in the same block would cause the balance "snapped" to be inaccurate.

Thank you for your additional insight :)

[frangio]

Thanks @mswezey23 and @nventuro!