unlock-protocol / unlock

Ʉnlock is a protocol for memberships built on a blockchain.
https://unlock-protocol.com
MIT License
842 stars 249 forks source link

Add /smart-contracts/build to .gitignore #363

Closed hakimelek closed 5 years ago

hakimelek commented 6 years ago

I am not sure if you think that this is worth doing or not but I think /smart-contracts/build is being generate on every npm prestart thus have different versions for different users. Maybe we can move this to .gitignore?

julien51 commented 6 years ago

Yeah, this is an issue, but we cannot put this into .gitignore either because these artifacts actually also keep track of the smart contracts being deployed on the actual networks (which we absolutely want to keep track of!). We need to research this further. Ideally, we'd have multiple artifacts file for each smart contract, on each network.

julien51 commented 6 years ago

I think @nfurfaro is looking into this!

cellog commented 6 years ago

Are you assuming that users will install from git? The solution to this issue could be to instead release unlock as an npm package. This would also address #468.

Either way, transitive files like this should not be stored in the application, but in a user-configured location or perhaps in a database (my opinion)

Adding a script that installs and configures unlock (a la create-react-app) would be an elegant solution to both.

julien51 commented 6 years ago

Well, I think the artifacts also kept track of the address where to the smart contract is deployed which means that every time it is being deployed we get a new "version" of the file. Since we all use different ganache instances we get a different version of the file every time.

We need to find a solution to that.

julien51 commented 6 years ago

@cellog the react app needs to know the address of the deploy smart contracts but also the "abi" (which is really a description of the interfaces offered by the smart contract). Both of these things are included in the artifacts files.

So in a way the artifacts are more "config" files for the react app than transient files which we could discard easily. The challenge comes from the fact that for each smart contract, there is only a single artifact file which contains the addresses of the deployed smart contract on each network (the test/local ones... which are not important to keep, but also the "production" versions which are extremely important to keep).

I hope this clarifies things.

nfurfaro commented 6 years ago

Yeah, this is an issue, but we cannot put this into .gitignore either because these artifacts actually also keep track of the smart contracts being deployed on the actual networks (which we absolutely want to keep track of!). We need to research this further. Ideally, we'd have multiple artifacts file for each smart contract, on each network.

Hmm. I'm wondering if this (multiple files) would interfere with network autodetection?

"Because the network is auto-detected by the contract artifacts at runtime, this means that you only need to deploy your application or frontend once. When you run your application, the running Ethereum client will determine which artifacts are used, and this will make your application very flexible. As an example, if you were to deploy a web application to http://mydapp.io, you could navigate to that address using your favorite wallet-browser (like MetaMask, or Mist) and your dapp would work correctly regardless of the Ethereum network the wallet-browser was connected to. If the wallet-browser was connected to the live network, your dapp would use the contracts you deployed on the live network. If on Ropsten, the contracts you deployed to Ropsten would be used."

https://truffleframework.com/docs/truffle/advanced/networks-and-app-deployment#application-deployment

nfurfaro commented 6 years ago

This is a possibility, but I'm still doing some reading on this.

TLDR; Modify the migration file to write out the relevant details to the specified file after each deployment. https://ethereum.stackexchange.com/questions/44271/best-practice-for-sharing-truffle-build-files-between-developers

nfurfaro commented 6 years ago

It looks like we have a few options here for managing truffle artifacts. It's worth pointing out a couple things. Each file in build/contracts tracks a few different things, which include the source, ABI, byte code and networks/addresses each contract has been deployed to. While the ABI and bytecode can easily be regenerated by running truffle compile, the deployed addresses cannot, and must be preserved. When deploying a contract to a new network on which it hasn't been deployed, a new entry will be created in the .Json file under networks. If redeploying to an existing network, the address and transaction hash are updated. From what I can see, 1 possible way to loose track of deployed addresses is if a developer (for whatever reason) deletes the build/ directory on their local branch. If they then run a compile or migration, truffle will recreate the build/ directory, but it won't contain the past deployment history. If this then gets added/commited and merged to the master branch via a pr, that would be bad.

1.) Add to .gitignore as suggested. Not ideal, but would prevent the scenario above. I think it's fair to say that "significant " deployments, say to Ropsten or Mainnet, will not be very common. It would be fairly easy to remove the build/ directory from the .gitignore temporarily after a significant deployment to allow updating the master branch. 2.) Similar to 1, it's possible to change the default directory where truffle writes the build artifacts. This is done in truffle.js. It doesn't look like this can be done on a per-network basis. This new default directory can be gitignored. As above, for a significant deployment, the developer would need to comment out this line, causing truffle to use the default build/ directory, which then gets commited. 3.) It looks like we could build up the network deployment details portion of the artifacts manually, on a per-network basis in the migrations file. The problem with this approach is that there's no way for truffle to know this. It will still always look to the default directory for artifacts when it needs them. 4.) When deploying via zos, this problem is basically solved. zOS seperates the files it produces upon deployment in a way that makes it easier to safely Gitignore some files but not others. Towards the bottom of the page linked here sre some ideas about version control and zos configuration files.

