Add initial version of genesis deployment tool #458

hobofan · 2018-11-14T22:15:45Z

Adds the first version of the genesis deployment script(s).

This allows us to generate the accounts section for a parity chainspec, taking as input the bytecode of the compiled contracts and their constructor arguments (+ ABI). This greatly simplifies the bootstrapping of a new Auxiliary chain.

Also adds eslint for JS style conformance as per #404.

TODO:

Find a way to be able to invoke script directly instead of via truffle exec (truffle exec has a poluted STDOUT output)
A bit simpler interface for adding Truffle contracts
Add script to merge the output of one of these deployment scripts into a chainspec
Allow for specifying a address for a contract instance instead of incrementing from a fixed address.
More robust way of incrementing addresses (that works for more than 255 addresses)
Better documentation of the ContractRegistry functions

@pgev : I gave you access to my fork

schemar · 2018-11-15T08:45:30Z

tools/deployment_tools.js

+
+  takeNextAddress() {
+    const addressHex = web3.utils.bytesToHex(this.nextAvailableAddress);
+    this.nextAvailableAddress[this.nextAvailableAddress.length - 1] += 1;


Wouldn't it be easier to have a BN or similar and actually increase that? If I understand this approach correctly it is bound to 10 addresses, which may or may not be sufficient in the future.

It's bound to 255 (as it increments the last byte, not string character), but yeah, probably possible to use BN instead. I'll look into that.

Doesn't it increment the last hex character (4 bits => max 16)? And does it know to go from 9 to a? JavaScript magic? 🤔

this.nextAvailableAddress is a byte array, so no magic there 😉

But that code is gone now anyway 😄

Ah, somehow I thought it was a hex string 😳

hobofan · 2018-11-19T07:39:30Z

From my side this would be mergeable in the current state. Should be in a usable state for @pgev's work on the ValidatorSet. Next step would be applying this to the deployment sequence for auxiliary, now that #448 is merged.

hobofan · 2018-11-22T14:41:54Z

WIP again following some feedback from @pgev

hobofan · 2018-12-11T15:13:01Z

Back to review.

@pgev I added the state machine we talked about. Also added some tests for core parts of the functionality and a referencing system for contract addresses in constructor arguments (so we can also order those correctly).

schemar

Thanks @hobofan 🙌
It is generally clear and understandable.
It is good that there are tests 👍 What is the rough coverage?

It would definitely make sense to have a short readme or comment somewhere that explains in which context and/or how to use these things.

Some functions are very long and should be split. For example Contract::instantiate().
I would prefer one class per file. Not sure if we have a rule for that. @pgev ?

See further comments in-line.

.eslintrc

package.json

tools/deploy_token_genesis.js

schemar · 2018-12-19T10:46:50Z

tools/deploy_token_genesis.js

+const registry = new ContractRegistry();
+registry.addContract(EIP20Token);
+
+console.log(JSON.stringify(registry.toParityGenesisAccounts(), null, 4));


What is this for? How is this used?

tools/deployment_tools.js

tools/merge_chainspec_accounts.js

schemar · 2018-12-19T12:08:59Z

tools/merge_chainspec_accounts.js

+  chainspec.accounts[address] = obj;
+});
+
+console.log(JSON.stringify(chainspec, null, 4));


What is the purpose of this?

I don't know enough about JS, but are you using a logging mechanism to give feedback to the user on stdout? Can I change the behavior of console.log() to log somewhere else? E.g. a file? In that case I would prefer explicitly writing to stdout and not a log.

tools/test/deployment_tools.test.js

hobofan · 2018-12-19T16:27:38Z

@schemar I've incorporated your feedback! I've added a README to the tools directory, which also gives a high-level introduction.

As for the test coverage, it is mostly focused on ensuring the "happy path" flow for both Genesis and Live deployment works as expected, and that the ordering and reference mechanisms work.

tools/README.md

package.json

schemar

Phew nice 🐎💨

Just some minor things left 👍

schemar · 2018-12-19T16:50:32Z

package.json

@@ -6,8 +6,13 @@
    "abi-decoder": "1.2.0",
    "assert": "1.4.1",
    "bn.js": "4.11.8",
+    "chai": "4.2.0",
+    "eslint": "5.9.0",


Not using 5.10 for a reason?

Nope! Updating!

schemar · 2018-12-19T16:57:31Z

tools/deployment_tool/contract.js

