Make flowkit work like a pure library and not an extention to the CLI
Instructions
Currently flowkit does two things that I do not belive that a pure library should do.
- It uses a spinner logger that does not work well if you use flowkit as the base of a damon. (I know my syslog is ugly because of it)
- It panics in some places and do not return errors.
Issue To Be Solved
Flowkit should fix the two above issues and ideally use the same logging framework as the other flow based go projects like the go-sdk uses (logrus)
Another option is to make it possible for StartProgressing to not spin.
Very good feedback, I also had in mind to do some refactoring of flowkit in the future and make it more official, some of the things are there just because how it was developed, but I strongly agree with both points and there might be even more to better define the APIs etc.
Also context should be able to be sent in if that is appropriate. I want to control when to cancel things in my code.
Also context should be able to be sent in if that is appropriate. I want to control when to cancel things in my code.
100%, it's on my to-do list to refactor API to take the context as the first argument as it is idiomatic in Go. My mistake making that in the first place when starting I wasn't as experienced in Go.
Also @bjartek I think this might be a great thread to list all the changes required to the API and then just make those in a release that would make the flowkit beta. I now see it as an alpha.
Some other ideas:
-
Accounts.CreateAPI change: refactorcontractArgs []stringargument to just take in paths as names can be extracted from the source, or change to typed contract struct that defines the name and filepath -
Blocks.GetBlockAPI change: refactorquery stringto be an enum, similar to how that was done in the Go SDK -
Blocks.GetLatestBlockHeightAPI remove: potentially remove this since it can be achieve with theGetBlockmethod - Idea: create event type which would include factory to create event types, so instead of passing values like
A.0x1.Foowe could have:
NewEvent(ForAccount(0x1), ForName("Foo"), ForNetwork(mainnet))
// or helper methods for native events
NewAccountCreatedEvent(ForNetwork(mainnet))
This is just to get the ball rolling, will continue with more soon.
Since flowkit is the bridge between flow and other go programs could we consider making some code to parse events into something that is easier for go to read? I have some code in overflow for this but it currently outputs everything in the values as strings and that is not ideal.
In general I think a session where we go through overflow and see what should be ported down and what should just stay in overflow could be useful.
For instance we have lots of helpers in overflow to create argument values from native go values.
@bjartek yes I agree, I was thinking about it in the past, I think there should even be an issue somewhere. Should we have a session during the next office hours?
Sure
Summary of Office Hours ideas:
We should break this tasks in two steps:
- First: we should address all current issues with the library
- Second: we should define the API of the beta version of the library and implement that as part of an epic
Things we should do to achieve each step (this list is WIP) First:
- Make sure to remove logging from lib or make it injectable
- Make sure to remove any unnecessary panics (validate but seems like done)
Second:
- Ideas from above
- Parsing of arguments from/to cadence being implemented in flowkit and abstracted
- Project deploys should be smarter (above)
- JSON output of events could be parsed and then possibly serialized to different formats - one format could be simplified version of the current complex Cadence JSON format
- More patterns and code in overflow should be put in flowkit (according to Bjartek so it could be reused by others more)
- We should further define "feature domains" for the SDKs as started here: https://github.com/onflow/sdks#design under feature domains, that way it would be clear what each SDK is for
- Better support contract registry, we could have support for downloading contracts during the project setup, which currently is a bit of a chore. Support for different registry would be possible with new import schema. We should keep more ideas around this in an existing issue: https://github.com/onflow/flow-cli/issues/331
I would like to have another brainstorming session soon (possibly next office hours).
cc @bjartek @psiemens @bluesign (tag any others interested - can't find their nickname =/)
The send transaction should take in the authorizers and payers as arguments as well https://github.com/onflow/flow-cli/blob/master/pkg/flowkit/services/transactions.go#L222
This should then further be refactored for the Send method to only call Build, Sign and SendSigned. The Build method should be refactored to not include the prompt.
When coding in overflow against flowkit and the emulator I end up with three different logging technologies. flowkit uses a home brewed one, Blockchain uses logrus and flow-emulator uses zerolog. It is possible to unify this? It is quite confusing having to work with so many different logging technologies.
When coding in overflow against flowkit and the emulator I end up with three different logging technologies. flowkit uses a home brewed one, Blockchain uses logrus and flow-emulator uses zerolog. It is possible to unify this? It is quite confusing having to work with so many different logging technologies.
Yep 100%.
Note to self: we should refactor prompts embedded in the flowkit out of flowkit and only pass to the method a "resolve" function which would in turn for the CLI have defined a prompt but by default if used as lib it would be nil and hence automatically continue and resolve.
These are some of the methods from overflow that I personally feel are more suited to be in flowkit:
- turn cadence.Value into compact json for printing https://github.com/bjartek/overflow/blob/api/overflow/cadence.go#L90 (see also json method above)
- https://github.com/bjartek/overflow/blob/api/overflow/argument.go: alot of the methods here to turn a go representation of an value into a cadence.Value. For overflow v3 alot of them will go away though.
- turn cadence.Value into compact json for printing https://github.com/bjartek/overflow/blob/api/overflow/cadence.go#L90 (see also json method above)
I plan to implement better support for parsing/convertin/serializing events in the Go SDK
the Deploy command to deploy contracts does not support specifying a relative path to where the flow.json is located as mentioned here https://github.com/bjartek/overflow/issues/9. This means that even if overflow uses a flow.json from another file we cannot deploy the contracts as the path to them is wrong.
Flowkit should be able to load relative paths to the config. This requires a refactor in how the config is handled. It must also provide a way to expose which config paths are loaded in an API since it's often needed.