https://docs.zeppelinos.org/docs/configuration.html

julien51 commented 6 years ago

That Zos approach is probably the best one? Unless there is a system of hooks we can use after a deploy to copy the artifacts into a non .gitignore-ed file only when the deploy was done on a non local network?

nfurfaro commented 6 years ago

@julien51 I think the zOS approach makes the most sense too. We're already using it for upgradeability, and then we basically get this cleaner artifact management for free. RE: "Unless there is a system of hooks we can use after a deploy ..." There is this approach, where we would change the migration file and write the ABI and its address after the deploy.


var MyContract = artifacts.require('./MyContract.sol');
var fileContent = require('../build/contracts/MyContract.json');

module.exports = function(deployer) {
  deployer.deploy(MyContract).then(() => {
    var contractConfiguration = {
        abi: fileContent.abi,
        address: MyContract.address
    };

    fs.writeFileSync('contractConfiguration/MyContract.json', JSON.stringify(contractConfiguration), { flag: 'w' });
  });
};```
nfurfaro commented 6 years ago

This could be made to depend on the network being used like this:


  if (network == "live") {
    // Do something specific to the network named "live".
  } else {
    // Perform a different step otherwise.
  }
}```

I think the problem with this way vs the zOS approach is that writing to different directories manually like this will likely mean that truffle won't know where the artifacts are stored, and I don't know that there's any way to change that.
nfurfaro commented 6 years ago

PR# 480 fixed this, so I'm closing.

julien51 commented 5 years ago

So, we just hit a dead end on our hunt for a solution where *.json files are not in git (see https://github.com/unlock-protocol/unlock/pull/546).

Basically, if there artifacts are not in version control, we need to generate them when running tests and when deploying... We can only generate them by running truffle compile. BUT (for now at least) we cannot compile the smart contracts when we deploy because this would require that the platform on which we deploy (netlify) actually has truffle...

So we need to add back the *.json files to git AND we need to find a way to just ignore "local" versions.

julien51 commented 5 years ago

@nfurfaro Do you mind adding a little more in here now that this is probably (?) solved through our use of zos?

nfurfaro commented 5 years ago

Update: Now that we're using zos, This seems to be solved for now. *.json files are added again and we're ignoring "local" versions. As Julien mentioned above, "we cannot compile the smart contracts when we deploy because this would require that the platform on which we deploy (netlify) actually has truffle..." One question that remains is, when zOS is used to deploy a contract to a "real" network, are the existing artifacts overwritten? That is, If we deploy to the mainnet or Ropsten, are the network details for Rinkeby in our *.json files overwritten? I don't believe they will be (this would be a bug in my mind), but the only way to test this is to deploy to another network.

There are some more details in the wiki here: https://github.com/unlock-protocol/unlock/wiki/Blockchain-layer-and-CI#questions

julien51 commented 5 years ago

So... I am still not sure what is going on here but the initial problem is still happening: every time I run the code locally the *.json files are changed and git wants me to commit the changes. Yet, we need these files in git because they are used to build/test/deploy the react application.

So I am still not sure how to achieve what we're looking for:

Please clarify why you think this is solved?

nfurfaro commented 5 years ago

@julien51 So, looking back to #480, I thought this was at least temporarily solved. There are some manual steps in smart-contracts/Deployment.md. TLDR; The plan was to have the build dir in .gitignore most of the time, and comment it out just when making a commit after a significant deployment or changes to the contracts. As the artifacts are already in git, the app still has access to them. However, if we change the .gitignore git will want us to add those changes as well. We would then need to change it back again and commit changes again, which seems crazy... So, this is still not solved, I take it back. There's an interesting issue on the zos github about some of the issue here that may help provide a cleaner solution. https://github.com/zeppelinos/zos/issues/420 as well as a good article on medium that is closely related. https://medium.com/@asselstine/integrating-zeppelin-os-with-truffle-841e89fa34b3 I'm particularly interested in a comment on issue #420 by asselstine:

"We also use Circle CI as our build server, so I needed a way to simply take the zos.ropsten.json proxy information and inject it into newly compiled contract Truffle artifacts. The artifacts are not committed to git, so naturally we have to compile them in CI."

nfurfaro commented 5 years ago

It looks like the approach he's taking is compiling the contracts in CI for a consistent build, and then running this script to inject the zos network details into the build artifacts. https://gist.github.com/asselstine/6c6f5ee04e1fae90ad0c70f282225b60 This package has been recently published. I've tried it out and it works. https://www.npmjs.com/package/zos-truffle-merge

cellog commented 5 years ago

@julien51 is this resolved? The only thing I still see not in .gitignore is artifacts

julien51 commented 5 years ago

Yes! Closing now