improve command UX

cmd/kvcli/main.go

@@ -141,6 +141,7 @@ func main() {
 	paychanCmd.AddCommand(
 		client.PostCommands(
 			paychancmd.CreateChannelCmd(cdc),
+			paychancmd.GetChannelCmd(cdc, "paychan"), // pass in storeKey
 			paychancmd.GeneratePaymentCmd(cdc),
 			paychancmd.VerifyPaymentCmd(cdc, "paychan"), // pass in storeKey
 			paychancmd.SubmitPaymentCmd(cdc),

internal/x/paychan/README.md

@@ -11,10 +11,14 @@ Simplifications:
  - Tidy up - method descriptions, heading comments, remove uneccessary comments, README/docs
  - Find a better name for Queue - clarify distinction between int slice and abstract queue concept
  - write some sort of integration test
+ 	- possible bug in submitting same update repeatedly
  - find nicer name for payout
  - add Gas usage
  - add tags (return channel id on creation)
  - refactor cmds to be able to test them, then test them
+ 	- verify doesn’t throw json parsing error on invalid json
+ 	- can’t submit an update from an unitialised account
+ 	- pay without a --from returns confusing error
  - use custom errors instead of using sdk.ErrInternal
  - split off signatures from update as with txs/msgs - testing easier, code easier to use, doesn't store sigs unecessarily on chain
  - consider removing pubKey from UpdateSignature - instead let channel module access accountMapper

internal/x/paychan/client/cmd/cmd.go

@@ -3,7 +3,6 @@ package cli
 import (
 	"fmt"
 	"io/ioutil"
-	"os"
 
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -34,7 +33,8 @@ func CreateChannelCmd(cdc *wire.Codec) *cobra.Command {
 
 			// Create a "client context" stuct populated with info from common flags
 			ctx := context.NewCoreContextFromViper().WithDecoder(authcmd.GetAccountDecoder(cdc))
-			// ctx.PrintResponse = true TODO is this needed for channelID
+			// TODO is this needed for channelID
+			// ctx.PrintResponse = true
 
 			// Get sender adress
 			sender, err := ctx.GetFromAddress()
@@ -80,26 +80,20 @@ func CreateChannelCmd(cdc *wire.Codec) *cobra.Command {
 }
 
 func GeneratePaymentCmd(cdc *wire.Codec) *cobra.Command {
-	flagId := "id"                   // ChannelID
-	flagReceiverAmount := "r-amount" // amount the receiver should received on closing the channel
-	flagSenderAmount := "s-amount"   //
+	flagId := "chan-id"
+	flagReceiverAmount := "rec-amt" // amount the receiver should received on closing the channel
+	flagSenderAmount := "sen-amt"
+	flagPaymentFile := "filename"
 
 	cmd := &cobra.Command{
 		Use:   "pay",
-		Short: "Generate a .", // TODO descriptions
-		Long:  "Generate a new ",
+		Short: "Generate a new payment.", // TODO descriptions
+		Long:  "Generate a payment file (json) to send to the receiver as a payment.",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 
 			// Create a "client context" stuct populated with info from common flags
 			ctx := context.NewCoreContextFromViper().WithDecoder(authcmd.GetAccountDecoder(cdc))
-			// ctx.PrintResponse = false TODO is this needed to stop any other output messing up json?
-
-			// Get sender adress
-			// senderAddress, err := ctx.GetFromAddress()
-			// if err != nil {
-			// 	return err
-			// }
 
 			// Get the paychan id
 			id := paychan.ChannelID(viper.GetInt64(flagId)) // TODO make this default to pulling id from chain
@@ -143,28 +137,35 @@ func GeneratePaymentCmd(cdc *wire.Codec) *cobra.Command {
 				CryptoSignature: sig,
 			}}
 
-			// Print out the update
+			// Write out the update
 			jsonUpdate, err := wire.MarshalJSONIndent(cdc, update)
 			if err != nil {
 				return err
 			}
-			fmt.Println(string(jsonUpdate))
+			paymentFile := viper.GetString(flagPaymentFile)
+			err = ioutil.WriteFile(paymentFile, jsonUpdate, 0644)
+			if err != nil {
+				return err
+			}
+			fmt.Printf("Written payment out to %v.\n", paymentFile)
 
 			return nil
 		},
 	}
 	cmd.Flags().Int(flagId, 0, "ID of the payment channel.")
-	cmd.Flags().String(flagSenderAmount, "", "")
-	cmd.Flags().String(flagReceiverAmount, "", "")
+	cmd.Flags().String(flagSenderAmount, "", "Total coins to payout to sender on channel close.")
+	cmd.Flags().String(flagReceiverAmount, "", "Total coins to payout to sender on channel close.")
+	cmd.Flags().String(flagPaymentFile, "payment.json", "File name to write the payment into.")
 	return cmd
 }
 
 func VerifyPaymentCmd(cdc *wire.Codec, paychanStoreName string) *cobra.Command {
+	flagPaymentFile := "payment"
 
 	cmd := &cobra.Command{
 		Use:   "verify",
-		Short: "", // TODO
-		Long:  "",
+		Short: "Verify a payment file.",
+		Long:  "Verify that a received payment can be used to close a channel.",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 
@@ -172,7 +173,7 @@ func VerifyPaymentCmd(cdc *wire.Codec, paychanStoreName string) *cobra.Command {
 			ctx := context.NewCoreContextFromViper()
 
 			// read in update
-			bz, err := ioutil.ReadAll(os.Stdin)
+			bz, err := ioutil.ReadFile(viper.GetString(flagPaymentFile))
 			if err != nil {
 				// TODO add nice message about how to feed in stdin
 				return err
@@ -190,24 +191,30 @@ func VerifyPaymentCmd(cdc *wire.Codec, paychanStoreName string) *cobra.Command {
 			cdc.MustUnmarshalBinary(res, &channel)
 
 			//verify
-			updateIsOK := paychan.VerifyUpdate(channel, update)
+			verificationError := paychan.VerifyUpdate(channel, update)
 
 			// print result
-			fmt.Println(updateIsOK)
-
+			if verificationError == nil {
+				fmt.Printf("Payment is valid for channel '%d'.\n", update.ChannelID)
+			} else {
+				fmt.Printf("Payment is NOT valid for channel '%d'.\n", update.ChannelID)
+				fmt.Println(verificationError)
+			}
 			return nil
 		},
 	}
+	cmd.Flags().String(flagPaymentFile, "payment.json", "File name to read the payment from.")
 
 	return cmd
 }
 
 func SubmitPaymentCmd(cdc *wire.Codec) *cobra.Command {
+	flagPaymentFile := "payment"
 
 	cmd := &cobra.Command{
 		Use:   "submit",
-		Short: "",
-		Long:  "",
+		Short: "Submit a payment to the blockchain to close the channel.",
+		Long:  fmt.Sprintf("Submit a payment to the blockchain to either close a channel immediately (if you are the receiver) or after a dispute period of %d blocks (if you are the sender).", paychan.ChannelDisputeTime),
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 
@@ -222,7 +229,7 @@ func SubmitPaymentCmd(cdc *wire.Codec) *cobra.Command {
 			}
 
 			// read in update
-			bz, err := ioutil.ReadAll(os.Stdin)
+			bz, err := ioutil.ReadFile(viper.GetString(flagPaymentFile))
 			if err != nil {
 				return err
 			}
@@ -248,152 +255,44 @@ func SubmitPaymentCmd(cdc *wire.Codec) *cobra.Command {
 			return nil
 		},
 	}
-
+	cmd.Flags().String(flagPaymentFile, "payment.json", "File to read the payment from.")
 	return cmd
 }
 
-/*
-func ClosePaychanCmd(cdc *wire.Codec) *cobra.Command {
-	flagState := "state"
-
+func GetChannelCmd(cdc *wire.Codec, paychanStoreName string) *cobra.Command {
+	flagId := "chan-id"
 	cmd := &cobra.Command{
-		Use:   "close",
-		Short: "Close a payment channel, given a state",
-		Long:  "Close an existing payment channel with a state received from a sender. This signs it as the receiver before submitting to the blockchain.",
+		Use:   "get",
+		Short: "Get info on a channel.",
+		Long:  "Get information on a non closed channel.",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			ctx := context.NewCoreContextFromViper().WithDecoder(authcmd.GetAccountDecoder(cdc))
 
-			// Get the sender-signed close tx
-			state := viper.GetString(flagState)
-			txBytes, err := base64.StdEncoding.DecodeString(state)
-			if err != nil {
-				return err
+			// Create a "client context" stuct populated with info from common flags
+			ctx := context.NewCoreContextFromViper()
+
+			// Get channel ID
+			id := paychan.ChannelID(viper.GetInt64(flagId))
+
+			// Get the channel from the node
+			res, err := ctx.QueryStore(paychan.GetChannelKey(id), paychanStoreName)
+			if len(res) == 0 || err != nil {
+				return errors.Errorf("channel with ID '%d' does not exist", id)
 			}
-			stdTx := auth.StdTx{}
-			cdc.UnmarshalBinary(txBytes, &stdTx)
+			var channel paychan.Channel
+			cdc.MustUnmarshalBinary(res, &channel)
 
-			// Sign close tx
-
-			// ensure contxt has up to date account and sequence numbers
-			ctx, err = Ensure(ctx)
-			if err != nil {
-				return err
-			}
-			// Sign message (asks user for password)
-			_, sig, err := UserSignMsg(ctx, ctx.FromAddressName, stdTx.Msg)
+			// Convert the channel to a json object for pretty printing
+			jsonChannel, err := wire.MarshalJSONIndent(cdc, channel)
 			if err != nil {
 				return err
 			}
 
-			// Append signature to close tx
-			stdTx.Signatures = append(stdTx.Signatures, sig)
-			// encode close tx
-			txBytes, err = cdc.MarshalBinary(stdTx)
-			if err != nil {
-				return err
-			}
-
-			// Broadcast close tx to the blockchain
-
-			res, err := ctx.BroadcastTx(txBytes)
-			if err != nil {
-				return err
-			}
-			fmt.Printf("Committed at block %d. Hash: %s\n", res.Height, res.Hash.String())
+			// print out json channel
+			fmt.Println(string(jsonChannel))
 			return nil
 		},
 	}
-	cmd.Flags().String(flagState, "", "State received from sender.")
+	cmd.Flags().Int(flagId, 0, "ID of the payment channel.")
 	return cmd
 }
-
-// HELPER FUNCTIONS
-// This is a partial refactor of cosmos-sdk/client/context.
-// Existing API was awkard to use for paychans.
-
-func Ensure(ctx context.CoreContext) (context.CoreContext, error) {
-
-	ctx, err := context.EnsureAccountNumber(ctx)
-	if err != nil {
-		return ctx, err
-	}
-	// default to next sequence number if none provided
-	ctx, err = context.EnsureSequence(ctx)
-	if err != nil {
-		return ctx, err
-	}
-	return ctx, nil
-}
-
-func UserSignMsg(ctx context.CoreContext, name string, msg sdk.Msg) (signMsg auth.StdSignMsg, stdSig auth.StdSignature, err error) {
-
-	// TODO check how to handle non error return values on error. Returning empty versions doesn't seem right.
-
-	passphrase, err := ctx.GetPassphraseFromStdin(name)
-	if err != nil {
-		return signMsg, stdSig, err
-	}
-
-	// build the Sign Messsage from the Standard Message
-	chainID := ctx.ChainID
-	if chainID == "" {
-		return signMsg, stdSig, errors.Errorf("Chain ID required but not specified")
-	}
-	accnum := ctx.AccountNumber
-	sequence := ctx.Sequence
-
-	signMsg = auth.StdSignMsg{
-		ChainID:        chainID,
-		AccountNumbers: []int64{accnum},
-		Sequences:      []int64{sequence},
-		Msg:            msg,
-		Fee:            auth.NewStdFee(ctx.Gas, sdk.Coin{}), // TODO run simulate to estimate gas?
-	}
-
-	keybase, err := keys.GetKeyBase()
-	if err != nil {
-		return signMsg, stdSig, err
-	}
-
-	// sign and build
-	bz := signMsg.Bytes()
-
-	sig, pubkey, err := keybase.Sign(name, passphrase, bz)
-	if err != nil {
-		return signMsg, stdSig, err
-	}
-	stdSig = auth.StdSignature{
-		PubKey:        pubkey,
-		Signature:     sig,
-		AccountNumber: accnum,
-		Sequence:      sequence,
-	}
-
-	return signMsg, stdSig, nil
-}
-
-func Build(cdc *wire.Codec, signMsg auth.StdSignMsg, sig auth.StdSignature) ([]byte, error) {
-	tx := auth.NewStdTx(signMsg.Msg, signMsg.Fee, []auth.StdSignature{sig})
-	return cdc.MarshalBinary(tx)
-}
-
-func EnsureSignBuild(ctx context.CoreContext, name string, msg sdk.Msg, cdc *wire.Codec) ([]byte, error) {
-	//Ensure context has up to date account and sequence numbers
-	ctx, err := Ensure(ctx)
-	if err != nil {
-		return nil, err
-	}
-	// Sign message (asks user for password)
-	signMsg, sig, err := UserSignMsg(ctx, name, msg)
-	if err != nil {
-		return nil, err
-	}
-	// Create tx and marshal
-	txBytes, err := Build(cdc, signMsg, sig)
-	if err != nil {
-		return nil, err
-	}
-	return txBytes, nil
-}
-*/

internal/x/paychan/wire.go

@@ -9,12 +9,10 @@ func RegisterWire(cdc *wire.Codec) {
 	cdc.RegisterConcrete(MsgSubmitUpdate{}, "paychan/MsgSubmitUpdate", nil)
 }
 
+// TODO move this to near the msg definitions
 var msgCdc = wire.NewCodec()
 
-/*
 func init() {
+	wire.RegisterCrypto(msgCdc)
 	RegisterWire(msgCdc)
-	// TODO is this needed?
-	//wire.RegisterCrypto(msgCdc)
 }
-*/