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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor artifact generation & improve sanity check #570

Merged
merged 6 commits into from Jul 3, 2019

Conversation

3 participants
@0xGabi
Copy link
Member

commented Jun 30, 2019

馃 Pull Request

close #365

Improve apm publish artifact generation task.
Generate code.sol for minor and patch.

鉁旓笍 PR Todo

  • Update unit tests for this change
  • Update the relevant documentation

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

@0xGabi 0xGabi marked this pull request as ready for review Jun 30, 2019

@0xGabi 0xGabi requested a review from 0x6431346e as a code owner Jun 30, 2019

@0xGabi 0xGabi moved this from In progress to Review in Aragon Mesh Team Jul 1, 2019

0xGabi and others added some commits Jul 1, 2019

Update packages/aragon-cli/src/commands/apm_cmds/publish.js
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
Merge branch 'fix/apm-publish-artifact-generation' of github.com:0xGa鈥
鈥i/aragon-cli into fix/apm-publish-artifact-generation
@0x6431346e
Copy link
Member

left a comment

LGTM!

I don't fully understand how artifact.json#environments is generated. Does it only include the environment you are deploying to?

If so, deploying from one environment to another means you cannot reuse the artifact and that a new ipfs hash will be generated, right?

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Yes artifact.json#environments only include the environment been deployed.

This was how the artifacts were generated so far. I haven't change that but I did include a sanity check to regenerate the artifacts as you mention.

Do you think we should include all environments instead? And might be the case that this is an old behavior from using network option?

@0x6431346e

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Do you think we should include all environments instead? And might be the case that this is an old behavior from using network option?

I want to say yes, but I don't know where and how they are use.

Would be much nicer UX if let's say the token-manager@7.0.0 had the same contentURI on mainnet and rinkeby. 馃

What do you think @sohkai ?

@0xGabi 0xGabi merged commit b786726 into aragon:master Jul 3, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-1.02%) to 3.092%
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@0xGabi 0xGabi deleted the 0xGabi:fix/apm-publish-artifact-generation branch Jul 3, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

I actually do like that the published artifact only has the correct environment specified, since it's then easier to know what was published... but having the same IPFS hash is also very useful 馃槃.

I think this is the only bit that would change the IPFS hash between environments, so I am now leaning towards keeping all the environments for the sake of simplicity (especially since the artifact file is the one that holds the ABI, and resolving it is key to doing everything).

return deprecated
reporter.warning(
`Cannot find artifacts for version ${version.version} in aragonPM repo. Please make sure the package was published and your IPFS or HTTP server are running.`
)

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 5, 2019

Member

We may want to also ask the user if they'd like to abort here... if I wanted deprecated methods but the old CID wasn't fetchable I'd want to stop the publishing process.

@0x6431346e

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I think this is the only bit that would change the IPFS hash between environments, so I am now leaning towards keeping all the environments for the sake of simplicity (especially since the artifact file is the one that holds the ABI, and resolving it is key to doing everything).

Would it make sense to include all environments in artifact.json and then extract the current one to environment.json ?

Also, is it used anywhere currently? Does aragon.js actually use the ensRegistry specified in artifact.json ?

@0x6431346e 0x6431346e moved this from Review to Done in Aragon Mesh Team Jul 5, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

AFAIK it is not used anywhere.

Extracting it into an environment.json would still change the root dir's CID, but would keep the artifact.json's CID intact? For simplicity, it might make sense just to keep the entire root CID the same across environments :shrug: (apps are usually built to be network-generic) :shrug:.

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Great discussion, I'll create a new issue to update artifact.json to use all environments on other PR.

@0x6431346e

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Extracting it into an environment.json would still change the root dir's CID, but would keep the artifact.json's CID intact?

Yes that would be an improvement.

For simplicity, it might make sense just to keep the entire root CID the same across environments :shrug: (apps are usually built to be network-generic) :shrug:.

Totally agree! especially since this is not used anywhere. It feels like this metadata should belong to APM.

  1. I imagine it would make generating and maintaining environments much easier: if I want to deploy voting.aragonpm.eth to xDAI all I have to do is a transaction with the version and the contentURI from mainnet. No need to recompile > less room for errors.
  2. Perhaps aragen could become a set of scripts that does exactly this: keeps apm registries on par across environments. (We can query the APM repo and download the ipfs-cache folder via IPFS).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.