From 7bdbc4629fe23977d4185bafdbbf3553ae081554 Mon Sep 17 00:00:00 2001 From: Ezequiel Raynaudo Date: Wed, 29 Nov 2023 11:38:08 -0300 Subject: [PATCH] fix(x/auth): Add fees on batch sign (#18564) --- tests/e2e/auth/suite.go | 26 +++---- .../integration/auth/client/cli/suite_test.go | 67 +++++++++++++++++-- x/auth/CHANGELOG.md | 2 + x/auth/client/cli/tx_sign.go | 12 +++- x/auth/client/testutil/helpers.go | 8 +-- 5 files changed, 90 insertions(+), 25 deletions(-) diff --git a/tests/e2e/auth/suite.go b/tests/e2e/auth/suite.go index cee85a1be372..c17d47ba12eb 100644 --- a/tests/e2e/auth/suite.go +++ b/tests/e2e/auth/suite.go @@ -226,40 +226,40 @@ func (s *E2ETestSuite) TestCLISignBatch() { clientCtx.HomeDir = strings.Replace(clientCtx.HomeDir, "simd", "simcli", 1) // sign-batch file - offline is set but account-number and sequence are not - _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--offline") + _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--offline") s.Require().EqualError(err, "required flag(s) \"account-number\", \"sequence\" not set") // sign-batch file - offline and sequence is set but account-number is not set - _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), "--offline") + _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), "--offline") s.Require().EqualError(err, "required flag(s) \"account-number\" not set") // sign-batch file - offline and account-number is set but sequence is not set - _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1"), "--offline") + _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1"), "--offline") s.Require().EqualError(err, "required flag(s) \"sequence\" not set") // sign-batch file - sequence and account-number are set when offline is false - res, err := authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1")) + res, err := authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1")) s.Require().NoError(err) s.Require().Equal(3, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // sign-batch file - res, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID)) + res, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID)) s.Require().NoError(err) s.Require().Equal(3, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // sign-batch file signature only - res, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") + res, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") s.Require().NoError(err) s.Require().Equal(3, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // Sign batch malformed tx file. malformedFile := testutil.WriteToNewTempFile(s.T(), fmt.Sprintf("malformed%s", generatedStd)) defer malformedFile.Close() - _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), malformedFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID)) + _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{malformedFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID)) s.Require().Error(err) // Sign batch malformed tx file signature only. - _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), malformedFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") + _, err = authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{malformedFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") s.Require().Error(err) // make a txn to increase the sequence of sender @@ -288,7 +288,7 @@ func (s *E2ETestSuite) TestCLISignBatch() { s.Require().Equal(seq+1, seq1) // signing sign-batch should start from the last sequence. - signed, err := authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") + signed, err := authclitestutil.TxSignBatchExec(clientCtx, val.GetAddress(), []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--signature-only") s.Require().NoError(err) signedTxs := strings.Split(strings.Trim(signed.String(), "\n"), "\n") s.Require().GreaterOrEqual(len(signedTxs), 1) @@ -1171,7 +1171,7 @@ func (s *E2ETestSuite) TestSignBatchMultisig() { addr1, err := account1.GetAddress() s.Require().NoError(err) // sign-batch file - res, err := authclitestutil.TxSignBatchExec(clientCtx, addr1, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") + res, err := authclitestutil.TxSignBatchExec(clientCtx, addr1, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") s.Require().NoError(err) s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // write sigs to file @@ -1181,7 +1181,7 @@ func (s *E2ETestSuite) TestSignBatchMultisig() { addr2, err := account2.GetAddress() s.Require().NoError(err) // sign-batch file with account2 - res, err = authclitestutil.TxSignBatchExec(clientCtx, addr2, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") + res, err = authclitestutil.TxSignBatchExec(clientCtx, addr2, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") s.Require().NoError(err) s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // write sigs to file2 @@ -1244,7 +1244,7 @@ func (s *E2ETestSuite) TestMultisignBatch() { // sign-batch file addr1, err := account1.GetAddress() s.Require().NoError(err) - res, err := authclitestutil.TxSignBatchExec(clientCtx, addr1, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), fmt.Sprintf("--%s", flags.FlagOffline), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, fmt.Sprint(account.GetAccountNumber())), fmt.Sprintf("--%s=%s", flags.FlagSequence, fmt.Sprint(account.GetSequence())), "--signature-only") + res, err := authclitestutil.TxSignBatchExec(clientCtx, addr1, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), fmt.Sprintf("--%s", flags.FlagOffline), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, fmt.Sprint(account.GetAccountNumber())), fmt.Sprintf("--%s=%s", flags.FlagSequence, fmt.Sprint(account.GetSequence())), "--signature-only") s.Require().NoError(err) s.Require().Equal(3, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // write sigs to file @@ -1254,7 +1254,7 @@ func (s *E2ETestSuite) TestMultisignBatch() { // sign-batch file with account2 addr2, err := account2.GetAddress() s.Require().NoError(err) - res, err = authclitestutil.TxSignBatchExec(clientCtx, addr2, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), fmt.Sprintf("--%s", flags.FlagOffline), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, fmt.Sprint(account.GetAccountNumber())), fmt.Sprintf("--%s=%s", flags.FlagSequence, fmt.Sprint(account.GetSequence())), "--signature-only") + res, err = authclitestutil.TxSignBatchExec(clientCtx, addr2, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, clientCtx.ChainID), "--multisig", addr.String(), fmt.Sprintf("--%s", flags.FlagOffline), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, fmt.Sprint(account.GetAccountNumber())), fmt.Sprintf("--%s=%s", flags.FlagSequence, fmt.Sprint(account.GetSequence())), "--signature-only") s.Require().NoError(err) s.Require().Equal(3, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) diff --git a/tests/integration/auth/client/cli/suite_test.go b/tests/integration/auth/client/cli/suite_test.go index f3a33e55cfa5..d74cf198b639 100644 --- a/tests/integration/auth/client/cli/suite_test.go +++ b/tests/integration/auth/client/cli/suite_test.go @@ -159,18 +159,75 @@ func (s *CLITestSuite) TestCLISignBatch() { s.clientCtx.HomeDir = strings.Replace(s.clientCtx.HomeDir, "simd", "simcli", 1) // sign-batch file - offline is set but account-number and sequence are not - _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--offline") + _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--offline") s.Require().EqualError(err, "required flag(s) \"account-number\", \"sequence\" not set") // sign-batch file - offline and sequence is set but account-number is not set - _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), "--offline") + _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagSequence, "1"), "--offline") s.Require().EqualError(err, "required flag(s) \"account-number\" not set") // sign-batch file - offline and account-number is set but sequence is not set - _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, outputFile.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1"), "--offline") + _, err = authtestutil.TxSignBatchExec(s.clientCtx, s.val, []string{outputFile.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), fmt.Sprintf("--%s=%s", flags.FlagAccountNumber, "1"), "--offline") s.Require().EqualError(err, "required flag(s) \"sequence\" not set") } +func (s *CLITestSuite) TestCLISignBatchTotalFees() { + txCfg := s.clientCtx.TxConfig + s.clientCtx.HomeDir = strings.Replace(s.clientCtx.HomeDir, "simd", "simcli", 1) + + testCases := []struct { + name string + args []string + numTransactions int + denom string + }{ + { + "Offline batch-sign one transaction", + []string{"--offline", "--account-number", "1", "--sequence", "1", "--append"}, + 1, + "stake", + }, + { + "Offline batch-sign two transactions", + []string{"--offline", "--account-number", "1", "--sequence", "1", "--append"}, + 2, + "stake", + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + // Create multiple transactions and write them to separate files + sendTokens := sdk.NewCoin(tc.denom, sdk.TokensFromConsensusPower(10, sdk.DefaultPowerReduction)) + expectedBatchedTotalFee := int64(0) + txFiles := make([]string, tc.numTransactions) + for i := 0; i < tc.numTransactions; i++ { + tx, err := s.createBankMsg(s.clientCtx, s.val, + sdk.NewCoins(sendTokens), clitestutil.TestTxConfig{GenOnly: true}) + s.Require().NoError(err) + txFile := testutil.WriteToNewTempFile(s.T(), tx.String()+"\n") + defer txFile.Close() + txFiles[i] = txFile.Name() + + unsignedTx, err := txCfg.TxJSONDecoder()(tx.Bytes()) + s.Require().NoError(err) + txBuilder, err := txCfg.WrapTxBuilder(unsignedTx) + expectedBatchedTotalFee += txBuilder.GetTx().GetFee().AmountOf(tc.denom).Int64() + } + + // Test batch sign + signedTx, err := authtestutil.TxSignBatchExec(s.clientCtx, s.val, txFiles, tc.args...) + s.Require().NoError(err) + signedFinalTx, err := txCfg.TxJSONDecoder()(signedTx.Bytes()) + s.Require().NoError(err) + txBuilder, err := txCfg.WrapTxBuilder(signedFinalTx) + s.Require().NoError(err) + finalTotalFee := txBuilder.GetTx().GetFee() + s.Require().Equal(expectedBatchedTotalFee, finalTotalFee.AmountOf(tc.denom).Int64()) + }) + } +} + func (s *CLITestSuite) TestCLIQueryTxCmdByHash() { sendTokens := sdk.NewInt64Coin("stake", 10) @@ -739,7 +796,7 @@ func (s *CLITestSuite) TestSignBatchMultisig() { addr1, err := account1.GetAddress() s.Require().NoError(err) // sign-batch file - res, err := authtestutil.TxSignBatchExec(s.clientCtx, addr1, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") + res, err := authtestutil.TxSignBatchExec(s.clientCtx, addr1, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") s.Require().NoError(err) s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // write sigs to file @@ -749,7 +806,7 @@ func (s *CLITestSuite) TestSignBatchMultisig() { addr2, err := account2.GetAddress() s.Require().NoError(err) // sign-batch file with account2 - res, err = authtestutil.TxSignBatchExec(s.clientCtx, addr2, filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") + res, err = authtestutil.TxSignBatchExec(s.clientCtx, addr2, []string{filename.Name()}, fmt.Sprintf("--%s=%s", flags.FlagChainID, s.clientCtx.ChainID), "--multisig", addr.String(), "--signature-only") s.Require().NoError(err) s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n"))) // write sigs to file2 diff --git a/x/auth/CHANGELOG.md b/x/auth/CHANGELOG.md index 85ad31d77969..fcabad64e900 100644 --- a/x/auth/CHANGELOG.md +++ b/x/auth/CHANGELOG.md @@ -32,3 +32,5 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes ### Bug Fixes + +* (client/cli) [#18564](https://github.com/cosmos/cosmos-sdk/pull/18564) Fix total fees calculation when batch signing diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 6ed9537ccaa7..280a5fd03a12 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -117,6 +117,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { appendMessagesToSingleTx, _ := cmd.Flags().GetBool(flagAppend) // Combines all tx msgs and create single signed transaction if appendMessagesToSingleTx { + var totalFees sdk.Coins txBuilder := txCfg.NewTxBuilder() msgs := make([]sdk.Msg, 0) newGasLimit := uint64(0) @@ -129,6 +130,13 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { } // increment the gas newGasLimit += fe.GetTx().GetGas() + // Individual fee values from each transaction need to be + // aggregated to calculate the total fee for the batch of transactions. + // https://github.com/cosmos/cosmos-sdk/issues/18064 + unmergedFees := fe.GetTx().GetFee() + for _, fee := range unmergedFees { + totalFees = totalFees.Add(fee) + } // append messages msgs = append(msgs, unsignedStdTx.GetMsgs()...) } @@ -140,13 +148,15 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { // set the memo,fees,feeGranter,feePayer from cmd flags txBuilder.SetMemo(txFactory.Memo()) - txBuilder.SetFeeAmount(txFactory.Fees()) txBuilder.SetFeeGranter(clientCtx.FeeGranter) txBuilder.SetFeePayer(clientCtx.FeePayer) // set the gasLimit txBuilder.SetGasLimit(newGasLimit) + // set the feeAmount + txBuilder.SetFeeAmount(totalFees) + // sign the txs from, _ := cmd.Flags().GetString(flags.FlagFrom) err := sigTxOrMultisig(clientCtx, txBuilder, txFactory, from, multisigKey) diff --git a/x/auth/client/testutil/helpers.go b/x/auth/client/testutil/helpers.go index 69d0b6ff380f..9dabe4c242d0 100644 --- a/x/auth/client/testutil/helpers.go +++ b/x/auth/client/testutil/helpers.go @@ -63,12 +63,8 @@ func TxMultiSignExec(clientCtx client.Context, from, filename string, extraArgs return clitestutil.ExecTestCLICmd(clientCtx, cli.GetMultiSignCommand(), append(args, extraArgs...)) } -func TxSignBatchExec(clientCtx client.Context, from fmt.Stringer, filename string, extraArgs ...string) (testutil.BufferWriter, error) { - args := []string{ - fmt.Sprintf("--from=%s", from.String()), - filename, - } - +func TxSignBatchExec(clientCtx client.Context, from fmt.Stringer, filenames []string, extraArgs ...string) (testutil.BufferWriter, error) { + args := append([]string{fmt.Sprintf("--from=%s", from.String())}, filenames...) return clitestutil.ExecTestCLICmd(clientCtx, cli.GetSignBatchCommand(), append(args, extraArgs...)) }