+    }
+
+    /**
+     * Helper for loading a contract saved in a format followin the truffle-contract-schema.


Suggested change

* Helper for loading a contract saved in a format followin the truffle-contract-schema.

* Helper for loading a contract saved in a format following the truffle-contract-schema.

schemar · 2018-12-19T16:58:24Z

tools/deployment_tool/contract.js

+     *
+     * @param {string} contractName Name of the contract to load.
+     * @param {Array<*>} constructorArgs Arguments for the contract constructor.
+     * @param {object} options


Do you expect more options in the future?

Then it could as well be a rootDir argument 🤷‍♂️

schemar · 2018-12-19T16:59:52Z

tools/deployment_tool/contract.js

+    /**
+     * Returns the contract's address as detemined by the provided AddressGenerator.
+     *
+     * @param {Object|string} addressGeneratorOrAddress The AddressGenerator used for


I mean. I think I can accept this. But it kind of freaks me out.

Someone else should probably comment on this @benjaminbollen @pgev @deepesh-kn

I too prefer separate functions like setAddress and generateAddress

schemar · 2018-12-19T17:06:07Z

tools/deployment_tool/contract.js

+     * @returns {Contract|null} The referenced contract.
+     */
+    _getReferenceContract(constructorArg) {
+        if (!((typeof constructorArg === 'object') && (constructorArg !== null)) || constructorArg.__type !== TYPE_CONTRACT_REFERENCE) {


Maybe something like the below. I am sure this can inspire you to do something readable 😄

if(!isReference(constructorArg)) { return null; } function isReference(constructorArg) { if (!isInstance(constructorArg) { return false; } return constructorArg.__type === TYPE_CONTRACT_REFERENCE } function isInstance(constructorArg) { return typeof constructorArg === 'object' && constructorArg !== null; }

tools/deployment_tool/contract.js

tools/deployment_tool/contract_registry.js

hobofan · 2018-12-19T18:01:52Z

🔁 :)

schemar

LGTM, but someone else should definitely have a look.

Summoning @benjaminbollen @pgev @deepesh-kn

schemar · 2018-12-20T10:08:57Z

tools/deployment_tool/contract.js

+     *
+     * @param {string} contractName Name of the contract to load.
+     * @param {Array<*>} constructorArgs Arguments for the contract constructor.
+     * @param {object} options


Then it could as well be a rootDir argument 🤷‍♂️

schemar · 2018-12-20T10:09:19Z

tools/deployment_tool/contract.js

+    /**
+     * Returns the contract's address as detemined by the provided AddressGenerator.
+     *
+     * @param {Object|string} addressGeneratorOrAddress The AddressGenerator used for


Someone else should probably comment on this @benjaminbollen @pgev @deepesh-kn

schemar · 2018-12-20T10:11:01Z

tools/merge_chainspec_accounts.js

+  chainspec.accounts[address] = obj;
+});
+
+console.log(JSON.stringify(chainspec, null, 4));


I don't know enough about JS, but are you using a logging mechanism to give feedback to the user on stdout? Can I change the behavior of console.log() to log somewhere else? E.g. a file? In that case I would prefer explicitly writing to stdout and not a log.

deepesh-kn

I liked the code.
It is clean, readable and documented.
💯:raised_hands: blue-> :large_blue_circle:
I have very minor inline comments.

tools/deployment_tool/contract.js

deepesh-kn · 2019-01-04T07:38:17Z

tools/deployment_tool/contract.js

+    /**
+     * Returns the contract's address as detemined by the provided AddressGenerator.
+     *
+     * @param {Object|string} addressGeneratorOrAddress The AddressGenerator used for


I too prefer separate functions like setAddress and generateAddress

tools/deployment_tool/test/deployment_tools.test.js

hobofan · 2019-01-07T14:08:42Z

@deepesh-kn @schemar

I addressed the feedback in the latest commit. setAddress has been split up into setAddress and setGeneratedAddress.

- Get rid of the "instances" concept - Order contracts before deployment - Introduce a Contract class instead of using plain objects - Split out address generation into IncrementingAddressGenerator - Add documentation in JSDoc format - Add eslint

- Use fixed dependency versions - Switch from Jest to Mocha + Chai to be closer to Truffle test setup - Remove unused tools/deploy_token_genesis.js - Split off Contract#_ensureStateOneOf from Contract#_ensureState - Refactor deployment_tool into multiple files - Add `npm run test:deployment_tool` and hook into travis - Add README to tools directory, explaining the usage of the deployment tool lib

- Added messages to asserts in tests - Split up Contract#setAddress into Contract#setAddress and Contract#setGeneratedAddress - Don't use console.log to write to STDOUT in merge_chainspec_accounts.js

deepesh-kn

LGTM 🚀

schemar reviewed Nov 15, 2018

View reviewed changes

hobofan force-pushed the feature/genesis-deployment branch 3 times, most recently from e7b6a83 to f2e3d15 Compare November 19, 2018 07:33

hobofan changed the title ~~[WIP] Add initial version of genesis deployment tool~~ Add initial version of genesis deployment tool Nov 19, 2018

hobofan force-pushed the feature/genesis-deployment branch from f2e3d15 to b506031 Compare November 19, 2018 08:40

benjaminbollen assigned hobofan Nov 20, 2018

benjaminbollen self-requested a review November 20, 2018 14:01

hobofan changed the title ~~Add initial version of genesis deployment tool~~ [WIP] Add initial version of genesis deployment tool Nov 22, 2018

hobofan force-pushed the feature/genesis-deployment branch 2 times, most recently from 727bd12 to 63a40ec Compare December 11, 2018 14:18

hobofan changed the title ~~[WIP] Add initial version of genesis deployment tool~~ Add initial version of genesis deployment tool Dec 11, 2018

hobofan force-pushed the feature/genesis-deployment branch from f0d7dd1 to f758283 Compare December 11, 2018 15:10

hobofan mentioned this pull request Dec 15, 2018

Add deployment scripts #550

Merged

hobofan force-pushed the feature/genesis-deployment branch 2 times, most recently from 34e2fd9 to f9f045d Compare December 18, 2018 14:20

schemar suggested changes Dec 19, 2018

View reviewed changes

hobofan force-pushed the feature/genesis-deployment branch 4 times, most recently from 45677fc to d2d75a9 Compare December 19, 2018 16:22

schemar reviewed Dec 19, 2018

View reviewed changes

tools/README.md Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

schemar suggested changes Dec 19, 2018

View reviewed changes

hobofan force-pushed the feature/genesis-deployment branch 2 times, most recently from c25ec99 to 352ca0f Compare December 19, 2018 17:59

schemar approved these changes Dec 20, 2018

View reviewed changes

schemar requested review from deepesh-kn and pgev December 20, 2018 10:11

deepesh-kn reviewed Jan 4, 2019

View reviewed changes

hobofan added 12 commits January 8, 2019 10:16

Add initial version of genesis deployment tool

d63e490

Add helper for loading truffle build artifacts without truffle exec

d90b56c

Add addTruffleContract fn to genesis deployer

012369f

Use BN for more robust address incrementation

aad55a4

Add option to specify address for contract/instance in deployer

ef22469

Add script to merge chainspec account section

a2e44e6

Simplify interface of deployment tool

69ee182

- Get rid of the "instances" concept - Order contracts before deployment - Introduce a Contract class instead of using plain objects - Split out address generation into IncrementingAddressGenerator - Add documentation in JSDoc format - Add eslint

Add state machine and test to deployment tools for more stable operation

ecdbda7

Add contract reference for constructor arguments to deployment tool

e128bb6

Add IncrementingNonceAddressGenerator for deployments on live networks

56bf014

Address PR feedback

bd9635e

- Added messages to asserts in tests - Split up Contract#setAddress into Contract#setAddress and Contract#setGeneratedAddress - Don't use console.log to write to STDOUT in merge_chainspec_accounts.js

hobofan force-pushed the feature/genesis-deployment branch from dc9bf04 to bd9635e Compare January 8, 2019 09:17

deepesh-kn approved these changes Jan 8, 2019

View reviewed changes

deepesh-kn merged commit 54d6269 into OpenST:develop Jan 8, 2019

hobofan mentioned this pull request Jan 8, 2019

JavaScript code linting and formatting #404

Closed

	* Helper for loading a contract saved in a format followin the truffle-contract-schema.
	* Helper for loading a contract saved in a format following the truffle-contract-schema.

Add initial version of genesis deployment tool #458

Add initial version of genesis deployment tool #458

Conversation

hobofan commented Nov 14, 2018 • 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

hobofan commented Nov 19, 2018

hobofan commented Nov 22, 2018

hobofan commented Dec 11, 2018

schemar 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

hobofan commented Dec 19, 2018

schemar 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

hobofan commented Dec 19, 2018

schemar 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

deepesh-kn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hobofan commented Jan 7, 2019

deepesh-kn left a comment

Choose a reason for hiding this comment

hobofan commented Nov 14, 2018 •

edited

Loading