search

makerdao / dss-deploy-scripts

GNU Affero General Public License v3.0
49 stars 59 forks source link

add separate faucet deploy step #55

Closed iamchrissmith closed 5 years ago

iamchrissmith commented 5 years ago

Separate out faucet deploy from base-deploy. This does not remove the requirement that a faucet is deployed; however, it does allow for different scripts to use different faucets

asymmetric commented 5 years ago

it does allow for different scripts to use different faucets

Do you mean this as "different deployment targets"? If so, I don't understand: they can already deploy different faucets, based on their JSON config.

Or do you mean different scripts under scripts/? If so, when do we need multiple faucets?

iamchrissmith commented 5 years ago

they can already deploy different faucets, based on their JSON config.

I'm not sure I follow this. My understanding is that we can currently deploy a "non-standard" faucet by: 1 - Manually deploying the faucet 2 - Exporting that address to $FAUCET 3 - Running the deployment script

I'm not sure how the JSON config comes into play.

If $FAUCET is not defined before the script runs, then it will launch a new "standard" faucet. My hopes here would be that the deploy-kovan script would call the scripts/faucet-deploy script, but if we wanted to launch a different faucet for mainnet then our deploy-mainnet script would call a new scripts/faucet-restricted-deploy script.

This would reduce the manual setup of having to deploy a faucet separate from the deployment script.

gbalabasquer commented 5 years ago

Do we really need to do a special script for something will be run just once or maximum a few times? Just wondering if adding the conditions are worth it.

iamchrissmith commented 5 years ago

@gbalabasquer My feeling is that any time we require a manual setup step before running the deployment it creates additional overhead and work for anyone who wants to run the deployments. While these are not going to be run in the wild too often, I can definitely see value in having them run frequently in local testing environments for us and others to build tools off of.

Moving the faucet out of base-deploy does contradicts the changes you recently made moving everything into base-deploy, so maybe the right way to configure this would be if a script needs to have a special faucet, it deploys that (in the script) and exports the address before running base-deploy. That would eliminate the conditionals and still allow for configuration in the scripts.

What do you all think?

gbalabasquer commented 5 years ago

@iamchrissmith yeah, that was I was thinking, to have the faucet deployed and exporting the variable so it doesn't execute a new deployment of it during the script running. The only thing I'm not sure is about "deploying in the same script". I wouldn't keep any custom faucet in this repo, so yeah maybe someone can design another script which calls this one but previously creates the custom faucet and exports it as FAUCET.

iamchrissmith commented 5 years ago

@gbalabasquer Great, sounds like I'm on the same page. However, I'm not sure about "not keeping it in this repo". Couldn't it be one of the "cases"? Or do you think it would be better to have

  1. a totally separate repo for the custom faucet deployment; or,
  2. add a deploy script to the repo where the custom faucet contract code is (right now I guess this) kind of similar to dss-deploy
iamchrissmith commented 5 years ago

won't merge as will be handled in a different way