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

cli: Initial Oasis CLI #595

Merged
merged 1 commit into from
Jan 6, 2022
Merged

cli: Initial Oasis CLI #595

merged 1 commit into from
Jan 6, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Oct 29, 2021

No description provided.

@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #595 (c293cfc) into main (3b19fa1) will decrease coverage by 1.53%.
The diff coverage is 28.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #595      +/-   ##
==========================================
- Coverage   73.07%   71.54%   -1.54%     
==========================================
  Files         102      109       +7     
  Lines        7777     8051     +274     
==========================================
+ Hits         5683     5760      +77     
- Misses       2072     2268     +196     
- Partials       22       23       +1     
Impacted Files Coverage Δ
client-sdk/go/config/denomination.go 0.00% <0.00%> (ø)
client-sdk/go/config/network.go 0.00% <0.00%> (ø)
client-sdk/go/config/paratime.go 0.00% <0.00%> (ø)
client-sdk/go/modules/contracts/contracts.go 0.00% <0.00%> (ø)
client-sdk/go/modules/contracts/types.go 40.00% <0.00%> (-60.00%) ⬇️
client-sdk/go/types/address.go 56.96% <0.00%> (-3.04%) ⬇️
client-sdk/go/types/transaction.go 61.80% <0.00%> (-0.88%) ⬇️
client-sdk/go/helpers/denomination.go 88.46% <88.46%> (ø)
client-sdk/go/config/identifier.go 100.00% <100.00%> (ø)
client-sdk/go/helpers/address.go 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b19fa1...c293cfc. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/cli branch 4 times, most recently from 08f5bd5 to 5158988 Compare November 5, 2021 12:53
@kostko kostko requested a review from tjanez November 8, 2021 12:58
@kostko kostko force-pushed the kostko/feature/cli branch 6 times, most recently from aacb4b3 to 993d02a Compare November 15, 2021 19:03
@kostko kostko force-pushed the kostko/feature/cli branch 7 times, most recently from 92e26a0 to b80b7fd Compare November 22, 2021 18:58
@pro-wh
Copy link
Contributor

pro-wh commented Nov 22, 2021

going to rebase and merge! lmao wrong tab. definitely not

@kostko kostko force-pushed the kostko/feature/cli branch 3 times, most recently from ee4acfd to d47d139 Compare November 29, 2021 09:40
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, some minor questions.


contractsCmd = &cobra.Command{
Use: "contracts",
Short: "WebAssembly smart contracts operations",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for Cipher paratime only, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently yes, but in theory for any ParaTime that includes the contracts module.

@kostko kostko force-pushed the kostko/feature/cli branch 4 times, most recently from 3e5ff85 to d5d89e5 Compare December 10, 2021 09:09
@kostko kostko marked this pull request as ready for review December 10, 2021 09:09
@kostko kostko requested a review from pro-wh as a code owner December 10, 2021 09:09
@kostko kostko force-pushed the kostko/feature/cli branch 7 times, most recently from 44042ef to 85f1649 Compare January 5, 2022 11:13
@Yawning
Copy link
Contributor

Yawning commented Jan 5, 2022

Bikeshedding this to hell and back is great and all, but it has utility functions that I need for other work.

@kostko kostko force-pushed the kostko/feature/cli branch from 85f1649 to 3516a98 Compare January 5, 2022 12:09
@kostko kostko force-pushed the kostko/feature/cli branch from 3516a98 to ac8e394 Compare January 5, 2022 12:55
questions := []*survey.Question{
{
Name: "description",
Prompt: &survey.Input{Message: "Description:"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bit of an inconsistency. In the oasis network list we use NAME and here we ask for Description.
I'm slightly more in favor of using Name here and perhaps network_name for the variables to avoid confusion with all other Name things in this part of the code 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two things, "name" and "description". The name is an identifier used in various places to reference networks, paratimes etc. while "description" is something human readable that is also shown when signing a transaction.

}

networkRmCmd = &cobra.Command{
Use: "rm <name>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be resolved in this PR, but could the shell completion be configured to list all the networks' names when one presses Tab after typing:

oasis network rm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this should be done in a separate PR, feel free to file an issue once this is merged.

configPath := filepath.Join(configDir, configFilename)

v.AddConfigPath(configDir)
v.SetConfigType("toml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think TOML will be better for the users 🙂 ?
I'm a bit more in favor or YAML 🙂

@Yawning
Copy link
Contributor

Yawning commented Jan 5, 2022

Since I'm blocked on this, I might as well request changes too:

  • Can you move cli/client to somewhere inside the actual client SDK so that people writing tooling aren't stuck either reimplementing it or pulling it in from the CLI.
  • Likewise, can you move cli/common/demonimation to somewhere inside the actual client SDK?

@tjanez
Copy link
Member

tjanez commented Jan 5, 2022

Since I'm blocked on this, I might as well request changes too:

* Can you move cli/client to somewhere inside the actual client SDK so that people writing tooling aren't stuck either reimplementing it or pulling it in from the CLI.

* Likewise, can you move cli/common/demonimation to somewhere inside the actual client SDK?

Yeah and perhaps extract this in a separate commit and PR so it can be reviewed and merged earlier?

@Yawning
Copy link
Contributor

Yawning commented Jan 6, 2022

Also config.ResolveAddress and all the other helpers that let me go from a paratime ID + account to a transaction would make my life easier.

@kostko kostko force-pushed the kostko/feature/cli branch 2 times, most recently from e8c117a to a393d1c Compare January 6, 2022 10:55
@kostko
Copy link
Member Author

kostko commented Jan 6, 2022

Moved some of the config, connection and helpers stuff to the Go Client SDK.

@kostko kostko force-pushed the kostko/feature/cli branch from a393d1c to 871f556 Compare January 6, 2022 11:04
@kostko kostko force-pushed the kostko/feature/cli branch from 871f556 to c293cfc Compare January 6, 2022 11:31
@kostko
Copy link
Member Author

kostko commented Jan 6, 2022

I'll merge the PR and we can improve the CLI in subsequent PRs.

@kostko kostko merged commit 89850c7 into main Jan 6, 2022
@kostko kostko deleted the kostko/feature/cli branch January 6, 2022 13:23
// See https://docs.oasis.dev/general/oasis-network/network-parameters.
"mainnet": {
ChainContext: "53852332637bacb61b91b6411ab4095168ba02a50be4c3f82448438826f23898",
RPC: "grpc.oasis.dev:443",
Copy link
Contributor

@pro-wh pro-wh Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resposting as this was lost in a file move:

does the grpc-web proxy lets us dial it from the normal grpc client? or is there an additional thing on that server that handles normal grpc?

tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants