feat(cli): add `--append` flag to `sign-batch` cli cmd #13147

gsk967 · 2022-09-04T08:03:09Z

Description

Added --append flags to sign-batch cli cmd, this will combine the messages which are generated from --generate-only and it will generate single transaction

ref: #13066

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-09-04T08:14:26Z

Codecov Report

Merging #13147 (d0780aa) into main (4fe7797) will decrease coverage by 1.50%.
The diff coverage is 24.13%.

❗ Current head d0780aa differs from pull request most recent head ea5d655. Consider uploading reports for the commit ea5d655 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13147      +/-   ##
==========================================
- Coverage   55.87%   54.37%   -1.51%     
==========================================
  Files         646      645       -1     
  Lines       54895    54924      +29     
==========================================
- Hits        30675    29863     -812     
- Misses      21762    22663     +901     
+ Partials     2458     2398      -60

Impacted Files	Coverage Δ
baseapp/grpcrouter.go	`90.00% <ø> (ø)`
baseapp/grpcrouter_helpers.go	`25.00% <ø> (ø)`
baseapp/grpcserver.go	`1.72% <ø> (ø)`
baseapp/msg_service_router.go	`85.29% <ø> (+4.41%)`	⬆️
baseapp/options.go	`67.92% <0.00%> (-0.60%)`	⬇️
client/broadcast.go	`54.54% <ø> (+2.99%)`	⬆️
client/cmd.go	`57.73% <ø> (ø)`
client/config/toml.go	`55.55% <ø> (ø)`
client/context.go	`54.49% <ø> (-1.79%)`	⬇️
client/flags/flags.go	`19.35% <ø> (-0.32%)`	⬇️
... and 255 more

x/auth/client/cli/tx_multisign.go

x/auth/client/cli/tx_sign.go

atheeshp · 2022-09-05T09:26:55Z

How is this different from sign-batch?

CHANGELOG.md

x/auth/client/cli/tx_multisign.go

alexanderbez · 2022-09-05T14:25:08Z

x/auth/client/cli/tx_multisign.go

+			txBuilder.SetMemo(fe.GetTx().GetMemo())
+			txBuilder.SetTip(fe.GetTx().GetTip())
+			txBuilder.SetGasLimit(fe.GetTx().GetGas())


We're doing this in a loop, so aren't we overriding these fields, for each tx?

Now I am adding memo, remaining fields to tx from cmd.Flags

likhita-809

good to add tests as well

x/auth/client/cli/tx_multisign.go

anilcse · 2022-09-06T07:26:41Z

How is this different from sign-batch?

Oh seems like it is. @gsk967 can we merge this solution with sign-batch to accept multiple tx files. How does that sound @alexanderbez . I like some refactoring in this pr tbh

alexanderbez · 2022-09-06T14:12:41Z

How is this different from sign-batch?

Oh seems like it is. @gsk967 can we merge this solution with sign-batch to accept multiple tx files. How does that sound @alexanderbez . I like some refactoring in this pr tbh

Yeah, sure!

sahith-narahari · 2022-09-06T15:59:34Z

How is this different from sign-batch?

Oh seems like it is. @gsk967 can we merge this solution with sign-batch to accept multiple tx files. How does that sound @alexanderbez . I like some refactoring in this pr tbh

How about we add a flag append to common TxFlags which takes file name and adds the tx to a common file. This should limit changes to sign-batch command and is better ux than handling one file per tx imo.

alexanderbez · 2022-09-06T20:02:57Z

How would that work? Say we have a new flag, e.g. multi-msg-file=<some-file>. Would this require the tx to be not signed and be appended to the file <some-file>? What changes would be made to sign-batch?

sahith-narahari · 2022-09-06T20:11:11Z

So sign-batch expects a file to have multiple unsigned txs seperated by next line. The command flow would look something like this

$ simd tx bank send <args...|flags...> --generate-only > txs.json
$ simd tx staking delegate <args...|flags...> --generate-only --append txs.json
$ simd auth sign-batch txs.json --from key1

alexanderbez · 2022-09-06T21:48:18Z

Makes sense. But I would have all txs use the new flag so make the UX easier. If the flag is present and the file is empty, then just populate the empty file 👍

atheeshp · 2022-09-07T06:54:25Z

So sign-batch expects a file to have multiple unsigned txs seperated by next line. The command flow would look something like this
$ simd tx bank send <args...|flags...> --generate-only > txs.json
$ simd tx staking delegate <args...|flags...> --generate-only --append txs.json
$ simd auth sign-batch txs.json --from key1

I think we don't need any flag to be introduced here, since we have >> (file append operation) by default.

