-
Notifications
You must be signed in to change notification settings - Fork 33
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 - Issue #112 #169
[Utility] Local Proof of Stake CLI - Issue #112 #169
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr Thank you for this! Simply having this as a baseline to look at, update and edit will make the implementation 1000x easier 💯
I took a first high-level round of reviews (see screenshot since incomplete) but wanted to leave some comments.
- Let's split the server and the client into two different PRs
- Left a couple actionables throughout
- I think merging it in with all the TODOs (no keybase, etc) AFTER the merge with the major config changes is more than a sufficient first step for us to iterate on.
Makefile
Outdated
@@ -113,7 +113,7 @@ client_start: docker_check | |||
.PHONY: client_connect | |||
## Connect to the running client debugging daemon | |||
client_connect: docker_check | |||
docker exec -it client /bin/bash -c "go run app/client/*.go" | |||
docker exec -it client /bin/bash -c "go run app/client/*.go debug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actionable.
Cc @okdas on this approach to have "different CLIs" all baked into the same executable
app/client/cli/actor.go
Outdated
|
||
_ = &types.MessageStake{ | ||
PublicKey: pk.PublicKey().Bytes(), | ||
Chains: []string{}, // TODO(deblasis): 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still in flux, but we will probably either end up having something similar to v0:
Or be able to pass in a path to a config to make it easier:
Lets leave as is for now and cc @andrewnguyen22 for context
app/client/cli/debug.go
Outdated
@@ -0,0 +1,144 @@ | |||
package cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actionable: Big fan of how this is in a separate file now.
We can do something similar for infra
soon. CC @okdas @vlad-she
app/client/cli/gov.go
Outdated
} | ||
|
||
_ = &types.MessageChangeParameter{ | ||
Signer: []byte{}, // TODO(deblasis): 👀 same as owner? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a TODO(andrew)
in message.proto
to document the difference?
For now, you can use the same ones.
The owner
is the owner of the funds staked (where the rewards go) and the signer
is the operator. Most of the time, if you run your own node, it's the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's an AI for Andrew, your explanation confirms my hunch and it makes total sense.
These "TODO 👀" are a reminder for me to dig deeper before raising questions (for which comments like yours make my life easier tho). Normally I wouldn't share but since I felt this PR could be a bit of a blocker, I wanted to share as much as possible so that you guys can figure out where we are at with this, including my doubts hehe.
One thing I figured is that I think I have to construct these messages server side and not on the client for various reasons:
- implementation details leaks + tight coupling
- on the server side (node) we know the signer address so I can populate this value correctly I guess
…ke_cli_CLI' into issue/utility_local_proof_of_stake_cli Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
…ke_cli_RPC' into issue/utility_local_proof_of_stake_cli Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
…utility_local_proof_of_stake_cli
…utility_local_proof_of_stake_cli
…utility_local_proof_of_stake_cli
for CLI/client use
calling updated QueryRPC to point at route from map
for CLI/client use
…utility_local_proof_of_stake_cli
Done! Since the branches were created off my private repo, I had to create a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments bust mostly ready.
rpc/doc/README.md
Outdated
|
||
This document is meant to be a starting point/placeholder for a full-fledged RPC specification that allows interaction with the nodes. | ||
|
||
## Contents <!-- omit in toc --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it nice how it immediately helps you figure out what you added in the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, nice one
app/client/cli/consensus.go
Outdated
func consensusCommands() []*cobra.Command { | ||
cmds := []*cobra.Command{ | ||
{ | ||
Use: "RoundState", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inspired by cosmos ;)
@@ -46,8 +46,9 @@ services: | |||
# Needed for DLV debugging | |||
security_opt: | |||
- "seccomp:unconfined" | |||
environment: | |||
- POCKET_RPC_USE_CORS=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were there issues with CORS disabled in this commit? Just curious to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, by default I left CORS disabled (I'd rather have it opt-in). This environment variable simply enables it.
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
|
||
### Synopsis | ||
|
||
Stake the Application actor into the network, making it available for service. | ||
Stake the node into the network, making it available for service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated 👍
…_proof_of_stake_cli_RPC Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
…_proof_of_stake_cli_RPC
…utility_local_proof_of_stake_cli Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
…utility_local_proof_of_stake_cli
…_proof_of_stake_cli
## Description This PR aims at introducing CLI commands relative to the Utility module: - Send - Stake - EditStake - Unstake - Unpause - ChangeParameter Consequently, it introduces an RPC server (HTTP) and the ability to "point" the CLI at specific nodes via flags Fixes [issue/112](#112) ## Type of change Please mark the options that are relevant. - [x] New feature (non-breaking change which adds functionality) ## How Has This Been Tested? - [x] `make test_all` - [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have tested my changes using the available tooling - [x] If applicable, I have made corresponding changes to related or global README - [x] If applicable, I have added added new diagrams using [mermaid.js](https://mermaid-js.github.io) - [x] If applicable, I have added tests that prove my fix is effective or that my feature works * fix(config.proto): updated timeout type * feat(RPC): basic RPC server with naive sync TX broadcast * fix(RPC): fixed HTTP method for Health and Version routes * chore(go.mod): tidy * style(RPC): format * chore(go.mod): tidy + vendor * feat(Utility): CLI RPC client function * feat(Utility): RPC defaults const * feat(Utility): GetSignBytes * feat(Utility): CLI utils * feat(Utility): stake cmd scaffolding * refactor(Utility): RPC server RoutesMap for CLI/client use * refactor(Utility): RPC server exporting RouteKey and RoutesMap for CLI/client use * feat(Utility): CLI calling updated QueryRPC to point at route from map * refactor(Utility): RPC server exporting RouteKey and RoutesMap for CLI/client use * chore(Utility): Removed TODO * fix(Utility): CLI: making use of the pwd flag * fix(Utility): CLI: code review feedback * fix(Utility): CLI custodial stake command: OutputAddr = fromAddr * refactor(Utility): CLI: refactor command bindings Also added missing functionality in commands other than Stake * fix(Utility): Fix route after merge * refactor(Utility): CLI: named return values fix * test(Utility): CLI: simplified tests for PK parsing from file * feat(Utility): RPC OpenApi spec and code generation * refactor(Utility): RPC server refactoring using code generation RPC server refactoring with code generation from openapi spec * feat(Utility): RPC OpenApi spec and code generation * feat(Utility): Updated RPC spec * feat(Utility): Regenerated RPC boilerplate and updates * docs(Utility): RPC and node docs barebones + RPC spec notes * feat(Utility): Updated RPC spec * docs(Utility): CLI docs barebones * fix(Utility): CLI: added missing message handling for account * fix(Shared): fixed RPC config * feat(Utility): CLI updated to use the generated RPC client * chore(go.mod): tidy * refactor(Utility): CLI + RPC models in server.gen.go * style(Utility): removed redundant struct definition * docs(README.md): fixed LICENSE reference * docs(README.md): updated with references to CLI, RPC and Node docs * refactor(Shared): RPC config defaults -changing soon,I'm centralizin * refactor(Shared): RPC config defaults -changing soon,I'm centralizin Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * fix(Utility): RPC: updated to use test_artifacts defaults * refactor(Shared): RPC config defaults -changing soon,I'm centralizin * feat(Utility): CLI: updated to use test_artifacts default * docs(Utility): RPC and node basic docs * docs(Utility): CLI: Changelog * fix(Utility): CLI: fixed output to user. It shouldn't be logging * fix(Utility): CLI code review feedback * style(Utility): code review feedback * feat(Tooling): Updated makefile commands to better handle codegen * feat(Tooling): Updated makefile commands to better handle codegen Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * fix(Utility): Fix duplicated models * chore(Utility): fixed versioning schema * feat(Utility): CLI documentation generator + first stab at docs * fix(Utility): CLI specific fixes * fix(Utility): types fixes * fix(Utility): RPC configs post-merge * feat(Consensus): configOptions * feat(P2P): configOptions * feat(Utility): CLI: using configOptions to inject PK * fix(Utility): RPC configs post-merge * feat(Consensus): configOptions * feat(P2P): configOptions * fix(Utility): RPC fix post merge * fix(Utility): RPC disabled by default because of TECHDEBT * chore(go.mod): tidy * fix(go.mod): tidy * fix(CLI): test_artifacts * fix(go.mod): tidy * fix(CLI): updated to use new typesUtil.ActorType * fix(CLI): runtime based init * fix(test_artifacts): fix * fix(CLI): runtime using WithRandomPK() * fix(CLI): fixed client-only initialization * fix(Makefile): protogen first * feat(Utility): GetActorName function (can we autogenerate these?) * fix(RPC): test_artifacts in runtime * fix(go.mod): tidy * fix(CLI): debug commands are now feature flagged * chore(go.mod): tidy * chore(CLI): git rm app/pocket/rpc/client.gen.go * chore(CLI): s/j/tx and s/prepareTx/prepareTxJson * refactor(shared): converters * fix(CLI): debug nits * refactor(CLI): confirmation and credentials * chore(CLI): removed HACK todo * fix(Makefile): premature protoc * fix(RPC): added imports used in codegen files * docs(RPC): added swagger editor link * fix(RPC): added imports used in codegen files * feat(RPC): config proto * feat(RPC): RPCModule and noopRpcModule * refactor(Shared): shared.Create -> shared.CreateNode * refactor(RPC): moved scaffolding into rpc module * fix(RPC): removed redundant file * fix(CLI): debug merge fix * docs(RPC): fixed link after refactoring * docs(RPC): updated code organization post refactoring * Update app/client/cli/utils.go Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * fix(RPC): gitignoring generated files * style(Persistence): reverting space change * refactor(Defaults): runtime/defaults package * chore(CLI): issue handle * chore(CLI): Issue #310 links * refactor(CLI): preallocation fix * style(CLI): oneMillion * style(CLI): s/RelayChainIDs/relayChainIDs * feat(Utility): ActorType.GetName() * chore(Utility): tracking issue #142 * chore(Utility): GetBytes -> GetCanonicalBytes * chore(CLI): actorCmd -> accountCmd * docs(CLI): fromAddr note (address currently sourced from pk) * chore(Runtime): new default value from main * refactor(CLI): moving utility functions in utils.go * chore(CLI): NewDebug -> NewDebugCommand * fix(RPC): gitignoring generated files Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * refactor(Consensus): mocks with Return(nil) and not Do(...) * docs(RPC): updated changelog versions * fix(Makefile): generate_rpc_openapi * fix(Makefile): fix for git clone + make develop_test * Update rpc/v1/openapi.yaml Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Update rpc/doc/CHANGELOG.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Update rpc/noop_module.go Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * chore(Shared): added TODOes for ValidateXXX() in modules * docs(RPC): broadcast_tx_sync summary updated * Update rpc/doc/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Update rpc/doc/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Update rpc/doc/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * fix(Makefile): gitignoring generated files breaks tests fix * docs(CLI): added todos for exactArgs * chore(RPC): added types.go * refactor(RPC): sourcing defaults from defaults not test_artifacts * feat(CLI): system commands RPC🤝CLI * feat(Consensus): CurrentRound() and CurrentStep() * feat(RPC): /v1/consensus/round_state * feat(RPC): /v1/consensus/round_state handler * feat(RPC): /v1/consensus/round_state * refactor(CLI): system command typoes copypastas * feat(CLI): consensus commands -RoundState and individual ones * chore(CLI): typo * docs(CLI): short commands descriptions * docs(Config): Adding some more color around configuration * fix(CLI): tx message signing * feat(CLI): reporting statuscode and body * fix(Proto): deterministic * refactor(CLI): prepareTxJSON -> prepareTxBytes * docs(Docs): Adding some more color config + raw_hex_bytes * refactor(CLI): Stake command * fix(CLI): tx message signing * feat(CLI): reporting statuscode and body * Merge branch 'issue/utility_local_proof_of_stake_cli_CLI' into issue/utility_local_proof_of_stake_cli * feat(Tooling): swagger-ui * feat(RPC): cors feature flag * Update utility/types/message.go Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * chore(Runtime): comment spacing * docs(CLI): Changelog * docs(CLI): changelog * docs(RPC): changelog * chore(Runtime): comment spacing * Update runtime/docs/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * chore(Consensus): SetBus mock Do(func(modules.Bus) {}) -> Return() * refactor(Consensus): mocks with Return(nil) and not Do(...) Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * chore(Consensus): SetBus mock Do(func(modules.Bus) {}) -> Return() Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * docs(Pocket): changelog * Update app/pocket/doc/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * Update app/pocket/doc/README.md Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * docs(RPC): changelog * docs(RPC): docs TOC * docs(RPC): Transports -> Endpoints * feat(Tooling): swagger-ui Signed-off-by: Alessandro De Blasis <alex@deblasis.net> * docs(RPC): swagger ui link to editor and ref to make cmd * feat(rpcServer): rpcServer is now IntegratableModule * chore(Shared): tracking TODO (implement validations) #334 * fix(RPC): merge fix * chore(go.mod): tidy * chore(go.mod): tidy * feat(Tooling): added empty mock_module package for cold start * docs(RPC): nit * Update app/client/cli/consensus.go Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> * docs(CLI): better commands descriptions * feat(CLI): boldText helper * style(CLI): nit: real estate * refactor(CLI): ConsensusState * docs(CLI): updated docs * docs(CLI): \n at the end of sentences in Stake command desc. * docs(CLI): regenerated docs * fix(RPC): int64 on RoundState fields * refactor(Shared): unexporting XXModuleName * feat(node): single source of truth for version + overridable Signed-off-by: Alessandro De Blasis <alex@deblasis.net> Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Description
This PR aims at introducing CLI commands relative to the Utility module:
Consequently, it introduces an RPC server (HTTP) and the ability to "point" the CLI at specific nodes via flags
Fixes issue/112
Type of change
Please mark the options that are relevant.
How Has This Been Tested?
make test_all
Checklist
Demo
In order to test the features introduced, we need to start LocalNet
in a separate console window, we can execute the newly introduced command
navigating to http://localhost:8080 we'll access a dockerized
swagger-ui
environment with injected our V1 RPC spec under development.Please note that the openapi spec generates the RPC scaffolding and also the web client that's used by the CLI internally (new feature as well)
in here we can easily invoke the methods since we enabled CORS, that is disabled by default for security reasons. We can override this via a feature flag via environment variable, another feature added with this PR
Now we can simply test our RPC layer:
In the meantime the node is logging the corresponding requests:
How about the CLI?
Since we haven't -yet- packaged it into a binary we can run it from the source-code but soon we'll simply replace the
go run
with apokt-client
orpocketcli
or any name @Olshansk can come up with given his track record :)go run app/client/*.go
This could look daunting, but our users are generally speaking quite tech savvy and used to this popular format (offered by the library
cobra
).Documentation is also generated in markdown starting having the CLI help as a source. (new feature) app/client/cli/doc/README.md
Now to the commands, there are some simple ones you might have spotted from the RPC:
We can obviously target a specific host as well
The tooling makes it easy to add new commands starting from the spec, these ones were implemented in few minutes:
I was inspired by the recent chain halt in V0
The 🥩ier stuff is about stake, unstake and these other commands.
As required in the initial issue, this PR limits itself to submitting the respective payload and make sure that signing and verification server side works properly.
Since we don't have a keybase yet, we need to supply a private key json that by default resides in the root folder (where you run
make
commands basically).A test
pk.json
could be:(note the double quotes... it's JSON! :) )
if we test a command
(please forgive the non-realistic arguments)
The CLI packages the message, signs it using the private key and sends it to the RPC, which calls
CheckTransaction
and if there are no errors we return HTTP 200.Now, if we try for a second time any other command that interacts with the Utility context we get this
But that's something unrelated that we'll fix separately.