WIP: Refactor aragen #105
WIP: Refactor aragen #105
Conversation
There was a problem hiding this comment.
Really good job overall! I think this is a big improvement from the previous process. After making sure the methodology in scripts/repos works in a CI environment I think it looks great and won't propose any major changes.
About the IPFS cache, I would compile a list of IPFS hashes as you get them in scripts/repos/index and store them in an artifact. Then after installing aragen if the user wants to have all the content in a local IPFS node just call a pre-made script that uses that list of IPFS hashes to pin them to the local node. To speed this process the script or command could also connect the local node to a known public Aragon IPFS node that is sure to have those files available
| @@ -37,7 +37,7 @@ module.exports = async ( | |||
| await logDeploy(factory, { verbose }) | |||
| const receipt = await factory.newENS(owner) | |||
|
|
|||
| const ensAddr = receipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens | |||
| const ensAddr = receipt.logs.filter((l) => l.event == 'DeployENS')[0].args.ens | |||
There was a problem hiding this comment.
To reduce the diff, create a .prettierrc which sets the parameters in order to prevent unnecessary changes. In this case, arrowParens: "avoid"
https://prettier.io/docs/en/options.html#arrow-function-parentheses
| @@ -0,0 +1,23 @@ | |||
| import net from 'net' | |||
|
|
|||
| export const isPortTaken = async (port, opts) => { | |||
There was a problem hiding this comment.
Would it be better to use https://www.npmjs.com/package/tcp-port-used ? While there's value in not depending on useless dependencies, a non-trivial like this one reduces the amount of code to maintain going forward.
| @@ -0,0 +1,115 @@ | |||
| const getAccounts = require('@aragon/os/scripts/helpers/get-accounts') | |||
There was a problem hiding this comment.
This file is missing its extension ".js". Is it a duplicate of index.js?
| @@ -0,0 +1,17 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this package.json is meant to be here? Same for the yarn.lock in the same dir
| } = require('./apm') | ||
| const { readJson } = require('./utils') | ||
|
|
||
| const ensAddress = '0x5f6f7e8cc7346a11ca2def8f827b7a0b612c56a1' |
There was a problem hiding this comment.
I would recommend moving any specific param to a file in the root dir that contains all this kind of data in a single place for better visibility and maintainability.
| @@ -0,0 +1,40 @@ | |||
| { | |||
There was a problem hiding this comment.
I would recommend moving this file to a higher directory with a self-descriptive name Aragon Apps Fetched For Aragen. It could be turned into .js that exports JSON so it can include a nice long comment on top explaining its purpose and methodology
Open a PR to add your app aragen's CI, etc
| const history = await etherscanProvider.getHistory(contractAddress, 0) | ||
| const deployTx = history[0] | ||
|
|
||
| log('Deploying Repo...') |
| from: owner, | ||
| to: txData.to, | ||
| data: encodePublishVersionTxData(txData), | ||
| gas: 4712388, |
There was a problem hiding this comment.
Declare a variable in the top as "veryBigGasAmout" or something similar to prevent using magic numbers
| : hex | ||
| } | ||
|
|
||
| main() |
There was a problem hiding this comment.
I would split the logic from the script that actually calls it. Maybe move all the logic to "deployAragonApps" and keep index as just something that imports and executes with the necessary parameters: testnet URL, app list, etc.
This PR is a WIP to refactor how we handle the devchain creation using the new tooling that was developed for the buidler-aragon plugin.