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

[Utility] Local Proof of Stake CLI - CLI and RPC client [part 1/2] - Issue #112 #177

Merged

Conversation

deblasis
Copy link
Contributor

@deblasis deblasis commented Aug 25, 2022

Description

This PR updates the CLI so that it provides access to the Utility module commands via RPC calls

Issue

Part of Issue #112 but we decided to split the work in 2 separate PRs

Type of change

Please mark the options that are relevant.

  • New feature, functionality or library

List of changes

  • Updated CLI
  • Refactored the previous implementation of the "debug" utility into it's own subcommand

Testing

  • make test_all
  • LocalNet w/ all of the steps outlined in the README

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • If applicable, I have made corresponding changes to related local or global README
  • If applicable, I have added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

@Olshansk Olshansk added core Core infrastructure - protocol related persistence Persistence specific changes priority:high labels Aug 27, 2022
@Olshansk Olshansk added client work needed to interface with the node (rpc, cli, etc..) tooling tooling to support development, testing et al and removed persistence Persistence specific changes labels Aug 27, 2022
@deblasis deblasis changed the title [WIP] [Utility] Local Proof of Stake CLI - CLI and RPC client [part 1/2] - Issue #112 [Utility] Local Proof of Stake CLI - CLI and RPC client [part 1/2] - Issue #112 Aug 29, 2022
@deblasis deblasis marked this pull request as ready for review August 29, 2022 15:05
@deblasis deblasis requested a review from a team as a code owner August 29, 2022 15:05
@deblasis deblasis mentioned this pull request Aug 29, 2022
16 tasks
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

Overall looks really good. Love Cobra - seems you took a lot from V0

I want a simple README in the CLI package laying out the CLI structure

We will also need user usage and specification documents - but that can be done in a separate issue (please open followup)

app/client/cli/actor.go Outdated Show resolved Hide resolved
app/client/cli/actor.go Outdated Show resolved Hide resolved
utility/types/message.go Outdated Show resolved Hide resolved
app/client/cli/utils.go Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member

@deblasis Please re-request the review when this is ready

@deblasis
Copy link
Contributor Author

@deblasis Please re-request the review when this is ready

I think I need an answer from @andrewnguyen22 here: #177 (comment) but apart from that it's ready I believe.

@Olshansk
Copy link
Member

Olshansk commented Nov 1, 2022

@deblasis Please re-request the review when this is ready

I think I need an answer from @andrewnguyen22 here: #177 (comment) but apart from that it's ready I believe.

https://github.com/pokt-network/pocket/pull/177/files/27e36c2e94150e8cec43e9c5caa383b2d9940a60#r1009984269

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Approving with a few minor comments, but otherwise approving because it's past the point of "good enough" and we'll iterate on main.

@deblasis I know we talked bout a Makefile or utility with examples of how to curl and such. Is that in a followup PR?

func accountCommands() []*cobra.Command {
cmds := []*cobra.Command{
{
Use: "Send <fromAddr> <to> <amount>",
Copy link
Member

Choose a reason for hiding this comment

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

I'm excited for this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

soon 🚀

if err != nil {
return err
}
// NOTE: since we don't have a keybase yet (tracked in #150), we are currently inferring the `fromAddr` from the PrivateKey supplied via the flag `--path_to_private_key_file`
Copy link
Member

Choose a reason for hiding this comment

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

Great comments.

CC @Jasonyou1995 for context

@@ -0,0 +1,315 @@
package cli
Copy link
Member

Choose a reason for hiding this comment

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

Resolving this per the TODOs left alongside cobra.ExactArgs

utility/types/message.go Outdated Show resolved Hide resolved
runtime/test_artifacts/generator.go Outdated Show resolved Hide resolved
app/client/cli/doc/CHANGELOG.md Show resolved Hide resolved
@deblasis
Copy link
Contributor Author

deblasis commented Nov 2, 2022

@Olshansk :

Approving with a few minor comments, but otherwise approving because it's past the point of "good enough" and we'll iterate on main.

@deblasis I know we talked bout a Makefile or utility with examples of how to curl and such. Is that in a followup PR?

#169 (comment)

scroll down a bit, under "demo". Obviously that's just markdown that we can package the way we want. Just let me know how and I'll do it.

deblasis and others added 3 commits November 2, 2022 21:54
@Olshansk
Copy link
Member

Olshansk commented Nov 2, 2022

@deblasis Noted that the demo is there. Will review #169 after we merge in 1/2 and 2/2 to make the conflict resolution easier.

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client work needed to interface with the node (rpc, cli, etc..) core Core infrastructure - protocol related tooling tooling to support development, testing et al
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants