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

reorganize basecli appTx commands #111

Merged
merged 1 commit into from
Jun 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 62 additions & 58 deletions cmd/basecli/commands/cmds.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,48 +10,56 @@ import (

"github.com/tendermint/light-client/commands"
txcmd "github.com/tendermint/light-client/commands/txs"
ctypes "github.com/tendermint/tendermint/rpc/core/types"
cmn "github.com/tendermint/tmlibs/common"

btypes "github.com/tendermint/basecoin/types"
)

/*** Here is the sendtx command ***/
/******** SendTx *********/

// SendTxCmd is CLI command to send tokens between basecoin accounts
var SendTxCmd = &cobra.Command{
Use: "send",
Short: "send tokens from one account to another",
RunE: doSendTx,
}

//nolint
const (
ToFlag = "to"
AmountFlag = "amount"
FeeFlag = "fee"
GasFlag = "gas"
SequenceFlag = "sequence"
FlagTo = "to"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I guess this is the more standard naming?

We should make sure we put Flag first in all repos then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this naming convention makes more sense, I will explore this issue of standardization across our repos, will also add to the coding standards documentation

FlagAmount = "amount"
FlagFee = "fee"
FlagGas = "gas"
FlagSequence = "sequence"
)

func init() {
flags := SendTxCmd.Flags()
flags.String(ToFlag, "", "Destination address for the bits")
flags.String(AmountFlag, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
flags.String(FeeFlag, "0mycoin", "Coins for the transaction fee of the format <amt><coin>")
flags.Int64(GasFlag, 0, "Amount of gas for this transaction")
flags.Int(SequenceFlag, -1, "Sequence number for this transaction")
flags.String(FlagTo, "", "Destination address for the bits")
flags.String(FlagAmount, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
flags.String(FlagFee, "0mycoin", "Coins for the transaction fee of the format <amt><coin>")
flags.Int64(FlagGas, 0, "Amount of gas for this transaction")
flags.Int(FlagSequence, -1, "Sequence number for this transaction")
}

// runDemo is an example of how to make a tx
func doSendTx(cmd *cobra.Command, args []string) error {
tx := new(btypes.SendTx)

// load data from json or flags
tx := new(btypes.SendTx)
found, err := txcmd.LoadJSON(tx)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed, as if found == false, LoadJSON will have done no work and err is guaranteed to be nil. So I optimized to use only one err checking block.

But this is more clear, and less likely to have subtle errors later introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, for more clarity use a nil assign found, _ :=

Copy link
Contributor

Choose a reason for hiding this comment

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

no, if found is true, then err is what we want

it made sense to me as i know the semantics of the function (and wrote all three cases in the header), but you are right, it is confusing. leaving it the way you put it.
one if statement costs the computer <1ns, but the lack of it costs a developer several minutes.

return err
}
if !found {
err = readSendTxFlags(tx)
}
if err != nil {
return err
}

// Wrap and add signer
send := &SendTx{
chainID: commands.GetChainID(),
Tx: tx,
Expand All @@ -64,34 +72,34 @@ func doSendTx(cmd *cobra.Command, args []string) error {
return err
}

// output result
// Output result
return txcmd.OutputTx(bres)
}

func readSendTxFlags(tx *btypes.SendTx) error {
// parse to address
to, err := ParseHexFlag(ToFlag)
to, err := ParseHexFlag(FlagTo)
if err != nil {
return errors.Errorf("To address is invalid hex: %v\n", err)
}

//parse the fee and amounts into coin types
tx.Fee, err = btypes.ParseCoin(viper.GetString(FeeFlag))
tx.Fee, err = btypes.ParseCoin(viper.GetString(FlagFee))
if err != nil {
return err
}
amountCoins, err := btypes.ParseCoins(viper.GetString(AmountFlag))
amountCoins, err := btypes.ParseCoins(viper.GetString(FlagAmount))
if err != nil {
return err
}

// set the gas
tx.Gas = viper.GetInt64(GasFlag)
tx.Gas = viper.GetInt64(FlagGas)

// craft the inputs and outputs
tx.Inputs = []btypes.TxInput{{
Coins: amountCoins,
Sequence: viper.GetInt(SequenceFlag),
Sequence: viper.GetInt(FlagSequence),
}}
tx.Outputs = []btypes.TxOutput{{
Address: to,
Expand All @@ -103,39 +111,53 @@ func readSendTxFlags(tx *btypes.SendTx) error {

/******** AppTx *********/

// BroadcastAppTx wraps, signs, and executes an app tx basecoin transaction
func BroadcastAppTx(tx *btypes.AppTx) (*ctypes.ResultBroadcastTxCommit, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good helper. Creating the tx from input is app-dependent, as is the output logic, but this should be the same for almost every app.


// Generate app transaction to be broadcast
appTx := WrapAppTx(tx)
appTx.AddSigner(txcmd.GetSigner())

// Sign if needed and post to the node. This it the work-horse
return txcmd.SignAndPostTx(appTx)
}

// AddAppTxFlags adds flags required by apptx
func AddAppTxFlags(fs *flag.FlagSet) {
fs.String(AmountFlag, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
fs.String(FeeFlag, "0mycoin", "Coins for the transaction fee of the format <amt><coin>")
fs.Int64(GasFlag, 0, "Amount of gas for this transaction")
fs.Int(SequenceFlag, -1, "Sequence number for this transaction")
fs.String(FlagAmount, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
fs.String(FlagFee, "0mycoin", "Coins for the transaction fee of the format <amt><coin>")
fs.Int64(FlagGas, 0, "Amount of gas for this transaction")
fs.Int(FlagSequence, -1, "Sequence number for this transaction")
}

// ReadAppTxFlags reads in the standard flags
// your command should parse info to set tx.Name and tx.Data
func ReadAppTxFlags(tx *btypes.AppTx) error {
//parse the fee and amounts into coin types
var err error
tx.Fee, err = btypes.ParseCoin(viper.GetString(FeeFlag))
func ReadAppTxFlags() (gas int64, fee btypes.Coin, txInput btypes.TxInput, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, seems a bit more verbose to me, but I see how you use it in the counter app.

If this usage seems more clear to you (no hidden modifications of the AppTx), then I am cool with it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is definitely my preference - another advantage I've been discovering while programming tracko is that the senders address is needed in the generation of the app.Data - it's nice to not need to reach into an incomplete transaction to pull this out and just use txInput.Address much more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

great argument. glad you are putting this to the test to make a better api. more useful and less magic


// Set the gas
gas = viper.GetInt64(FlagGas)

// Parse the fee and amounts into coin types
fee, err = btypes.ParseCoin(viper.GetString(FlagFee))
if err != nil {
return err
return
}
amountCoins, err := btypes.ParseCoins(viper.GetString(AmountFlag))

// craft the inputs
var amount btypes.Coins
amount, err = btypes.ParseCoins(viper.GetString(FlagAmount))
if err != nil {
return err
return
}

// set the gas
tx.Gas = viper.GetInt64(GasFlag)

// craft the inputs and outputs
tx.Input = btypes.TxInput{
Coins: amountCoins,
Sequence: viper.GetInt(SequenceFlag),
txInput = btypes.TxInput{
Coins: amount,
Sequence: viper.GetInt(FlagSequence),
}

return nil
return
}

// WrapAppTx wraps the transaction with chain id
func WrapAppTx(tx *btypes.AppTx) *AppTx {
return &AppTx{
chainID: commands.GetChainID(),
Expand All @@ -145,25 +167,7 @@ func WrapAppTx(tx *btypes.AppTx) *AppTx {

/** TODO copied from basecoin cli - put in common somewhere? **/

// ParseHexFlag parses a flag string to byte array
func ParseHexFlag(flag string) ([]byte, error) {
return hex.DecodeString(StripHex(viper.GetString(flag)))
}

// Returns true for non-empty hex-string prefixed with "0x"
func isHex(s string) bool {
if len(s) > 2 && s[:2] == "0x" {
_, err := hex.DecodeString(s[2:])
if err != nil {
return false
}
return true
}
return false
}

func StripHex(s string) string {
if isHex(s) {
return s[2:]
}
return s
return hex.DecodeString(cmn.StripHex(viper.GetString(flag)))
}
56 changes: 30 additions & 26 deletions cmd/basecli/counter/counter.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,68 +12,72 @@ import (
btypes "github.com/tendermint/basecoin/types"
)

//CounterTxCmd is the CLI command to execute the counter
// through the appTx Command
var CounterTxCmd = &cobra.Command{
Use: "counter",
Short: "add a vote to the counter",
Long: `Add a vote to the counter.

You must pass --valid for it to count and the countfee will be added to the counter.`,
RunE: doCounterTx,
RunE: counterTxCmd,
}

const (
CountFeeFlag = "countfee"
ValidFlag = "valid"
flagCountFee = "countfee"
flagValid = "valid"
)

func init() {
fs := CounterTxCmd.Flags()
bcmd.AddAppTxFlags(fs)
fs.String(CountFeeFlag, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
fs.Bool(ValidFlag, false, "Is count valid?")
fs.String(flagCountFee, "", "Coins to send in the format <amt><coin>,<amt><coin>...")
fs.Bool(flagValid, false, "Is count valid?")
}

func doCounterTx(cmd *cobra.Command, args []string) error {
tx := new(btypes.AppTx)
func counterTxCmd(cmd *cobra.Command, args []string) error {
// Note: we don't support loading apptx from json currently, so skip that

// read the standard flags
err := bcmd.ReadAppTxFlags(tx)
// Read the app-specific flags
name, data, err := getAppData()
if err != nil {
return err
}

// now read the app-specific flags
err = readCounterFlags(tx)
// Read the standard app-tx flags
Copy link
Contributor

Choose a reason for hiding this comment

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

@rigelrozanski : possible improvement...

If we want to get crazy with helper functions. It seems that everything from here until BroadcastAppTx is the same on every app.

So in addition to BroadcastAppTx, we could have PrepareAppTx(name, data) (*types.AppTx, error) that creates the apptx. Maybe even combine them into PrepareAndBroadcastAppTx(...).... but then again, maybe that is going overboard and would obscure the funcionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had actually originally set this up to go Cr4zY with the helper functions, but I decided that it makes sense to have some of the process still encapsulated in the application command - This is specifically in reference to the many variations of process flow I envisioned. There are really 4 elements here:

  • Get standard flags
  • Get plugin-specific flags
  • Create/Broadcast AppTx
  • Output
    Already while programming trackotron I've noticed how the generating the plugin-specific flags can require niche information from processed standard flags, I supposed we can pass in all the information in the form of a semi-complete AppTx - but this seems messy and unnecessary. In more complex (maybe multi-transaction) commands, some of these variables may be reused or reassigned before added to an AppTx and broadcast, and lastly a command may want to use different output.
    This tiny bit more verboseness ultimately makes the code much more flexible/clear

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the sanity check ;)

gas, fee, txInput, err := bcmd.ReadAppTxFlags()
if err != nil {
return err
}

app := bcmd.WrapAppTx(tx)
app.AddSigner(txcmd.GetSigner())

// Sign if needed and post. This it the work-horse
bres, err := txcmd.SignAndPostTx(app)
// Create AppTx and broadcast
tx := &btypes.AppTx{
Gas: gas,
Fee: fee,
Name: name,
Input: txInput,
Data: data,
}
res, err := bcmd.BroadcastAppTx(tx)
if err != nil {
return err
}

// output result
return txcmd.OutputTx(bres)
// Output result
return txcmd.OutputTx(res)
}

// readCounterFlags sets the app-specific data in the AppTx
func readCounterFlags(tx *btypes.AppTx) error {
countFee, err := btypes.ParseCoins(viper.GetString(CountFeeFlag))
func getAppData() (name string, data []byte, err error) {
countFee, err := btypes.ParseCoins(viper.GetString(flagCountFee))
if err != nil {
return err
return
}
ctx := counter.CounterTx{
Valid: viper.GetBool(ValidFlag),
Valid: viper.GetBool(flagValid),
Fee: countFee,
}

tx.Name = counter.New().Name()
tx.Data = wire.BinaryBytes(ctx)
return nil
name = counter.New().Name()
data = wire.BinaryBytes(ctx)
return
}
20 changes: 3 additions & 17 deletions cmd/basecli/counter/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import (
"github.com/tendermint/basecoin/plugins/counter"
)

//CounterQueryCmd CLI command to query the counter state
var CounterQueryCmd = &cobra.Command{
Use: "counter",
Short: "Query counter state, with proof",
RunE: doCounterQuery,
RunE: counterQueryCmd,
}

func doCounterQuery(cmd *cobra.Command, args []string) error {
func counterQueryCmd(cmd *cobra.Command, args []string) error {
key := counter.New().StateKey()

var cp counter.CounterPluginState
Expand All @@ -25,18 +26,3 @@ func doCounterQuery(cmd *cobra.Command, args []string) error {

return proofcmd.OutputProof(cp, proof.BlockHeight())
}

/*** doesn't seem to be needed anymore??? ***/

// type CounterPresenter struct{}

// func (_ CounterPresenter) MakeKey(str string) ([]byte, error) {
// key := counter.New().StateKey()
// return key, nil
// }

// func (_ CounterPresenter) ParseData(raw []byte) (interface{}, error) {
// var cp counter.CounterPluginState
// err := wire.ReadBinaryBytes(raw, &cp)
// return cp, err
// }
9 changes: 4 additions & 5 deletions cmd/basecli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,25 @@ tmcli to work for any custom abci app.
func main() {
commands.AddBasicFlags(BaseCli)

// prepare queries
// Prepare queries
pr := proofs.RootCmd
// these are default parsers, but you optional in your app
// These are default parsers, but you optional in your app
pr.AddCommand(proofs.TxCmd)
pr.AddCommand(proofs.KeyCmd)
pr.AddCommand(bcmd.AccountQueryCmd)
pr.AddCommand(bcount.CounterQueryCmd)

// here is how you would add the custom txs... but don't really add demo in your app
// Here is how you add custom txs... but don't really add counter in your app
proofs.TxPresenters.Register("base", bcmd.BaseTxPresenter{})
tr := txs.RootCmd
tr.AddCommand(bcmd.SendTxCmd)
tr.AddCommand(bcount.CounterTxCmd)

// TODO

// txs.Register("send", bcmd.SendTxMaker{})
// txs.Register("counter", bcount.CounterTxMaker{})

// set up the various commands to use
// Set up the various commands to use
BaseCli.AddCommand(
commands.InitCmd,
commands.ResetCmd,
Expand Down