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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deploy-contracts command #45

Closed
wants to merge 68 commits into from
Closed

Add deploy-contracts command #45

wants to merge 68 commits into from

Conversation

psiemens
Copy link
Contributor

@psiemens psiemens commented Feb 1, 2021

Closes #30

Description

This PR introduces the flow beta deploy-contracts command, which automates the contract deployment process for Flow developers.

This command reads contract source files defined in flow.json and maps them to their declared target accounts. The following features are supported:

  • A contract can be mapped to a different account on each network (emulator, testnet, mainnet)
  • A contract can import other contracts by filename. Filenames are replaced with their target addresses (declared in flow.json) before deployment
  • A contract can import a contract by alias. Aliases are replaced before deployment using the "aliases" map in flow.json.
  • The deployment command resolves dependencies between contracts and deploys them in deterministic topological ordering so that a contract is only deployed after its dependencies. Import cycles result in an error.

cli-contract-deployment-6

Below is the flow.json config used in the above screen recording:

{
  "emulator": {
    "default": {
      "port": 3569,
      "serviceKey": {
        "privateKey": "40055011eda87a6c7b38943d1cc81c013eeb5bf791ad5330a375ab229a401e95",
        "signatureAlgorithm": "ECDSA_P256",
        "hashAlgorithm": "SHA3_256"
      }
    }
  },
  "networks": {
    "emulator": {
      "host": "127.0.0.1:3569",
      "chain": "flow-emulator"
    }
  },
  "aliases": {
    "FungibleToken": {
      "emulator": "0xee82856bf20e2aa6"
    }
  },
  "contracts": {
    "nft": {
      "target": {
        "emulator": "emulator-service-account"
      },
      "source": {
        "NonFungibleToken": "./cadence/kittyItems/contracts/NonFungibleToken.cdc"
      }
    },
    "kitty-items": {
      "target": {
        "emulator": "emulator-service-account"
      },
      "source": {
        "Kibble": "./cadence/kibble/contracts/Kibble.cdc",
        "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
        "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc"
      }
    }
  },
  "accounts": {
    "emulator-service-account": {
      "chain": "flow-emulator",
      "address": "service",
      "keys": [
        {
          "signatureAlgorithm": "ECDSA_P256",
          "hashAlgorithm": "SHA3_256",
          "type": "hex",
          "index": 0,
          "context": {
            "privateKey": "40055011eda87a6c7b38943d1cc81c013eeb5bf791ad5330a375ab229a401e95"
          }
        }
      ]
    }
  }
}

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@psiemens psiemens added the Feature A new user feature or a new package API label Feb 1, 2021
@sideninja
Copy link
Contributor

Documentation
I think it would be great to have better documentation on the command and more verbose error handling in cases contracts are not deployed. One such case is when the flow emulator returns an error.

Sync Configuration
Another issue might be that developers will init configuration, run the emulator, change the configuration file, and then since the emulator will not be restarted there will be a mismatch between emulator and deployer.

Security
One concern I have is security with flow.json file since the nature of this file is to be included in svn the same way as package.json is. There is one important difference between those two, flow.json includes secret keys and I can imagine devs will use this for mainnet deployment and they will forget to remove the secret from that configuration. Something to keep in mind.

Remote Imports
It could also be cool to have an option to include contracts from URLs and github. Maybe even built into tool some most standard contracts such as NFT so you don't even have to provide URLs but the tool knows where the contracts are (this second step might be useful once the community establish standard contracts the same way as for an example erc on ethereum)

