Skip to content
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

Publish script for aragen && include register-packages option on deployer #137

Merged
merged 2 commits into from Sep 11, 2019

Conversation

@0xGabi
Copy link
Member

commented Aug 18, 2019

This PR will help with the automation of aragen releases.

The current deployer script don't took in consideration the case that we don't want to register the packages even if we are in a local devchain. As a consequence when trying to publish to aragonPM with the aragonCLI aragon apm publish command we get a VM exception with the error: revert REPO_INVALID_VERSION.

The solution I have implemented provide a new option to the deployer, register-pacakges, that allow to prevent packages registration during template deployment.

I also notice that after a packages was registered using the _registerPackage function it return the following:

❯ aragon apm info bare-template
✖ Fetching bare-template.aragonpm.eth@latest
    → The parameter must be a valid HEX string.

@0xGabi 0xGabi added the enhancement label Aug 18, 2019

@0xGabi 0xGabi requested review from sohkai and facuspagnuolo Aug 18, 2019

@0xGabi 0xGabi added this to In progress in Aragon Mesh Team via automation Aug 18, 2019

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Btw how do you usually configure your editor when there is no .rc files on the repo? 🤔

@sohkai
Copy link
Member

left a comment

Looking good, but overall it would be much easier to review if you split the commits out into one that formatted the code and one that made the actual changes :).

"publish:staging": "aragon apm publish major $(npm run deploy:rinkeby | tail -n 1) --environment rinkeby",
"publish:rinkeby": "aragon apm publish major $(npm run deploy:rinkeby | tail -n 1) --environment rinkeby",
"publish:ropsten": "aragon apm publish major $(npm run deploy:ropsten | tail -n 1) --environment ropsten",
"publish:mainnet": "aragon apm publish major $(npm run deploy:mainnet | tail -n 1) --environment mainnet"

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 18, 2019

Member

We should keep these other publishing scripts.

@facuspagnuolo
Copy link
Member

left a comment

Shouldn't the deployer be fetching the implementations from a local ENS in case these are already published instead of forcing it to avoid registering them?
Additionally, I'd rollback most part of the breakdowns since we're not following those in the rest of the JS code, at least in this repo.

let { ens, owner, verbose, daoFactory, miniMeFactory } = require('yargs')
.option('e', { alias: 'ens', describe: 'ENS address', type: 'string' })
.option('o', { alias: 'owner', describe: 'Sender address. Will use first address if no one is given.', type: 'string' })
.option('v', { alias: 'verbose', describe: 'Verbose mode', type: 'boolean', default: false })
.option('df', { alias: 'dao-factory', describe: 'DAO Factory address. Will deploy new instance if not given.', type: 'string' })
.option('mf', { alias: 'mini-me-factory', describe: 'MiniMe Factory address. Will deploy new instance if not given.', type: 'string' })
.option('rp', { alias: 'register-packages', describe: 'Whether the script will register the packages to aragon', type: 'boolean', default: true })

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 22, 2019

Member

WDYT about using simply register?

verbose,
daoFactory,
miniMeFactory,
})

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 22, 2019

Member

You're not passing the register argument, right?

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Thanks for the feedback Facu. Actually I think it will only prevent to register in case there are no previous version for that package.

I don't fully understand why you need to register if there is no version. This line I'm referring to

@sohkai
Copy link
Member

left a comment

Shouldn't the deployer be fetching the implementations from a local ENS in case these are already published instead of forcing it to avoid registering them?

I think this is just to help aragen deploy properly; the current TemplatesDeployer as pointed out by @0xGabi does not use the correct IPFS CID (it just uses '' which is fine for testing).

I don't think this local deployer should be trying to build the correct CID, and that's why it's nice to do the deployment on two steps:

  1. Deploy contracts
  2. Use aragonCLI to publish repos

Btw how do you usually configure your editor when there is no .rc files on the repo?

You don't 😄. Generally just go with the flow that's already there. I'd avoid installing prettier and ESLint globally, and only install them in the local packages to avoid having your editor format everything it touches if it's not supposed to be formatted.

const aragonIDAddress = await this._fetchRegisteredAragonID()
if (!aragonIDAddress) this.error('Local aragon ID deployment failed, aborting.')
if (!aragonIDAddress)
this.error('Local aragon ID deployment failed, aborting.')

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 11, 2019

Member

Let's put braces around this if

this.log(`Template addresses saved to ${await this.arapp.filePath()}`)
const { constructor: { contractName } } = template
await this.arapp.write(templateName, contractName, this.ens.address)
this.log(`Template addresses saved to ${await this.arapp.filePath()}`)

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 11, 2019

Member

This indentation change seems to be weird

.help('help')
.parse()

if (!web3) errorOut('Missing "web3" object. This script must be run with a "web3" object globally defined, for example through "truffle exec".')
if (!artifacts) errorOut('Missing "artifacts" object. This script must be run with an "artifacts" object globally defined, for example through "truffle exec".')
if (!web3)

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 11, 2019

Member

We should put braces for all of these multiline ifs; they get very confusing very quickly without.

"deploy:rinkeby": "npm run compile && truffle exec ./scripts/deploy.js --network rinkeby --ens 0x98Df287B6C145399Aaa709692c8D308357bC085D --dao-factory 0xfdef49fbfe37704af55636bdd4b6bc8cd19143f6 --mini-me-factory 0x6ffeb4038f7f077c4d20eaf1706980caec31e2bf",
"deploy:ropsten": "npm run compile && truffle exec ./scripts/deploy.js --network ropsten --ens 0x6afe2cacee211ea9179992f89dc61ff25c61e923 --dao-factory 0x3f2aa9dd22e97070518ba7988fe9b8724129d497 --mini-me-factory 0x1ce5621d386b2801f5600f1dbe29522805b8ac11",
"deploy:mainnet": "npm run compile && truffle exec ./scripts/deploy.js --network mainnet --ens 0x314159265dd8dbb310642f98f50c066173c1259b --dao-factory 0xc29f0599df12eb4cbe1a34354c4bac6d944071d1 --mini-me-factory 0x909d05f384d0663ed4be59863815ab43b4f347ec",
"publish:aragen": "aragon apm publish major $(npm run deploy:aragen | tail -n 1) --environment default",

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 11, 2019

Member

Perhaps we can move the publish:aragen command to be just before the publish:staging command in all of these templates?

These have been ordered in terms of "network severity" (i.e. rpc -> devnet -> staging -> rinkeby -> ropsten -> mainnet)

@0xGabi 0xGabi force-pushed the 0xGabi:update-scripts-aragen branch from b6b2213 to 9b73b91 Sep 11, 2019

@0xGabi 0xGabi merged commit e73a246 into aragon:master Sep 11, 2019

3 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

Aragon Mesh Team automation moved this from In progress to Done Sep 11, 2019

@0xGabi 0xGabi deleted the 0xGabi:update-scripts-aragen branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.