> replaces text in file
>> appends to the file with newline delimiter

The following lines will do the same as above mentioned lines by @sahith-narahari .

$ simd tx bank send <args...|flags...> --generate-only >> txs.json
$ simd tx staking delegate <args...|flags...> --generate-only >> txs.json
$ simd tx sign-batch txs.json --from key1

We can just update the documentation.

cc: @anilcse , @alexanderbez

sahith-narahari · 2022-09-07T06:57:31Z

Oh right, we can close the issue in this case then

alexanderbez · 2022-09-07T15:53:13Z

@atheeshp I don't understand how directing output will work though? The >> operator will just append one JSON blob into an existing file, giving you malformed JSON. What we want is a single tx, i.e. a single JSON blob that appends messages.

…dk into sai/multi_msg_sign_13066

gsk967 · 2022-09-22T06:00:23Z

@atheeshp @alexanderbez @AmauryM @anilcse

Added --append flag to the sign-batch cli cmd to combine multiple messages and generate the single signed tx

x/auth/client/cli/tx_sign.go

+					return err
+				}
+				if ms == "" {
+					from, _ := cmd.Flags().GetString(flags.FlagFrom)


x/auth/client/cli/tx_sign.go

-			txBuilder, err := txCfg.WrapTxBuilder(unsignedStdTx)
-			if err != nil {
-				return err
+		appendMessagesToSingleMsg, _ := cmd.Flags().GetBool(flagAppend)


x/auth/client/cli/tx_sign.go

 			}
+			// set the new appened msgs into builder
+			txBuilder.SetMsgs(msgs...)


alexanderbez · 2022-09-22T21:21:06Z

Amazing! @julienrbrt or @facundomedica would be willing to help test this feature with me please?

julienrbrt · 2022-09-22T21:50:10Z

Amazing! @julienrbrt or @facundomedica would be willing to help test this feature with me please?

I can check tomorrow morning :)

julienrbrt

tACK

it's working

julienrbrt · 2022-09-24T23:20:10Z

Tested and works, but if there is documentation to update, we should do it in this PR.

anilcse

Lgtm

alexanderbez · 2022-09-26T20:26:22Z

There are definitely CLI docs that we can look to update. Doesn't have to be super in depth, but maybe just a note in a CLI doc?

julienrbrt · 2022-09-26T21:58:43Z

There are definitely CLI docs that we can look to update. Doesn't have to be super in depth, but maybe just a note in a CLI doc?

Looking at the auth docs, there is actually no documentation for any tx auth commands: https://docs.cosmos.network/main/modules/auth/section_06.html.
It probably shouldn't be done in this issue then. I'll create an issue to do this as follow-up.

feat(cli): add multi-msg-sign

14afa31

github-actions bot added C:CLI C:x/auth labels Sep 4, 2022

chore: update the changelog

419b65d

chore: fix the lint issue

8c87572

gsk967 marked this pull request as ready for review September 4, 2022 08:34

gsk967 requested a review from a team as a code owner September 4, 2022 08:34

alexanderbez previously requested changes Sep 4, 2022

View reviewed changes

x/auth/client/cli/tx_multisign.go Outdated Show resolved Hide resolved

x/auth/client/cli/tx_multisign.go Outdated Show resolved Hide resolved

x/auth/client/cli/tx_sign.go Show resolved Hide resolved

x/auth/client/cli/tx_sign.go Outdated Show resolved Hide resolved

chore: address the pr comments

fe690ca

Merge branch 'main' into sai/multi_msg_sign_13066

f5e4c65

alexanderbez reviewed Sep 5, 2022

View reviewed changes

gsk967 added 3 commits September 5, 2022 20:05

Merge branch 'main' into sai/multi_msg_sign_13066

1774549

Merge branch 'main' into sai/multi_msg_sign_13066

f932afa

Merge branch 'main' into sai/multi_msg_sign_13066

7e8f9f0

likhita-809 reviewed Sep 6, 2022

View reviewed changes

x/auth/client/cli/tx_multisign.go Outdated Show resolved Hide resolved

x/auth/client/cli/tx_multisign.go Outdated Show resolved Hide resolved

x/auth/client/cli/tx_multisign.go Outdated Show resolved Hide resolved

Merge branch 'main' into sai/multi_msg_sign_13066

9956878

gsk967 and others added 3 commits September 7, 2022 21:40

Merge branch 'main' into sai/multi_msg_sign_13066

abb13b5

Merge branch 'sai/multi_msg_sign_13066' of github.com:gsk967/cosmos-s…

606758b

…dk into sai/multi_msg_sign_13066

