New Migrations #1028

cgewecke · 2018-06-18T03:04:56Z

Opening to keep track of what still needs to be done this week.

Color
Remove support for batch deployments
Fix E2E CI at wild-truffle for monorepo and check colonyNetwork / aragon migrations
Should we have a classic migrations output option? To minimize noise in CI?
Publish hd-wallet-provider V5 (fork provider-engine to Truffle / merge cgewecke's in)

Gists of the output (So we can start to make this look better)

simple migration
production migration w/ dry-run (against geth --dev)
error case: revert
error case: oog

E2E Testing vs. Rinkeby / Ropsten / MintTestNet (Settlemint/Parity POA network)

Everything is set up here to deploy Consensys/PLCRToken to these testnets. Just clone repo (recursively), set up a secrets.js with a funded mnenomic, and follow README instructions to install and run at this branch's current commit.

Known Problems

~~geth ws is broken for large data sends & web3 1.0 has broken confirmations polling over rpc.~~ (Fix w/ block-polling)
HDWallet uses web3-provider-engine which isn't web3 1.0 compatible because the latter stopped supporting the sendAsync method, renaming it send. provider-engine hates send though because it used to be sync. Believe this will require we temporarily fork provider-engine.
~~Account are not really forking.~~ (Fixed at ganache, awaiting publication)

Goals

Add substantive tests / fixtures for this feature so it can be maintained & debugged.
Make async/await behave intuitively without breaking the current 'then-able' pattern.
Meaningful error handling. Should detect constructor reverts/OOG/nonce etc and offer advice.
Expose revert reason strings (ganache only atm)
Configurable confirmation delays (by network)
Configurable block timeouts (by network)
Display more data, more clearly
--dry-run
- Auto-runs when it detects that a migration targets a public network
- Interactive prompt before running real migration
Decouple logging and migrations logic with an async Reporter

Example command

$ truffle migrate --network rinkeby --interactive

Example config

networks: {
  rinkeby: {
    provider: () => new HDWalletProvider(mnemonic, `https://rinkeby.infura.io/${infura}`),
    network_id: 4,
    gas: 6000000,
    gasPrice: 20000000000,
    confirmations: 2,     // # confirmations to wait between each deployment
    timeoutBlocks: 75   // # of blocks to wait for network to mine a deployment before erroring
  },
}

GIF

Visual Review (future improvements):

@gnidan thinks migrations have too much data.
dry-run should be clearly distinguished
4 second increments are weird (we should lie)
more interactivity
Make sure block limits are same dry-run and real.
Move error handling to truffle-contract / truffle-error
--only flag
Spinner for saving migrations

cgewecke · 2018-07-03T18:44:16Z

packages/truffle-deployer/src/deployment.js

+      await Promise.all(deployments);
+      await self.emitter.emit('postDeployMany', arr);
+    };
+  }


Delete this.

gnidan

(first pass)

gnidan · 2018-07-03T18:12:57Z

packages/truffle-contract/lib/execute.js

+      }
+    }
+
+    if (isString && res.result.includes('0x08c379a0')){


gnidan · 2018-07-03T18:40:16Z

packages/truffle-deployer/src/deployment.js

+  }
+};
+
+module.exports = Deployment;


No new line at end of file

For those of us old-heads that like to cat their files on the terminal

But really I came here to say that _methods should not be called outside of this class

gnidan · 2018-07-03T18:43:32Z

packages/truffle-deployer/src/deployment.js

+      contract.transactionHash = instance.transactionHash;
+      return instance;
+    };
+  }


This method is a bit long? Maybe worth chopping it into pieces? Up to you

Ah but we should at least move the public methods above the private ones.

gnidan · 2018-07-03T18:50:18Z

packages/truffle-migrate/migration.js

+      if (options.save === false) return;
+
+      // Write migrations record to chain
+      const Migrations = resolver.require("./Migrations.sol");


resolver.require("Migrations")

Hmmm. Need to investigate this a bit. Is the hard-coding of the path (carried over from original) the cause of the bug identified here. Or.....?

It's just more canonical. Artifacts are by contract name, not filename.

The older migrate tests at truffle-core crash when this change is made. I want to deal with separately from this PR as a bug in the resolver - not sure what's going here....

