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

[Epic] New Contract Dependency Manager #929

Open
3 of 5 tasks
nvdtf opened this issue Mar 24, 2023 · 20 comments
Open
3 of 5 tasks

[Epic] New Contract Dependency Manager #929

nvdtf opened this issue Mar 24, 2023 · 20 comments
Assignees
Labels
Feature A new user feature or a new package API

Comments

@nvdtf
Copy link
Member

nvdtf commented Mar 24, 2023

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

Preview Give feedback
  1. Feature
    chasefleming nvdtf
  2. Feature Feedback L3
    chasefleming nvdtf
  3. Feature
    chasefleming nvdtf
  4. Feature
    chasefleming nvdtf
  5. Feature
    chasefleming nvdtf

References

@nvdtf nvdtf added Feature A new user feature or a new package API Feedback labels Mar 24, 2023
@nvdtf nvdtf self-assigned this Mar 24, 2023
@nvdtf nvdtf removed the Feedback label Mar 24, 2023
@bjartek
Copy link
Collaborator

bjartek commented Mar 30, 2023

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
Copy link

jrkhan commented Mar 30, 2023

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
Copy link

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
Copy link
Collaborator

bluesign commented Mar 30, 2023

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
Copy link
Member Author

nvdtf commented Apr 5, 2023

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: #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.

@devbugging devbugging moved this to 🔖 Ready for Pickup in 🌊 Flow 4D Apr 11, 2023
@chasefleming chasefleming self-assigned this Oct 3, 2023
@btspoony
Copy link

@chasefleming
Copy link
Member

chasefleming commented Nov 29, 2023

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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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
Copy link
Member Author

nvdtf commented Nov 29, 2023

@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
Copy link
Member

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
Copy link
Contributor

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

  1. How to guarantee that the contract deployments on each chain are the same?
  2. 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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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
Copy link
Member Author

nvdtf commented Nov 29, 2023

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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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
Copy link
Member Author

nvdtf commented Nov 29, 2023

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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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
Copy link
Collaborator

bjartek commented Nov 29, 2023

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

@chasefleming
Copy link
Member

chasefleming commented Dec 13, 2023

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
Copy link
Collaborator

bjartek commented Dec 14, 2023

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

@gregsantos gregsantos moved this from 🔖 Ready for Pickup to 🏗 In progress in 🌊 Flow 4D Jan 8, 2024
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
Status: EPICs In Progress
Development

No branches or pull requests

7 participants