[Client] [Tooling] refactor: CLI (#806)

## Description

<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 13 Jun 23 12:53 UTC
This pull request includes various updates across the codebase,
including changes to CLI flags, import statements, and default values.
It also involves refactoring code to use shared helper functions,
removing unused code, and improving error handling. Some variables have
been renamed to be more descriptive. The specific changes include
modifications to files such as `validator.go`, `cluster-manager.yaml`,
`cli-client.yaml`, `utils.go`, and `runtime_defaults.go`. For example,
the `RPC_HOST` variable has been replaced with `POCKET_REMOTE_CLI_URL`.
A new `helper` package has been added, and a new `flags` package
contains several CLI flags. Code has also been updated to use
`flags.RemoteCLIURL` instead of `remoteCLIURL`. Some constants have been
renamed to include `Hostname`, and there are updates to endpoint
hostnames for some validators.
<!-- reviewpad:summarize:end -->

## Issue

Dependants:
- #730

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Exported `rootCmd` to support cross-package usage (i.e. subcommands w/
own pkgs)
- Refactored common CLI code
  - Moved & refactored `PersistentPreRunE()` helper
  - Moved & exported `busCLICtxKey`
  - Exported `GetValueFromCLIContext()` and `SetValueInCLIContext()`
  - Moved `setupAndStartP2PModule`
  - Moved `setupCurrentHeightProvider`
  - Moved `setupPeerstoreProvider`
- Refactored CLI flags to own package for cross-package use
- Replaced RPC_HOST with POCKET_REMOTE_CLI_URL where appropriate
- Added `remote-cli-url` flag to cluster manager & refactor
- Added support for overriding of `remote-cli-url` flag with env var
- Append "Hostname" to validator endpoint hostname constants
- Promoted string literal to `RandomValidatorEndpointK8SHostname`
constant
- Improved e2e tests error messaging
- Simplify e2e debug command calls
- Improve error handling in client CLI


## Testing

- [ ] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [x] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made


## Required Checklist

- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)

Makefile

@@ -155,7 +155,7 @@ rebuild_client_start: docker_check ## Rebuild and run a client daemon which is o
 
 .PHONY: client_connect
 client_connect: docker_check ## Connect to the running client debugging daemon
-	docker exec -it client /bin/bash -c "POCKET_P2P_IS_CLIENT_ONLY=true go run -tags=debug app/client/*.go debug"
+	docker exec -it client /bin/bash -c "POCKET_P2P_IS_CLIENT_ONLY=true go run -tags=debug app/client/*.go debug --remote_cli_url=http://validator1:50832"
 
 .PHONY: build_and_watch
 build_and_watch: ## Continous build Pocket's main entrypoint as files change
@@ -510,7 +510,7 @@ localnet_up: ## Starts up a k8s LocalNet with all necessary dependencies (tl;dr
 
 .PHONY: localnet_client_debug
 localnet_client_debug: ## Opens a `client debug` cli to interact with blockchain (e.g. change pacemaker mode, reset to genesis, etc). Though the node binary updates automatiacally on every code change (i.e. hot reloads), if client is already open you need to re-run this command to execute freshly compiled binary.
-	kubectl exec -it deploy/dev-cli-client --container pocket -- p1 debug
+	kubectl exec -it deploy/dev-cli-client --container pocket -- p1 debug --remote_cli_url http://pocket-validators:50832
 
 .PHONY: localnet_shell
 localnet_shell: ## Opens a shell in the pod that has the `client` cli available. The binary updates automatically whenever the code changes (i.e. hot reloads).

app/client/cli/account.go

