Updating deployment process for better external use of the project #489

josojo · 2020-01-21T20:38:07Z

This PR modifies the way artifacts are handled in the migration process. This enables the batch exchange deployment from external projects, without publishing the contracts in the npm package.

closes: #449

An example of how this repo can be used for an external project can be found here:
https://github.com/gnosis/dex-contracts-integration-example

testplan:
Run the commands from the readme on
https://github.com/gnosis/dex-contracts-integration-example

fleupold

Smallish comments.

fleupold · 2020-01-22T08:38:37Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@gnosis.pm/dex-contracts",
-  "version": "0.1.3",
+  "version": "0.1.4",


Should the version bump be its own commit?

I also notice that 0.1.4 tag was pushed and auto-deployed to NPM. I think we should include a check deploy will fail if the tag is not on master.

Should I yank the version from NPM?

How would I test the deployment scripts for
https://github.com/gnosis/dex-contracts-integration-example, I need the package in there... Having it published only locally makes it testable for me, but not for the others.

Should I do a manual 0.1.4-dev npm release?

src/migration/utilities.js

nlordell

I like the changes, but I wonder if we have to fix our dependencies to make this work. We might have to include the solidity libraries as dependencies as well as truffle contract.

nlordell · 2020-01-22T09:14:10Z

src/migration/utilities.js

@@ -1,14 +1,32 @@
-function getDependency(artifacts, network, deployer, path) {
+function initializeContract(path, deployer, accounts) {
+  const contract = require("truffle-contract")


I think we might need to add truffle-contract to the dependencies or peerDependencies? That might be a "heavy" dependency that we were trying to avoid. @anxolin what do you think?

Yeah, I tried to avoid it and put it all just in the npm package.json of
https://github.com/gnosis/dex-contracts-integration-example
While this is not "very clean", it keeps the size down of dex-contracts npm package.

For which applications is the small size important?

Yes, agree with Nick.

@josojo , super nice that you are doing an example on how to deploy the contracts from another project.

I expect that many projects won't even do this, and they will just use an interface or the original contract and use the rinkeby/mainnet deployments. Sometimes that approach is more straightforward.

Alex, for https://github.com/gnosis/dex-contracts-integration-example I think it would be better if you define the steps in the README to add it to any project.

npm install dex-contracts npm install <dependencies like truffle> ...

This way is more explicit what we depend on.

WDYT?

I disagree.
For every other project, owl-token, solidity-datastructures, etc, we always create the npm modules, such that later on others can easily build on them.
And we benefit from that a lot. For example, imagine the OWL contracts, all libraries, and all the other things are would be in this dex-contracts project. Then it would be harder to navigate for sure.

I expect that many projects won't even do this, and they will just use an interface or the original contract and use the rinkeby/mainnet deployments. Sometimes that approach is more straightforward.

If you wanna write any meaningful tests, then you need to have the deployment scripts etc...

I've got a bit lost.

My understanding is that we want to decide if we add the dependencies: For example truffle-contracts in our dependencies.

I would say to not do this, even as peerDependencies. A simplistic integration might be interested ONLY in the networks.json and the ABI, so we don't want to bring all the dependencies.

This is why, I thought of adding additional manual npm install. An alternative would be to create another npm-package or using yarn workspaces to create a package that depends on dex-contracts and all the other stuff

If you wanna write any meaningful tests, then you need to have the deployment scripts etc..

I agree, for some people.
Other people can be happy with dex-contracts tests already (nice job), so in their project, they just mock dFusion contracts, and test their own and the interactions with the mocked dFusion contracts

Okay, @anxolin :) then we are on the same page. Sorry, I was also a little bit confused. But actually we were in favor of the same thing!

I will not list all the dependencies in a readme in https://github.com/gnosis/dex-contracts-integration-example, they just go into the package.json of the dex-contracts-integration-example.

src/migration/utilities.js

bh2smith

Looks pretty good. I wouldn't mind seeing some comments in the scripts describing what is happening in some places.

bh2smith · 2020-01-22T10:44:11Z

I have tried to run the test plan on the example repo as follows;

Clone the integration repo
run the commands from the readme

And have run into several problems:

missing truffle dependency, no mention of need for ganache in readme, some other missing dependencies required by truffle-config (i.e. dotenv). I am have reported a few issues in the new repo and still working towards making this work. Will report back with, hopefully, some answers soon.

fedgiac

I can't compile dex-contracts-integration-example, otherwise it looks good.

fleupold · 2020-01-22T14:43:35Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@gnosis.pm/dex-contracts",
-  "version": "0.1.3",
+  "version": "0.1.5-dev",


It's pretty annoying having to publish versions to test this PR that are then public for the rest of time. Can we not use something like https://stackoverflow.com/questions/15806241/how-to-specify-local-modules-as-npm-package-dependencies to test this locally without publicly pushing new versions?

Can we please revert and delete those releases?

#489 introduced that change Version bumps should always happen in their own PR (and once that PR is merged to master, it should be tagged with the version). Moreover this skipped v0.1.4 and was likely just merged to master by accident.

In the last PR: #489 a bug was introduced as the for nonDevelopment networks, also the BatchExchange as a dependency would be tried to be pulled, although we have to get it from the usual build artifacts.

josojo added 2 commits January 21, 2020 21:35

updating deployment process for better external use of the project

c40e711

0.1.4

f0ff650

josojo requested review from fedgiac, bh2smith and nlordell January 21, 2020 22:16

josojo marked this pull request as ready for review January 21, 2020 22:39

adding explainatory comment

3beb616

fleupold approved these changes Jan 22, 2020

View reviewed changes

nlordell reviewed Jan 22, 2020

View reviewed changes

bh2smith approved these changes Jan 22, 2020

View reviewed changes

josojo added 2 commits January 22, 2020 12:52

your comments

8b3c210

updating version to 0.1.5-dev

2be5b8d

fedgiac approved these changes Jan 22, 2020

View reviewed changes

josojo added 2 commits January 22, 2020 15:15

updating readme

ec9bcab

Merge branch 'master' into newMigrationLogic

f42b5e8

josojo merged commit 647875d into master Jan 22, 2020

josojo deleted the newMigrationLogic branch January 22, 2020 14:29

fleupold reviewed Jan 22, 2020

View reviewed changes

josojo mentioned this pull request Jan 22, 2020

[Fix] non-development migrations #494

Merged

fleupold mentioned this pull request Jan 22, 2020

[Easy] Revert version bump #495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating deployment process for better external use of the project #489

Updating deployment process for better external use of the project #489

josojo commented Jan 21, 2020 •

edited

fleupold left a comment

fleupold Jan 22, 2020

nlordell Jan 22, 2020 •

edited

josojo Jan 22, 2020 •

edited

nlordell left a comment

nlordell Jan 22, 2020

josojo Jan 22, 2020

anxolin Jan 22, 2020 •

edited

josojo Jan 22, 2020

josojo Jan 22, 2020

anxolin Jan 22, 2020 •

edited

anxolin Jan 22, 2020 •

edited

josojo Jan 22, 2020

bh2smith left a comment

bh2smith commented Jan 22, 2020

fedgiac left a comment

fleupold Jan 22, 2020

Updating deployment process for better external use of the project #489

Updating deployment process for better external use of the project #489

Conversation

josojo commented Jan 21, 2020 • edited

fleupold left a comment

Choose a reason for hiding this comment

fleupold Jan 22, 2020

Choose a reason for hiding this comment

nlordell Jan 22, 2020 • edited

Choose a reason for hiding this comment

josojo Jan 22, 2020 • edited

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

nlordell Jan 22, 2020

Choose a reason for hiding this comment

josojo Jan 22, 2020

Choose a reason for hiding this comment

anxolin Jan 22, 2020 • edited

Choose a reason for hiding this comment

josojo Jan 22, 2020

Choose a reason for hiding this comment

josojo Jan 22, 2020

Choose a reason for hiding this comment

anxolin Jan 22, 2020 • edited

Choose a reason for hiding this comment

anxolin Jan 22, 2020 • edited

Choose a reason for hiding this comment

josojo Jan 22, 2020

Choose a reason for hiding this comment

bh2smith left a comment

Choose a reason for hiding this comment

bh2smith commented Jan 22, 2020

fedgiac left a comment

Choose a reason for hiding this comment

fleupold Jan 22, 2020

Choose a reason for hiding this comment

josojo commented Jan 21, 2020 •

edited

nlordell Jan 22, 2020 •

edited

josojo Jan 22, 2020 •

edited

anxolin Jan 22, 2020 •

edited

anxolin Jan 22, 2020 •

edited

anxolin Jan 22, 2020 •

edited