Adds new verify contracts script #108

dkent600 · 2019-03-26T19:58:02Z

Adds new verify contracts script. See Verify.md for instructions on how to use.

Note: The Etherscan API 'verifysourcecode' method in some cases returns success even when it has failed to verify the contract. The returned GUID is not helpful, as for any given GUID, the 'checkverifystatus' method always returns "Error! Missing Or invalid Module name", regardless of verification success or failure.

I have submitted a ticket on this to etherscan, they have responded with a request for further info, which I have provided. See https://etherscancom.freshdesk.com/public/tickets/582fc02dfc53677ce6d9a02bbeb788fcf8111b6fc5f61b16556570d05da8a276

dkent600 · 2019-03-26T20:05:45Z

Ticket on etherscan for their issue: https://etherscancom.freshdesk.com/public/tickets/582fc02dfc53677ce6d9a02bbeb788fcf8111b6fc5f61b16556570d05da8a276

jellegerbrandy · 2019-03-27T10:09:48Z

package.json

    "prune-arc-build": "node ./pruneArcBuild.js",
-    "postinstall": "npm run prune-arc-build"
+    "postinstall": "npm run prune-arc-build",
+    "verify.initialize": "cd ./node_modules/@daostack/arc && npm install openzeppelin-solidity && cd ../infra && npm install openzeppelin-solidity",


perhaps npm explore @doastack/arc -- npm install openzeppelin-solidity is a bit more robust.

Good idea, forgot about that ability

jellegerbrandy · 2019-03-27T10:12:38Z

Verify.md

+
+## Verify
+
+### Verify all of the migrated contracts


So what does this mean exactly? There are different contract versions on chain, and different versions of arc etc.
I'd expect "very the migrated contracts" means: take the current versino of arc (from package.json), take the addresses from migration.json, and check if the compiled code of arc corresponded to the deployed bytecode.

Is that what is happening?

Also, there is some codce in the migartion script that compares bytecodes, perhaps you can look at that as well?

What we are doing is, for each contract whose address appears in migration.json (and therefore is a "migrated contract"), we send the contract's solidity code to etherscan.io which will attempt to "verify" that the given code corresponds to the deployed bytecodes.

I will explain this in the md file.

ben-kaufman · 2019-03-31T12:32:12Z

@dkent600 can you please add this to the prepare-release.sh or the release.sh so it's done automatically when publishing a new version?

dkent600 · 2019-04-02T14:46:01Z

@ben-kaufman

can you please add this to the prepare-release.sh or the release.sh

I added to prepare-release.sh

orenyodfat · 2019-04-15T06:26:29Z

Verify.md

+## Setup
+1. In the project root, create an ".env" file (or use the existing one) that contains:
+```
+APIKEY=[API key from etherscan.io]


please add a link to etherscan api key ..how too.
https://etherscan.io/apis

orenyodfat · 2019-04-15T06:29:50Z

package.json

    "prune-arc-build": "node ./pruneArcBuild.js",
-    "postinstall": "npm run prune-arc-build"
+    "postinstall": "npm run prune-arc-build",
+    "verify.initialize": "npm explore @daostack/arc -- npm install openzeppelin-solidity && npm explore @daostack/arc/node_modules/@daostack/infra -- npm install openzeppelin-solidity && npm explore @daostack/arc-hive -- npm install openzeppelin-solidity",


why we need to re install ? migration repo already deploy arc , openzeppelin ,infra, arc-hive
And if there is a need(not sure) , which versions it is install ? it need to be aligned with the migration packages version.no?

Need to reinstall because truffle-flattener explicitly requires that it be run from within a folder that contains the truffle project that contains the contract being flattened. Otherwise truffle-flattener won't work.

npm will install whatever version the package says needs to be installed. I'm not sure how this could not be aligned with what the migration package is expecting.

@orenyodfat OK, I think I see how there can be misalignment. The only way I can see to fix this is to change, for example, "openzeppelin-solidity": "^2.2.0" to "openzeppelin-solidity": "2.2.0" in the arc package. This would avoid picking up 2.2.1 when the contracts were compiled with 2.2.0.

which version it will install on npm install openzeppelin-solidity ?

@orenyodfat

which version it will install on npm install openzeppelin-solidity ?

It depends on the project and what is the current version available.

According to the package.json files:

Arc: ^2.1.3
Infra: ^2.1.3
arc-hive: ^2.2.0

These are the versions being installed today.

But what if openzeppelin-solidity releases a new version and suddenly there is a 2.2.1, and suppose this happens after the release of arc-hive. So arc-hive will have been compiled with 2.2.0, but will be flattened with 2.2.1, which could prevent the verification from succeeding.

The only reasonable way to solve this, that I see, is to remove the '^' from the openzeppelin-solidity version specification in the package.json files.

but the script does that without version .
how about edit the script to do npm install openzeppelin-solidity 2.2.1 ?

@orenyodfat

but the script does that without version
how about edit the script to do npm install openzeppelin-solidity 2.2.1 ?

How is the script supposed to know what is the correct version? I could write some code to parse the three package.json files to obtain the version number for openzeppelin-solidity in each package, but that would not be robust. It would fail to obtain the correct version in the following case:

Assume Arc has ^2.2.0 in package.json

Assume the current version of openzeppelin-solidity is 2.2.1.

The user runs truffle compile on Arc prior to creating an NPM package for Arc.

RESULT:
The contracts are compiled with 2.2.1 but the package.json remains unchanged, still referring to ^2.2.0.

Then when the verify script parses the package.json, it will get 2.2.0, which is the wrong version, and thus verification may fail.

I think it is the right thing for us to do in general, not just for verify, to be explicit in the Arc, etc package.json files about exactly what version the contracts are compiled with.

orenyodfat · 2019-04-15T06:40:14Z

Verify.md

+### Verify all of the migrated contracts
+
+```script
+npm run verify


