cli: improve structure and reduce magic

[ebuchman]

This is a more concrete replacement of #560

The CLI/REST is a bit of a mess right now because of the abusive use of viper and stdin. There is common functionality to both the CLI and REST that is the actual stuff we want to do. This should be in its own package and well contained and easily testable without any dependence on viper or stdin. This is the goal of the client/builder, which we should call client/core. The surrounding client package can have convenience functions for mapping from viper/stdin to the things we need in core.

Here are some more concrete points for what we should do.

Naming

• Functions that return commands: eg. CmdAccount, CmdAccountDefault
• Function that take codec and return decoder func: eg. AccountDecoderFn
• client/builder -> client/core

Note in general functions that return functions should end in Fn.

Structure

• Each concern (x/auth, x/bank, client/keys, etc.) should have cli and rest pkgs whose primary objectives are to parse all the arguments from either viper/cobra or http and forward them to the client/builder.

• Currently the builder calls client.GetNode and makes various calls to viper. We should eliminate those fully - the builder should have ZERO calls to global functions/variables.

• To avoid dumping args into the builder functions, they can be methods on one or some structs. These structs should have all the things we would otherwise get from viper, stdin, etc., like node address, chain-id, password, etc.

• We should have helper functions in client that read from viper/stdin/etc. into the builder structs

[ebuchman]

Here's an example of the kind of madness we currently employ to help illustrate the problem with the current design: https://github.com/cosmos/cosmos-sdk/blob/master/x/bank/rest/sendtx.go#L77

This is in a REST handler:

		viper.Set(client.FlagSequence, m.Sequence)
		txBytes, err := builder.SignAndBuild(m.LocalAccountName, m.Password, msg, c.Cdc)
		if err != nil {
			w.WriteHeader(http.StatusUnauthorized)
			w.Write([]byte(err.Error()))
			return
		}

viper.Set there is a clear sign something is very wrong.

I think this helps justify that the builder functions should be methods on an object that has the things it needs like chainid, sequence, etc.

[cwgoes]

Are plaintext passwords passed through REST, or do we want the ability to do that? I don't see anything other than reading the password from stdin.

[ebuchman]

Currently they are passed through REST yes

[ebuchman]

Please see some relevant discussion here: #595

[cwgoes]

Are modules usually expected to expose the same functionality in CLI and REST? Might be worth separating further - each module could include three packages: commands, with all functionality (and calls to client/core), rest, which translates HTTP requests to commands, and cli, which translates CLI options to commands.

[rigelrozanski]

@cwgoes @ebuchman what's the status of this issue? Have we sufficiently reduced the amount of magic? Doesn't quite sound like it aka. this should still be open right?

[cwgoes]

No, I think we should still make sure modules keep their "command" functions (e.g. SendCoins) separate, then pass parameters to them from REST/CLI as appropriate (see above comment) - that isn't done yet.

Also not sure about the Naming category in the original issue - do we want to change names globally? Only within some subset of the code?

[cwgoes]

Renaming done by #880.

Closing in favor of #849, which covers the remaining checkbox.