gnidan

Here's another round of review. I think you said you have some more changes you want to make, so I'll wait for those to then try to catch anything I missed.

All-in-all, this is an impressive effort.

gnidan · 2018-07-09T22:18:10Z

packages/truffle-contract/lib/execute.js

+        }
+
+        web3
+          .eth


I kind of prefer the way you had it with web3.eth. I'd also be into web3.eth.estimateGas(params) all on one line, but I can see why separating it looks nice.

gnidan · 2018-07-09T22:21:48Z

packages/truffle-contract/lib/execute.js

+      web3.currentProvider.send(packet, (err, response) => {
+        if (response && (response.error || response.result)) {
+          reason = execute.extractReason(response, web3)
+        }


Can the if just be moved into extractReason here?

Ah so provider.send() still only uses a callback

gnidan · 2018-07-09T22:32:44Z

packages/truffle-contract/lib/execute.js

+            (params.gas)
+              ? accept({gas: params.gas, error: err})
+              : accept({gas: null, error: err});
+          })


This is all really pretty

gnidan · 2018-07-09T22:35:45Z

packages/truffle-contract/lib/execute.js

+        return web3.eth.abi.decodeParameter('string', data[hash].return.slice(10))
+      }
+
+    } else if (isString && res.result.includes('0x08c379a0')){


What's that value? I think let's make this a const at the top of the function so it gets a name.

I guess derived from this axic comment in the EIP ? 🙂

Now we could instead just assume that there is a generic error with a signature of Error(string), which hashes to 08c379a0 followed by an ABI encoded string, therefore revert("oh noes") would result in the data 08c379a0 00...20 00..07 6f68206e6f6573..00

gnidan · 2018-07-09T22:38:35Z

packages/truffle-contract/lib/execute.js

@@ -131,6 +176,7 @@ var execute = {
      var args = Array.prototype.slice.call(arguments);
      var params = utils.getTxParams.call(constructor, args);
      var promiEvent = new Web3PromiEvent();
+      var reason;


This doesn't appear to be used

gnidan · 2018-07-09T23:39:25Z

packages/truffle-migrate/migration.js

+
+const util = require('util');
+
+class Migration {


Oh nice, one of these!

gnidan · 2018-07-09T23:40:15Z

packages/truffle-migrate/migration.js

+    };
+  }
+
+  // ------------------------------------- Public -------------------------------------------------


Oh wait you're doing private first? That's cool as long as we're consistent.

gnidan · 2018-07-09T23:40:49Z

packages/truffle-migrate/reporter/indentedSpinner.js

+    "   ⠏"
+  ]
+}
+


I feel like there's gotta be a better way to get indents under control.

gnidan · 2018-07-09T23:41:10Z

packages/truffle-migrate/reporter/messages.js

+  }
+}
+
+module.exports = MigrationsMessages;


I dig the messages separation of concerns

gnidan · 2018-07-09T23:42:04Z

packages/truffle-migrate/reporter/reporter.js

+}
+
+module.exports = Reporter;
+


Add truffle-reporters

gnidan

🎉

gnidan · 2018-07-12T18:04:08Z

packages/truffle-contract/test/sources/Example.sol