need to set network ?
Running this produce : 'network' is required. Run 'npm run help' for cli information.

orenyodfat · 2019-04-15T06:40:58Z

verify.ts

+  { name: 'check', alias: 'g', type: String, description: 'Given verify GUID, check contract verification status.  Ignores other options.' },
+  { name: 'outputFlattened', alias: 'f', type: String, description: 'When verifying, absolute path where to save the flattened .sol file to flattened.["contractName"].sol.' },
+];
+// tslint:enable: max-line-length


is that not enable by default ?

just a few lines above it is disabled

orenyodfat · 2019-04-15T06:41:25Z

verify.ts

+  } else {
+    if (options.contractName) {
+      if (!allAddresses[options.contractName]) {
+        // tslint:disable-next-line: max-line-length


why to disable that here?

i removed it and split the following line.

orenyodfat · 2019-04-15T06:41:32Z

verify.ts

+
+  try {
+
+    // tslint:disable-next-line: max-line-length


why to disable that here?

i removed it and split the following line.

orenyodfat · 2019-04-15T06:41:45Z

verify.ts

+    // tslint:disable-next-line: max-line-length
+    const etherscanUrl = `https://api${options.network === 'main' ? '' : ('-' + options.network)}.etherscan.io/api`;
+
+    // const flattenedFake = fs.readFileSync('./flattened.sol', 'UTF-8');


please remove this commented code

orenyodfat · 2019-04-15T06:41:58Z

verify.ts

+          fs.writeFileSync(path.join(options.outputFlattened, `flattened.${contractName}.sol`), flattened);
+        }
+
+        // tslint:disable-next-line: max-line-length


why to disable that here?

i removed it and split the following line.

orenyodfat · 2019-04-15T06:42:13Z

verify.ts

+          sourceCode: flattened,
+        };
+
+        // console.dir(Object.assign({}, apiConfig, { sourceCode: `${flattened.substr(0, 50)}...` }));


please remove this commented code line

orenyodfat · 2019-04-15T06:42:20Z

verify.ts

+            // tslint:disable-next-line: max-line-length
+            spinner.fail(`${contractName} at ${addresses[contractName]} ${verifyResult.result}, GUID: ${result.result}`);
+          } else {
+            // tslint:disable-next-line: max-line-length


why to disable that here?

i removed it and split the following line.

dkent600 · 2019-05-01T10:40:41Z

@orenyodfat

#127 (comment) breaks this line:

migration/verify.ts

Line 144 in e57b81c

    
           [options.network === 'mainnet' ? '0xd3BA32dd207Db75f535001FAC749c925423D8A6f' :  // DAOstack multisig :

Need to change the address for mainnet to: '0x85e7fa550b534656d04d143b9a23a11e05077da3' and change 'multisig' to 'controlled account'

orenyodfat · 2019-05-01T11:37:13Z

@orenyodfat

#127 (comment) breaks this line:

migration/verify.ts

Line 144 in e57b81c

[options.network === 'mainnet' ? '0xd3BA32dd207Db75f535001FAC749c925423D8A6f' : // DAOstack multisig :

Need to change the address for mainnet to: '0x85e7fa550b534656d04d143b9a23a11e05077da3' and change 'multisig' to 'controlled account'

the verifier should not depend on constructor params.
you should read the deployed byte code from the blockchain , derived the constructor params and then verify.

dkent600 · 2019-05-01T18:23:19Z

@orenyodfat

you should read the deployed byte code from the blockchain , derived the constructor params and then verify.

Having to access the chain adds a whole new dimension to the script. Just FYI this will take a little more time...

orenyodfat · 2019-05-01T18:47:44Z

verify.ts

            constructorArguments =
              abi.rawEncode(['address'],
-                [options.network === 'mainnet' ? '0xd3BA32dd207Db75f535001FAC749c925423D8A6f' :  // DAOstack multisig :
+                [options.network === 'mainnet' ? '0x85e7fa550b534656d04d143b9a23a11e05077da3' :  // DAOstack controlled account


how this is related to the verifier ?

When you verify a contract you have to supply the arguments (if there are any) by which it was constructed at the given address.

right. as comment above you should read the deployed byte code from the blockchain , derived the constructor params and then verify.

This is done. It adds a new "-p" option to the command line to supply a provider url for web3.

The technique I used is simple but presumes to know the types (not the values) of contructor parameters in order to know how to parse the bytecodes to obtain the param values.

tsuberim · 2019-05-03T06:49:32Z

I think the verification should just happen during migration just after deploying a contract. Why does it have to be a whole set of new commands?
This also solves the previous question of how to get the constructor parents (they are available in the deploy script).

dkent600 · 2019-05-03T14:52:26Z

@tsuberim

think the verification should just happen during migration just after deploying a contract.

It can, but there are use cases where it may need to be doable outside of that process as well.

dkent600 · 2019-05-03T15:07:33Z

@tsuberim

PS -- the verify scripts are automatically run as part of the "release" process.

tsuberim · 2019-05-06T16:16:08Z

package.json

    "prune-arc-build": "node ./pruneArcBuild.js",
-    "postinstall": "npm run prune-arc-build"
+    "postinstall": "npm run prune-arc-build",
+    "verify.initialize": "npm explore @daostack/arc -- npm install openzeppelin-solidity @daostack/infra --no-save && npm explore @daostack/infra -- npm install openzeppelin-solidity --no-save && npm explore @daostack/arc-hive -- npm install openzeppelin-solidity --no-save",


Seems weird to install a package inside an installed package on an npm-script. Why is'nt this part of devDeps

truffle-flattener won't work otherwise

weird, I don't understand the details but at the very least we can move this into postinstall to avoid having to remember to invoke this step manually if we can't get rid of it entirely.

verify.initialize and verify.build are already run in prepare-release, prior to running verify.

tsuberim · 2019-05-06T16:16:32Z

package.json

-    "postinstall": "npm run prune-arc-build"
+    "postinstall": "npm run prune-arc-build",
+    "verify.initialize": "npm explore @daostack/arc -- npm install openzeppelin-solidity @daostack/infra --no-save && npm explore @daostack/infra -- npm install openzeppelin-solidity --no-save && npm explore @daostack/arc-hive -- npm install openzeppelin-solidity --no-save",
+    "verify.build": "node node_modules/typescript/bin/tsc verify.ts -sourceMap",


there's ts-node that you could use that executes typescript directly.

what is there is sufficient without adding another tool

but it saves you the mental headache of remembering to do the build step.

verify.initialize and verify.build are already run in prepare-release, prior to running verify.

tsuberim · 2019-05-06T16:17:10Z

package.json

+    "postinstall": "npm run prune-arc-build",
+    "verify.initialize": "npm explore @daostack/arc -- npm install openzeppelin-solidity @daostack/infra --no-save && npm explore @daostack/infra -- npm install openzeppelin-solidity --no-save && npm explore @daostack/arc-hive -- npm install openzeppelin-solidity --no-save",
+    "verify.build": "node node_modules/typescript/bin/tsc verify.ts -sourceMap",
+    "verify.help": "node verify.js -h",


I think this is redundant, everyone knows how to use -h or --help

they don't know to how to run node verify.js -h

npm run verify -- -h or yarn verify -h

i don't see any reason not to have this helpful shortcut. risk-wise it is trivial

tsuberim · 2019-05-06T16:17:47Z

package.json

+    "verify.initialize": "npm explore @daostack/arc -- npm install openzeppelin-solidity @daostack/infra --no-save && npm explore @daostack/infra -- npm install openzeppelin-solidity --no-save && npm explore @daostack/arc-hive -- npm install openzeppelin-solidity --no-save",
+    "verify.build": "node node_modules/typescript/bin/tsc verify.ts -sourceMap",
+    "verify.help": "node verify.js -h",
+    "verify.lint": "tslint -c tslint.json verify.ts",


Could be more than just about verify.ts (all typescript files in the project)

sorry, I don't understand the suggestion. are you suggesting to convert all scripts to ts?

no, but there's no particular reason to limit this to only verify.ts, there's also no particular reason to expand it to all typescript files (as of yet, but there might be in the future).

for now, verify is all there is. we can cross the wider bridge when we come to it.

tsuberim · 2019-05-06T16:20:26Z

package.json

+    "verify.help": "node verify.js -h",
+    "verify.lint": "tslint -c tslint.json verify.ts",
+    "verify.lint.andFix": "npm run verify.lint -- --fix",
+    "verify": "node verify.js"


can be replaced with ts-node as I mentioed (take a look at this for a solution to a problem you might encounter)

On your suggestion, I looked into using ts-node, but it seems there are a lot of issues with it and using source maps in debuggers. Since the current soluton works for now, I'm not inclined to going down that rabbit hole.

tsuberim · 2019-05-06T16:21:35Z

verify.ts

@@ -0,0 +1,265 @@
+const commandLineArgs = require('command-line-args');


other scripts uses yargs already

I understand the desire to have only one system here, though it is not a huge deal. I think command-line-args has superior output and would use it instead of yargs.

I took another look at this and find that migrate.js is using functionality from yargs that is not available in command-line-args, but the yargs funtionality comes at the cost of ugly and difficult-to-parse output. The verify code doesn't need the extra functionality of yargs, and by using command-line-args has much nicer output. So I am leaving well-enough alone here.

tsuberim · 2019-05-06T16:23:27Z

@dkent600

When did this need last occur (that is not satisfied by just having it as part of regular migration)?
I just think there might be a little extra complexity around initializing, building, having another set of commands related to verifying when it could just be a simple one-liner (with a helper function if necessary) in the migration itself.
Having it inside the migration solves the problem of getting the constructor args (there's no need to)

working (on Bash) working except for contracts with constructor args constructor params added 'check' update package-lock lint added verify.md. -f for writing flattened .sol fix status message package-lock PR suggestions, support for arc-hive contracts corrected little readme typo another .md typo fix install infra so universal schemes will flatten package-lock run automatically in prepare-release do npm install from scratch "press any key" before verifying in prepare-releas fix verify of DAORegistry fix verify status check rebase, fix breakage due to infra moving requested PR changes fix web3, fix infra reference update package-lock fix initialize script main => mainnet fix npm install adding '^' to package.json files. controlled account against mainnet for DAORegistry more flexible means of obtaining constructor args readme for provider option

ben-kaufman

Looks good now. Let's merge and maybe later make further improvements if we need to.

This is an old review...

dkent600 requested review from ben-kaufman, jellegerbrandy and orenyodfat March 26, 2019 20:03

jellegerbrandy reviewed Mar 27, 2019

View reviewed changes

jellegerbrandy previously requested changes Mar 27, 2019

View reviewed changes

ben-kaufman requested a review from jellegerbrandy March 31, 2019 12:30

dkent600 force-pushed the verify-contracts branch from 28276d6 to 8792a4a Compare April 1, 2019 08:56

dkent600 force-pushed the verify-contracts branch 4 times, most recently from 9611bf7 to c247dd7 Compare April 8, 2019 14:09

orenyodfat reviewed Apr 15, 2019

View reviewed changes

dkent600 force-pushed the verify-contracts branch from f8e91de to b61c353 Compare April 15, 2019 13:40

orenyodfat mentioned this pull request May 1, 2019

update DAOstack's controlled account #127

Merged

dkent600 force-pushed the verify-contracts branch from e663a88 to a6bdf5a Compare May 1, 2019 18:28

orenyodfat reviewed May 1, 2019

View reviewed changes

tsuberim reviewed May 6, 2019

View reviewed changes

dkent600 force-pushed the verify-contracts branch 2 times, most recently from c13e9ac to 44761dc Compare May 16, 2019 16:46

dkent600 force-pushed the verify-contracts branch from 44761dc to 188b06f Compare June 25, 2019 02:20

enable to specify Arc version

1a6fd61

ben-kaufman approved these changes Jun 25, 2019

View reviewed changes

ben-kaufman merged commit 87c732e into master Jun 25, 2019

ben-kaufman deleted the verify-contracts branch June 25, 2019 07:47

dkent600 mentioned this pull request Jun 25, 2019

Auto contracts verification at etherscan #61

Closed

		@@ -0,0 +1,265 @@
		const commandLineArgs = require('command-line-args');

Adds new verify contracts script #108

Adds new verify contracts script #108

Uh oh!

Conversation

dkent600 commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkent600 commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-kaufman commented Mar 31, 2019

Uh oh!

dkent600 commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenyodfat Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkent600 Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkent600 Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkent600 Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenyodfat Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dkent600 commented Mar 26, 2019 •

edited

Loading

orenyodfat Apr 15, 2019 •

edited

Loading

dkent600 Apr 15, 2019 •

edited

Loading

dkent600 Apr 16, 2019 •

edited

Loading

dkent600 Apr 17, 2019 •

edited

Loading

orenyodfat Apr 15, 2019 •

edited

Loading

dkent600 commented May 1, 2019 •

edited

Loading

dkent600 commented May 1, 2019 •

edited

Loading

tsuberim commented May 3, 2019 •

edited

Loading

dkent600 commented May 3, 2019 •

edited

Loading