Configuration Flow
Lastly I think besides having improved documentation for CLI in general and specifically for this subcommand, there could also be a subcommand that would allow adding contracts to the configuration so developers wouldn't have to write config manually. Pros of this in my opinion are avoiding typos, commands could offer natural flow with documentation so even if a developer won't read the documentation (I prefer using tools that don't come with a manual you must read), they can figure out how it works by just trying out (which is almost impossible in case of manual configuration), another benefit of this subcommand is potentially having an ability to just add contracts in a folder and not specifying one by one (some kind of discovery).

@sideninja
Copy link
Contributor

sideninja commented Feb 3, 2021

Feedback on the flow.json schema, I agree flow.private.json is a good idea to maybe avoid pushing secrets to github, it would also be helpful to have a warning in CLI when ran to warn you to be careful with the secret when running on mainnet? Or should CLI even discourage using that for mainnet?

In any case, I strongly think there should be JSON schema validation. If devs will change and insert JSON conf by hand CLI should detect errors and even better suggest fixes.

I think schema can be further simplified by not specifying default values. Such an example is:

"signatureAlgorithm": "ECDSA_P256",
"hashAlgorithm": "SHA3_256",
"type": "hex",
"index": 0,

These keys are default and I think should be avoided in config but if a developer wants to change them they can.
My thinking is to have as little as possible in config so it is easier to read and understand.

Another thing that might be confusing a bit is that under the contracts key the first key are not contracts themselves but accounts. I'm not even sure what that first key represents and how can it be used (but that still might be because of my lack of understanding the code):

"contracts": {
    "nft": {
      "target": {
        "emulator": "emulator-service-account"
      },
      "source": {
        "NonFungibleToken": "./cadence/kittyItems/contracts/NonFungibleToken.cdc"
      }
    },

Is nft key meant to reference anything? If not it might be a good idea to just make an array with objects.

Are emulator, testnet and mainet meant to be magical values with special meaning or just something we should define in config? In one way it could be good to have this predefined values but also allow for user to define more, since I think there can also be other networks (testing, qa... which could be different environments devs will potentially have setup locally or something).

@sideninja
Copy link
Contributor

sideninja commented Feb 3, 2021

I created my take on a config schema that is compared to the current config for an example project.

Test project:
Kitty-items

Deployed to emulator on two accounts,

Deployed to testnet on two accounts

I managed to lower the complexity of config (line count) by 58% for same configuration - 49 lines vs 114 lines

schema current:

{
  "emulator": {
    "default": {
      "port": 3569,
      "serviceKey": {
        "privateKey": "dd72967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47",
        "signatureAlgorithm": "ECDSA_P256",
        "hashAlgorithm": "SHA3_256"
      }
    }
  },

  "networks": {
    "emulator": {
      "host": "127.0.0.1:3569",
      "chain": "flow-emulator"
    },
    "testnet": {
      "host": "access.testnet.nodes.onflow.org:9000",
      "chain": "testnet"
    }
  },

  "aliases": null,

  "contracts": {
    "test": {
      "target": {
        "emulator": "emulator-service-account-2",
        "testnet": "testnet-account-2"
      },
      "source": {
        "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
        "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc"
      }
    },
    "hungry-kitties": {
      "target": {
        "emulator": "emulator-service-account",
        "testnet": "testnet-account"
      },
      "source": {
        "NonFungibleToken": "../hungry-kitties/cadence/contracts/NonFungibleToken.cdc",
        "FungibleToken": "../hungry-kitties/cadence/contracts/FungibleToken.cdc",
        "Kibble": "./cadence/kibble/contracts/Kibble.cdc",
        "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
        "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc"
      }
    }
  },
  
  "accounts": {
    "testnet-account-2": {
      "address": "0x123123",
      "chain": "testnet",
      "keys": [
        {
          "type": "hex",
          "index": 0,
          "signatureAlgorithm": "ECDSA_P256",
          "hashAlgorithm": "SHA3_256",
          "context": {
            "privateKey": "1232967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47"
          }
        }
      ]
    },
    "testnet-account": {
      "address": "0x222222",
      "chain": "testnet",
      "keys": [
        {
          "type": "hex",
          "index": 0,
          "signatureAlgorithm": "ECDSA_P256",
          "hashAlgorithm": "SHA3_256",
          "context": {
            "privateKey": "222967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47"
          }
        }
      ]
    },		
    "emulator-service-account": {
      "address": "service",
      "chain": "flow-emulator",
      "keys": [
        {
          "type": "hex",
          "index": 0,
          "signatureAlgorithm": "ECDSA_P256",
          "hashAlgorithm": "SHA3_256",
          "context": {
            "privateKey": "dd72967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47"
          }
        }
      ]
    },
    "emulator-service-account-2": {
      "address": "0x2412848124",
      "chain": "flow-emulator",
      "keys": [
        {
          "type": "hex",
          "index": 0,
          "signatureAlgorithm": "ECDSA_P256",
          "hashAlgorithm": "SHA3_256",
          "context": {
            "privateKey": "30dd224db9c7a70f035f8092f6c5a44c920bb6cc4c86c4d7b1b34cb32d2f936f"
          }
        }
      ]
    }
  }
}

shorter version:

  • remove default values:
    • account remove all the keys default values, make it either string for a single key with all default values or array for multiple keys as in old format but since most cases will probably be single key with default algos maybe this is better
  • I think contracts section can define key value for each contract and from that point on config just reference them by name - so we don't have duplication of paths as visible in above scenario and also allows us to define array of contracts for each account (shorter more visible)
  • remove chain value since everything can be referenced by network name although I might not understand that correctly
  • emulator under network? since we define networks can that be a place for emulator config to also be defined since emulator is treated as a network in this case? Also multiple networks could have their own config in there - example gcloud-testnet could include also keys for api etc
{
  "contracts": {
    "NonFungibleToken": "../hungry-kitties/cadence/contracts/NonFungibleToken.cdc",
    "FungibleToken": "../hungry-kitties/cadence/contracts/FungibleToken.cdc",
    "Kibble": "./cadence/kibble/contracts/Kibble.cdc",
    "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
    "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc"
  },

  "deploy": {
    "account-1": ["KittyItems", "KittyItemsMarket"],
    "account-2": ["NonFungibleToken", "FungibleToken", "Kibble", "KittyItems", "KittyItemsMarket"],
    "account-3": ["KittyItems", "KittyItemsMarket"],
    "account-4": ["NonFungibleToken", "FungibleToken", "Kibble", "KittyItems", "KittyItemsMarket"]
  },

  "accounts": {
    "account-1": {
      "address": "0xabd6e0586b0af502",
      "keys": "1232967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47",
      "network": "testnet"
    },
    "account-2": {
      "address": "0x2c1162386b0a245f",
      "keys": "22232967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47",
      "network": "testnet"
    },
    "account-3": {
      "address": "service",
      "keys": "service",
      "network": "emulator"
    },
    "account-4": {
      "address": "0xf8d6e0586b0a20c7",
      "keys": "4442967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47",
      "network": "emulator"
    }
  },

  "networks": {
    "emulator": {
      "host": "127.0.0.1:3569",
      "keys": "dd72967fd2bd75234ae9037dd4694c1f00baad63a10c35172bf65fbb8ad74b47"
    },
    "testnet": "access.testnet.nodes.onflow.org:9000"
  },
  
  "aliases": null
}

@psiemens
Copy link
Contributor Author

psiemens commented Feb 3, 2021

Some sketches from our call:

flow project deploy account-2 account-3 account-4

flow project deploy --network emulator

flow project deploy nft kitty-items

flow project deploy kitty-items
"contracts": {
  "NonFungibleToken": "../hungry-kitties/cadence/contracts/NonFungibleToken.cdc",
  "Kibble": "./cadence/kibble/contracts/Kibble.cdc",
  "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
  "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc"
},
"aliases: {
  "emulator": {
      "FungibleToken": "0xabc",
  },
  "testnet": {
    "FungibleToken": "0xabc",
    "NonFungibleToken": "0x1234",
  }
}
"deploy": {
    "testnet": {
      "account-1": ["KittyItems", "KittyItemsMarket"],
      "account-2": ["Kibble", "KittyItems", "KittyItemsMarket"],
    }, 
    "emulator": {
      "account-3": ["KittyItems", "KittyItemsMarket"],
      "account-4": ["NonFungibleToken", "Kibble", "KittyItems", "KittyItemsMarket"]
    }
  },
  "contracts": {
    "nft": {
      "target": {
        "emulator": "emulator-service-account",
        "testnet": "0xabc"
      },
      "source": {
        "NonFungibleToken": "./cadence/kittyItems/contracts/NonFungibleToken.cdc"
      }
    },
    "kitty-items": {
      "target": {
        "emulator": "emulator-service-account",
         "testnet": "testnet-admin-account"
      },
      "source": {
        "Kibble": "./cadence/kibble/contracts/Kibble.cdc",
        "KittyItems": "./cadence/kittyItems/contracts/KittyItems.cdc",
        "KittyItemsMarket": "./cadence/kittyItemsMarket/contracts/KittyItemsMarket.cdc",
      }
    }
  }

@bjartek
Copy link
Collaborator

bjartek commented Feb 5, 2021

it would be nice to have some cli features to help generate/modify that flow.json file.

  • when I add an account it can be added to the file
  • if i have a folder of contracts i want to be added to an account i can do that

@10thfloor
Copy link
Contributor

@sideninja

My thinking is to have as little as possible in config
👍🏼

@turbolent
Copy link
Member

This looks great! 👏 🎉

@10thfloor
Copy link
Contributor

10thfloor commented Feb 5, 2021

This is amazing. This config #45 (comment) looks really promising.

Quick thought relevent to this thread: What would be the command for doing a 'reset' of a deployment?

flow project reset 

Followed by a confirmation step?

Are you sure you want to reset "project-kittyItems" ? (N/y)

@10thfloor
Copy link
Contributor

10thfloor commented Feb 5, 2021

Also, some contracts could be deployed to multiple accounts no?
We could allow for:

...
 "contracts": {
    "Profile": {
      "target": {
        "emulator": "emulator-service-account",
        "testnet": ["0xabc", "0xdef"] // List of accounts
      },
      "source": {
        "Profile": "./cadence/socialapp/contracts/Profile.cdc"
      }
    },
  ...

Copy link
Contributor Author

@psiemens psiemens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great refactor! @sideninja

My comments are minor and mostly cosmetic. Apart from my suggested changes, I'd say this is pretty much ready to go 💯

flow/beta/cli/config/json/account.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/account.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/account.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/account.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/account.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/config.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/contract.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/deploy.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/deploy.go Outdated Show resolved Hide resolved
flow/beta/cli/config/json/network.go Outdated Show resolved Hide resolved
sideninja and others added 9 commits February 15, 2021 16:21
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
Co-authored-by: Peter Siemens <peterjsiemens@gmail.com>
@sideninja
Copy link
Contributor

could be deployed to multiple accounts no

The way you can deploy a contract to multiple accounts with the new configuration is simply:

{
...
"deploys": {
		"emulator": {
			"emulator-account": ["FungibleToken", "NonFungibleToken"],
			"admin": ["FungibleToken"]
		}
}
...

But this feature is currently disabled as it introduces some problems - one of most important is how to know which deployment of contract gets then imported in all the imports in the contract code files. I'm sure it can be solved down the line but until it's proved to be a really useful funcionality we maybe can give priority to some other things.

@psiemens
Copy link
Contributor Author

Closing this since most of the commits are from @sideninja now. Can you open another PR from the same branch?

@psiemens psiemens closed this Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new user feature or a new package API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants