onflow / flow-cli

The Flow CLI is a command-line interface that provides useful utilities for building Flow applications
https://onflow.org
Apache License 2.0
206 stars 66 forks source link

[EPIC] CLI new import schema #711

Closed sideninja closed 1 year ago

sideninja commented 1 year ago

Import Schema

****Simple import schema****

The identifier will specify the name of the contract developer wishes to import. If no account is specified the default service account will be used. That will only be possible on an emulator network. When moving to the testnet or mainnet or any other networks import will have to also specify the name of the account as in the account import schema.

import {IDENTIFIER}

An example of the simple import schema is:

import Foo

Account **import schema**

Extending the simple import schema account is specifying the name of the account where the contract will be found.

import {IDENTIFIER} from "{ACCOUNT NAME}"

An example of the account import schema is:

import Foo from "Alice"

The identifier should match the contract name (although this might not be required it should be enforced for consistency). The resolution of an identifier to the contract source code and the account name to an address will be done using the flow.json configuration.

Flow.json

CLI configuration will have to be adjusted to support resolving the new imports. This affects the contracts and deployments sections of the flow.json.

**Contracts**

The contracts section will be changed to support the import schema.

...
"contracts": {
    "{IDENTIFIER}": "{SOURCE}"
}
...

Where the identifier is the same identifier as used in the import schema and it should be the same as the contract name. The source is the location of the source code, which can be anything as long as there is a resolver implemented that knows how to fetch the code from the location. That is to future-proof any possible implementations.

Examples of contracts would look like this (keep in mind we won’t support all sources in the beginning):

"contracts": {
    "Foo": "./contracts/Foo.cdc", // file location source
    "Bar": "github.com/onflow/core-contracts/contracts/Bar.cdc", // github location
    "Zoo": "ipfs://Fosdd27VE9Kn/Zoo.cdc", // IPFS location
}

There will be different resolvers implemented in the libraries supporting the location and based on the format executed to get the contract source code. That is a whole topic we won’t be covering here for the same of only supporting file location in the beginning.

****Accounts****

Accounts will change to support import schema. We now require the account with the same name to exist on different networks and that has to be expressed in configuration.

"accounts": {
    "{ACCOUNT NAME}": "{Address}",
    "{MULTI-NETWORK ACCOUNT NAME}": {
        "{NETWORK NAME}": "{Address}"
    }
} 

Examples of accounts would look like this:

"accounts": {
    "Alice": "0x01",
    "Bob": {
        "emulator": "0x01",
        "testnet": "0x1231232123",
    }
}

As a side effect of this change, we are removing the keys section. This is intentional although should not be discussed in detail in this document, it addresses issues with keys being included and eventually leaked as part of CLI configuration. All accounts in the flow.json will become “read-only” and then only when executing the actions that require access to specific keys should that action require those keys as input, those can then be sourced from different locations such as using KMS, using keystore, using files etc.

**Deployments**

Will be removed since all the information can be found in the source code and it only provides a layer of configuration that can get out-of-sync with the source of truth (contract source code).

bluesign commented 1 year ago

