Adding tokens/ERC20/PreserveBalancesOnTransferToken.sol

[AnthonyAkentiev]

🚀 Description

Adding files:

1. contract/tokens/ERC20/PreserveBalancesOnTransferToken.sol
2. test/tokens/ERC20/PreserveBalancesOnTransferToken.test.sol

What is PreserveBalancesOnTransferToken?

Please see this Rationale with example here - https://medium.com/thetta/how-non-blocking-votings-work-in-thetta-46706f05b382

PreserveBalancesOnTransferToken can be used to preserve the ERC20 balances after some EVENT happens (voting is started, didivends are calculated, etc) without blocking the token transfers! Please notice that EVENT in this case has nothing to do with Ethereum events.

 * Example of usage (pseudocode):
 *   token.mint(ADDRESS_A, 100);
 *   assert.equal(token.balanceOf(ADDRESS_A), 100);
 *   assert.equal(token.balanceOf(ADDRESS_B), 0);
 *
 *   uint someEventID_1 = token.startNewEvent();
 *   token.transfer(ADDRESS_A, ADDRESS_B, 30);
 *
 *   assert.equal(token.balanceOf(ADDRESS_A), 70);
 *   assert.equal(token.balanceOf(ADDRESS_B), 30);
 *
 *   assert.equal(token.getBalanceAtEvent(someEventID_1, ADDRESS_A), 100);
 *   assert.equal(token.getBalanceAtEvent(someEventID_1, ADDRESS_B), 0);
 *
 *   token.finishEvent(someEventID_1);
 */

How do we use that in Thetta?

Please see https://github.com/Thetta/Thetta-DAO-Framework/blob/dev2/contracts/tokens/StdDaoToken.sol

We need PreserveBalancesOnTransferToken because we should not block transfers when new voting is started.

p.s. Also, i don't like the name of the PreserveBalancesOnTransferToken, but still has no other ideas((
Maybe NonBlockingToken?

===

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[nventuro]

Hi @AnthonyAkentiev, thanks for your contribution! We already have something along these lines in progress (#991), would that suit these needs?

[AnthonyAkentiev]

Hi, @nventuro.
I’ve updated the source code and added SnapshotToken to get balances easier!
So now we have 2 usage scenarious:

One

PreserveBalancesOnTransferToken token;

token.mint(ADDRESS_A, 100);
assert.equal(token.balanceOf(ADDRESS_A), 100);
assert.equal(token.balanceOf(ADDRESS_B), 0);

SnapshotToken snapshot = token.createNewSnapshot();
token.transfer(ADDRESS_A, ADDRESS_B, 30);

assert.equal(token.balanceOf(ADDRESS_A), 70);
assert.equal(token.balanceOf(ADDRESS_B), 30);

// see here. snapshot is ERC20 compatible, but transfers are locked
assert.equal(snapshot.balanceOf(ADDRESS_A), 100);
assert.equal(snapshot.balanceOf(ADDRESS_B), 0);

token.stopSnapshot(snapshot);

Two

PreserveBalancesOnTransferToken token;

token.mint(ADDRESS_A, 100);
assert.equal(token.balanceOf(ADDRESS_A), 100);
assert.equal(token.balanceOf(ADDRESS_B), 0);

uint someEventID_1 = token.startNewEvent();
token.transfer(ADDRESS_A, ADDRESS_B, 30);

assert.equal(token.balanceOf(ADDRESS_A), 70);
assert.equal(token.balanceOf(ADDRESS_B), 30);

assert.equal(token.getBalanceAtEvent(someEventID_1, ADDRESS_A), 100);
assert.equal(token.getBalanceAtEvent(someEventID_1, ADDRESS_B), 0);

token.finishEvent(someEventID_1);

Yes, seems like in #991 is about the same. I haven't seen it when we started to implement the PreserveBalancesOnTransferToken :-(

However, we have a different design and i think that our implementation (+tests, +code coverage, etc) is little better. How to deal with that? Is there any way we could move forward with the code review?

p.s. I will fix the build little bit later (within 1-2 days)

[frangio]

Hi @AnthonyAkentiev! It's great to see that we felt the need for the same feature.

I actually like the implementation in #991 a lot more. It was only a prototype so it doesn't have any tests, as you noted, or documentation. However, I think it is a lot simpler and easier to understand, which is important to verify correctness and security.

There is no arbitrary limit to the number of snapshots in #991, like the limit of 20 concurrent events here. This means there's no need for a notion of stopping and starting events, reducing the conceptual complexity, and no need to restrict taking snapshots to an owner account to prevent attacks. This is accomplished by using a binary search to get past balances, which does add some complexity to the implementation, but it is definitely justified by the resulting simplicity of the rest of the contract.

The idea of the auxiliary SnapshotToken included in this PR is quite interesting, but I'm not sure it's actually needed. Did you find any practical situations where you need to expose past balances through an ERC20 interface?

As for how to move forward with this PR, I'm afraid I have a strong preference for #991. If you're interested in collaborating on getting this feature included in OpenZeppelin, we would definitely welcome tests and documentation over there.

[AnthonyAkentiev]

@frangio Thx for the reply.

There is no arbitrary limit to the number of snapshots in #991, like the limit of 20 concurrent events here.

I can refactor the code so that no limit will be set.

The idea of the auxiliary SnapshotToken included in this PR is quite interesting, but I'm not sure it's actually needed. Did you find any practical situations where you need to expose past balances through an ERC20 interface?

Yes, this is a mandatory feature (in case of our DAO Framework for example). You can pass this snapshot token to any method/contract that is ERC20-compatible-only and get the balances right at the time event is started. There is no need to pass any interface and rewrite the code. Just pass an address of the Snapshot and everything will work fine. I think that it's great and simplifies everything. Under the hood code is a bit more complex of course, but the clients will not see that.

So what we have now:

1. 991: no tests, code is not ready, no SnapshotToken, has no limit, no burn()/mint() support, little bit simpler
2. 1100 (this): good tests, code is ready, SnapshotToken (what is think is a good feature), has limit of 20. Also we have code for mint() and burn() that is compatible with snapshotting.

Of course guys from 991 can add tests and improve the code.
And i can remove the limit and simplify things.

So comparison will be then like this:

1. 991: no SnapshotToken but started earlier.
2. 1100: SnapshotToken and started a bit later.

So if comparing like that, is there any possibility so that i should continue the 1100 and fix everything?
I think that SnapshotToken worth it?

Thx!

[AnthonyAkentiev]

@frangio Ah, you are the 991 creator?

No stopSnapshot in 991

Also, i wanted to say about the limit to 20 snapshots. This is just in order to reduce gas costs.
I see that you use 'binary search' that still need while() loop to move through all snapshots.
Still, i can eliminate the magic number 20 and allow infinite snapshots just like in your case.

Still:
In my implementation you can stop the particular snapshot by calling stopSnapshot and that will make sure we don't spend gas for unused anymore snapshots. In your implementation you CAN NOT stop the snapshot from updating (as far as i understood). Just imagine your token and 5 years of snapshotting. We will end up with 100500 snapshots that will eat gas in your case, because you don't have the stopSnapshot method.

[frangio]

I see that you use 'binary search' that still need while() loop to move through all snapshots.

The loop doesn't move through all snapshots. Because it's a binary search it will only move through O(log(snapshot count)) snapshots, and in practice it will be much lower than that. Crucially, though, this relatively expensive loop is only done in the special function balanceOfAt, whereas the normal ERC20 operations have only a constant overhead compared to the standard token implementation:

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/61d16d6998055de79570558b717314e00d9bcbc2/contracts/token/ERC20/SnapshotToken.sol#L54-L59

I went through your implementation in more detail. It potentially loops over all snapshots in every transfer operation, which is why you initially capped it to 20. Removing the cap is not a good idea because the loop can eventually become arbitrarily large resulting in prohibitive gas costs for using the token. The cap was the right choice for this implementation, but while it may be enough for a particular use case, it's not quite enough for a general-purpose library like OpenZeppelin.

The point about minting and burning is a good one. There was no way for #991 to correctly handle that, but after #1197 is merged it will be simple to fix because of the new internal _mint and _burn functions.

I can't deny that #991 is way behind this PR in terms of tests and documentation. But unfortunately there is problems in the implementation here that I think will be too much work to fix compared to taking #991 to completion.

I'm closing this because of everything explained. Thank you for taking the time to contribute, regardless!

[AnthonyAkentiev]

@frangio Thank you for the reply.

Still i don't agree with you and think that our implementation is better (it REMOVES the unused snapshots from the array, supports parallel snapshots, has SnapshotToken ERC20 wrapper, tests are ready, _mint and _burn already supported, etc).

Ok, we will just continue to use our code and it will be the part of the Thetta DAO Framework.

I hope that your decision is not based of "politics" but only on the technical side. Still looking forward for cooperation.