-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add basic cli & devchain cmd #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Tested it and it works great. I wonder if we could provide an option to show what's happening in the chain similar to the output ganache-cli
has.
I believe this is a great addition to the dev experience and allow to use of truffle debug. Can we merge this? @perissology @izqui Also it would be great to release the new version to been able to merge aragon/aragon-cli#246 |
Discussed in aragon-cli/issues/321
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good! Have a couple of comments, since there's been a few changes in aragonCLI since this PR.
scripts/start-ganache
Outdated
@@ -1,6 +1,9 @@ | |||
#!/bin/bash | |||
|
|||
SCRIPTPATH="$( cd "$(dirname "$0")" ; pwd -P )" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do the cd
in a different command, especially as right now this script would actually be going into scripts/
before running the rest.
We should do something like:
BASEPATH="$( dirname "$(dirname "$0")" )"
mnemonic=$(node -p "require(require('path').join(\"${BASEPATH}\", 'src/ganache-vars')).MNEMONIC")
cd "$BASEPATH"
...
(We want aragon-ganache/
to be at the top level of the repo).
src/commands/devchain.js
Outdated
const mkDir = promisify(mkdirp) | ||
const recursiveCopy = promisify(ncp) | ||
|
||
const snapshotPath = path.join(os.homedir(), `.aragon/ganache-db-${port}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have to figure out how to port this over from aragonCLI since we've since changed the snapshot directory to be postfixed by the version of the CLI.
One idea could be just to prefix the version instead to the version of aragen, since this is where the snapshot is actually coming from.
Let's double check there's no other modifications to the part of aragonCLI that currently handles the devchain from November onwards.
I updated a few more things to the new changes, I believe is ready to merge. @sohkai what do you think? |
scripts/start-ganache
Outdated
@@ -1,8 +1,9 @@ | |||
#!/bin/bash | |||
|
|||
SCRIPTPATH="$( cd "$(dirname "$0")" ; pwd -P )" | |||
BASEPATH="$( dirname "$(dirname "$0")" )" | |||
mnemonic=$(node -p "require(require('path').join(\"${SCRIPTPATH}\", '../src/ganache-vars')).MNEMONIC") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be changed to BASEPATH
and to remove the ../
in ../src/..
.
src/cli.js
Outdated
"Address of the ENS registry. This will be overwritten if the selected '--environment' from your arapp.json includes a `registry` property", | ||
default: require('../index').ens, | ||
}) | ||
cmd.group(['apm.ens-registry'], 'APM:') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't include these APM options, since aragen is meant to deploy to a consistent ENS address each time.
src/commands/devchain.js
Outdated
const { privateKeys, mnemonic } = await task.run() | ||
exports.printAccounts(reporter, privateKeys) | ||
exports.printMnemonic(reporter, mnemonic) | ||
exports.printResetNotice(reporter, reset) | ||
|
||
reporter.info(`ENS instance deployed at ${apmOptions['ens-registry']}\n`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This address should always be 0x5f6f7e8cc7346a11ca2def8f827b7a0b612c56a1
due to how we deterministically deploy ENS using the same account and nonce each time.
@@ -40,7 +40,7 @@ | |||
"@babel/polyfill": "^7.0.0", | |||
"chalk": "^2.1.0", | |||
"figures": "^2.0.0", | |||
"ganache-core": "^2.3.2", | |||
"ganache-core": "^2.2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not pin this here, but pin it in aragonCLI so we can release that. Let's keep testing this after the aragonCLI release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sohkai I'm not sure about this one. You meant to leave v2.3.2 or do something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, hope is that everything will work with v2.3.2 (but maybe not D:). If it doesn't, then let's make sure to pin it to 2.2.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Let's merge! Updated this to publish a beta version of 3.0.0
instead, in case it doesn't work. We may need to tweak travis to get it to run properly.
fixes #12
would also resolve #16 as
aragon-cli
would no longer need the mnenomic b/c thedevchain
cmd would be imported fromaragen
I wasn't sure about the ConsoleReporter. It seemed a little weird to include it here, but it will be needed for both
aragon-cli
&aragen
. Would it make sense to put it in another repo?