@@ -3,9 +3,11 @@ package cli
 import (
 	"fmt"
 
+	"github.com/spf13/cobra"
+
+	"github.com/pokt-network/pocket/app/client/cli/flags"
 	"github.com/pokt-network/pocket/shared/crypto"
 	"github.com/pokt-network/pocket/utility/types"
-	"github.com/spf13/cobra"
 )
 
 func init() {
@@ -48,7 +50,7 @@ func accountCommands() []*cobra.Command {
 					return err
 				}
 
-				if !nonInteractive {
+				if !flags.NonInteractive {
 					pwd = readPassphrase(pwd)
 				}
 

app/client/cli/actor.go

@@ -6,10 +6,12 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/spf13/cobra"
+
+	"github.com/pokt-network/pocket/app/client/cli/flags"
 	coreTypes "github.com/pokt-network/pocket/shared/core/types"
 	"github.com/pokt-network/pocket/shared/crypto"
 	typesUtil "github.com/pokt-network/pocket/utility/types"
-	"github.com/spf13/cobra"
 )
 
 func init() {
@@ -100,7 +102,7 @@ If no changes are desired for the parameter, just enter the current param value
 				return err
 			}
 
-			if !nonInteractive {
+			if !flags.NonInteractive {
 				pwd = readPassphrase(pwd)
 			}
 
@@ -169,7 +171,7 @@ func newEditStakeCmd(cmdDef actorCmdDef) *cobra.Command {
 				return err
 			}
 
-			if !nonInteractive {
+			if !flags.NonInteractive {
 				pwd = readPassphrase(pwd)
 			}
 
@@ -233,7 +235,7 @@ func newUnstakeCmd(cmdDef actorCmdDef) *cobra.Command {
 				return err
 			}
 
-			if !nonInteractive {
+			if !flags.NonInteractive {
 				pwd = readPassphrase(pwd)
 			}
 			pk, err := kb.GetPrivKey(fromAddrHex, pwd)
@@ -284,7 +286,7 @@ func newUnpauseCmd(cmdDef actorCmdDef) *cobra.Command {
 				return err
 			}
 
-			if !nonInteractive {
+			if !flags.NonInteractive {
 				pwd = readPassphrase(pwd)
 			}
 

app/client/cli/cmd.go

@@ -2,40 +2,42 @@ package cli
 
 import (
 	"context"
+	"log"
 
-	"github.com/pokt-network/pocket/runtime/configs"
-	"github.com/pokt-network/pocket/runtime/defaults"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
+
+	"github.com/pokt-network/pocket/app/client/cli/flags"
+	"github.com/pokt-network/pocket/runtime/configs"
+	"github.com/pokt-network/pocket/runtime/defaults"
 )
 
 const (
 	cliExecutableName = "p1"
+	flagBindErrFormat = "could not bind flag %q: %v"
 )
 
-var (
-	remoteCLIURL   string
-	dataDir        string
-	configPath     string
-	nonInteractive bool
-	verbose        bool
-	cfg            *configs.Config
-)
+var cfg *configs.Config
 
 func init() {
-	rootCmd.PersistentFlags().StringVar(&remoteCLIURL, "remote_cli_url", defaults.DefaultRemoteCLIURL, "takes a remote endpoint in the form of <protocol>://<host> (uses RPC Port)")
-	rootCmd.PersistentFlags().BoolVar(&nonInteractive, "non_interactive", false, "if true skips the interactive prompts wherever possible (useful for scripting & automation)")
+	rootCmd.PersistentFlags().StringVar(&flags.RemoteCLIURL, "remote_cli_url", defaults.DefaultRemoteCLIURL, "takes a remote endpoint in the form of <protocol>://<host>:<port> (uses RPC Port)")
+	// ensure that this flag can be overridden by the respective viper-conventional environment variable (i.e. `POCKET_REMOTE_CLI_URL`)
+	if err := viper.BindPFlag("remote_cli_url", rootCmd.PersistentFlags().Lookup("remote_cli_url")); err != nil {
+		log.Fatalf(flagBindErrFormat, "remote_cli_url", err)
+	}
+
+	rootCmd.PersistentFlags().BoolVar(&flags.NonInteractive, "non_interactive", false, "if true skips the interactive prompts wherever possible (useful for scripting & automation)")
 
 	// TECHDEBT: Why do we have a data dir when we have a config path if the data dir is only storing keys?
-	rootCmd.PersistentFlags().StringVar(&dataDir, "data_dir", defaults.DefaultRootDirectory, "Path to store pocket related data (keybase etc.)")
-	rootCmd.PersistentFlags().StringVar(&configPath, "config", "", "Path to config")
+	rootCmd.PersistentFlags().StringVar(&flags.DataDir, "data_dir", defaults.DefaultRootDirectory, "Path to store pocket related data (keybase etc.)")
+	rootCmd.PersistentFlags().StringVar(&flags.ConfigPath, "config", "", "Path to config")
 	if err := viper.BindPFlag("root_directory", rootCmd.PersistentFlags().Lookup("data_dir")); err != nil {
-		panic(err)
+		log.Fatalf(flagBindErrFormat, "data_dir", err)
 	}
 
-	rootCmd.PersistentFlags().BoolVar(&verbose, "verbose", false, "Show verbose output")
+	rootCmd.PersistentFlags().BoolVar(&flags.Verbose, "verbose", false, "Show verbose output")
 	if err := viper.BindPFlag("verbose", rootCmd.PersistentFlags().Lookup("verbose")); err != nil {
-		panic(err)
+		log.Fatalf(flagBindErrFormat, "verbose", err)
 	}
 }
 
@@ -45,7 +47,10 @@ var rootCmd = &cobra.Command{
 	Long:  "The CLI is meant to be an user but also a machine friendly way for interacting with Pocket Network.",
 	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 		// by this time, the config path should be set
-		cfg = configs.ParseConfig(configPath)
+		cfg = configs.ParseConfig(flags.ConfigPath)
+
+		// set final `remote_cli_url` value; order of precedence: flag > env var > config > default
+		flags.RemoteCLIURL = viper.GetString("remote_cli_url")
 		return nil
 	},
 }

app/client/cli/consensus.go

@@ -2,9 +2,10 @@ package cli
 
 import (
 	"fmt"
+	"github.com/spf13/cobra"
 
+	"github.com/pokt-network/pocket/app/client/cli/flags"
 	"github.com/pokt-network/pocket/rpc"
-	"github.com/spf13/cobra"
 )
 
 func init() {
@@ -96,7 +97,7 @@ func consensusCommands() []*cobra.Command {
 }
 
 func getConsensusState(cmd *cobra.Command) (*rpc.GetV1ConsensusStateResponse, error) {
-	client, err := rpc.NewClientWithResponses(remoteCLIURL)
+	client, err := rpc.NewClientWithResponses(flags.RemoteCLIURL)
 	if err != nil {
 		return nil, nil
 	}

app/client/cli/debug.go

@@ -6,19 +6,16 @@ import (
 	"os"
 
 	"github.com/manifoldco/promptui"
+	"github.com/spf13/cobra"
+	"google.golang.org/protobuf/types/known/anypb"
+
+	"github.com/pokt-network/pocket/app/client/cli/helpers"
 	"github.com/pokt-network/pocket/logger"
-	"github.com/pokt-network/pocket/p2p"
 	"github.com/pokt-network/pocket/p2p/providers/current_height_provider"
-	rpcCHP "github.com/pokt-network/pocket/p2p/providers/current_height_provider/rpc"
 	"github.com/pokt-network/pocket/p2p/providers/peerstore_provider"
-	rpcABP "github.com/pokt-network/pocket/p2p/providers/peerstore_provider/rpc"
 	typesP2P "github.com/pokt-network/pocket/p2p/types"
-	"github.com/pokt-network/pocket/runtime"
-	"github.com/pokt-network/pocket/runtime/defaults"
 	"github.com/pokt-network/pocket/shared/messaging"
 	"github.com/pokt-network/pocket/shared/modules"
-	"github.com/spf13/cobra"
-	"google.golang.org/protobuf/types/known/anypb"
 )
 
 // TECHDEBT: Lowercase variables / constants that do not need to be exported.
@@ -33,9 +30,6 @@ const (
 )
 
 var (
-	// A P2P module is initialized in order to broadcast a message to the local network
-	p2pMod modules.P2PModule
-
 	items = []string{
 		PromptPrintNodeState,
 		PromptTriggerNextView,
@@ -45,28 +39,12 @@ var (
 		PromptSendMetadataRequest,
 		PromptSendBlockRequest,
 	}
-
-	genesisPath string = runtime.GetEnv("GENESIS_PATH", "build/config/genesis.json")
-	rpcHost     string
 )
 
-// NOTE: this is required by the linter, otherwise a simple string constant would have been enough
-type cliContextKey string
-
-const busCLICtxKey = "bus"
-
 func init() {
 	dbg := NewDebugCommand()
 	dbg.AddCommand(NewDebugSubCommands()...)
 	rootCmd.AddCommand(dbg)
-
-	// by default, we point at the same endpoint used by the CLI but the debug client is used either in docker-compose of K8S, therefore we cater for overriding
-	validator1Endpoint := defaults.Validator1EndpointDockerCompose
-	if runtime.IsProcessRunningInsideKubernetes() {
-		validator1Endpoint = defaults.Validator1EndpointK8S
-	}
-
-	rpcHost = runtime.GetEnv("RPC_HOST", validator1Endpoint)
 }
 
 // NewDebugSubCommands builds out the list of debug subcommands by matching the
@@ -77,10 +55,8 @@ func NewDebugSubCommands() []*cobra.Command {
 	commands := make([]*cobra.Command, len(items))
 	for idx, promptItem := range items {
 		commands[idx] = &cobra.Command{
-			Use: promptItem,
-			PersistentPreRun: func(cmd *cobra.Command, args []string) {
-				persistentPreRun(cmd, args)
-			},
+			Use:               promptItem,
+			PersistentPreRunE: helpers.P2PDependenciesPreRunE,
 			Run: func(cmd *cobra.Command, args []string) {
 				handleSelect(cmd, cmd.Use)
 			},
@@ -93,70 +69,11 @@ func NewDebugSubCommands() []*cobra.Command {
 // NewDebugCommand returns the cobra CLI for the Debug command.
 func NewDebugCommand() *cobra.Command {
 	return &cobra.Command{
-		Use:   "debug",
-		Short: "Debug utility for rapid development",
-		Args:  cobra.MaximumNArgs(0),
-		PersistentPreRun: func(cmd *cobra.Command, args []string) {
-			persistentPreRun(cmd, args)
-		},
-		RunE: runDebug,
-	}
-}
-
-// persistentPreRun is called by both debug and debug sub-commands before runs
-func persistentPreRun(cmd *cobra.Command, _ []string) {
-	// TECHDEBT: this is to keep backwards compatibility with localnet
-	configPath = runtime.GetEnv("CONFIG_PATH", "build/config/config.validator1.json")
-	rpcURL := fmt.Sprintf("http://%s:%s", rpcHost, defaults.DefaultRPCPort)
-
-	runtimeMgr := runtime.NewManagerFromFiles(
-		configPath, genesisPath,
-		runtime.WithClientDebugMode(),
-		runtime.WithRandomPK(),
-	)
-
-	bus := runtimeMgr.GetBus()
-	setValueInCLIContext(cmd, busCLICtxKey, bus)
-
-	setupPeerstoreProvider(*runtimeMgr, rpcURL)
-	setupCurrentHeightProvider(*runtimeMgr, rpcURL)
-	setupAndStartP2PModule(*runtimeMgr)
-}
-
-func setupPeerstoreProvider(rm runtime.Manager, rpcURL string) {
-	bus := rm.GetBus()
-	modulesRegistry := bus.GetModulesRegistry()
-	pstoreProvider := rpcABP.Create(
-		rpcABP.WithP2PConfig(rm.GetConfig().P2P),
-		rpcABP.WithCustomRPCURL(rpcURL),
-	)
-	modulesRegistry.RegisterModule(pstoreProvider)
-}
-
-func setupCurrentHeightProvider(rm runtime.Manager, rpcURL string) {
-	bus := rm.GetBus()
-	modulesRegistry := bus.GetModulesRegistry()
-	currentHeightProvider := rpcCHP.NewRPCCurrentHeightProvider(
-		rpcCHP.WithCustomRPCURL(rpcURL),
-	)
-	modulesRegistry.RegisterModule(currentHeightProvider)
-}
-
-func setupAndStartP2PModule(rm runtime.Manager) {
-	bus := rm.GetBus()
-	mod, err := p2p.Create(bus)
-	if err != nil {
-		logger.Global.Fatal().Err(err).Msg("Failed to create p2p module")
-	}
-
-	var ok bool
-	p2pMod, ok = mod.(modules.P2PModule)
-	if !ok {
-		logger.Global.Fatal().Msgf("unexpected P2P module type: %T", mod)
-	}
-
-	if err := p2pMod.Start(); err != nil {
-		logger.Global.Fatal().Err(err).Msg("Failed to start p2p module")
+		Use:               "debug",
+		Short:             "Debug utility for rapid development",
+		Args:              cobra.MaximumNArgs(0),
+		PersistentPreRunE: helpers.P2PDependenciesPreRunE,
+		RunE:              runDebug,
 	}
 }
 
@@ -269,7 +186,7 @@ func broadcastDebugMessage(cmd *cobra.Command, debugMsg *messaging.DebugMessage)
 		if err != nil {
 			logger.Global.Fatal().Err(err).Msg("Failed to convert validator address into pocketCrypto.Address")
 		}
-		if err := p2pMod.Send(addr, anyProto); err != nil {
+		if err := helpers.P2PMod.Send(addr, anyProto); err != nil {
 			logger.Global.Error().Err(err).Msg("Failed to send debug message")
 		}
 	}
@@ -299,14 +216,14 @@ func sendDebugMessage(cmd *cobra.Command, debugMsg *messaging.DebugMessage) {
 		logger.Global.Fatal().Err(err).Msg("Failed to convert validator address into pocketCrypto.Address")
 	}
 
-	if err := p2pMod.Send(validatorAddress, anyProto); err != nil {
+	if err := helpers.P2PMod.Send(validatorAddress, anyProto); err != nil {
 		logger.Global.Error().Err(err).Msg("Failed to send debug message")
 	}
 }
 
 // fetchPeerstore retrieves the providers from the CLI context and uses them to retrieve the address book for the current height
 func fetchPeerstore(cmd *cobra.Command) (typesP2P.Peerstore, error) {
-	bus, ok := getValueFromCLIContext[modules.Bus](cmd, busCLICtxKey)
+	bus, ok := helpers.GetValueFromCLIContext[modules.Bus](cmd, helpers.BusCLICtxKey)
 	if !ok || bus == nil {
 		return nil, errors.New("retrieving bus from CLI context")
 	}

app/client/cli/flags/flags.go

@@ -0,0 +1,24 @@
+package flags
+
+var (
+	// RemoveCLIURL is the URL of the remote RPC node which the CLI will interact with.
+	// Formatted as <protocol>://<host>:<port> (uses RPC Port).
+	// (see: --help the root command for more info).
+	RemoteCLIURL string
+
+	// DataDir a path to store pocket related data (keybase etc.).
+	// (see: --help the root command for more info).
+	DataDir string
+
+	// ConfigPath is the path to the node config file.
+	// (see: --help the root command for more info).
+	ConfigPath string
+
+	// If true skips the interactive prompts wherever possible (useful for scripting & automation)
+	// (see: --help the root command for more info).
+	NonInteractive bool
+
+	// Show verbose output
+	// (see: --help the root command for more info).
+	Verbose bool
+)