I think multiple sources are a really bad idea here, we should somehow limit it to chain and file system. Otherwise, places supporting this standard will generate a huge compatibility matrix. (ex: I can put GitHub URL in flow.json in CLI, but tool X ( js-testing, e.g. ) doesn't support it.

Also here when we support chain and new import syntax, we will not no longer need the standard contracts ? Or how it will work ? Let's say FungibleToken from testnet and mainnet differ.

the Account also changes meaning ( from flow account key to flow account group ) I guess here, does this also invalidate deployments?

Moving keys out is great.

sideninja commented 1 year ago

I think multiple sources are a really bad idea here, we should somehow limit it to chain and file system. Otherwise, places supporting this standard will generate a huge compatibility matrix. (ex: I can put GitHub URL in flow.json in CLI, but tool X ( js-testing, e.g. ) doesn't support it.

It's not intended for that to be implemented yet, but more of a freedom we have with this approach when we possibly decide we would want to or not. I feel that a "contract package manager" could be a tool doing that so it shouldn't matter which SDK or tool is consuming that. I could see that also being part of another file, but then again this is not intended to be worked on now as a lot of things are not yet clear, but if we are making changes I want them to be flexible enough for future tools and I feel restricting us to file location was not smart as there are many places where that location is not accessible (web apps).

Also here when we support chain and new import syntax, we will not no longer need the standard contracts ? Or how it will work ? Let's say FungibleToken from testnet and mainnet differ.

What do you mean support chain and new import syntax? Standard contracts won't be treated differently, for FT to work you would define FT account and then specify addresses on each network.

the Account also changes meaning ( from flow account key to flow account group ) I guess here, does this also invalidate deployments?

Deployments could at later point be removed since we can infer everything from source code which should be source of truth probably for this.

bluesign commented 1 year ago

What do you mean support chain and new import syntax? Standard contracts won't be treated differently, for FT to work you would define FT account and then specify addresses on each network.

"contracts": {
    "Foo": "./contracts/Foo.cdc", // file location source
    "Moo": "0x0123123123", // Flow location
    "Bar": "github.com/onflow/core-contracts/contracts/Bar.cdc", // github location
    "Zoo": "ipfs://Fosdd27VE9Kn/Zoo.cdc", // IPFS location
}

Now in this example, let's say I have FungibleToken ( a standard token ) and it is on testnet and mainnet. ( But as we don't have sync in between them, they have different content ) It seems like there is no by network config for source.

Something like:

"contracts": {
    "Moo": {
                "mainnet" : "0x0123123123", // Flow location,
                "testnet": "0x098989898"
        }  
}
sideninja commented 1 year ago

@bluesign I don't think we will allow addresses to concrete contract above. It's a bad example, will fix. The contracts location will be referenced by account containing that contract and then source could be referenced by the name of that account. Example:

"contracts": {
    "Moo": "Alice" 
}, 
"accounts": {
         "Alice": { "testnet": "0x1", "mainnet": "0x02", "emulator": "0x03" }
}

Please note all of the above is not intended to be solved now, it just shows flexibility in the format so we can do that in the future. This is not something we will be adding yet, we just want to support the new format.

Because this also opens a question whether that's correct. Because isn't it more correct to just provide address on Flow like in the first example because why would source change implicitly when switching networks, that might cause issues dunno. I feel there's two concepts here, one is deployment instruction (where to deploy a contract) and the other is source (where we can retrieve contract source) and I'm not sure the second one should be changing. But then again, not yet the purpose to solve but very good questions to ask.

justinbarry commented 1 year ago

Interesting! I love seeing the key section disappear. Can we add a version to the schema to help with backward compatibility?

What about the cases where:

  1. I am locally developing contracts AND a version of that contract is already deployed?
  2. What if the contracts are deployed to different accounts on different networks?

It seems there was some accommodation made for this here

sideninja commented 1 year ago

@justinbarry

Can we add a version to the schema to help with backward compatibility? Backward compatibility is really important for sure, however, how we've done it so far was to just detect the format, so we didn't need to add a version to it. I'm aware of some arguments for and against, so I would just judge it based on the usecase, which for now was mostly CLI handling this and thus we decided to detect the schema format. If you will have issues detecting that in FCL let me know.

I am locally developing contracts AND a version of that contract is already deployed? Good question. The idea is you can provide multiple networks for contract as well, something like:

"contracts": {
"Moo": {
"mainnet" : "FooAccount",
"emulator": "./Foo.cdc"
}  
}

But I'm open to hear more ideas.

What if the contracts are deployed to different accounts on different networks? So my thinking for this is that an account with same name should be used for all networks. I might be wrong. But the way I'm thinking is that it's nice to organise the code that way. Any usecases you see where this would be a problem?

justinbarry commented 1 year ago

@justinbarry

Can we add a version to the schema to help with backward compatibility? Backward compatibility is really important for sure, however, how we've done it so far was to just detect the format, so we didn't need to add a version to it. I'm aware of some arguments for and against, so I would just judge it based on the usecase, which for now was mostly CLI handling this and thus we decided to detect the schema format. If you will have issues detecting that in FCL let me know.

Every version would presumably map to a schema defined in a flip, correct? A version would be nice to avoid any future ambiguity in whatever patterns we are using to detect the version. Is it a problematic thing to add?

I am locally developing contracts AND a version of that contract is already deployed? Good question. The idea is you can provide multiple networks for contract as well, something like:

"contracts": {
  "Moo": {
                "mainnet" : "FooAccount",
                "emulator": "./Foo.cdc"
        }  
}

But I'm open to hear more ideas.

What if the contracts are deployed to different accounts on different networks? So my thinking for this is that an account with same name should be used for all networks. I might be wrong. But the way I'm thinking is that it's nice to organise the code that way. Any usecases you see where this would be a problem?

This would be supported by the multiple source per contract example you pasted above. Which is great!

justinbarry commented 1 year ago

@sideninja

"accounts": {
    "{ACCOUNT NAME}": "{Address}",
    "{MULTI-NETWORK ACCOUNT NAME}": {
        "{NETWORK NAME}": "{Address}"
    }
} 

What network are we assuming ACCOUNT NAME is on?

sideninja commented 1 year ago

The default was always the emulator.