onflow / fcl-js

FCL (Flow Client Library) - The best tool for building JavaScript (browser & NodeJS) applications on Flow 🌊
https://onflow.org
Apache License 2.0
323 stars 117 forks source link

Use flow.json with confg #1449

Closed srinjoyc closed 1 year ago

srinjoyc commented 2 years ago

Instructions

Issue To Be Solved

As a developer new to Flow, I want to be able to:

Solution - TBD

Freshmint is using webpack and onflow/cadut to achieve similar functionality: https://github.com/packagelabs/freshmint/tree/alpha/packages/cadence-loader

We would like to do something similar.

Requirements

chasefleming commented 2 years ago

Feels like this is two separate items:

  1. What FCL can handle in terms of assisting in the address import replacement from flow.json. Right now, file path replacements are being handled in multiple different ways. Always making more work on the developer.
  2. Build related features: watching for changes, cadence imports, and outputting new contracts with replaced imports.

I'd make the case that point two should be left to build tools like Vite and Webpack and should not be the responsibility of FCL. This would overcomplicate the issue. Watching already exists there, as well as loaders for different file types.

I believe Flow CLI already handles the address replacements when deploying contracts to different networks, so that problem should be solved. The problem really arises in imports for scripts and transactions since those are not handled by the Flow CLI and still need to be replaced.

We don't want to replace file paths with addresses in actual files because that requires outputting of new files and we are back to a build tool.

Proposed Solution

A more ideal solution would be to make something build/framework agnostic that works with FCL's existing config address placeholders and flow.json.

Say for example, your flow.json contains the following:

{
  "contracts": {
    "NonFungibleToken": {
      "source": "./cadence/contracts/NonFungibleToken.cdc",
      "aliases": {
        "emulator": "0xf8d6e0586b0a20c7",
        "testnet": "0x631e88ae7f1d7c20"
      }
    }
  }
}

And let's say our transaction looks like this:

import NonFungibleToken from "../../contracts/NonFungibleToken.cdc"

transaction() {
  prepare() {}
}

We have everything here we need in order to do the replacements within FCL, we just don't have a system for handling flow.json yet.

Instead of file replacements, let's use the placeholder system and load flow.json into FCL with something like this:

import flowJson from "./flow.json"
import * as fcl from "@onflow/fcl"

fcl.config().load(flowJson)

Under the hood, this will create placeholders based on the contract name. For example:

{ "0xNonFungibleToken.testnet": "0x631e88ae7f1d7c20" }

Then inside the resolveCadence step where we normally handle address replacement we'll do a more advanced detection of file paths and replace them with the appropriate address based on the "flow.network" config value.

We should require files be named the same as their contract name in order to avoid having to read the file system and determine/match path.

End result is that a developer can leave their contracts with file imports, it requires no running of a compile command, and they can continue to use their build tools easily.

sideninja commented 1 year ago

@chasefleming cc @srinjoyc I feel you are right about the build tool. That should be leveraged with existing technology. About the address replacement I like your approach but have some suggestion basing that on the problems I’ve described in the FLIP ___. The problem with keeping the “import Foo from 0xFoo” syntax is that this transaction and script files then are published to GitHub and any developers that want to use those same transactions or scripts with CLI will have to change the import syntax to a file referencing syntax (that’s already being done by community and also complained about). My take is that we should unify the format in the first place. So what I’m suggesting is to drop the “import Foo from 0xFoo” syntax but only keep “import Foo” (I’m working on final ideas about this format with Bastian and Pete but this is the current idea, however even if this changes it shouldn’t affect your implementation too much as that part just changes the implementation of the function detecting the imports and replacing them with addresses - I suggest even having multiple implementations of that function one for backward compatibility and then the second for this new format). That way all contracts no matter if used by CLI or FCL will function as they will share the syntax. Leveraging the flow.json to define where the contract resolves is also a good idea so we can when we publish those contracts on a repo (which often is not shared with the app code) tell the cloners of repo what the 0xFoo actually means and resolves to, (think of it as package.json explaining what the import actually maps to). This flexibility in the import schema also allows us to future-proof it, so I can imagine having “package managers” for Cadence code, and this format would allow us to specify in flow.json what the actual source is (which in case of package managers could be package manager repository of all the packages). Now to touch a bit on the flow.json as I know that was previously a bit of a problem for FCL. I would say ENV vars could be used if that’s wished from FCL implementation perspective, and then all I need to do is add a CLI comand that knows how to export the ENVs from flow.json, that way you are a bit more agnostic to concrete schema of flow.json and you can leverage build systems relaying on the ENVs in the first place. This is totally up to all of you to decide, just as an idea. The part I feel strongly about is the import schema unification.

psiemens commented 1 year ago

I'd make the case that point two should be left to build tools like Vite and Webpack and should not be the responsibility of FCL. This would overcomplicate the issue. Watching already exists there, as well as loaders for different file types.

@chasefleming I agree with this. No need to reinvent because these tools exist and every developer has a preferred bundler.

Instead of file replacements, let's use the placeholder system and load flow.json into FCL

I don't think we should encourage developers to import flow.json inside their apps. My main concern with loading flow.json in runtime code is that it has the potential to put private keys into a bundled app without a developer realizing it. This is partially the fault of flow.json itself -- I think we may want to find a way to allow contract configuration to live separately from key configuration, but that's a separate issue.

I really like @sideninja's thoughts regarding a new import convention for Cadence files that relies on identifiers rather than file paths. Identifiers are easy to replace in any app. I think that helps here.

The qualities I want to preserve are:

chasefleming commented 1 year ago

I don't think we should encourage developers to import flow.json inside their apps. My main concern with loading flow.json in runtime code is that it has the potential to put private keys into a bundled app without a developer realizing it. This is partially the fault of flow.json itself -- I think we may want to find a way to allow contract configuration to live separately from key configuration, but that's a separate issue.

Completely agree, the private keys are an issue. Was thinking we could enforce/recommend keys being broken into a separate json file which we don't ingest and error if we see them, but there is still some friction there for a developer having to have two json files. I don't love it.

Re Cadence file imports it feels like we should actually get away from this. Since we can't read the file system with FCL the only thing we can really do is say if you file name is the same as your contract name we could replace the import. Still don't love it. File paths are sort of meaningless and you're right can be imported with your bundler so we may just want to let outside libraries handle these in various build plugins.

I also don't think that a developer should have to add a bunch of env vars. I personally don't even love that you have to manually set placeholders right now. It's an error prone, manual process.

Proposal: What if we could marry these ideas with the identifiers, ingestion of flow.json, and removal of private keys? @psiemens @sideninja Is there a way CLI could do the following:

Then in FCL, it would:

Benefits of this would be:

sideninja commented 1 year ago

I only feel very strongly about one problem, and that is the import schema format. I wrote about it in the FLIP if you want to get into details, but in short, FCL has to stop (ofc with deprecation period) consuming code that has imports like import Foo from 0xFoo. There are numerous downsides to that ranging from: it's invalid Cadence syntax so syntax highlining is broken, it's a unique pattern to only FCL (so when used by CLI and other tools it doesn't work) etc etc. What we are currently proposing is to use a new import schema that will look like:

import Foo
import Bar from Chase

The first import is a default import for referencing a default service account, the second you can also provide the name of the account if you like. Those are defined in flow.json as well as the source where you can get the contract source code. In any case, I suggest you refactor (if needed) the part that knows how to parse these imports and then replace it with a specific address to be flexible enough that any modification to the proposal above won't be hard to implement. Any other things like possibly having a command to export non-sensitive data etc I'm pretty flexible, but it's also worth noting that the credentials shouldn't be part of flow.json anyway and we are thinking how to further improve and prevent that pattern. My opinion on whole getting the Cadence files (but I don't have much context so would like to chat more about it) is that it should probably just be part of the build tool. But maybe lets talk about that in a call.

bluesign commented 1 year ago

I think ingesting flow.json to fcl is a bad idea. I think code generation via flow-cli can be much better. flow-cli can export js module file for cadence files which can be imported into the project..

sideninja commented 1 year ago

@bluesign not sure if CLI is the place for that, because I don't want it to become a build tool for all possible languages, but I feel the notion of leaving that to be solved by existing built tools for languages would be better.

bluesign commented 1 year ago

@sideninja that's valid point. Main problem I see here is flow.json moving somewhere as config file ( which limits flow.json content in the future unnecessarily in my opinion ) flow.json should stay as a project file in my opinion.

Generating js code for cadence folder ( scripts , transactions etc ) very easy thing, but also has added benefit of IDE integration such as code completion etc, very easy to add to build pipeline too.

Maybe better I push an example somewhere to explain what I mean more clearly.

sideninja commented 1 year ago

Flow.json always has to stay as a project file!! I'm working on a thing that will do a lot of things you said. Let me share a demo with you.

bluesign commented 1 year ago

ok wrote a poc generator in python( which is not the optimum but to shows what I mean )

For kitty items generates this: ( using flow json and cadence folder ) https://gist.github.com/bluesign/fef93cdfdf4a01df0beedb98b8ae46b1

which has great IDE integration and auto complete and it is just a single js file. ( also no need to import cdc files )

s3 s4
sideninja commented 1 year ago

The gist was auto-generated? @bluesign

bluesign commented 1 year ago

@sideninja yes, but just a quick poc, it can be improved much.

sideninja commented 1 year ago

@bluesign I really like this. I feel this would be a great way to avoid doing a bunch of monkey code and also decoupling the tools. Also FCL and other libs not include logic to do replacement of addresses etc, at that point we would need different tools for different langs but getting ahead of myself.

I feel that this approach doesn't have to block the PR tho, because we can add this as we go and replace at that point.

bjartek commented 1 year ago

@bluesign not sure if CLI is the place for that, because I don't want it to become a build tool for all possible languages, but I feel the notion of leaving that to be solved by existing built tools for languages would be better.

Overflow has this. We use it in .find

bjartek commented 1 year ago

I firmly belive that a cadence/blockchain dev should not have to be a frontend dev. And the other way around.

Cadence code should be tested and verified long before it hits the ui.

Many devs currently hold both hats but encouraging patterns where that is always the case is not optimal in my view.

sideninja commented 1 year ago

@bjartek I agree. I feel Cadence should be written first in isolation and only then integrated into other parts. Similar to how you develop the backend in isolation from the frontend. We were supporting this as a good practice, saying your project should start with Cadence. But once the integration is needed it's good to have tools in place that helps you do that. So I don't think this tooling goes against that, but I can see it could encourage doing both things at the same time.