From 351c28f0e14ec882607332cb78b28995822e0926 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Tue, 8 Dec 2020 00:44:00 +0100 Subject: [PATCH 01/42] fix: Signature only flag bug on tx sign command 7632 --- client/context.go | 10 +++++++-- x/auth/client/cli/cli_test.go | 4 +--- x/auth/client/cli/decode.go | 3 +-- x/auth/client/cli/encode.go | 3 +-- x/auth/client/cli/tx_multisign.go | 6 +++-- x/auth/client/cli/tx_sign.go | 37 +++++++++++++------------------ 6 files changed, 30 insertions(+), 33 deletions(-) diff --git a/client/context.go b/client/context.go index 7afddcf43119..89aec804cca6 100644 --- a/client/context.go +++ b/client/context.go @@ -197,14 +197,20 @@ func (ctx Context) WithInterfaceRegistry(interfaceRegistry codectypes.InterfaceR return ctx } -// PrintString prints the raw string to ctx.Output or os.Stdout +// PrintString prints the raw string to ctx.Output if it's defined, otherwise to or os.Stdout func (ctx Context) PrintString(str string) error { + return ctx.PrintBytes([]byte(str)) +} + +// PrintBytes prints the raw bytes to ctx.Output if it's defined, otherwise to or os.Stdout. +// NOTE: for printing a complex state object, you should use ctx.PrintOutput +func (ctx Context) PrintBytes(o []byte) error { writer := ctx.Output if writer == nil { writer = os.Stdout } - _, err := writer.Write([]byte(str)) + _, err := writer.Write(o) return err } diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index e6cc5230ea8a..efb1a38fb268 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -492,10 +492,8 @@ func (s *IntegrationTestSuite) TestCLIMultisignInsufficientCosigners() { multiSigWith1SignatureFile, cleanup3 := testutil.WriteToNewTempFile(s.T(), multiSigWith1Signature.String()) defer cleanup3() - exec, err := authtest.TxValidateSignaturesExec(val1.ClientCtx, multiSigWith1SignatureFile.Name()) + _, err = authtest.TxValidateSignaturesExec(val1.ClientCtx, multiSigWith1SignatureFile.Name()) s.Require().Error(err) - - fmt.Printf("%s", exec) } func (s *IntegrationTestSuite) TestCLIEncode() { diff --git a/x/auth/client/cli/decode.go b/x/auth/client/cli/decode.go index b83e8e405969..e1807efb099e 100644 --- a/x/auth/client/cli/decode.go +++ b/x/auth/client/cli/decode.go @@ -3,7 +3,6 @@ package cli import ( "encoding/base64" "encoding/hex" - "fmt" "github.com/spf13/cobra" @@ -43,7 +42,7 @@ func GetDecodeCommand() *cobra.Command { return err } - return clientCtx.PrintString(fmt.Sprintf("%s\n", json)) + return clientCtx.PrintBytes(json) }, } diff --git a/x/auth/client/cli/encode.go b/x/auth/client/cli/encode.go index 92db2e0a8171..c883980de956 100644 --- a/x/auth/client/cli/encode.go +++ b/x/auth/client/cli/encode.go @@ -2,7 +2,6 @@ package cli import ( "encoding/base64" - "fmt" "github.com/spf13/cobra" @@ -38,7 +37,7 @@ If you supply a dash (-) argument in place of an input filename, the command rea // base64 encode the encoded tx bytes txBytesBase64 := base64.StdEncoding.EncodeToString(txBytes) - return clientCtx.PrintString(fmt.Sprintf("%s\n", txBytesBase64)) + return clientCtx.PrintString(txBytesBase64 + "\n") }, } diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go index fc5e907de0a2..dd59226425d0 100644 --- a/x/auth/client/cli/tx_multisign.go +++ b/x/auth/client/cli/tx_multisign.go @@ -183,9 +183,11 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return err } - defer fp.Close() + if err = fp.Close(); err != nil { + return err + } - return clientCtx.PrintString(fmt.Sprintf("%s\n", json)) + return clientCtx.PrintBytes(json) } } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index a9c698078282..fcf0f4a72638 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -211,11 +211,12 @@ func preSignCmd(cmd *cobra.Command, _ []string) { func makeSignCmd() func(cmd *cobra.Command, args []string) error { return func(cmd *cobra.Command, args []string) error { clientCtx := client.GetClientContextFromCmd(cmd) - clientCtx, err := client.ReadTxCommandFlags(clientCtx, cmd.Flags()) + f := cmd.Flags() + clientCtx, err := client.ReadTxCommandFlags(clientCtx, f) if err != nil { return err } - txFactory := tx.NewFactoryCLI(clientCtx, cmd.Flags()) + txFactory := tx.NewFactoryCLI(clientCtx, f) clientCtx, txF, newTx, err := readTxAndInitContexts(clientCtx, cmd, args[0]) if err != nil { @@ -230,11 +231,9 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } - // if --signature-only is on, then override --append - generateSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly) - multisigAddrStr, _ := cmd.Flags().GetString(flagMultisig) - - from, _ := cmd.Flags().GetString(flags.FlagFrom) + generateSignatureOnly, _ := f.GetBool(flagSigOnly) + multisigAddrStr, _ := f.GetString(flagMultisig) + from, _ := f.GetString(flags.FlagFrom) _, fromName, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly) if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) @@ -242,20 +241,17 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if multisigAddrStr != "" { var multisigAddr sdk.AccAddress - multisigAddr, err = sdk.AccAddressFromBech32(multisigAddrStr) if err != nil { return err } - err = authclient.SignTxWithSignerAddress( txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, ) generateSignatureOnly = true } else { - flagAppend, _ := cmd.Flags().GetBool(flagAppend) - appendSig := flagAppend && !generateSignatureOnly - if appendSig { + flagAppend, _ := f.GetBool(flagAppend) + if flagAppend || generateSignatureOnly { err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline) } } @@ -263,25 +259,21 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } - aminoJSON, _ := cmd.Flags().GetBool(flagAmino) - + aminoJSON, err := f.GetBool(flagAmino) if err != nil { return err } var json []byte - if aminoJSON { stdTx, err := tx.ConvertTxToStdTx(clientCtx.LegacyAmino, txBuilder.GetTx()) if err != nil { return err } - req := rest.BroadcastReq{ Tx: stdTx, Mode: "block|sync|async", } - json, err = clientCtx.LegacyAmino.MarshalJSON(req) if err != nil { return err @@ -303,16 +295,17 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return err } - defer fp.Close() + if err = fp.Close(); err != nil { + return err + } - return clientCtx.PrintString(fmt.Sprintf("%s\n", json)) + return clientCtx.PrintBytes(json) } } -func marshalSignatureJSON(txConfig client.TxConfig, txBldr client.TxBuilder, generateSignatureOnly bool) ([]byte, error) { +func marshalSignatureJSON(txConfig client.TxConfig, txBldr client.TxBuilder, signatureOnly bool) ([]byte, error) { parsedTx := txBldr.GetTx() - - if generateSignatureOnly { + if signatureOnly { sigs, err := parsedTx.GetSignaturesV2() if err != nil { return nil, err From 4308f1cf5fc56651df51524badb2897f9305b4ad Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Tue, 8 Dec 2020 01:36:21 +0100 Subject: [PATCH 02/42] Update client/context.go Co-authored-by: Cory --- client/context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/context.go b/client/context.go index 89aec804cca6..1cb8cc9eded7 100644 --- a/client/context.go +++ b/client/context.go @@ -197,7 +197,7 @@ func (ctx Context) WithInterfaceRegistry(interfaceRegistry codectypes.InterfaceR return ctx } -// PrintString prints the raw string to ctx.Output if it's defined, otherwise to or os.Stdout +// PrintString prints the raw string to ctx.Output if it's defined, otherwise to os.Stdout func (ctx Context) PrintString(str string) error { return ctx.PrintBytes([]byte(str)) } From 86e1ca7b4febc1cb9fca7b2f9edcb550698ecb1f Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Tue, 8 Dec 2020 12:33:07 +0100 Subject: [PATCH 03/42] Update client/context.go Co-authored-by: Cory --- client/context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/context.go b/client/context.go index 1cb8cc9eded7..6374a091cc1b 100644 --- a/client/context.go +++ b/client/context.go @@ -202,7 +202,7 @@ func (ctx Context) PrintString(str string) error { return ctx.PrintBytes([]byte(str)) } -// PrintBytes prints the raw bytes to ctx.Output if it's defined, otherwise to or os.Stdout. +// PrintBytes prints the raw bytes to ctx.Output if it's defined, otherwise to os.Stdout. // NOTE: for printing a complex state object, you should use ctx.PrintOutput func (ctx Context) PrintBytes(o []byte) error { writer := ctx.Output From 09984c6efa164f1f3e12ddb23c81ed0eae8cf96e Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 8 Dec 2020 13:08:42 +0000 Subject: [PATCH 04/42] use named return value and closure (#8111) This is to correctly handle deferred Close() calls on writable files. --- x/auth/client/cli/tx_multisign.go | 16 +++++++++++----- x/auth/client/cli/tx_sign.go | 18 ++++++++++++------ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go index dd59226425d0..56e1e1b5c9d2 100644 --- a/x/auth/client/cli/tx_multisign.go +++ b/x/auth/client/cli/tx_multisign.go @@ -60,7 +60,7 @@ recommended to set such parameters manually. return cmd } -func makeMultiSignCmd() func(cmd *cobra.Command, args []string) error { +func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) { return func(cmd *cobra.Command, args []string) (err error) { clientCtx := client.GetClientContextFromCmd(cmd) clientCtx, err = client.ReadTxCommandFlags(clientCtx, cmd.Flags()) @@ -183,11 +183,17 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return err } - if err = fp.Close(); err != nil { - return err - } - return clientCtx.PrintBytes(json) + defer func() { + err2 := fp.Close() + if err == nil { + err = err2 + } + }() + + err = clientCtx.PrintBytes(json) + + return } } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index fcf0f4a72638..2f3f87c8e61b 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -209,10 +209,10 @@ func preSignCmd(cmd *cobra.Command, _ []string) { } func makeSignCmd() func(cmd *cobra.Command, args []string) error { - return func(cmd *cobra.Command, args []string) error { + return func(cmd *cobra.Command, args []string) (err error) { clientCtx := client.GetClientContextFromCmd(cmd) f := cmd.Flags() - clientCtx, err := client.ReadTxCommandFlags(clientCtx, f) + clientCtx, err = client.ReadTxCommandFlags(clientCtx, f) if err != nil { return err } @@ -295,11 +295,17 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return err } - if err = fp.Close(); err != nil { - return err - } - return clientCtx.PrintBytes(json) + defer func() { + err2 := fp.Close() + if err == nil { + err = err2 + } + }() + + err = clientCtx.PrintBytes(json) + + return } } From b97a939bcd34bde7ecc86902bd527ca128a4535a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Tue, 8 Dec 2020 23:14:40 +0100 Subject: [PATCH 05/42] set the right 'append' logic for signing transactions --- x/auth/client/cli/tx_sign.go | 7 ++++--- x/auth/client/tx.go | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 2f3f87c8e61b..467328d95dc5 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -171,7 +171,7 @@ func GetSignCommand() *cobra.Command { It will read a transaction from [file], sign it, and print its JSON encoding. If the flag --signature-only flag is set, it will output a JSON representation -of the generated signature only. +of the signatures only. The --offline flag makes sure that the client will not reach out to full node. As a result, the account and sequence number queries will not be performed and @@ -189,7 +189,7 @@ be generated via the 'multisign' command. cmd.Flags().String(flagMultisig, "", "Address of the multisig account on behalf of which the transaction shall be signed") cmd.Flags().Bool(flagAppend, true, "Append the signature to the existing ones. If disabled, old signatures would be overwritten. Ignored if --multisig is on") - cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit") + cmd.Flags().Bool(flagSigOnly, false, "Print only the signature") cmd.Flags().String(flags.FlagOutputDocument, "", "The document will be written to the given file instead of STDOUT") cmd.Flags().String(flags.FlagChainID, "", "The network chain ID") cmd.Flags().Bool(flagAmino, false, "Generate Amino encoded JSON suitable for submiting to the txs REST endpoint") @@ -251,7 +251,8 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { generateSignatureOnly = true } else { flagAppend, _ := f.GetBool(flagAppend) - if flagAppend || generateSignatureOnly { + // txBuilder.GetTx().GetSignaturesV2() + if flagAppend { err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline) } } diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go index 79570d519054..8d79adc0fcff 100644 --- a/x/auth/client/tx.go +++ b/x/auth/client/tx.go @@ -43,8 +43,8 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk. return err } -// SignTx appends a signature to a transaction. If appendSig -// is false, it replaces the signatures already attached with the new signature. +// SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase. +// The new signature is appended to to the TxBuilder. // Don't perform online validation or lookups if offline is true. func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline bool) error { info, err := txFactory.Keybase().Key(name) From 7d96902174b3980f842b17ef4311bf84d4be67fd Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Tue, 8 Dec 2020 23:55:07 +0100 Subject: [PATCH 06/42] cleanup --- client/tx/tx.go | 9 +++++---- x/auth/client/cli/tx_sign.go | 2 +- x/auth/tx/builder.go | 4 ---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index d1814c36359f..116c8061d1be 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -375,8 +375,9 @@ func SignWithPrivKey( return sigV2, nil } -// Sign signs a given tx with the provided name and passphrase. The bytes signed -// over are canconical. The resulting signature will be set on the transaction. +// Sign signs a given tx with a named key. The bytes signed over are canconical. +// The resulting signature will be set in the transaction builder +// overwriting previous signatures. // An error is returned upon failure. func Sign(txf Factory, name string, txBuilder client.TxBuilder) error { if txf.keybase == nil { @@ -423,13 +424,13 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error { } // Generate the bytes to be signed. - signBytes, err := txf.txConfig.SignModeHandler().GetSignBytes(signMode, signerData, txBuilder.GetTx()) + bytesToSign, err := txf.txConfig.SignModeHandler().GetSignBytes(signMode, signerData, txBuilder.GetTx()) if err != nil { return err } // Sign those bytes - sigBytes, _, err := txf.keybase.Sign(name, signBytes) + sigBytes, _, err := txf.keybase.Sign(name, bytesToSign) if err != nil { return err } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 467328d95dc5..0faec49a9be0 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -251,7 +251,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { generateSignatureOnly = true } else { flagAppend, _ := f.GetBool(flagAppend) - // txBuilder.GetTx().GetSignaturesV2() + // txBuilder.GetTx().Si if flagAppend { err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline) } diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index 4c76d529a9de..fae3c3d4fec4 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -157,10 +157,6 @@ func (w *wrapper) GetMemo() string { return w.tx.Body.Memo } -func (w *wrapper) GetSignatures() [][]byte { - return w.tx.Signatures -} - // GetTimeoutHeight returns the transaction's timeout height (if set). func (w *wrapper) GetTimeoutHeight() uint64 { return w.tx.Body.TimeoutHeight From a0d26fb10d99829de5db6fda152eac2ba4e458ef Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 00:08:40 +0100 Subject: [PATCH 07/42] update tx.Sign interface by adding overwrite option --- client/tx/tx.go | 15 +++++++++++---- x/auth/client/cli/tx_sign.go | 2 +- x/auth/client/tx.go | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index 116c8061d1be..284cf1df7700 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -376,10 +376,10 @@ func SignWithPrivKey( } // Sign signs a given tx with a named key. The bytes signed over are canconical. -// The resulting signature will be set in the transaction builder -// overwriting previous signatures. +// The resulting signature will be added to the transaction builder overwriting the previous +// ones if overwrite=true (otherwise, ne signature will be appended). // An error is returned upon failure. -func Sign(txf Factory, name string, txBuilder client.TxBuilder) error { +func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwrite bool) error { if txf.keybase == nil { return errors.New("keybase must be set prior to signing a transaction") } @@ -419,6 +419,10 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error { Data: &sigData, Sequence: txf.Sequence(), } + var previousSignatures []signing.SignatureV2 + if overwrite { + previousSignatures = txBuilder.GetTx().GetSignaturesV2() + } if err := txBuilder.SetSignatures(sig); err != nil { return err } @@ -446,7 +450,10 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error { Sequence: txf.Sequence(), } - // And here the tx is populated with the signature + if overwrite { + previousSignatures = append(previousSignatures, sig) + return txBuilder.SetSignatures(previousSignatures...) + } return txBuilder.SetSignatures(sig) } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 0faec49a9be0..30f80c1f88e9 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -253,7 +253,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { flagAppend, _ := f.GetBool(flagAppend) // txBuilder.GetTx().Si if flagAppend { - err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline) + err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, !flagAppend) } } if err != nil { diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go index 8d79adc0fcff..4c84fbb6a07c 100644 --- a/x/auth/client/tx.go +++ b/x/auth/client/tx.go @@ -46,7 +46,7 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk. // SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase. // The new signature is appended to to the TxBuilder. // Don't perform online validation or lookups if offline is true. -func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline bool) error { +func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwrite bool) error { info, err := txFactory.Keybase().Key(name) if err != nil { return err @@ -62,7 +62,7 @@ func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx c } } - return tx.Sign(txFactory, name, stdTx) + return tx.Sign(txFactory, name, stdTx, overwrite) } // SignTxWithSignerAddress attaches a signature to a transaction. From ac324b379808ed31162faf5b8f4d17e639e72ddc Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 00:11:36 +0100 Subject: [PATCH 08/42] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99d256e7b061..b9dd0be255a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#7918](https://github.com/cosmos/cosmos-sdk/pull/7918) Add x/capability safety checks: * All outward facing APIs will now check that capability is not nil and name is not empty before performing any state-machine changes * `SetIndex` has been renamed to `InitializeIndex` +* (client/tx, x/auth/client) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) Added `overwrite` option to the `tx.Sign` and `client.SignTx` functions. ### Features From 1b16901a7f29310abbd14483a434621d625a1d02 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 00:12:18 +0100 Subject: [PATCH 09/42] sign command cleanup --- x/auth/client/cli/tx_sign.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 30f80c1f88e9..85693619e1e7 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -251,10 +251,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { generateSignatureOnly = true } else { flagAppend, _ := f.GetBool(flagAppend) - // txBuilder.GetTx().Si - if flagAppend { - err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, !flagAppend) - } + err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, !flagAppend) } if err != nil { return err From 9d5ce978ba886d46a708607d81f80ae005031f41 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 11:30:36 +0100 Subject: [PATCH 10/42] implementation and changelog update --- CHANGELOG.md | 7 ++++--- client/tx/tx.go | 12 ++++++------ x/auth/client/cli/tx_sign.go | 12 ++++++------ x/auth/client/tx.go | 4 ++-- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9dd0be255a9..0d7752d65adb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,14 +56,15 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes * (crypto) [\#7966](https://github.com/cosmos/cosmos-sdk/issues/7966) `Bip44Params` `String()` function now correctly returns the absolute HD path by adding the `m/` prefix. - +* (x/auth/client/cli) [\#7632](https://github.com/cosmos/cosmos-sdk/issues/7632) fixing regression bugs in transaction signing. ### API Breaking * [\#8080](https://github.com/cosmos/cosmos-sdk/pull/8080) Updated the `codec.Marshaler` interface * Moved `MarshalAny` and `UnmarshalAny` helper functions to `codec.Marshaler` and renamed to `MarshalInterface` and `UnmarshalInterface` respectively. These functions must take interface as a parameter (not a concrete type nor `Any` object). Underneath they use `Any` wrapping for correct protobuf serialization. - - +* (x/auth/tx) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) change related to missing append functionality in client transaction signing + + added `overwriteSig` argument to `x/auth/client.SignTx` and `client/tx.Sign` functions. + + removed `x/auth/tx.go:wrapper.GetSignatures`. The `wrapper` provides `TxBuilder` functionality, and it's a private structure. That function was not used at all and it's not exposed through the `TxBuilder` interface. ## [v0.40.0-rc3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.0-rc3) - 2020-11-06 diff --git a/client/tx/tx.go b/client/tx/tx.go index 284cf1df7700..eaac3a33fadd 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -379,7 +379,7 @@ func SignWithPrivKey( // The resulting signature will be added to the transaction builder overwriting the previous // ones if overwrite=true (otherwise, ne signature will be appended). // An error is returned upon failure. -func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwrite bool) error { +func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig bool) error { if txf.keybase == nil { return errors.New("keybase must be set prior to signing a transaction") } @@ -420,7 +420,7 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwrite bool) Sequence: txf.Sequence(), } var previousSignatures []signing.SignatureV2 - if overwrite { + if overwriteSig { previousSignatures = txBuilder.GetTx().GetSignaturesV2() } if err := txBuilder.SetSignatures(sig); err != nil { @@ -450,11 +450,11 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwrite bool) Sequence: txf.Sequence(), } - if overwrite { - previousSignatures = append(previousSignatures, sig) - return txBuilder.SetSignatures(previousSignatures...) + if overwriteSig { + return txBuilder.SetSignatures(sig) } - return txBuilder.SetSignatures(sig) + previousSignatures = append(previousSignatures, sig) + return txBuilder.SetSignatures(previousSignatures...) } // GasEstimateResponse defines a response definition for tx gas estimation. diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 85693619e1e7..d32d16344501 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -16,10 +16,10 @@ import ( ) const ( - flagMultisig = "multisig" - flagAppend = "append" - flagSigOnly = "signature-only" - flagAmino = "amino" + flagMultisig = "multisig" + flagOverwrite = "overwrite" + flagSigOnly = "signature-only" + flagAmino = "amino" ) // GetSignBatchCommand returns the transaction sign-batch command. @@ -188,7 +188,7 @@ be generated via the 'multisign' command. } cmd.Flags().String(flagMultisig, "", "Address of the multisig account on behalf of which the transaction shall be signed") - cmd.Flags().Bool(flagAppend, true, "Append the signature to the existing ones. If disabled, old signatures would be overwritten. Ignored if --multisig is on") + cmd.Flags().Bool(flagOverwrite, false, "Append the signature to the existing ones. If disabled, old signatures would be overwritten. Ignored if --multisig is on") cmd.Flags().Bool(flagSigOnly, false, "Print only the signature") cmd.Flags().String(flags.FlagOutputDocument, "", "The document will be written to the given file instead of STDOUT") cmd.Flags().String(flags.FlagChainID, "", "The network chain ID") @@ -250,7 +250,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { ) generateSignatureOnly = true } else { - flagAppend, _ := f.GetBool(flagAppend) + flagAppend, _ := f.GetBool(flagOverwrite) err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, !flagAppend) } if err != nil { diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go index 4c84fbb6a07c..304e66fe02e2 100644 --- a/x/auth/client/tx.go +++ b/x/auth/client/tx.go @@ -46,7 +46,7 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk. // SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase. // The new signature is appended to to the TxBuilder. // Don't perform online validation or lookups if offline is true. -func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwrite bool) error { +func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwriteSig bool) error { info, err := txFactory.Keybase().Key(name) if err != nil { return err @@ -62,7 +62,7 @@ func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx c } } - return tx.Sign(txFactory, name, stdTx, overwrite) + return tx.Sign(txFactory, name, stdTx, overwriteSig) } // SignTxWithSignerAddress attaches a signature to a transaction. From e774b31309f0c655dfac3a533b04072e2300c431 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 16:56:10 +0100 Subject: [PATCH 11/42] fix SignTx and tx.Sign calls --- client/tx/tx.go | 13 ++++++++----- client/tx/tx_test.go | 6 +++--- simapp/simd/cmd/testnet.go | 2 +- testutil/network/network.go | 2 +- x/auth/client/cli/tx_sign.go | 30 +++++++++++++----------------- x/auth/client/rest/rest_test.go | 2 +- x/auth/client/tx.go | 6 +++--- x/auth/tx/service_test.go | 2 +- x/genutil/client/cli/gentx.go | 6 +++--- 9 files changed, 34 insertions(+), 35 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index eaac3a33fadd..caa1f2004bf8 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -117,7 +117,7 @@ func BroadcastTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error { } } - err = Sign(txf, clientCtx.GetFromName(), tx) + err = Sign(txf, clientCtx.GetFromName(), tx, true) if err != nil { return err } @@ -419,9 +419,12 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo Data: &sigData, Sequence: txf.Sequence(), } - var previousSignatures []signing.SignatureV2 + var prevSignatures []signing.SignatureV2 if overwriteSig { - previousSignatures = txBuilder.GetTx().GetSignaturesV2() + prevSignatures, err = txBuilder.GetTx().GetSignaturesV2() + if err != nil { + return err + } } if err := txBuilder.SetSignatures(sig); err != nil { return err @@ -453,8 +456,8 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo if overwriteSig { return txBuilder.SetSignatures(sig) } - previousSignatures = append(previousSignatures, sig) - return txBuilder.SetSignatures(previousSignatures...) + prevSignatures = append(prevSignatures, sig) + return txBuilder.SetSignatures(prevSignatures...) } // GasEstimateResponse defines a response definition for tx gas estimation. diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 3325e738d553..323c0b6cd2c7 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -147,7 +147,7 @@ func TestSign(t *testing.T) { require.NoError(t, err) t.Log("should failed if txf without keyring") - err = tx.Sign(txf, from, txn) + err = tx.Sign(txf, from, txn, true) require.Error(t, err) txf = tx.Factory{}. @@ -160,10 +160,10 @@ func TestSign(t *testing.T) { WithChainID("test-chain") t.Log("should succeed if txf with keyring") - err = tx.Sign(txf, from, txn) + err = tx.Sign(txf, from, txn, true) require.NoError(t, err) t.Log("should fail for non existing key") - err = tx.Sign(txf, "non_existing_key", txn) + err = tx.Sign(txf, "non_existing_key", txn, true) require.Error(t, err) } diff --git a/simapp/simd/cmd/testnet.go b/simapp/simd/cmd/testnet.go index 3355c04b4e71..5144373f9157 100644 --- a/simapp/simd/cmd/testnet.go +++ b/simapp/simd/cmd/testnet.go @@ -230,7 +230,7 @@ func InitTestnet( WithKeybase(kb). WithTxConfig(clientCtx.TxConfig) - if err := tx.Sign(txFactory, nodeDirName, txBuilder); err != nil { + if err := tx.Sign(txFactory, nodeDirName, txBuilder, true); err != nil { return err } diff --git a/testutil/network/network.go b/testutil/network/network.go index a50c6fa935af..23706c12ca80 100644 --- a/testutil/network/network.go +++ b/testutil/network/network.go @@ -323,7 +323,7 @@ func New(t *testing.T, cfg Config) *Network { WithKeybase(kb). WithTxConfig(cfg.TxConfig) - err = tx.Sign(txFactory, nodeDirName, txBuilder) + err = tx.Sign(txFactory, nodeDirName, txBuilder, true) require.NoError(t, err) txBz, err := cfg.TxConfig.TxJSONEncoder()(txBuilder.GetTx()) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index d32d16344501..0deca4067e0d 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -67,14 +67,10 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { return err } txFactory := tx.NewFactoryCLI(clientCtx, cmd.Flags()) - txCfg := clientCtx.TxConfig - generateSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly) - - var ( - multisigAddr sdk.AccAddress - infile = os.Stdin - ) + printSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly) + infile := os.Stdin + var multisigAddr sdk.AccAddress // validate multisig address if there's any if ms, _ := cmd.Flags().GetString(flagMultisig); ms != "" { @@ -114,7 +110,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("error getting account from keybase: %w", err) } - err = authclient.SignTx(txFactory, clientCtx, fromName, txBuilder, true) + err = authclient.SignTx(txFactory, clientCtx, fromName, txBuilder, true, true) if err != nil { return err } @@ -122,14 +118,15 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED { txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) } - err = authclient.SignTxWithSignerAddress(txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, clientCtx.Offline) + err = authclient.SignTxWithSignerAddress( + txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, true) } if err != nil { return err } - json, err := marshalSignatureJSON(txCfg, txBuilder, generateSignatureOnly) + json, err := marshalSignatureJSON(txCfg, txBuilder, printSignatureOnly) if err != nil { return err } @@ -231,7 +228,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } - generateSignatureOnly, _ := f.GetBool(flagSigOnly) + printSignatureOnly, _ := f.GetBool(flagSigOnly) multisigAddrStr, _ := f.GetString(flagMultisig) from, _ := f.GetString(flags.FlagFrom) _, fromName, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly) @@ -239,6 +236,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return fmt.Errorf("error getting account from keybase: %w", err) } + overwrite, _ := f.GetBool(flagOverwrite) if multisigAddrStr != "" { var multisigAddr sdk.AccAddress multisigAddr, err = sdk.AccAddressFromBech32(multisigAddrStr) @@ -246,12 +244,10 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } err = authclient.SignTxWithSignerAddress( - txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, - ) - generateSignatureOnly = true + txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, overwrite) + printSignatureOnly = true } else { - flagAppend, _ := f.GetBool(flagOverwrite) - err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, !flagAppend) + err = authclient.SignTx(txF, clientCtx, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, overwrite) } if err != nil { return err @@ -277,7 +273,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { return err } } else { - json, err = marshalSignatureJSON(txCfg, txBuilder, generateSignatureOnly) + json, err = marshalSignatureJSON(txCfg, txBuilder, printSignatureOnly) if err != nil { return err } diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index fb9dd73d75f9..6db5a65a8f11 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -379,7 +379,7 @@ func (s *IntegrationTestSuite) createTestStdTx(val *network.Validator, accNum, s WithSequence(sequence) // sign Tx (offline mode so we can manually set sequence number) - err := authclient.SignTx(txFactory, val.ClientCtx, val.Moniker, txBuilder, true) + err := authclient.SignTx(txFactory, val.ClientCtx, val.Moniker, txBuilder, true, true) s.Require().NoError(err) stdTx := txBuilder.GetTx().(legacytx.StdTx) diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go index 304e66fe02e2..70229c39e5c2 100644 --- a/x/auth/client/tx.go +++ b/x/auth/client/tx.go @@ -44,7 +44,7 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk. } // SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase. -// The new signature is appended to to the TxBuilder. +// The new signature is appended to the TxBuilder when overwrite=false or overwritten otherwise. // Don't perform online validation or lookups if offline is true. func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwriteSig bool) error { info, err := txFactory.Keybase().Key(name) @@ -69,7 +69,7 @@ func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx c // Don't perform online validation or lookups if offline is true, else // populate account and sequence numbers from a foreign account. func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, addr sdk.AccAddress, - name string, txBuilder client.TxBuilder, offline bool) (err error) { + name string, txBuilder client.TxBuilder, offline, overwrite bool) (err error) { // check whether the address is a signer if !isTxSigner(addr, txBuilder.GetTx().GetSigners()) { @@ -83,7 +83,7 @@ func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, add } } - return tx.Sign(txFactory, name, txBuilder) + return tx.Sign(txFactory, name, txBuilder, overwrite) } // Read and decode a StdTx from the given filename. Can pass "-" to read from stdin. diff --git a/x/auth/tx/service_test.go b/x/auth/tx/service_test.go index 341684acbe26..2fffdb902338 100644 --- a/x/auth/tx/service_test.go +++ b/x/auth/tx/service_test.go @@ -448,7 +448,7 @@ func (s IntegrationTestSuite) mkTxBuilder() client.TxBuilder { WithSignMode(signing.SignMode_SIGN_MODE_DIRECT) // Sign Tx. - err := authclient.SignTx(txFactory, val.ClientCtx, val.Moniker, txBuilder, false) + err := authclient.SignTx(txFactory, val.ClientCtx, val.Moniker, txBuilder, false, true) s.Require().NoError(err) return txBuilder diff --git a/x/genutil/client/cli/gentx.go b/x/genutil/client/cli/gentx.go index 33a37760eda8..790cc9921b3e 100644 --- a/x/genutil/client/cli/gentx.go +++ b/x/genutil/client/cli/gentx.go @@ -41,9 +41,9 @@ func GenTxCmd(mbm module.BasicManager, txEncCfg client.TxEncodingConfig, genBalI Long: fmt.Sprintf(`Generate a genesis transaction that creates a validator with a self-delegation, that is signed by the key in the Keyring referenced by a given name. A node ID and Bech32 consensus pubkey may optionally be provided. If they are omitted, they will be retrieved from the priv_validator.json -file. The following default parameters are included: +file. The following default parameters are included: %s - + Example: $ %s gentx my-key-name --home=/path/to/home/dir --keyring-backend=os --chain-id=test-chain-1 \ --amount=1000000stake \ @@ -164,7 +164,7 @@ $ %s gentx my-key-name --home=/path/to/home/dir --keyring-backend=os --chain-id= return fmt.Errorf("error creating tx builder: %w", err) } - err = authclient.SignTx(txFactory, clientCtx, name, txBuilder, true) + err = authclient.SignTx(txFactory, clientCtx, name, txBuilder, true, true) if err != nil { return errors.Wrap(err, "failed to sign std tx") } From e731b5a88146de0760295793c751ea7e83a065a5 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 17:03:08 +0100 Subject: [PATCH 12/42] fix: sign didn't write to a file --- x/auth/client/cli/tx_sign.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 0deca4067e0d..8e8092991813 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -289,7 +289,6 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { if err != nil { return err } - defer func() { err2 := fp.Close() if err == nil { @@ -297,8 +296,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { } }() - err = clientCtx.PrintBytes(json) - + _, err = fp.Write(json) return } } From 4784d9365cbb70a81eaae8d0f079a092e7224a8a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 17:21:54 +0100 Subject: [PATCH 13/42] update flags description --- x/auth/client/cli/tx_sign.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 8e8092991813..fa0a5761aa3e 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -29,11 +29,10 @@ func GetSignBatchCommand() *cobra.Command { Short: "Sign transaction batch files", Long: `Sign batch files of transactions generated with --generate-only. The command processes list of transactions from file (one StdTx each line), generate -signed transactions or signatures and print their JSON encoding, delimited by '\n'. -As the signatures are generated, the command updates the sequence number accordingly. +signed transactions print their JSON encoding, delimited by '\n'. +As the signatures are generated, the command updates the account sequence number accordingly. -If the flag --signature-only flag is set, it will output a JSON representation -of the generated signature only. +If the --signature-only flag is set, it will output the signatures part only. The --offline flag makes sure that the client will not reach out to full node. As a result, the account and the sequence number queries will not be performed and @@ -163,12 +162,11 @@ func setOutputFile(cmd *cobra.Command) (func(), error) { func GetSignCommand() *cobra.Command { cmd := &cobra.Command{ Use: "sign [file]", - Short: "Sign transactions generated offline", - Long: `Sign transactions created with the --generate-only flag. + Short: "Sign transaction generated offline", + Long: `Sign transaction created with the --generate-only flag. It will read a transaction from [file], sign it, and print its JSON encoding. -If the flag --signature-only flag is set, it will output a JSON representation -of the signatures only. +If the --signature-only flag is set, it will output the signatures part only. The --offline flag makes sure that the client will not reach out to full node. As a result, the account and sequence number queries will not be performed and @@ -185,8 +183,8 @@ be generated via the 'multisign' command. } cmd.Flags().String(flagMultisig, "", "Address of the multisig account on behalf of which the transaction shall be signed") - cmd.Flags().Bool(flagOverwrite, false, "Append the signature to the existing ones. If disabled, old signatures would be overwritten. Ignored if --multisig is on") - cmd.Flags().Bool(flagSigOnly, false, "Print only the signature") + cmd.Flags().Bool(flagOverwrite, false, "Overwrite existing signatures with a new one. If disabled, new signature will be appended") + cmd.Flags().Bool(flagSigOnly, false, "Print only the signatures") cmd.Flags().String(flags.FlagOutputDocument, "", "The document will be written to the given file instead of STDOUT") cmd.Flags().String(flags.FlagChainID, "", "The network chain ID") cmd.Flags().Bool(flagAmino, false, "Generate Amino encoded JSON suitable for submiting to the txs REST endpoint") From b1e3cbd9c3bd9f2579d66b4f739844c76b823d90 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 9 Dec 2020 22:39:11 +0100 Subject: [PATCH 14/42] Add tx.Sign tests --- client/tx/tx.go | 2 +- client/tx/tx_test.go | 92 +++++++++++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index f9b1774fce55..d54293413ba0 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -420,7 +420,7 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo Sequence: txf.Sequence(), } var prevSignatures []signing.SignatureV2 - if overwriteSig { + if !overwriteSig { prevSignatures, err = txBuilder.GetTx().GetSignaturesV2() if err != nil { return err diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 323c0b6cd2c7..1a9499a3e034 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -10,6 +10,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keyring" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" txtypes "github.com/cosmos/cosmos-sdk/types/tx" @@ -121,49 +122,80 @@ func TestBuildUnsignedTx(t *testing.T) { } func TestSign(t *testing.T) { + requireT := require.New(t) path := hd.CreateHDPath(118, 0, 0).String() kr, err := keyring.New(t.Name(), "test", t.TempDir(), nil) - require.NoError(t, err) - - var from = "test_sign" - - _, seed, err := kr.NewMnemonic(from, keyring.English, path, hd.Secp256k1) - require.NoError(t, err) - require.NoError(t, kr.Delete(from)) + requireT.NoError(err) - info, err := kr.NewAccount(from, seed, "", path, hd.Secp256k1) - require.NoError(t, err) + var from1 = "test_key1" + var from2 = "test_key2" - txf := tx.Factory{}. - WithTxConfig(NewTestTxConfig()). - WithAccountNumber(50). - WithSequence(23). - WithFees("50stake"). - WithMemo("memo"). - WithChainID("test-chain") + // create a new key using a mnemonic generator and test if we can reuse seed to recreate that account + _, seed, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1) + requireT.NoError(err) + requireT.NoError(kr.Delete(from1)) + info1, _, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1) - msg := banktypes.NewMsgSend(info.GetAddress(), sdk.AccAddress("to"), nil) - txn, err := tx.BuildUnsignedTx(txf, msg) - require.NoError(t, err) + info2, err := kr.NewAccount(from2, seed, "", path, hd.Secp256k1) + requireT.NoError(err) + requireT.NotEqual(info1.GetPubKey().Bytes(), info2.GetPubKey().Bytes()) - t.Log("should failed if txf without keyring") - err = tx.Sign(txf, from, txn, true) - require.Error(t, err) + pubKey1 := info1.GetPubKey() + pubKey2 := info2.GetPubKey() + t.Log("Pub keys:", pubKey1, pubKey2) - txf = tx.Factory{}. - WithKeybase(kr). + txfNoKeybase := tx.Factory{}. WithTxConfig(NewTestTxConfig()). WithAccountNumber(50). WithSequence(23). WithFees("50stake"). WithMemo("memo"). WithChainID("test-chain") + txf := txfNoKeybase.WithKeybase(kr) + msg := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) + txn, err := tx.BuildUnsignedTx(txfNoKeybase, msg) + requireT.NoError(err) - t.Log("should succeed if txf with keyring") - err = tx.Sign(txf, from, txn, true) - require.NoError(t, err) + testCases := []struct { + name string + txf tx.Factory + from string + overwrite bool + expectedPKs []cryptotypes.PubKey + }{ + {"should fail if txf without keyring", + txfNoKeybase, from1, true, nil}, + {"should fail for non existing key", + txf, "unknown", true, nil}, + {"should succeed if txf with keyring", + txf, from1, true, []cryptotypes.PubKey{pubKey1}}, + /**** test overwrite ****/ + {"should succeed to append a second signature and not overwrite", + txf, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}}, + {"should succeed to overwrite a signature", + txf, from2, true, []cryptotypes.PubKey{pubKey2}}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err = tx.Sign(tc.txf, tc.from, txn, tc.overwrite) + if len(tc.expectedPKs) == 0 { + requireT.Error(err) + } else { + requireT.NoError(err) + } + testSigners(requireT, txn.GetTx(), tc.expectedPKs...) + }) + } +} - t.Log("should fail for non existing key") - err = tx.Sign(txf, "non_existing_key", txn, true) - require.Error(t, err) +func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes.PubKey) { + signers := tr.GetPubKeys() + require.Len(signers, len(pks)) + sigs, err := tr.GetSignaturesV2() + require.NoError(err) + require.Len(sigs, len(pks)) + for i := range pks { + require.True(signers[i].Equals(pks[i])) + require.True(sigs[i].PubKey.Equals(pks[i]), "Signature is signed with a wrong pubkey. Got: %s, expected: %s", sigs[i].PubKey, pks[i]) + } } From edc19dcd8c351bad2af5ebba94bea6dffbab26c9 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 00:16:09 +0100 Subject: [PATCH 15/42] fix grpc/server_test.go --- server/grpc/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/grpc/server_test.go b/server/grpc/server_test.go index 5648233f8f32..846ce31a5968 100644 --- a/server/grpc/server_test.go +++ b/server/grpc/server_test.go @@ -154,7 +154,7 @@ func (s *IntegrationTestSuite) TestGRPCServer_BroadcastTx() { WithSignMode(signing.SignMode_SIGN_MODE_DIRECT) // Sign Tx. - err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, false) + err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, false, true) s.Require().NoError(err) txBytes, err := val0.ClientCtx.TxConfig.TxEncoder()(txBuilder.GetTx()) From 8818f64601dd7434b8ed1601f550689170cd5d39 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 00:23:35 +0100 Subject: [PATCH 16/42] Update client/tx/tx.go Co-authored-by: Cory --- client/tx/tx.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index d54293413ba0..19ab73c0920d 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -377,7 +377,7 @@ func SignWithPrivKey( // Sign signs a given tx with a named key. The bytes signed over are canconical. // The resulting signature will be added to the transaction builder overwriting the previous -// ones if overwrite=true (otherwise, ne signature will be appended). +// ones if overwrite=true (otherwise, the signature will be appended). // An error is returned upon failure. func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig bool) error { if txf.keybase == nil { From 7e39258e4ca5f8db306e3e224c7ec5ee7319b490 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 01:04:34 +0100 Subject: [PATCH 17/42] changelog update --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de885ae240cb..ef26e06202d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,7 +105,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#7918](https://github.com/cosmos/cosmos-sdk/pull/7918) Add x/capability safety checks: * All outward facing APIs will now check that capability is not nil and name is not empty before performing any state-machine changes * `SetIndex` has been renamed to `InitializeIndex` -* (client/tx, x/auth/client) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) Added `overwrite` option to the `tx.Sign` and `client.SignTx` functions. ### Features From 3ed4d3675c56e1c2fb98457ece55c05618111f50 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 01:04:42 +0100 Subject: [PATCH 18/42] Add test to verify matching signatures --- client/tx/tx_test.go | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 1a9499a3e034..a84739f5264b 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -14,6 +14,7 @@ import ( "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" txtypes "github.com/cosmos/cosmos-sdk/types/tx" + signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" "github.com/cosmos/cosmos-sdk/x/auth/signing" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" ) @@ -157,24 +158,26 @@ func TestSign(t *testing.T) { requireT.NoError(err) testCases := []struct { - name string - txf tx.Factory - from string - overwrite bool - expectedPKs []cryptotypes.PubKey + name string + txf tx.Factory + from string + overwrite bool + expectedPKs []cryptotypes.PubKey + matchingSigs []int // if not nil, check matching signature against old ones. }{ {"should fail if txf without keyring", - txfNoKeybase, from1, true, nil}, + txfNoKeybase, from1, true, nil, nil}, {"should fail for non existing key", - txf, "unknown", true, nil}, + txf, "unknown", true, nil, nil}, {"should succeed if txf with keyring", - txf, from1, true, []cryptotypes.PubKey{pubKey1}}, + txf, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, /**** test overwrite ****/ {"should succeed to append a second signature and not overwrite", - txf, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}}, + txf, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, {"should succeed to overwrite a signature", - txf, from2, true, []cryptotypes.PubKey{pubKey2}}, + txf, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, } + var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { err = tx.Sign(tc.txf, tc.from, txn, tc.overwrite) @@ -183,12 +186,16 @@ func TestSign(t *testing.T) { } else { requireT.NoError(err) } - testSigners(requireT, txn.GetTx(), tc.expectedPKs...) + sigs := testSigners(requireT, txn.GetTx(), tc.expectedPKs...) + if tc.matchingSigs != nil { + requireT.Equal(prevSigs[tc.matchingSigs[0]], sigs[tc.matchingSigs[1]]) + } + prevSigs = sigs }) } } -func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes.PubKey) { +func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes.PubKey) []signingtypes.SignatureV2 { signers := tr.GetPubKeys() require.Len(signers, len(pks)) sigs, err := tr.GetSignaturesV2() @@ -198,4 +205,5 @@ func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes. require.True(signers[i].Equals(pks[i])) require.True(sigs[i].PubKey.Equals(pks[i]), "Signature is signed with a wrong pubkey. Got: %s, expected: %s", sigs[i].PubKey, pks[i]) } + return sigs } From cd6b700a15a1ab9be7c81f26f1b939d6dbeb9b5b Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 03:43:47 +0100 Subject: [PATCH 19/42] cli_test: add integration tests for sign CMD --- x/auth/client/cli/cli_test.go | 118 ++++++++++++++++++++++++---------- 1 file changed, 85 insertions(+), 33 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 17cd3c2c4309..99385c209f1d 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -2,6 +2,7 @@ package cli_test import ( "context" + "encoding/json" "fmt" "io/ioutil" "path/filepath" @@ -76,24 +77,11 @@ func (s *IntegrationTestSuite) TearDownSuite() { func (s *IntegrationTestSuite) TestCLIValidateSignatures() { val := s.network.Validators[0] - res, err := bankcli.MsgSendExec( - val.ClientCtx, - val.Address, - val.Address, - sdk.NewCoins( - sdk.NewCoin(fmt.Sprintf("%stoken", val.Moniker), sdk.NewInt(10)), - sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)), - ), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), - ) - s.Require().NoError(err) + res := s.createBankMsg(val, val.Address) // write unsigned tx to file unsignedTx := testutil.WriteToNewTempFile(s.T(), res.String()) - res, err = authtest.TxSignExec(val.ClientCtx, val.Address, unsignedTx.Name()) + res, err := authtest.TxSignExec(val.ClientCtx, val.Address, unsignedTx.Name()) s.Require().NoError(err) signedTx, err := val.ClientCtx.TxConfig.TxJSONDecoder()(res.Bytes()) s.Require().NoError(err) @@ -115,26 +103,11 @@ func (s *IntegrationTestSuite) TestCLIValidateSignatures() { func (s *IntegrationTestSuite) TestCLISignBatch() { val := s.network.Validators[0] - generatedStd, err := bankcli.MsgSendExec( - val.ClientCtx, - val.Address, - val.Address, - sdk.NewCoins( - sdk.NewCoin(fmt.Sprintf("%stoken", val.Moniker), sdk.NewInt(10)), - sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)), - ), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), - ) - - s.Require().NoError(err) - - // Write the output to disk + generatedStd := s.createBankMsg(val, val.Address) outputFile := testutil.WriteToNewTempFile(s.T(), strings.Repeat(generatedStd.String(), 3)) - // sign-batch file - offline is set but account-number and sequence are not val.ClientCtx.HomeDir = strings.Replace(val.ClientCtx.HomeDir, "simd", "simcli", 1) + + // sign-batch file - offline is set but account-number and sequence are not res, err := authtest.TxSignBatchExec(val.ClientCtx, val.Address, outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, val.ClientCtx.ChainID), "--offline") s.Require().EqualError(err, "required flag(s) \"account-number\", \"sequence\" not set") @@ -158,6 +131,67 @@ func (s *IntegrationTestSuite) TestCLISignBatch() { s.Require().Error(err) } +func (s *IntegrationTestSuite) TestCLISign() { + require := s.Require() + val1 := s.network.Validators[0] + txCfg := val1.ClientCtx.TxConfig + txBz := s.createBankMsg(val1, val1.Address) + // val.ClientCtx.HomeDir = strings.Replace(val.ClientCtx.HomeDir, "simd", "simcli", 1) + fileUnsigned := testutil.WriteToNewTempFile(s.T(), txBz.String()) + chainFlag := fmt.Sprintf("--%s=%s", flags.FlagChainID, val1.ClientCtx.ChainID) + sigOnlyFlag := "--signature-only" + + // SIC! validators have same key names and same adddresses as those registered in the keyring, + // BUT the keys are different! + valInfo, err := val1.ClientCtx.Keyring.Key(val1.Moniker) + require.NoError(err) + + /**** test signature-only ****/ + res, err := authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, + sigOnlyFlag) + require.NoError(err) + checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey()) + + /**** test full output ****/ + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag) + require.NoError(err) + + // txCfg.UnmarshalSignatureJSON can't unmarshal a fragment of the signature, so we create this structure. + type txFragment struct { + Signatures []json.RawMessage + } + var txOut txFragment + err = json.Unmarshal(res.Bytes(), &txOut) + require.NoError(err) + require.Len(txOut.Signatures, 1) + fileSigned := testutil.WriteToNewTempFile(s.T(), res.String()) + + /**** try to append to the previously signed transaction ****/ + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileSigned.Name(), chainFlag, + sigOnlyFlag) + checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey(), valInfo.GetPubKey()) + + /**** try to overwrite the previously signed transaction ****/ + + // We can't sign with other address, because the bank send message supports only one signer for a simple + // account. We may update this test with other message or multisig account. + // Changing the file is too much hacking, because TxDecoder returns sdk.Tx, which doesn't provide + // functionality to check / manage `auth_info` + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileSigned.Name(), chainFlag, + sigOnlyFlag, "--overwrite") + checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey()) +} + +func checkSignatures(require *require.Assertions, txCfg client.TxConfig, output []byte, pks ...cryptotypes.PubKey) { + sigs, err := txCfg.UnmarshalSignatureJSON(output) + require.NoError(err, string(output)) + require.Len(sigs, len(pks)) + for i := range pks { + require.True(sigs[i].PubKey.Equals(pks[i]), "Pub key doesn't match. Got: %s, expected: %s, idx: %d", sigs[i].PubKey, pks[i], i) + require.NotEmpty(sigs[i].Data) + } +} + func (s *IntegrationTestSuite) TestCLIQueryTxCmd() { val := s.network.Validators[0] @@ -955,6 +989,24 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() { s.Require().NoError(err) } +func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk.AccAddress) testutil.BufferWriter { + res, err := bankcli.MsgSendExec( + val.ClientCtx, + val.Address, + toAddr, + sdk.NewCoins( + sdk.NewCoin(fmt.Sprintf("%stoken", val.Moniker), sdk.NewInt(10)), + sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)), + ), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), + ) + s.Require().NoError(err) + return res +} + func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) } From fe3acef4d3bdf37f889494a0b6338c632a15294a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 13:18:26 +0100 Subject: [PATCH 20/42] add output-file flag test --- x/auth/client/cli/cli_test.go | 15 ++++++++++++--- x/auth/client/cli/tx_sign.go | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 99385c209f1d..82a69bdb548d 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -164,11 +164,20 @@ func (s *IntegrationTestSuite) TestCLISign() { err = json.Unmarshal(res.Bytes(), &txOut) require.NoError(err) require.Len(txOut.Signatures, 1) - fileSigned := testutil.WriteToNewTempFile(s.T(), res.String()) + + /**** test file output ****/ + filenameSigned := filepath.Join(s.T().TempDir(), "test_sign_out.json") + fileFlag := fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, filenameSigned) + _, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, fileFlag) + require.NoError(err) + fContent, err := ioutil.ReadFile(filenameSigned) + require.NoError(err) + require.Equal(res.String(), string(fContent)) /**** try to append to the previously signed transaction ****/ - res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileSigned.Name(), chainFlag, + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag, sigOnlyFlag) + require.NoError(err) checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey(), valInfo.GetPubKey()) /**** try to overwrite the previously signed transaction ****/ @@ -177,7 +186,7 @@ func (s *IntegrationTestSuite) TestCLISign() { // account. We may update this test with other message or multisig account. // Changing the file is too much hacking, because TxDecoder returns sdk.Tx, which doesn't provide // functionality to check / manage `auth_info` - res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileSigned.Name(), chainFlag, + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag, sigOnlyFlag, "--overwrite") checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey()) } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index fa0a5761aa3e..6c7d09bf3a64 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -294,7 +294,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { } }() - _, err = fp.Write(json) + _, err = fp.Write(append(json, '\n')) return } } From c2c74c65a6f2150c777a8a9acc2c71b19cf4b9c1 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 13:59:44 +0100 Subject: [PATCH 21/42] add flagAmino test --- x/auth/client/cli/cli_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 82a69bdb548d..00807db732d5 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -183,12 +183,20 @@ func (s *IntegrationTestSuite) TestCLISign() { /**** try to overwrite the previously signed transaction ****/ // We can't sign with other address, because the bank send message supports only one signer for a simple - // account. We may update this test with other message or multisig account. - // Changing the file is too much hacking, because TxDecoder returns sdk.Tx, which doesn't provide - // functionality to check / manage `auth_info` + // account. Changing the file is too much hacking, because TxDecoder returns sdk.Tx, which doesn't + // provide functionality to check / manage `auth_info`. + // Cases with different keys are are covered in unit tests of `tx.Sign`. res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag, sigOnlyFlag, "--overwrite") checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey()) + + /**** test flagAmino ****/ + res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag, + sigOnlyFlag, "--amino=true") + require.NoError(err) + err = json.Unmarshal(res.Bytes(), &txOut) + require.NoError(err) + require.Len(txOut.Signatures, 1) } func checkSignatures(require *require.Assertions, txCfg client.TxConfig, output []byte, pks ...cryptotypes.PubKey) { From 82d28b8c36a164f06ee77129366b59a5bd8005ba Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 15:27:26 +0100 Subject: [PATCH 22/42] Update x/auth/client/cli/tx_sign.go Co-authored-by: Alessio Treglia --- x/auth/client/cli/tx_sign.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 6c7d09bf3a64..e4ce81f86222 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -162,8 +162,8 @@ func setOutputFile(cmd *cobra.Command) (func(), error) { func GetSignCommand() *cobra.Command { cmd := &cobra.Command{ Use: "sign [file]", - Short: "Sign transaction generated offline", - Long: `Sign transaction created with the --generate-only flag. + Short: "Sign a transaction generated offline", + Long: `Sign a transaction created with the --generate-only flag. It will read a transaction from [file], sign it, and print its JSON encoding. If the --signature-only flag is set, it will output the signatures part only. From 87d027a845ac90c8c3d09869d3bcbdf6e1c2d2bd Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 10 Dec 2020 14:55:23 +0000 Subject: [PATCH 23/42] Update x/auth/client/cli/tx_sign.go --- x/auth/client/cli/tx_sign.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index e4ce81f86222..7b43731a4621 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -29,7 +29,7 @@ func GetSignBatchCommand() *cobra.Command { Short: "Sign transaction batch files", Long: `Sign batch files of transactions generated with --generate-only. The command processes list of transactions from file (one StdTx each line), generate -signed transactions print their JSON encoding, delimited by '\n'. +signed transactions or signatures and print their JSON encoding, delimited by '\n'. As the signatures are generated, the command updates the account sequence number accordingly. If the --signature-only flag is set, it will output the signatures part only. From b8256a83c09fcdd73dc6638a74cda98f238dee4e Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 14:44:32 +0100 Subject: [PATCH 24/42] update amino serialization test --- x/auth/client/cli/cli_test.go | 11 ++++++++--- x/auth/client/cli/tx_sign.go | 4 ++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 00807db732d5..5d600e897479 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -30,6 +30,7 @@ import ( "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" authcli "github.com/cosmos/cosmos-sdk/x/auth/client/cli" + authrest "github.com/cosmos/cosmos-sdk/x/auth/client/rest" authtest "github.com/cosmos/cosmos-sdk/x/auth/client/testutil" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/testutil" @@ -192,11 +193,15 @@ func (s *IntegrationTestSuite) TestCLISign() { /**** test flagAmino ****/ res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag, - sigOnlyFlag, "--amino=true") + "--amino=true") require.NoError(err) - err = json.Unmarshal(res.Bytes(), &txOut) + + var txAmino authrest.BroadcastReq + err = val1.ClientCtx.LegacyAmino.UnmarshalJSON(res.Bytes(), &txAmino) require.NoError(err) - require.Len(txOut.Signatures, 1) + require.Len(txAmino.Tx.Signatures, 2) + require.Equal(txAmino.Tx.Signatures[0].PubKey, valInfo.GetPubKey()) + require.Equal(txAmino.Tx.Signatures[1].PubKey, valInfo.GetPubKey()) } func checkSignatures(require *require.Assertions, txCfg client.TxConfig, output []byte, pks ...cryptotypes.PubKey) { diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 7b43731a4621..ad008e476f60 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -32,7 +32,7 @@ The command processes list of transactions from file (one StdTx each line), gene signed transactions or signatures and print their JSON encoding, delimited by '\n'. As the signatures are generated, the command updates the account sequence number accordingly. -If the --signature-only flag is set, it will output the signatures part only. +If the --signature-only flag is set, it will output the signature parts only. The --offline flag makes sure that the client will not reach out to full node. As a result, the account and the sequence number queries will not be performed and @@ -166,7 +166,7 @@ func GetSignCommand() *cobra.Command { Long: `Sign a transaction created with the --generate-only flag. It will read a transaction from [file], sign it, and print its JSON encoding. -If the --signature-only flag is set, it will output the signatures part only. +If the --signature-only flag is set, it will output the signature parts only. The --offline flag makes sure that the client will not reach out to full node. As a result, the account and sequence number queries will not be performed and From a00e8ec8f2881a613cd18272200f163776e48477 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Thu, 10 Dec 2020 15:42:43 +0100 Subject: [PATCH 25/42] TestSign: adding unit test for signing with different modes --- client/tx/tx_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index a84739f5264b..dd41d1e79396 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -151,8 +151,10 @@ func TestSign(t *testing.T) { WithSequence(23). WithFees("50stake"). WithMemo("memo"). - WithChainID("test-chain") + WithChainID("test-chain"). + WithSignMode(signingtypes.SignMode_SIGN_MODE_DIRECT) txf := txfNoKeybase.WithKeybase(kr) + txfAmino := txf.WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) msg := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) txn, err := tx.BuildUnsignedTx(txfNoKeybase, msg) requireT.NoError(err) @@ -172,10 +174,12 @@ func TestSign(t *testing.T) { {"should succeed if txf with keyring", txf, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, /**** test overwrite ****/ - {"should succeed to append a second signature and not overwrite", + {"should append a second signature and not overwrite", txf, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, - {"should succeed to overwrite a signature", + {"should overwrite a signature", txf, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, + {"should append a signature with different mode", + txfAmino, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, []int{0, 0}}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { From c0c1c481d4b857884ced02e7413707d2408d6aee Mon Sep 17 00:00:00 2001 From: Amaury Date: Fri, 11 Dec 2020 19:48:04 +0100 Subject: [PATCH 26/42] Add test with Multi Signers into Robert's TxSign PR (#8142) * Add test with Multi Signers * remove true false * Use SIGN_MODE_DIRECT * Fix litn * Use correct pubkeys * Correct accNum and seq * Use amino --- x/auth/client/cli/broadcast.go | 4 +++ x/auth/client/cli/cli_test.go | 58 +++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/x/auth/client/cli/broadcast.go b/x/auth/client/cli/broadcast.go index d234b0252163..0a728bb9b566 100644 --- a/x/auth/client/cli/broadcast.go +++ b/x/auth/client/cli/broadcast.go @@ -26,6 +26,10 @@ $ tx broadcast ./mytxn.json Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { clientCtx := client.GetClientContextFromCmd(cmd) + clientCtx, err := client.ReadTxCommandFlags(clientCtx, cmd.Flags()) + if err != nil { + return err + } if offline, _ := cmd.Flags().GetBool(flags.FlagOffline); offline { return errors.New("cannot broadcast tx during offline mode") diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 5d600e897479..2844addd6a26 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -48,7 +48,7 @@ func (s *IntegrationTestSuite) SetupSuite() { s.T().Log("setting up integration test suite") cfg := network.DefaultConfig() - cfg.NumValidators = 1 + cfg.NumValidators = 2 s.cfg = cfg s.network = network.New(s.T(), cfg) @@ -1029,6 +1029,62 @@ func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk. return res } +func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() { + val0, val1 := s.network.Validators[0], s.network.Validators[1] + + val0Coin := sdk.NewCoin(fmt.Sprintf("%stoken", val0.Moniker), sdk.NewInt(10)) + val1Coin := sdk.NewCoin(fmt.Sprintf("%stoken", val1.Moniker), sdk.NewInt(10)) + + _, _, addr1 := testdata.KeyTestPubAddr() + + // Creating a tx with 2 msgs from 2 signers: val0 and val1. + // The validators sign with SIGN_MODE_LEGACY_AMINO_JSON by default. + // Since we we amino, we don't need to pre-populate signer_infos. + txBuilder := val0.ClientCtx.TxConfig.NewTxBuilder() + txBuilder.SetMsgs( + banktypes.NewMsgSend(val0.Address, addr1, sdk.NewCoins(val0Coin)), + banktypes.NewMsgSend(val1.Address, addr1, sdk.NewCoins(val1Coin)), + ) + txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)))) + txBuilder.SetGasLimit(testdata.NewTestGasLimit()) + s.Require().Equal([]sdk.AccAddress{val0.Address, val1.Address}, txBuilder.GetTx().GetSigners()) + + // Write the unsigned tx into a file. + txJSON, err := val0.ClientCtx.TxConfig.TxJSONEncoder()(txBuilder.GetTx()) + s.Require().NoError(err) + unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON)) + + // Let val0 sign first the file with the unsignedTx. + signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite") + s.Require().NoError(err) + signedByVal0File := testutil.WriteToNewTempFile(s.T(), signedByVal0.String()) + + // Then let val1 sign the file with signedByVal0. + val1AccNum, val1Seq, err := val0.ClientCtx.AccountRetriever.GetAccountNumberSequence(val0.ClientCtx, val1.Address) + s.Require().NoError(err) + signedTx, err := authtest.TxSignExec( + val1.ClientCtx, val1.Address, signedByVal0File.Name(), + "--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq), + ) + s.Require().NoError(err) + signedTxFile := testutil.WriteToNewTempFile(s.T(), signedTx.String()) + + // Now let's try to send this tx. + res, err := authtest.TxBroadcastExec(val0.ClientCtx, signedTxFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock)) + s.Require().NoError(err) + var txRes sdk.TxResponse + s.Require().NoError(val0.ClientCtx.JSONMarshaler.UnmarshalJSON(res.Bytes(), &txRes)) + s.Require().Equal(uint32(0), txRes.Code) + + // Make sure the addr1's balance got funded. + queryResJSON, err := bankcli.QueryBalancesExec(val0.ClientCtx, addr1) + s.Require().NoError(err) + var queryRes banktypes.QueryAllBalancesResponse + err = val0.ClientCtx.JSONMarshaler.UnmarshalJSON(queryResJSON.Bytes(), &queryRes) + s.Require().NoError(err) + s.Require().Equal(sdk.NewCoins(val0Coin, val1Coin), queryRes.Balances) +} + func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) } From 6405295ee01ec0a03499bc5f409b7c26c3237b08 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Fri, 11 Dec 2020 20:05:23 +0100 Subject: [PATCH 27/42] cleanups --- x/auth/client/cli/cli_test.go | 72 +++++++++++++++++------------------ 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 2844addd6a26..d30da2c287bb 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -474,7 +474,6 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() { func (s *IntegrationTestSuite) TestCLIMultisignInsufficientCosigners() { val1 := s.network.Validators[0] - codec := codec2.NewLegacyAmino() sdk.RegisterLegacyAminoCodec(codec) banktypes.RegisterLegacyAminoCodec(codec) @@ -555,17 +554,14 @@ func (s *IntegrationTestSuite) TestCLIEncode() { fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), "--memo", "deadbeef", ) s.Require().NoError(err) - - // Save tx to file savedTxFile := testutil.WriteToNewTempFile(s.T(), normalGeneratedTx.String()) // Encode encodeExec, err := authtest.TxEncodeExec(val1.ClientCtx, savedTxFile.Name()) s.Require().NoError(err) - trimmedBase64 := strings.Trim(encodeExec.String(), "\"\n") - // Check that the transaction decodes as expected + // Check that the transaction decodes as expected decodedTx, err := authtest.TxDecodeExec(val1.ClientCtx, trimmedBase64) s.Require().NoError(err) @@ -1005,36 +1001,22 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() { // Broadcast tx, test that it shouldn't panic. val1.ClientCtx.BroadcastMode = flags.BroadcastSync out, err := authtest.TxBroadcastExec(val1.ClientCtx, signedTxFile.Name()) + s.Require().NoError(err) var res sdk.TxResponse s.Require().NoError(val1.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &res)) s.Require().NotEqual(0, res.Code) - s.Require().NoError(err) -} - -func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk.AccAddress) testutil.BufferWriter { - res, err := bankcli.MsgSendExec( - val.ClientCtx, - val.Address, - toAddr, - sdk.NewCoins( - sdk.NewCoin(fmt.Sprintf("%stoken", val.Moniker), sdk.NewInt(10)), - sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)), - ), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), - ) - s.Require().NoError(err) - return res } func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() { - val0, val1 := s.network.Validators[0], s.network.Validators[1] + // test case: + // Create a transaction with 2 messages which has to be signed with 2 different keys + // Sign and append the signatures using the CLI with Amino signing mode. + // Finally send the transaction to the blockchain. It should work. + require := s.Require() + val0, val1 := s.network.Validators[0], s.network.Validators[1] val0Coin := sdk.NewCoin(fmt.Sprintf("%stoken", val0.Moniker), sdk.NewInt(10)) val1Coin := sdk.NewCoin(fmt.Sprintf("%stoken", val1.Moniker), sdk.NewInt(10)) - _, _, addr1 := testdata.KeyTestPubAddr() // Creating a tx with 2 msgs from 2 signers: val0 and val1. @@ -1047,42 +1029,60 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() { ) txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)))) txBuilder.SetGasLimit(testdata.NewTestGasLimit()) - s.Require().Equal([]sdk.AccAddress{val0.Address, val1.Address}, txBuilder.GetTx().GetSigners()) + require.Equal([]sdk.AccAddress{val0.Address, val1.Address}, txBuilder.GetTx().GetSigners()) // Write the unsigned tx into a file. txJSON, err := val0.ClientCtx.TxConfig.TxJSONEncoder()(txBuilder.GetTx()) - s.Require().NoError(err) + require.NoError(err) unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON)) // Let val0 sign first the file with the unsignedTx. signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite") - s.Require().NoError(err) + require.NoError(err) signedByVal0File := testutil.WriteToNewTempFile(s.T(), signedByVal0.String()) // Then let val1 sign the file with signedByVal0. val1AccNum, val1Seq, err := val0.ClientCtx.AccountRetriever.GetAccountNumberSequence(val0.ClientCtx, val1.Address) - s.Require().NoError(err) + require.NoError(err) signedTx, err := authtest.TxSignExec( val1.ClientCtx, val1.Address, signedByVal0File.Name(), "--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq), ) - s.Require().NoError(err) + require.NoError(err) signedTxFile := testutil.WriteToNewTempFile(s.T(), signedTx.String()) // Now let's try to send this tx. res, err := authtest.TxBroadcastExec(val0.ClientCtx, signedTxFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock)) - s.Require().NoError(err) + require.NoError(err) var txRes sdk.TxResponse - s.Require().NoError(val0.ClientCtx.JSONMarshaler.UnmarshalJSON(res.Bytes(), &txRes)) - s.Require().Equal(uint32(0), txRes.Code) + require.NoError(val0.ClientCtx.JSONMarshaler.UnmarshalJSON(res.Bytes(), &txRes)) + require.Equal(uint32(0), txRes.Code) // Make sure the addr1's balance got funded. queryResJSON, err := bankcli.QueryBalancesExec(val0.ClientCtx, addr1) - s.Require().NoError(err) + require.NoError(err) var queryRes banktypes.QueryAllBalancesResponse err = val0.ClientCtx.JSONMarshaler.UnmarshalJSON(queryResJSON.Bytes(), &queryRes) + require.NoError(err) + require.Equal(sdk.NewCoins(val0Coin, val1Coin), queryRes.Balances) +} + +func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk.AccAddress) testutil.BufferWriter { + res, err := bankcli.MsgSendExec( + val.ClientCtx, + val.Address, + toAddr, + sdk.NewCoins( + sdk.NewCoin(fmt.Sprintf("%stoken", val.Moniker), sdk.NewInt(10)), + sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)), + ), + fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), + fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), + fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), + fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), + ) s.Require().NoError(err) - s.Require().Equal(sdk.NewCoins(val0Coin, val1Coin), queryRes.Balances) + return res } func TestIntegrationTestSuite(t *testing.T) { From d61f340ab1f9a7b27237ef6d4c28a8afdafe180a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Fri, 11 Dec 2020 21:30:03 +0100 Subject: [PATCH 28/42] client.Sign: raise error when signing tx with multiple signers in Direct + added more unit tests --- client/tx/tx.go | 14 +++++++++++++- client/tx/tx_test.go | 34 ++++++++++++++++++---------------- types/errors/errors.go | 4 ++++ 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/client/tx/tx.go b/client/tx/tx.go index 19ab73c0920d..ff941df21fbe 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -375,9 +375,19 @@ func SignWithPrivKey( return sigV2, nil } +func checkMultipleSigners(mode signing.SignMode, tx authsigning.Tx) error { + if mode == signing.SignMode_SIGN_MODE_DIRECT && + len(tx.GetSigners()) > 1 { + return sdkerrors.Wrap(sdkerrors.ErrNotSupported, "Signing in DIRECT mode is only supported for transactions with one signer only") + } + return nil +} + // Sign signs a given tx with a named key. The bytes signed over are canconical. // The resulting signature will be added to the transaction builder overwriting the previous // ones if overwrite=true (otherwise, the signature will be appended). +// Signing a transaction with mutltiple signers in the DIRECT mode is not supprted and will +// return an error. // An error is returned upon failure. func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig bool) error { if txf.keybase == nil { @@ -389,12 +399,14 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo // use the SignModeHandler's default mode if unspecified signMode = txf.txConfig.SignModeHandler().DefaultMode() } + if err := checkMultipleSigners(signMode, txBuilder.GetTx()); err != nil { + return err + } key, err := txf.keybase.Key(name) if err != nil { return err } - pubKey := key.GetPubKey() signerData := authsigning.SignerData{ ChainID: txf.chainID, diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index dd41d1e79396..5c25106f81c1 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -151,12 +151,14 @@ func TestSign(t *testing.T) { WithSequence(23). WithFees("50stake"). WithMemo("memo"). - WithChainID("test-chain"). + WithChainID("test-chain") + txfDirect := txfNoKeybase. + WithKeybase(kr). WithSignMode(signingtypes.SignMode_SIGN_MODE_DIRECT) - txf := txfNoKeybase.WithKeybase(kr) - txfAmino := txf.WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) + txfAmino := txfDirect. + WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) msg := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) - txn, err := tx.BuildUnsignedTx(txfNoKeybase, msg) + txb, err := tx.BuildUnsignedTx(txfNoKeybase, msg) requireT.NoError(err) testCases := []struct { @@ -170,27 +172,27 @@ func TestSign(t *testing.T) { {"should fail if txf without keyring", txfNoKeybase, from1, true, nil, nil}, {"should fail for non existing key", - txf, "unknown", true, nil, nil}, - {"should succeed if txf with keyring", - txf, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, - /**** test overwrite ****/ - {"should append a second signature and not overwrite", - txf, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, - {"should overwrite a signature", - txf, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, - {"should append a signature with different mode", - txfAmino, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, []int{0, 0}}, + txfAmino, "unknown", true, nil, nil}, + {"should succeed with keyring Amino", + txfAmino, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, + /**** test overwrite AMINO ****/ + {"should append a second signature and not overwrite Amino", + txfAmino, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, + {"should overwrite a signature Amino", + txfAmino, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, + {"should append a signature with different mode Amino", + txfDirect, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, []int{0, 0}}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = tx.Sign(tc.txf, tc.from, txn, tc.overwrite) + err = tx.Sign(tc.txf, tc.from, txb, tc.overwrite) if len(tc.expectedPKs) == 0 { requireT.Error(err) } else { requireT.NoError(err) } - sigs := testSigners(requireT, txn.GetTx(), tc.expectedPKs...) + sigs := testSigners(requireT, txb.GetTx(), tc.expectedPKs...) if tc.matchingSigs != nil { requireT.Equal(prevSigs[tc.matchingSigs[0]], sigs[tc.matchingSigs[1]]) } diff --git a/types/errors/errors.go b/types/errors/errors.go index 0c949208217e..d08412524c40 100644 --- a/types/errors/errors.go +++ b/types/errors/errors.go @@ -131,6 +131,10 @@ var ( // the same resource and one of them fails. ErrConflict = Register(RootCodespace, 36, "conflict") + // ErrNotSupported is returned when we call a branch of a code which is currently not + // supported. + ErrNotSupported = Register(RootCodespace, 37, "feature not supported") + // ErrPanic is only set when we recover from a panic, so we know to // redact potentially sensitive system info ErrPanic = Register(UndefinedCodespace, 111222, "panic") From 8cbcff15755a7072f2bc32f1ab000ea62e5e56b4 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Fri, 11 Dec 2020 21:40:11 +0100 Subject: [PATCH 29/42] add more tests --- client/tx/tx_test.go | 57 ++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 5c25106f81c1..922d625d32cc 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -136,13 +136,14 @@ func TestSign(t *testing.T) { requireT.NoError(err) requireT.NoError(kr.Delete(from1)) info1, _, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1) + requireT.NoError(err) info2, err := kr.NewAccount(from2, seed, "", path, hd.Secp256k1) requireT.NoError(err) - requireT.NotEqual(info1.GetPubKey().Bytes(), info2.GetPubKey().Bytes()) pubKey1 := info1.GetPubKey() pubKey2 := info2.GetPubKey() + requireT.NotEqual(pubKey1.Bytes(), pubKey2.Bytes()) t.Log("Pub keys:", pubKey1, pubKey2) txfNoKeybase := tx.Factory{}. @@ -157,58 +158,74 @@ func TestSign(t *testing.T) { WithSignMode(signingtypes.SignMode_SIGN_MODE_DIRECT) txfAmino := txfDirect. WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) - msg := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) - txb, err := tx.BuildUnsignedTx(txfNoKeybase, msg) + msg1 := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil) + msg2 := banktypes.NewMsgSend(info2.GetAddress(), sdk.AccAddress("to"), nil) + txb, err := tx.BuildUnsignedTx(txfNoKeybase, msg1, msg2) + requireT.NoError(err) + txbSimple, err := tx.BuildUnsignedTx(txfNoKeybase, msg2) requireT.NoError(err) testCases := []struct { name string txf tx.Factory + txb client.TxBuilder from string overwrite bool expectedPKs []cryptotypes.PubKey matchingSigs []int // if not nil, check matching signature against old ones. }{ {"should fail if txf without keyring", - txfNoKeybase, from1, true, nil, nil}, + txfNoKeybase, txb, from1, true, nil, nil}, {"should fail for non existing key", - txfAmino, "unknown", true, nil, nil}, + txfAmino, txb, "unknown", true, nil, nil}, {"should succeed with keyring Amino", - txfAmino, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, - /**** test overwrite AMINO ****/ + txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, + + /**** test double sign AMINO ****/ + {"should sign tx2 Amino", + txfAmino, txb, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, {"should append a second signature and not overwrite Amino", - txfAmino, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, + txfAmino, txb, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, {"should overwrite a signature Amino", - txfAmino, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, - {"should append a signature with different mode Amino", - txfDirect, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, []int{0, 0}}, + txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, + {"should fail to append a signature with different mode Direct", + txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, + + /**** test double sign DIRECT + signing transaction with more than 2 signers should fail in DIRECT mode ****/ + {"should fail to sign tx2 DIRECT", + txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, + {"should fail to overwrite tx2 DIRECT", + txfDirect, txb, from1, true, []cryptotypes.PubKey{}, nil}, + + /**** test simple DIRECT ****/ + {"should succeed with keyring DIRECT", + txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = tx.Sign(tc.txf, tc.from, txb, tc.overwrite) + err = tx.Sign(tc.txf, tc.from, tc.txb, tc.overwrite) if len(tc.expectedPKs) == 0 { requireT.Error(err) } else { requireT.NoError(err) + sigs := testSigners(requireT, tc.txb.GetTx(), tc.expectedPKs...) + if tc.matchingSigs != nil { + requireT.Equal(prevSigs[tc.matchingSigs[0]], sigs[tc.matchingSigs[1]]) + } + prevSigs = sigs } - sigs := testSigners(requireT, txb.GetTx(), tc.expectedPKs...) - if tc.matchingSigs != nil { - requireT.Equal(prevSigs[tc.matchingSigs[0]], sigs[tc.matchingSigs[1]]) - } - prevSigs = sigs }) } } func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes.PubKey) []signingtypes.SignatureV2 { - signers := tr.GetPubKeys() - require.Len(signers, len(pks)) sigs, err := tr.GetSignaturesV2() + require.Len(sigs, len(pks)) require.NoError(err) require.Len(sigs, len(pks)) for i := range pks { - require.True(signers[i].Equals(pks[i])) require.True(sigs[i].PubKey.Equals(pks[i]), "Signature is signed with a wrong pubkey. Got: %s, expected: %s", sigs[i].PubKey, pks[i]) } return sigs From 25e49156b8dda2d655f04903913dd4e444d850d5 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Sat, 12 Dec 2020 13:04:37 +0100 Subject: [PATCH 30/42] Update client/tx/tx_test.go Co-authored-by: Cory --- client/tx/tx_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 922d625d32cc..65a044130466 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -200,7 +200,7 @@ func TestSign(t *testing.T) { /**** test simple DIRECT ****/ {"should succeed with keyring DIRECT", - txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, + txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { From 17f210b2c43aa64162712b78e6993d4a2ab8ba20 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Sat, 12 Dec 2020 13:55:52 +0100 Subject: [PATCH 31/42] fix TestGetBroadcastCommand_WithoutOfflineFlag --- x/auth/client/cli/cli_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index d30da2c287bb..37cd90fece59 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -889,7 +889,6 @@ func TestGetBroadcastCommand_WithoutOfflineFlag(t *testing.T) { cmd := authcli.GetBroadcastCommand() _, out := testutil.ApplyMockIO(cmd) - testDir := t.TempDir() // Create new file with tx builder := txCfg.NewTxBuilder() @@ -901,13 +900,14 @@ func TestGetBroadcastCommand_WithoutOfflineFlag(t *testing.T) { err = builder.SetMsgs(banktypes.NewMsgSend(from, to, sdk.Coins{sdk.NewInt64Coin("stake", 10000)})) require.NoError(t, err) txContents, err := txCfg.TxJSONEncoder()(builder.GetTx()) - txFileName := filepath.Join(testDir, "tx.json") - err = ioutil.WriteFile(txFileName, txContents, 0644) require.NoError(t, err) + txFile := testutil.WriteToNewTempFile(t, string(txContents)) - cmd.SetArgs([]string{txFileName}) - require.Error(t, cmd.ExecuteContext(ctx)) - require.Contains(t, out.String(), "unsupported return type ; supported types: sync, async, block") + cmd.SetArgs([]string{txFile.Name()}) + err = cmd.ExecuteContext(ctx) + require.Error(t, err) + require.Contains(t, err.Error(), "connect: connection refused") + require.Contains(t, out.String(), "connect: connection refused") } func (s *IntegrationTestSuite) TestQueryParamsCmd() { From 46b50b2bedee11127474fb3f1857cd1ae77186ca Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 18:02:33 +0100 Subject: [PATCH 32/42] Any.UnsafeSetCachedValue --- codec/types/any.go | 19 +++++++++++++++++++ types/tx/types.go | 4 ++++ x/auth/tx/builder.go | 27 ++++++++++++--------------- x/bank/client/cli/cli_test.go | 1 - 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/codec/types/any.go b/codec/types/any.go index 4a30ddad4d0f..d2b015f5c649 100644 --- a/codec/types/any.go +++ b/codec/types/any.go @@ -69,6 +69,19 @@ func NewAnyWithValue(value proto.Message) (*Any, error) { return any, nil } +// TODO: this creates an import cycle +// func NewAnyFromServiceMsg(msg sdk.ServiceMsg) (*Any, error) { +// bz, err := proto.Marshal(msg.Request) +// if err != nil { +// return err +// } +// return &Any{ +// TypeUrl: msg.MethodName, +// Value: bz, +// cachedValue: msg, +// } +// } + // Pack packs the value x in the Any or returns an error. This also caches // the packed value so that it can be retrieved from GetCachedValue without // unmarshaling @@ -127,6 +140,12 @@ func (any *Any) ClearCachedValue() { any.cachedValue = nil } +// UnsafeSetCachedValue sets cached value to a given interface. +// This method is for internal use only! +func (any *Any) UnsafeSetCachedValue(x interface{}) { + any.cachedValue = x +} + // IntoAny represents a type that can be wrapped into an Any. type IntoAny interface { AsAny() *Any diff --git a/types/tx/types.go b/types/tx/types.go index c572d448a898..0392b2fcfd08 100644 --- a/types/tx/types.go +++ b/types/tx/types.go @@ -28,6 +28,10 @@ func (t *Tx) GetMsgs() []sdk.Msg { for i, any := range anys { var msg sdk.Msg if isServiceMsg(any.TypeUrl) { + req := any.GetCachedValue() + if req == nil { + panic("Any cached value is nil. Transaction messages must be correctly packed Any values.") + } msg = sdk.ServiceMsg{ MethodName: any.TypeUrl, Request: any.GetCachedValue().(sdk.MsgRequest), diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index fae3c3d4fec4..a973d853fc89 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -200,25 +200,22 @@ func (w *wrapper) SetMsgs(msgs ...sdk.Msg) error { var err error switch msg := msg.(type) { case sdk.ServiceMsg: - { - bz, err := proto.Marshal(msg.Request) - if err != nil { - return err - } - anys[i] = &codectypes.Any{ - TypeUrl: msg.MethodName, - Value: bz, - } + // anys[i], err = codectypes.NewAnyFromServiceMsgNewAnyWithValue(msg) + bz, err := proto.Marshal(msg.Request) + if err != nil { + return err + } + anys[i] = &codectypes.Any{ + TypeUrl: msg.MethodName, + Value: bz, } + anys[i].UnsafeSetCachedValue(msg) default: - { - anys[i], err = codectypes.NewAnyWithValue(msg) - if err != nil { - return err - } + anys[i], err = codectypes.NewAnyWithValue(msg) + if err != nil { + return err } } - } w.tx.Body.Messages = anys diff --git a/x/bank/client/cli/cli_test.go b/x/bank/client/cli/cli_test.go index 9b8c5cf612ad..a8e94d24569f 100644 --- a/x/bank/client/cli/cli_test.go +++ b/x/bank/client/cli/cli_test.go @@ -335,7 +335,6 @@ func (s *IntegrationTestSuite) TestBankMsgService() { s.Run(tc.name, func() { clientCtx := val.ClientCtx - bz, err := banktestutil.ServiceMsgSendExec(clientCtx, tc.from, tc.to, tc.amount, tc.args...) if tc.expectErr { s.Require().Error(err) From 8e55e135e3be5e957ba33a4a636311140e3850b0 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 19:22:01 +0100 Subject: [PATCH 33/42] fix note packed messages in tx builder --- codec/types/any.go | 24 ++++++++++++------------ x/auth/tx/builder.go | 11 +---------- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/codec/types/any.go b/codec/types/any.go index d2b015f5c649..de834d168a0e 100644 --- a/codec/types/any.go +++ b/codec/types/any.go @@ -69,18 +69,18 @@ func NewAnyWithValue(value proto.Message) (*Any, error) { return any, nil } -// TODO: this creates an import cycle -// func NewAnyFromServiceMsg(msg sdk.ServiceMsg) (*Any, error) { -// bz, err := proto.Marshal(msg.Request) -// if err != nil { -// return err -// } -// return &Any{ -// TypeUrl: msg.MethodName, -// Value: bz, -// cachedValue: msg, -// } -// } +// NewAnyWithCustomTypeURL same as NewAnyWithValue, but sets a custom type url, instead +// using the one from proto.Message. +// NOTE: This functions should be only used for types with additional logic bundled +// into the protobuf Any serialization. For simple marshaling you should use NewAnyWithValue. +func NewAnyWithCustomTypeURL(v proto.Message, typeUrl string) (*Any, error) { + bz, err := proto.Marshal(v) + return &Any{ + TypeUrl: typeUrl, + Value: bz, + cachedValue: v, + }, err +} // Pack packs the value x in the Any or returns an error. This also caches // the packed value so that it can be retrieved from GetCachedValue without diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index a973d853fc89..03abcae637f8 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -200,16 +200,7 @@ func (w *wrapper) SetMsgs(msgs ...sdk.Msg) error { var err error switch msg := msg.(type) { case sdk.ServiceMsg: - // anys[i], err = codectypes.NewAnyFromServiceMsgNewAnyWithValue(msg) - bz, err := proto.Marshal(msg.Request) - if err != nil { - return err - } - anys[i] = &codectypes.Any{ - TypeUrl: msg.MethodName, - Value: bz, - } - anys[i].UnsafeSetCachedValue(msg) + anys[i], err = codectypes.NewAnyWithCustomTypeURL(msg.Request, msg.MethodName) default: anys[i], err = codectypes.NewAnyWithValue(msg) if err != nil { From a263570c5b23b4b420ffe2690fc040bdc1d8d3e9 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 19:01:50 +0100 Subject: [PATCH 34/42] reorder unit tests --- client/tx/tx_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 65a044130466..9b9a32df52b4 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -180,6 +180,8 @@ func TestSign(t *testing.T) { txfAmino, txb, "unknown", true, nil, nil}, {"should succeed with keyring Amino", txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, + {"should succeed with keyring DIRECT", + txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, /**** test double sign AMINO ****/ {"should sign tx2 Amino", @@ -197,10 +199,6 @@ func TestSign(t *testing.T) { txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, {"should fail to overwrite tx2 DIRECT", txfDirect, txb, from1, true, []cryptotypes.PubKey{}, nil}, - - /**** test simple DIRECT ****/ - {"should succeed with keyring DIRECT", - txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { From 68d9b37691e251ba288330586781996ce2b47b4f Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 19:35:44 +0100 Subject: [PATCH 35/42] Changelog update --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef26e06202d2..134ee61bc78f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking Changes -* (x/upgrade) [\#7979](https://github.com/cosmos/cosmos-sdk/pull/7979) keeper pubkey storage serialization migration from bech32 to protobuf. +* (x/upgrade) [\#7632](https://github.com/cosmos/cosmos-sdk/issues/7632) keeper pubkey storage serialization migration from bech32 to protobuf. + + +### Features +* (codec/types) []() Adding `NewAnyWithCustomTypeURL` to correctly marshal Messages in TxBuilder. ### Bug Fixes From febbca98cc281b79e314603a83d938c16b5427dc Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 19:49:31 +0100 Subject: [PATCH 36/42] cleaning / linting --- client/tx/tx_test.go | 22 +++++++++++----------- codec/types/any.go | 4 ++-- x/auth/client/cli/cli_test.go | 1 - x/auth/tx/builder.go | 6 +++--- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 9b9a32df52b4..f6e7e2137c36 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -178,26 +178,26 @@ func TestSign(t *testing.T) { txfNoKeybase, txb, from1, true, nil, nil}, {"should fail for non existing key", txfAmino, txb, "unknown", true, nil, nil}, - {"should succeed with keyring Amino", + {"amino: should succeed with keyring", txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, - {"should succeed with keyring DIRECT", + {"direct: should succeed with keyring", txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, - /**** test double sign AMINO ****/ - {"should sign tx2 Amino", + /**** test double sign Amino mode ****/ + {"amino: should sign multi-signers tx", txfAmino, txb, from1, true, []cryptotypes.PubKey{pubKey1}, nil}, - {"should append a second signature and not overwrite Amino", + {"amino: should append a second signature and not overwrite", txfAmino, txb, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}}, - {"should overwrite a signature Amino", + {"amino: should overwrite a signature", txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}}, - {"should fail to append a signature with different mode Direct", - txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, - /**** test double sign DIRECT + /**** test double sign Direct mode signing transaction with more than 2 signers should fail in DIRECT mode ****/ - {"should fail to sign tx2 DIRECT", + {"direct: should fail to append a signature with different mode", + txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, + {"direct: should fail to sign multi-signers tx", txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, - {"should fail to overwrite tx2 DIRECT", + {"direct: should fail to overwrite multi-signers tx", txfDirect, txb, from1, true, []cryptotypes.PubKey{}, nil}, } var prevSigs []signingtypes.SignatureV2 diff --git a/codec/types/any.go b/codec/types/any.go index de834d168a0e..ce7f43f3c5f4 100644 --- a/codec/types/any.go +++ b/codec/types/any.go @@ -73,10 +73,10 @@ func NewAnyWithValue(value proto.Message) (*Any, error) { // using the one from proto.Message. // NOTE: This functions should be only used for types with additional logic bundled // into the protobuf Any serialization. For simple marshaling you should use NewAnyWithValue. -func NewAnyWithCustomTypeURL(v proto.Message, typeUrl string) (*Any, error) { +func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) { bz, err := proto.Marshal(v) return &Any{ - TypeUrl: typeUrl, + TypeUrl: typeURL, Value: bz, cachedValue: v, }, err diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 37cd90fece59..042b26dc20d7 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -137,7 +137,6 @@ func (s *IntegrationTestSuite) TestCLISign() { val1 := s.network.Validators[0] txCfg := val1.ClientCtx.TxConfig txBz := s.createBankMsg(val1, val1.Address) - // val.ClientCtx.HomeDir = strings.Replace(val.ClientCtx.HomeDir, "simd", "simcli", 1) fileUnsigned := testutil.WriteToNewTempFile(s.T(), txBz.String()) chainFlag := fmt.Sprintf("--%s=%s", flags.FlagChainID, val1.ClientCtx.ChainID) sigOnlyFlag := "--signature-only" diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index 03abcae637f8..18a555f9099e 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -203,9 +203,9 @@ func (w *wrapper) SetMsgs(msgs ...sdk.Msg) error { anys[i], err = codectypes.NewAnyWithCustomTypeURL(msg.Request, msg.MethodName) default: anys[i], err = codectypes.NewAnyWithValue(msg) - if err != nil { - return err - } + } + if err != nil { + return err } } From ae2ce73a0672710e75e4b9c47ac28fb818eb8d76 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 20:18:20 +0100 Subject: [PATCH 37/42] cli_tes: copy validator object instead of modifying it's shared codec --- x/auth/client/cli/cli_test.go | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 042b26dc20d7..6b17adb839b2 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -15,7 +15,6 @@ import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/flags" - codec2 "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keyring" @@ -472,11 +471,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() { } func (s *IntegrationTestSuite) TestCLIMultisignInsufficientCosigners() { - val1 := s.network.Validators[0] - codec := codec2.NewLegacyAmino() - sdk.RegisterLegacyAminoCodec(codec) - banktypes.RegisterLegacyAminoCodec(codec) - val1.ClientCtx.LegacyAmino = codec + val1 := s.validatorWithCustomCodec(0) // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -573,12 +568,7 @@ func (s *IntegrationTestSuite) TestCLIEncode() { } func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { - val1 := s.network.Validators[0] - - codec := codec2.NewLegacyAmino() - sdk.RegisterLegacyAminoCodec(codec) - banktypes.RegisterLegacyAminoCodec(codec) - val1.ClientCtx.LegacyAmino = codec + val1 := s.validatorWithCustomCodec(0) // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -670,12 +660,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { } func (s *IntegrationTestSuite) TestCLIMultisign() { - val1 := s.network.Validators[0] - - codec := codec2.NewLegacyAmino() - sdk.RegisterLegacyAminoCodec(codec) - banktypes.RegisterLegacyAminoCodec(codec) - val1.ClientCtx.LegacyAmino = codec + val1 := s.validatorWithCustomCodec(0) // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -1084,6 +1069,15 @@ func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk. return res } +func (s *IntegrationTestSuite) validatorWithCustomCodec(idx int) network.Validator { + val := *s.network.Validators[0] + // codec := codec2.NewLegacyAmino() + // sdk.RegisterLegacyAminoCodec(codec) + // banktypes.RegisterLegacyAminoCodec(codec) + // val.ClientCtx.LegacyAmino = codec + return val +} + func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) } From 1407caad3e0da8ccd369f16ae3b17d4c5b45dd96 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 20:26:40 +0100 Subject: [PATCH 38/42] x/auth cli_test: remove custom codec creation in tests --- x/auth/client/cli/cli_test.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 6b17adb839b2..6a6a9b1d559e 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -471,7 +471,7 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() { } func (s *IntegrationTestSuite) TestCLIMultisignInsufficientCosigners() { - val1 := s.validatorWithCustomCodec(0) + val1 := *s.network.Validators[0] // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -568,7 +568,7 @@ func (s *IntegrationTestSuite) TestCLIEncode() { } func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { - val1 := s.validatorWithCustomCodec(0) + val1 := *s.network.Validators[0] // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -660,7 +660,7 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() { } func (s *IntegrationTestSuite) TestCLIMultisign() { - val1 := s.validatorWithCustomCodec(0) + val1 := *s.network.Validators[0] // Generate 2 accounts and a multisig. account1, err := val1.ClientCtx.Keyring.Key("newAccount1") @@ -1069,15 +1069,6 @@ func (s *IntegrationTestSuite) createBankMsg(val *network.Validator, toAddr sdk. return res } -func (s *IntegrationTestSuite) validatorWithCustomCodec(idx int) network.Validator { - val := *s.network.Validators[0] - // codec := codec2.NewLegacyAmino() - // sdk.RegisterLegacyAminoCodec(codec) - // banktypes.RegisterLegacyAminoCodec(codec) - // val.ClientCtx.LegacyAmino = codec - return val -} - func TestIntegrationTestSuite(t *testing.T) { suite.Run(t, new(IntegrationTestSuite)) } From df7be40dcc85de8992ed8d0dbdb7872fc082d779 Mon Sep 17 00:00:00 2001 From: Cory Date: Mon, 14 Dec 2020 13:22:36 -0800 Subject: [PATCH 39/42] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 134ee61bc78f..23657581e8a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,7 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes * (crypto) [\#7966](https://github.com/cosmos/cosmos-sdk/issues/7966) `Bip44Params` `String()` function now correctly returns the absolute HD path by adding the `m/` prefix. -* (x/auth/client/cli) [\#7632](https://github.com/cosmos/cosmos-sdk/issues/7632) fixing regression bugs in transaction signing. +* (x/auth/client/cli) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) fixing regression bugs in transaction signing. ### API Breaking From 94c42a08d6c8f8b435e7d4bf8b216475e857dea9 Mon Sep 17 00:00:00 2001 From: Cory Date: Mon, 14 Dec 2020 13:23:15 -0800 Subject: [PATCH 40/42] updates to CHANGELOG.md --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23657581e8a2..847242542e96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,11 +53,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking Changes -* (x/upgrade) [\#7632](https://github.com/cosmos/cosmos-sdk/issues/7632) keeper pubkey storage serialization migration from bech32 to protobuf. +* (x/upgrade) [\#7979](https://github.com/cosmos/cosmos-sdk/pull/7979) keeper pubkey storage serialization migration from bech32 to protobuf. ### Features -* (codec/types) []() Adding `NewAnyWithCustomTypeURL` to correctly marshal Messages in TxBuilder. +* (codec/types) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) Adding `NewAnyWithCustomTypeURL` to correctly marshal Messages in TxBuilder. ### Bug Fixes From 48659f8a9769f00fcb223d050d9c17c417918412 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 22:33:22 +0100 Subject: [PATCH 41/42] remove unused method --- codec/types/any.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/codec/types/any.go b/codec/types/any.go index ce7f43f3c5f4..d978199fe692 100644 --- a/codec/types/any.go +++ b/codec/types/any.go @@ -140,12 +140,6 @@ func (any *Any) ClearCachedValue() { any.cachedValue = nil } -// UnsafeSetCachedValue sets cached value to a given interface. -// This method is for internal use only! -func (any *Any) UnsafeSetCachedValue(x interface{}) { - any.cachedValue = x -} - // IntoAny represents a type that can be wrapped into an Any. type IntoAny interface { AsAny() *Any From 5547aaa5353328fb4c4bea99ff2c4568de878f5b Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 14 Dec 2020 22:33:44 +0100 Subject: [PATCH 42/42] add new instance of transaction builder for TestSign --- client/tx/tx_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index f6e7e2137c36..3c7a4bebef0e 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -162,6 +162,8 @@ func TestSign(t *testing.T) { msg2 := banktypes.NewMsgSend(info2.GetAddress(), sdk.AccAddress("to"), nil) txb, err := tx.BuildUnsignedTx(txfNoKeybase, msg1, msg2) requireT.NoError(err) + txb2, err := tx.BuildUnsignedTx(txfNoKeybase, msg1, msg2) + requireT.NoError(err) txbSimple, err := tx.BuildUnsignedTx(txfNoKeybase, msg2) requireT.NoError(err) @@ -196,9 +198,9 @@ func TestSign(t *testing.T) { {"direct: should fail to append a signature with different mode", txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, {"direct: should fail to sign multi-signers tx", - txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil}, + txfDirect, txb2, from1, false, []cryptotypes.PubKey{}, nil}, {"direct: should fail to overwrite multi-signers tx", - txfDirect, txb, from1, true, []cryptotypes.PubKey{}, nil}, + txfDirect, txb2, from1, true, []cryptotypes.PubKey{}, nil}, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases {