nvdtf commented 1 year ago

Overview

Following the declarative design principles of Flow CLI we want to build a simple dependency manager that enables developers to:

Import a known contract from supported locations such as Flow networks (mainnet, testnet, …), GitHub repositories, or the NFT Catalog into the current project.
Deliver code with dependencies without re-shipping copies of dependencies.
Checkout missing dependencies based on flow.json

Design

Each contract is identified in flow.json by a unique identifier. It can be imported into another contract, transaction, or script like this:

import "MyContract"

The pre-processor will replace the imports with valid Cadence syntax and addresses based on flow.json:

"contracts": {
    "MyContract": "./contracts/MyContract.cdc", // local file location
    "TopShot": "mainnet/0x0123123123/TopShot", // live network address
    "NonFungibleToken": "github.com/onflow/flow-nft/NonFungibleToken.cdc", // github
    "CoolNFT": "nft-catalog/mainnet/CoolNFT"
}

The simple syntax can be evolved in the future to support specific commit hashes or versions for GitHub, or block heights for addresses. Flow CLI will support checking for missing dependencies and checking them out into a local imports folder. The folder can be added to .gitignoreso other developers always pull the dependencies from the original source-of-truth.

`flow contracts install`

Flow CLI can check all dependencies and fetch missing ones into imports with this command:

flow contracts install

`flow contracts add`

From live Flow networks:

flow contracts add mainnet/0x0123123123/TopShot

A copy of the contract is fetched into the imports folder. For each import, a subfolder is created and the same operation is executed recursively. An alias entry is also created in flow.json to prevent re-deployments. Check out cadence-import to see this in action.

From NFT Catalog:

flow contracts add nft-catalog/mainnet/CoolNFT

The contract address is resolved via NFT Catalog. The same steps as importing from networks are applied.

From GitHub:

flow contracts add github.com/onflow/flow-nft/NonFungibleToken.cdc

A copy of the contract is fetched into the imports folder from the latest commit. The name of the contract will be resolved from the contents. If the contract has imports, the repository’s root flow.json is checked to resolve them.

### Issues
- [ ] #910 
- [ ] #331 
- [ ] #944
- [ ] #957
- [ ] #958

References

https://github.com/jrkhan/cadence-import

bjartek commented 1 year ago

Note that some contracts are very unfriendly to deploy and install on emulator, if they have very many arguments to the initializer. FiatToken i am looking at you.

We have solved this now by simple providing a shim for it that is a lot simpler. I would very much like the system to support shims for emulators that behave as the deployed version but it a lot easier to work with.

jrkhan commented 1 year ago

Thanks @nvdtf this would be quite helpful for local development, and might also help improve contract deployment pipelines. Would also like to suggest -n should be a valid alternative for specifying network in the live flow Network flavor as it would align with usage for other commands:

flow contracts add 0x0123123123/TopShot -n mainnet

btspoony commented 1 year ago

For GitHub import, can we introduce the concept of exporting contracts? There will be no ambiguity when importing.

Firstly, add an 'export' field in flow.json.

{
  "contracts": {
    "MyContract": "./contracts/PathToFolder/MyContract.cdc", // local file location, 
  },
  "exports": [
    "MyContract"
  ]
}

Then when using it, do as follows:

{
  "contracts": {
    "MyContract": "github.com/username/reponame/MyContract", // GitHub, can only import contracts from exports, no worry about the path
  },
}

bluesign commented 1 year ago

I think we need to use more advanced format, something like:

{
    "contracts": {
        "MyContract": {
            "type" : "local",
            "localPath": "./contracts/MyContract.cdc"
        },
        "TopShot":{
            "type": "flow",
            "localPath": "./contracts/utility/TopShot.cdc",
            "remotePath":{
                "mainnet": "0123123123.TopShot",
                "testnet":  "042424242.TopShot"
            }
        },
        "NonFungibleToken":{
            "type": "github",
            "localPath": "./contracts/utility/NonFungibleToken.cdc",
            "remotePath":{
                "mainnet": "github.com/onflow/flow-nft/NonFungibleToken.cdc",
                "testnet": "github.com/onflow/flow-nft/NonFungibleToken.cdc"
            },
            "aliases":{
                "mainnet": "0x0123123123",
                "testnet": "0x0424242424"
            }
        },
        "CoolNFT":{
            "type": "nftcatalog",
            "localPath": "./contracts/CoolNFT.cdc",
            "remotePath":{
                "mainnet": "mainnet/CoolNFT",
                "testnet": "testnet/CoolNFT"
            },
            "aliases":{
                "mainnet": "0x0123123123",
                "testnet": "0x0424242424"
            }
        },
    }
}

also deploy shim idea is also great ( or some init args in flow.json )

nvdtf commented 1 year ago

Note that some contracts are very unfriendly to deploy and install on emulator, if they have very many arguments to the initializer. FiatToken i am looking at you.

We have solved this now by simple providing a shim for it that is a lot simpler. I would very much like the system to support shims for emulators that behave as the deployed version but it a lot easier to work with.

@bjartek You're right. I created another issue for this, since it seems like there is no support for contract init params: https://github.com/onflow/flow-cli/issues/964 Feel free to comment if you have any suggestions on how to change flow.json for this.

Thanks @nvdtf this would be quite helpful for local development, and might also help improve contract deployment pipelines. Would also like to suggest -n should be a valid alternative for specifying network in the live flow Network flavor as it would align with usage for other commands:
flow contracts add 0x0123123123/TopShot -n mainnet

@jrkhan I re-used the simple syntax from flow.json for simplicity. We can also check if -n is present and support dropping the network name from the contract path. This can work with NFT catalog entries too. Thanks for the suggestion!

For GitHub import, can we introduce the concept of exporting contracts? There will be no ambiguity when importing.

Firstly, add an 'export' field in flow.json.
{
  "contracts": {
    "MyContract": "./contracts/PathToFolder/MyContract.cdc", // local file location, 
  },
  "exports": [
    "MyContract"
  ]
}
Then when using it, do as follows:
{
  "contracts": {
    "MyContract": "github.com/username/reponame/MyContract", // GitHub, can only import contracts from exports, no worry about the path
  },
}

@btspoony The problem with supporting exports is that the original developer must be mindful of future reusability. I think we need to have exports, but with higher-level constructs like JS packages or FLIX interactions. I don't see a problem with being able to import any given contract from anywhere.

I think we need to use more advanced format, something like:

{
    "contracts": {
        "MyContract": {
            "type" : "local",
            "localPath": "./contracts/MyContract.cdc"
        },
        "TopShot":{
            "type": "flow",
            "localPath": "./contracts/utility/TopShot.cdc",
            "remotePath":{
                "mainnet": "0123123123.TopShot",
                "testnet":  "042424242.TopShot"
            }
        },
        "NonFungibleToken":{
            "type": "github",
            "localPath": "./contracts/utility/NonFungibleToken.cdc",
            "remotePath":{
                "mainnet": "github.com/onflow/flow-nft/NonFungibleToken.cdc",
                "testnet": "github.com/onflow/flow-nft/NonFungibleToken.cdc"
            },
            "aliases":{
                "mainnet": "0x0123123123",
                "testnet": "0x0424242424"
            }
        },
        "CoolNFT":{
            "type": "nftcatalog",
            "localPath": "./contracts/CoolNFT.cdc",
            "remotePath":{
                "mainnet": "mainnet/CoolNFT",
                "testnet": "testnet/CoolNFT"
            },
            "aliases":{
                "mainnet": "0x0123123123",
                "testnet": "0x0424242424"
            }
        },
    }
}

also deploy shim idea is also great ( or some init args in flow.json )

@bluesign Definitely looking to expand the simple syntax into advanced with more config options. I was also thinking of branch commit hash, content hash, ... I'll open another issue in the future to expand the syntax.

btspoony commented 8 months ago

New thoughts: https://discord.com/channels/613813861610684416/1123314820763111465/1164943141699342527

chasefleming commented 7 months ago

The initial proposal is clean, but there seems to be a significant leap from that to the extended format, which appears necessary to convey sufficient information across networks. In a more comprehensive format, we can also embrace the existing terminology and structure familiar to developers, reducing the potential for confusion and difficulty. Consider the following structure:

{
  "contracts": {
    // local contracts
  },
  "dependencies": {
    "TopShot": {
      "path": "./imports/TopShot",
      "remoteSource": "testnet/0x0123123123.TopShot",
      "aliases": {
        "testnet": "0x0123123123",
        "mainnet": "0x0123123124"
      }
    }
  },
  "networks": {
    "emulator": "127.0.0.1:3569",
    "mainnet": "access.mainnet.nodes.onflow.org:9000",
    "testnet": "access.devnet.nodes.onflow.org:9000"
  },
  "accounts": {
    // account configurations
  }
}

In this format, dependencies from the contract manager that are not developed locally are segregated into a distinct dependencies section. This separation clarifies potential differences in key/value pairs. Due to the issue of different networks having varied contract versions, developers should be able to define a single 'source of truth' for local development, identified as the remoteSource. aliases are maintained for deploying against networks like testnet and mainnet, aligning with patterns developers are accustomed to in the contracts section.

"dependencies": {
    "TopShot": {
      "remoteSource": "testnet/0x0123123123.TopShot",
    }
  },

Executing flow contracts install would generate the path. Curious to hear feedback.

bjartek commented 7 months ago

There needs to be a mapping between networks for a contract somewhere. Personally i think the best place for this mapping is on chain. Could this be in pragmas in contracts? (all contracts must be updated for stable-cadence regardless)

nvdtf commented 7 months ago

@chasefleming We can remove path in dependencies since the local path will be dictated automatically (like dependencies/mainnet/0x1/A.cdc). We also don't expect the developer to set a custom path. For simplicity, we can also always check out the mainnet version of a dependency locally when it's defined on both networks.

chasefleming commented 7 months ago

Yeah I agree path seems unnecessary and we don't want developer manually editing them by mistake. Let's remove that. Based on the feedback, it would be this:

"dependencies": {
        "TopShot": {
            "remoteSource": "testnet/0x0123123123.TopShot",
            "aliases": {
                "testnet": "0x0123123123",
                "mainnet": "0x0123123124"
            }
        }
    }

@bjartek I also agree a mapping would be nice, but I'm not sure we can rely on every contract having it so we'd need to have this working without that first and if there was a way to group down the road that could speed things up for a developer.

jribbink commented 7 months ago

One of the things I'm trying to wrap my head around is how to trust the remoteSource, if I single one is being used like @chasefleming suggested. In particular two points

How to guarantee that the contract deployments on each chain are the same?
If it were a pragma like @bjartek said, is this vulnerable to contract updates? What if the contract developer maliciously changes the mainnet address to another malicious address. The aliases field acts as a dependency pin against this sort of I guess.

An answer to this is possibly to check for equivalence of all network deployments of a contract when installed and relying on the aliases from this point forward instead of the pragma. This puts a burden on the external developer to continuously keep their contracts in sync - which may or may not be an issue? Curious what thoughts are here.

bjartek commented 7 months ago

If you cant trust the author of a contract you should not use it. You have to trust somebody. And since this just populates local files and flow.json you can always validate right?

bjartek commented 7 months ago

Another option besides pragma is a resource you can store in your accoung. We talked about this in the flix wg yesterday, but not sure how well it would scale. @JeffreyDoyle

nvdtf commented 7 months ago

I was thinking we only define dependencies so:

Their addresses are aliased and translated correctly on each network since we won't be deploying them again alongside our own contracts.
We can run them in the emulator locally.

On the first point, if a given contract's mainnet/testnet code differs, we aren't impacted since we rely only on translated import addresses. The checked-out code is only useful for the second point, local deployment. This means that we're grabbing contracts from a source of truth (mainnet/testnet) and will run it in the emulator. We can argue at this point if we have a contract that has a different code on mainnet/testnet it won't probably work in emulator anyway since it's relying on network addresses in its code. Based on this point, we can have a rule like "always get the mainnet code" and simplify. Let me know what you think.

bjartek commented 7 months ago

Some projects that you might want to implement might not be on mainnet yet. We need to cover that case to.

What to do with Fiat token for instance? Personally i think USDC should bust be on emulator like FlowToken.

nvdtf commented 7 months ago

We need to support contract init args before we think about FiatToken 🥲 I agree it should be on the emulator. I support adding lots of more base contracts to the emulator in favor of better DX.

bjartek commented 7 months ago

Personally i think flow init should have a flag to populate flow.json with all the aliases that are deployed when you start emulator. That makes bootstrapping way easier.

bjartek commented 7 months ago

But we are not now only talking about a dependenxcy manager for contracts. What about flix/tx/scripts to initialize an «addon».

chasefleming commented 6 months ago

As I began coding and reached the step for deploying dependencies, I realized that instead of trying to integrate dependencies into commands like flow project deploy, it's more seamless to build upon the existing flow.json patterns. In these patterns, you list dependencies that ultimately get added to contracts. This approach ensures that everything continues to work smoothly with the Flow CLI. I've modified the code I'm working on to operate as follows:

Initially, you would list dependencies in this format, which is easier to add (I will incorporate the :// suggestion from @bjartek mentioned during office hours):

{
    "contracts": {
        "HelloWorld": "cadence/contracts/HelloWorld.cdc"
    },
    "dependencies": {
        "TopShot": "testnet://877931736ee77cff.TopShot"
    },
    "networks": {
        "emulator": "127.0.0.1:3569",
        "mainnet": "access.mainnet.nodes.onflow.org:9000",
        "testnet": "access.devnet.nodes.onflow.org:9000"
    },
    "accounts": {
        "emulator-account": {
            "address": "f8d6e0586b0a20c7",
            "key": "1a42a3e48bd7f09ae61e9d39e2cf7045f5499424468dcf8d7e39f62426e238d0"
        }
    }
}

After running install, the dependencies will be in the flowkit state (shown visually here but will be under the hood) as contracts:

{
    "contracts": {
        "FungibleToken": {
            "source": "imports/9a0766d93b6608b7/FungibleToken",
            "aliases": {
                "testnet": "9a0766d93b6608b7"
            }
        },
        "HelloWorld": "cadence/contracts/HelloWorld.cdc",
        "MetadataViews": {
            "source": "imports/631e88ae7f1d7c20/MetadataViews",
            "aliases": {
                "testnet": "631e88ae7f1d7c20"
            }
        },
        "NonFungibleToken": {
            "source": "imports/631e88ae7f1d7c20/NonFungibleToken",
            "aliases": {
                "testnet": "631e88ae7f1d7c20"
            }
        },
        "TopShot": {
            "source": "imports/877931736ee77cff/TopShot",
            "aliases": {
                "testnet": "877931736ee77cff"
            }
        },
        "TopShotLocking": {
            "source": "imports/877931736ee77cff/TopShotLocking",
            "aliases": {
                "testnet": "877931736ee77cff"
            }
        }
    },
    "dependencies": {
        "FungibleToken": "testnet//:9a0766d93b6608b7.FungibleToken",
        "MetadataViews": "testnet//:631e88ae7f1d7c20.MetadataViews",
        "NonFungibleToken": "testnet//:631e88ae7f1d7c20.NonFungibleToken",
        "TopShot": "testnet//:877931736ee77cff.TopShot",
        "TopShotLocking": "testnet//:877931736ee77cff.TopShotLocking"
    },
    "networks": {
        "emulator": "127.0.0.1:3569",
        "mainnet": "access.mainnet.nodes.onflow.org:9000",
        "testnet": "access.devnet.nodes.onflow.org:9000"
    },
    "accounts": {
        "emulator-account": {
            "address": "f8d6e0586b0a20c7",
            "key": "1a42a3e48bd7f09ae61e9d39e2cf7045f5499424468dcf8d7e39f62426e238d0"
        }
    }
}

Now, you can run flow project deploy without encountering any issues.

bjartek commented 6 months ago

This makes sense to me, have you considered using either identifiers for imports or replace the . With /? So either

testnet://A.9a0766d93b6608b7.FungibleToken

Or testnet://0x9a0766d93b6608b7/FungibleToken

onflow / flow-cli

[Epic] New Contract Dependency Manager #929

Overview

Design

`flow contracts install`

`flow contracts add`

References