Merge branch 'main' into sai/multi_msg_sign_13066

07b4a71

gsk967 added 2 commits September 22, 2022 11:26

refactor: refactored sign-batch to generate single signed tx

565ed1a

chore: updated the changelog

6fb556e

fix: fix the lint

861e5c5

github-advanced-security bot found potential problems Sep 22, 2022

View reviewed changes

test: fix the tests on auth

a5c6e15

Merge branch 'main' into sai/multi_msg_sign_13066

6fc9145

gsk967 changed the title ~~feat(cli): add multi-msg-sign~~ feat(cli): add --append flag to sign-batch cli cmd Sep 23, 2022

julienrbrt self-assigned this Sep 23, 2022

julienrbrt and others added 3 commits September 23, 2022 22:45

Merge branch 'main' into sai/multi_msg_sign_13066

382d63a

Merge branch 'main' into sai/multi_msg_sign_13066

9fd6081

Merge branch 'main' into sai/multi_msg_sign_13066

4206be9

julienrbrt approved these changes Sep 24, 2022

View reviewed changes

Merge branch 'main' into sai/multi_msg_sign_13066

42a0d2f

julienrbrt and others added 3 commits September 25, 2022 14:29

Merge branch 'main' into sai/multi_msg_sign_13066

7ff9c11

Merge branch 'main' into sai/multi_msg_sign_13066

162e22e

Merge branch 'main' into sai/multi_msg_sign_13066

d0780aa

anilcse approved these changes Sep 26, 2022

View reviewed changes

alexanderbez approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'main' into sai/multi_msg_sign_13066

ea5d655

julienrbrt merged commit 445a99f into cosmos:main Sep 26, 2022

julienrbrt mentioned this pull request Sep 26, 2022

Add auth tx commands documentation #13395

Closed

LampardNguyen234 mentioned this pull request Aug 15, 2023

deps: bump cosmos-sdk v0.46 and evmos v12 AstraProtocol/astra#209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add `--append` flag to `sign-batch` cli cmd #13147

feat(cli): add `--append` flag to `sign-batch` cli cmd #13147

gsk967 commented Sep 4, 2022 •

edited

Loading

codecov bot commented Sep 4, 2022 •

edited

Loading

atheeshp commented Sep 5, 2022 •

edited

Loading

alexanderbez Sep 5, 2022

gsk967 Sep 8, 2022

likhita-809 left a comment

anilcse commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

sahith-narahari commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

sahith-narahari commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

atheeshp commented Sep 7, 2022 •

edited

Loading

sahith-narahari commented Sep 7, 2022

alexanderbez commented Sep 7, 2022 •

edited

Loading

gsk967 commented Sep 22, 2022

alexanderbez commented Sep 22, 2022

julienrbrt commented Sep 22, 2022

julienrbrt left a comment

julienrbrt commented Sep 24, 2022

anilcse left a comment

alexanderbez commented Sep 26, 2022 •

edited

Loading

julienrbrt commented Sep 26, 2022

feat(cli): add --append flag to sign-batch cli cmd #13147

feat(cli): add --append flag to sign-batch cli cmd #13147

Conversation

gsk967 commented Sep 4, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Sep 4, 2022 • edited Loading

Codecov Report

atheeshp commented Sep 5, 2022 • edited Loading

alexanderbez Sep 5, 2022

Choose a reason for hiding this comment

gsk967 Sep 8, 2022

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

anilcse commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

sahith-narahari commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

sahith-narahari commented Sep 6, 2022

alexanderbez commented Sep 6, 2022

atheeshp commented Sep 7, 2022 • edited Loading

sahith-narahari commented Sep 7, 2022

alexanderbez commented Sep 7, 2022 • edited Loading

gsk967 commented Sep 22, 2022

alexanderbez commented Sep 22, 2022

julienrbrt commented Sep 22, 2022

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt commented Sep 24, 2022

anilcse left a comment

Choose a reason for hiding this comment

alexanderbez commented Sep 26, 2022 • edited Loading

julienrbrt commented Sep 26, 2022

feat(cli): add `--append` flag to `sign-batch` cli cmd #13147

feat(cli): add `--append` flag to `sign-batch` cli cmd #13147

gsk967 commented Sep 4, 2022 •

edited

Loading

codecov bot commented Sep 4, 2022 •

edited

Loading

atheeshp commented Sep 5, 2022 •

edited

Loading

atheeshp commented Sep 7, 2022 •

edited

Loading

alexanderbez commented Sep 7, 2022 •

edited

Loading

alexanderbez commented Sep 26, 2022 •

edited

Loading