Add ERC20 opt-in migration contract #1054

facuspagnuolo · 2018-06-29T12:00:28Z

🚀 Description

This PR adds a new contract to migrate ERC20 tokens with an opt-in strategy.
Additionally, I'm extracting the behavior expected of the StandardToken so we can start reusing it in different test files.

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

spalladino

Looks good! There are a few changes I'd require though, and left a few thoughts as well. One pending question: what's the status on the new upcoming version of StandardToken with the _mint function? Can we count on that one for the next release, or is it planned for 2.0 @frangio?

spalladino · 2018-07-17T21:05:50Z

test/token/ERC20/StandardToken.test.js

@@ -1,469 +1,11 @@
-import assertRevert from '../../helpers/assertRevert';
+import shouldBehaveLikeStandardToken from './behaviors/StandardToken.behavior';


There is a typo in behavior, it should be behaviour

Jokes aside, all other behaviour files are using british spelling; we should remain consistent with that.

spalladino · 2018-07-17T21:09:51Z

test/token/ERC20/MigratableERC20.test.js


-contract('OptInERC20Migration', function ([_, owner, recipient, anotherAccount]) {
+contract.only('OptInERC20Migration', function ([_, owner, recipient, anotherAccount]) {


Remove this .only, I think it was the cause of coverage falling to about 7% :-P

spalladino · 2018-07-17T21:16:22Z

contracts/token/ERC20/MigratableERC20.sol

@@ -56,17 +55,12 @@ contract OptInERC20Migration is StandardToken {
   */
  function migrateTokenTo(address _to, uint256 _amount) public {
    _mint(_to, _amount);
-    legacyToken.transferFrom(msg.sender, BURN_ADDRESS, _amount);
+    legacyToken.safeTransferFrom(msg.sender, BURN_ADDRESS, _amount);


I'd consider swapping the two calls. I couldn't find any attack vector, but I find it good practice to decrement balance before incrementing it, and not the other way around.

I see your point but note that we are not decreasing and incrementing the same balance here. Given that we need to call a foreign contract, I went with the check-effects-interactions pattern instead.

spalladino · 2018-07-17T21:19:36Z

test/token/ERC20/BurnableToken.test.js

@@ -1,4 +1,4 @@
-import shouldBehaveLikeBurnableToken from './BurnableToken.behaviour';
+import shouldBehaveLikeBurnableToken from './behaviors/BurnableToken.behaviour';


I'd leave this refactor for a different PR, otherwise we are risking a lot of conflicts while we review this one. If we do decide to keep it here, then I'd use behaviours for consistency 🇬🇧.

spalladino · 2018-07-17T21:22:46Z

test/token/ERC20/MigratableERC20.test.js

+  });
+
+  describe('migrate', function () {
+    describe('when the approved balance is higher or equal to the owned balance', function () {


Add a test to check that the balance of the owner in legacyToken is zero. This applies also to the other describe blocks. You may want to add a single shouldHaveTransferred that sums up that:

the balance of the owner in the new contract was updated

the balance of the owner in the old contract was updated

the balance of 0xdead in the old contract was updated

total supply for the new token was updated

facuspagnuolo · 2018-07-19T01:06:12Z

Comments addressed, please take another look @spalladino

frangio

Babel has been removed (#1074) and we're no longer using ES6 modules. Please change from import statements to const xyz = require('xyz'); statements!

frangio · 2018-07-23T14:42:27Z

contracts/token/ERC20/MigratableERC20.sol

+   * @dev Migrates the total balance of the token holder to this token contract
+   * @dev This function will burn the old token balance and mint the same balance in the new token contract
+   */
+  function migrate() public {


Suggested renaming:

migrate → migrateAll

migrateToken → migrate

frangio · 2018-07-23T14:42:44Z

contracts/token/ERC20/MigratableERC20.sol

+   * @param _amount uint256 representing the amount of tokens to be migrated
+   * @param _to address the recipient that will receive the new minted tokens
+   */
+  function migrateTokenTo(address _to, uint256 _amount) public {


I would remove migrateTokenTo, and put all the migration logic in migrate. migrateTokenTo is like a migration + a transfer, and keeping those things separate sounds more simpler and correct to me.

I guess I'm not following, do you prefer holding all the logic together in migrate or separated somehow?

frangio · 2018-07-23T18:58:04Z

contracts/token/ERC20/MigratableERC20.sol

+/**
+ * @title MigratableERC20
+ * @dev This strategy carries out an optional migration of the token balances. This migration is performed and paid for
+ * @dev by the token holders. The new token contract starts with no initial supply and no balances. The only way to


Please remove the @dev tag from all but the first line here.

frangio · 2018-07-23T18:59:54Z

contracts/token/ERC20/MigratableERC20.sol

+ * @dev "mint" the new tokens is for users to "turn in" their old ones. This is done by first approving the amount they
+ * @dev want to migrate via `ERC20.approve(newTokenAddress, amountToMigrate)` and then calling a function of the new
+ * @dev token called `migrateTokens`. The old tokens are sent to a burn address, and the holder receives an equal amount
+ * @dev in the new contract.


These inline docs explain how the contract works but not how it's used. I'd add a note saying at least that it's an abstract contract that must be combined with StandardToken or the user's own token implementation (with a _mint internal function), and if we have a link to a guide for how to use it I'd put that here as well!

facuspagnuolo · 2018-07-27T09:09:11Z

@frangio I had already changed all the import statements in favour of require ones. I'm not finding any imports left, please point them out in case you see one.

…_optin_migration_contract

Refactor

nventuro · 2018-09-07T14:38:29Z

contracts/drafts/ERC20Migrator.sol

+ * `isMinter(address)` and `mint(address, amount)`. The migrator will check
+ * that it is a minter for the token.
+ * The balance from the legacy token will be transfered to the migrator, as it
+ * is migrated, and remain here forever.


Consider replacing here for there

nventuro · 2018-09-07T14:39:26Z

contracts/drafts/ERC20Migrator.sol

+ * is migrated, and remain here forever.
+ *
+ * Although this contract can be used in many different scenarios, the main
+ * motivation was to provide a way of how an ERC20 token can be migrated to an


Consider replacing a way of how an ERC20 token can be migrated to an for a way to migrate ERC20 tokens into an

nventuro · 2018-09-07T14:41:40Z

contracts/drafts/ERC20Migrator.sol

+  }
+
+  /**
+   * @dev Burns a given amount of the old token contract for a token holder and mints the same amount of


Consider rephrasing; tokens aren't burned, but instead transfered here.

nventuro · 2018-09-07T14:42:18Z

contracts/drafts/ERC20Migrator.sol

+   */
+  function migrate(address account, uint256 amount) public {
+    _newToken.mint(account, amount);
+    _legacyToken.safeTransferFrom(account, this, amount);


I'd swap these two statements, for clarity (i.e. first transfer to migrator, then mint new tokens).

nventuro · 2018-09-07T14:44:54Z

contracts/drafts/ERC20Migrator.sol

+
+  /**
+   * @dev Migrates the total balance of the token holder to this token contract
+   * @dev This function will burn the old token balance and mint the same balance in the new token contract


Again, burn is not accurate.

nventuro · 2018-09-07T14:51:15Z

test/drafts/ERC20Migrator.test.js

+  });
+
+  describe('beginMigration', function () {
+    it('rejects zero address for token', async function () {


As per #1157, this should read something like 'reverts with a null new token address'

nventuro · 2018-09-07T14:51:47Z

test/drafts/ERC20Migrator.test.js

+  });
+
+  describe('once migration began', function () {
+    beforeEach('', async function () {


Remove the empty block description

nventuro · 2018-09-07T14:53:18Z

test/drafts/ERC20Migrator.test.js

+          await this.legacyToken.approve(this.migrator.address, amount, { from: owner });
+        });
+
+        beforeEach('migrating token', async function () {


This describe has two beforeEach blocks, which are not independent: I'm not sure if Mocha specifies in which order they'll be run (or even if both will be run), but we should avoid this situation IMO.

nventuro · 2018-09-07T14:54:14Z

test/drafts/ERC20Migrator.test.js

+    describe('migrateAll', function () {
+      const baseAmount = totalSupply;
+
+      describe('when the approved balance is higher or equal to the owned balance', function () {


nit: this only tests for equal to balance, not higher

nventuro · 2018-09-07T14:55:49Z

test/drafts/ERC20Migrator.test.js

+        await this.legacyToken.approve(this.migrator.address, baseAmount, { from: owner });
+      });
+
+      describe('when the amount is lower or equal to the one approved', function () {


This only tests for equal

spalladino

Looks good to merge. I added several notes on comments and the tests, but nothing too important, and can be reviewed later. The code itself is solid. The only thing I'd consider before merging is whether we want to allow migrations with amount=zero (not sure if I'm for or against this, just throwing it out for consideration).

spalladino · 2018-09-07T14:24:40Z

contracts/drafts/ERC20Migrator.sol

+
+/**
+ * @title ERC20Migrator
+ * @dev This contract can be used to to migrate an ERC20 token from one


to to migrate

spalladino · 2018-09-07T14:42:54Z

contracts/drafts/ERC20Migrator.sol

+  ERC20Mintable private _newToken;
+
+  /**
+   * @dev Constructor function. It initializes the new token contract


It initializes the new token contract

legacy

spalladino · 2018-09-07T14:43:34Z

contracts/drafts/ERC20Migrator.sol

+    _legacyToken = legacyToken;
+  }
+
+  function beginMigration(ERC20Mintable newToken) public {


Missing natspec docs in this function

spalladino · 2018-09-07T14:45:53Z

contracts/drafts/ERC20Migrator.sol

+ * upgradeable version of it using ZeppelinOS. To read more about how this can
+ * be done using this implementation, please follow the official documentation
+ * site of ZeppelinOS: https://docs.zeppelinos.org/docs/erc20_onboarding.html
+ */


I'd add an example here of how to set up the ERC20 migration. Something like:

const migrator = await ERC20Migrator.new(legacyToken.address); await newToken.addMinter(migrator.address); await migrator.beginMigration(newToken.address);

spalladino · 2018-09-07T14:50:47Z

contracts/drafts/ERC20Migrator.sol

+   * @param account uint256 representing the amount of tokens to be migrated
+   * @param amount address the recipient that will receive the new minted tokens
+   */
+  function migrate(address account, uint256 amount) public {


Should we require that amount > 0? Just asking

The (soft) policy we currently have is that no-op transactions (barring events being emitted) are allowed, to prevent burdening the caller with the responsibility of checking for non-zero.

spalladino · 2018-09-07T14:56:16Z

test/drafts/ERC20Migrator.test.js

+    describe('migrateAll', function () {
+      const baseAmount = totalSupply;
+
+      describe('when the approved balance is higher or equal to the owned balance', function () {


Note that this block is only testing approved balance equal to the owned balance, not higher to. There is another block missing, where approved is amount + 1.

spalladino · 2018-09-07T14:57:01Z

test/drafts/ERC20Migrator.test.js

+        await this.legacyToken.approve(this.migrator.address, baseAmount, { from: owner });
+      });
+
+      describe('when the amount is lower or equal to the one approved', function () {


Same here, it's testing equal only

spalladino · 2018-09-07T14:57:50Z

test/drafts/ERC20Migrator.test.js

+        it('updates the total supply', async function () {
+          const currentSupply = await this.newToken.totalSupply();
+          currentSupply.should.be.bignumber.equal(amount);
+        });


These three tests (mints amount, burns, updates) are repeated here and in the previous block. I'd add a helper that is parameterized with amount and runs the three tests, to avoid code duplication.

spalladino · 2018-09-07T14:58:28Z

test/drafts/ERC20Migrator.test.js

+  .use(require('chai-bignumber')(BigNumber))
+  .should();
+
+contract('ERC20Migrator', function ([_, owner, recipient, anotherAccount]) {


I'd add a test for two subsequent migrations of the same account

spalladino · 2018-09-07T15:00:57Z

contracts/drafts/ERC20Migrator.sol

+   */
+  function migrate(address account, uint256 amount) public {
+    _newToken.mint(account, amount);
+    _legacyToken.safeTransferFrom(account, this, amount);


Since minting happens before transferring, there could be some odd behaviours if the legacy and new token are the same (you could migrate a near-infinite amount). But I'm not sure that's a scenario we even want to consider.

Hm, I guess we could make sure legacyToken and newToken are not the same contract?

Not sure if there is any point in it, as one could be a proxy, so you have no way to compare. I mentioned it just because it's generally a good practice to decrement (ie transfer) before incrementing (ie minting).

I thought about that, but thought there was value in preventing a simple user error.

…y into feature/add_erc20_optin_migration_contract

frangio · 2018-09-07T17:13:18Z

Tests passed locally.

facuspagnuolo mentioned this pull request Jun 29, 2018

Add ERC20 opt-in migration contract for zOS #1052

Closed

4 tasks

facuspagnuolo force-pushed the feature/add_erc20_optin_migration_contract branch from 0d06f87 to 624cf45 Compare June 29, 2018 12:08

facuspagnuolo force-pushed the feature/add_erc20_optin_migration_contract branch 2 times, most recently from db9db8e to 6da80e5 Compare July 12, 2018 20:54

spalladino previously requested changes Jul 17, 2018

View reviewed changes

facuspagnuolo force-pushed the feature/add_erc20_optin_migration_contract branch 6 times, most recently from 1cd0f47 to 37083f4 Compare July 19, 2018 01:05

frangio added feature New contracts, functions, or helpers. contracts Smart contract code. labels Jul 20, 2018

frangio mentioned this pull request Jul 20, 2018

Migratable token #264

Closed

frangio previously requested changes Jul 23, 2018

View reviewed changes

facuspagnuolo force-pushed the feature/add_erc20_optin_migration_contract branch 2 times, most recently from 2263d32 to 33fa961 Compare July 27, 2018 09:17

frangio added this to the v2.0 milestone Aug 17, 2018

frangio self-assigned this Aug 29, 2018

facuspagnuolo and others added 9 commits September 5, 2018 19:12

Extract standard token behaviuor to reuse it in other tests

a1f6cf5

Add opt in ERC20 migration contract

7f285cc

Make migration contract not to depend from standard token

2fa7975

Changes based on feedback

7f206de

Improve MigratableERC20 inline documentation

c1b6966

move behaviors to behaviors directory

248b86a

refactor MigratableERC20 into ERC20Migrator

0d9911e

Merge remote-tracking branch 'upstream/master' into feature/add_erc20…

8787c2d

…_optin_migration_contract

fix errors

c48371d

frangio added 2 commits September 7, 2018 09:42

change expectEvent to support multiple events with same name

a2daf43

fix tests

2b7b3e1

frangio force-pushed the feature/add_erc20_optin_migration_contract branch from 37a2110 to 2b7b3e1 Compare September 7, 2018 12:49

update documentation

e5b92de

frangio requested a review from nventuro September 7, 2018 13:10

frangio added 4 commits September 7, 2018 10:22

rename MigratableERC20 files to ERC20Migrator

917b078

move to drafts

0ebd63f

test beginMigration

1b335fe

rename to ERC20Migrator

1daa82b

frangio requested a review from spalladino September 7, 2018 13:39

frangio added 2 commits September 7, 2018 11:12

missing semicolon (╯°□°）╯︵ ┻━┻

dab2b12

add non-zero check

397755d

nventuro reviewed Sep 7, 2018

View reviewed changes

spalladino previously approved these changes Sep 7, 2018

View reviewed changes

frangio added 6 commits September 7, 2018 12:13

improve documentation based on review comments

313acc5

improve test descriptions

e6d3913

improve docs

e2a9fed

add getters

1ff1258

fix contract

04f76ac

improve tests

8fa9add

frangio dismissed spalladino’s stale review via 8fa9add September 7, 2018 16:02

nventuro approved these changes Sep 7, 2018

View reviewed changes

Merge branch 'master' of github.com:OpenZeppelin/openzeppelin-solidit…

e31fc16

…y into feature/add_erc20_optin_migration_contract

frangio merged commit 92133be into OpenZeppelin:master Sep 7, 2018

		@@ -1,469 +1,11 @@
		import assertRevert from '../../helpers/assertRevert';
		import shouldBehaveLikeStandardToken from './behaviors/StandardToken.behavior';


		contract('OptInERC20Migration', function ([_, owner, recipient, anotherAccount]) {
		contract.only('OptInERC20Migration', function ([_, owner, recipient, anotherAccount]) {

		@@ -1,4 +1,4 @@
		import shouldBehaveLikeBurnableToken from './BurnableToken.behaviour';
		import shouldBehaveLikeBurnableToken from './behaviors/BurnableToken.behaviour';

Add ERC20 opt-in migration contract #1054

Add ERC20 opt-in migration contract #1054

Conversation

facuspagnuolo commented Jun 29, 2018

🚀 Description

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Jul 19, 2018

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio commented Sep 7, 2018

facuspagnuolo Jul 19, 2018 •

edited

Loading

facuspagnuolo Jul 27, 2018 •

edited

Loading