@@ -13,6 +13,7 @@ contract Example {
    // Constructor revert
    require(val != 13);
    require(val != 2001, 'reasonstring');
+    require(val != 20001, 'solidity storage is a fun lesson in endianness');


cgewecke · 2018-07-12T20:57:21Z

Aiiii!!!! It's over.

cgewecke force-pushed the async-deployer branch from d100eca to 4ba5caa Compare June 21, 2018 07:28

cgewecke mentioned this pull request Jun 24, 2018

A misleading error message if an interface is not implemented completely #1063

Closed

cgewecke force-pushed the async-deployer branch 2 times, most recently from f9fcd8a to 5a40e27 Compare June 29, 2018 05:44

cgewecke force-pushed the next branch from 8cc5dab to 293be2c Compare June 29, 2018 21:39

cgewecke requested a review from gnidan July 2, 2018 15:11

cgewecke added 24 commits July 2, 2018 13:11

Migrations to ES6

fdff074

Add migrations emitter and reporter draft

61af79f

Add reporter details, formatting

f1f96bb

Working async/await and basic reporter format

10242e4

Remove unused code

3e62aac

Add test fixtures, deployer tests

d6cab54

Convert deployer to class

9c71d07

Convert DeferredChain to class

9b3d10b

Add async/await test cases

995f540

First draft: async-eventemitter

f53e932

Second draft: reporters / working

2d15b92

Third draft: errors

e16cd4b

Revert changes to deferredchain / add test

8348d62

Migrations scenario tests (draft)

c32b116

Update yarn.lock

a409d36

Add dry run report (draft)

deaebcc

Copy gasPrice and gasLimit on fork

40dbbd3

Move reporter to outer scope

fd99765

Upgrade ganche-core dev dep to 2.1.2

6125d4a

Add reason string handling (truffle-contract)

1513236

Fix extra require/yarn.lock

63ae9eb

Fix err.return check

ca16e6f

Add geth specific error handling

de626c8

Add revert with reason test

76ca3ae

cgewecke commented Jul 3, 2018

View reviewed changes

gnidan reviewed Jul 3, 2018

View reviewed changes

cgewecke added 7 commits July 3, 2018 15:58

Fix newlines / error msg spacing

9c98ee8

Deprecate batch deployments

663c96c

Clarify public/private methods in deployment class

f325fd2

Clarify extractReasonString conditional logic

b8a5f17

Stop using deployer array syntax for solidity test setup

73f0cfa

Clean up migrate command

2af8387

Fix production tests for Geth

54fd631

cgewecke changed the title ~~[WIP] New Migrations~~ New Migrations Jul 4, 2018

Make sure fork tracks parent block gasLimit

d2c03e0

gnidan reviewed Jul 9, 2018

View reviewed changes

cgewecke added 12 commits July 11, 2018 19:32

Add reason string to error messages

509a543

Clean up reason string logic

d0ce12e

Use canonical migrations name

0e1f8dd

Add note about --exit flag

5d39172

Cleaup if copy fails

d9ea3f3

Add test for long reason string

c754c89

Remove unused logic for 50 blocks override

e4d3da9

Move migrations reporter to truffle-reporters

05719d4

Fix deployer tests

9218386

Remove test reporter

fb288f0

Remove unused batch deployment logic in reporter

393f992

Merge pull request #1099 from trufflesuite/add-truffle-reporters

59067fa

Add truffle-reporters

gnidan approved these changes Jul 12, 2018

View reviewed changes

cgewecke merged commit 298523c into next Jul 12, 2018

mikeseese deleted the async-deployer branch November 20, 2018 23:04

ldct mentioned this pull request Dec 4, 2018

skip already linked contracts instead of all contracts #1489

Merged

haltman-at mentioned this pull request Feb 11, 2022

Dry-run public networks list is out of date #4720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Migrations #1028

New Migrations #1028

cgewecke commented Jun 18, 2018 •

edited

Loading

cgewecke Jul 3, 2018

gnidan left a comment

gnidan Jul 3, 2018

cgewecke Jul 3, 2018

gnidan Jul 3, 2018

gnidan Jul 3, 2018

cgewecke Jul 4, 2018

gnidan Jul 3, 2018

gnidan Jul 9, 2018

gnidan Jul 3, 2018

cgewecke Jul 3, 2018

cgewecke Jul 6, 2018

gnidan Jul 10, 2018

cgewecke Jul 12, 2018

gnidan left a comment •

edited

Loading

gnidan Jul 9, 2018

cgewecke Jul 10, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

cgewecke Jul 12, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

cgewecke Jul 10, 2018

cgewecke Jul 12, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

gnidan Jul 9, 2018

gnidan left a comment

gnidan Jul 12, 2018

cgewecke commented Jul 12, 2018

+                      }
+                      web3
+                        .eth

		}

		module.exports = Reporter;

New Migrations #1028

New Migrations #1028

Conversation

cgewecke commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

gnidan 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

gnidan left a comment • edited Loading

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

Choose a reason for hiding this comment

gnidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgewecke commented Jul 12, 2018

cgewecke commented Jun 18, 2018 •

edited

Loading

gnidan left a comment •

edited

Loading