Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/auth: sign-batch command #6350

Merged
merged 3 commits into from
Jun 8, 2020
Merged

x/auth: sign-batch command #6350

merged 3 commits into from
Jun 8, 2020

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 5, 2020 •
edited
Loading

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
incremnts the sequence number automatically.

This command is particularly useful when signing
batches of multisig transactions.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@alessio alessio added the WIP label Jun 5, 2020
@codecov
Copy link

codecov bot commented Jun 5, 2020 •
edited
Loading

Codecov Report

Merging #6350 into master will decrease coverage by 3.77%.
The diff coverage is 11.45%.

@@            Coverage Diff             @@
##           master    #6350      +/-   ##
==========================================
- Coverage   59.46%   55.68%   -3.78%     
==========================================
  Files         341      451     +110     
  Lines       20508    27254    +6746     
==========================================
+ Hits        12195    15177    +2982     
- Misses       7434    10985    +3551     
- Partials      879     1092     +213

@alessio alessio force-pushed the aib/auth-client-sign-batch branch 2 times, most recently from d6711a2 to 9de4288 Compare June 5, 2020 17:16
@alessio alessio added the C:CLI label Jun 5, 2020
@alessio alessio force-pushed the aib/auth-client-sign-batch branch from 20aacef to 21a8a63 Compare June 5, 2020 17:18
@alessio alessio added R4R and removed WIP labels Jun 5, 2020
@alessio alessio marked this pull request as ready for review June 5, 2020 17:18
@alessio
Copy link
Contributor Author

alessio commented Jun 5, 2020

We'll shortly upstream another multisig-related tool used internally at AiB.

@alessio alessio changed the title sign-batch command x/auth: sign-batch command Jun 5, 2020
@alessio alessio requested a review from jgimeno June 5, 2020 17:42
@alessio alessio mentioned this pull request Jun 5, 2020
Closed
alessio
alessio commented Jun 5, 2020
View reviewed changes
x/auth/client/cli/tx_sign.go Outdated Show resolved Hide resolved
aaronc
aaronc reviewed Jun 5, 2020
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

We're going to need to rewrite this pretty soon to use the new Tx type and client/tx but maybe it's okay to merge and have someone migrate in a week or two. Or we can try to do it right away in this PR. I've already started breaking #6216 up into smaller PR's and a redesigned client/tx that could be used for this command should be R4R early next week.

@alessio
Copy link
Contributor Author

alessio commented Jun 5, 2020 •
edited
Loading

@aaronc let's merge this in so that we have the logic ready to be tested immediately. A refactor will ensure shortly I guess. Shall we?

@aaronc
Copy link
Member

aaronc commented Jun 6, 2020

@aaronc let's merge this in so that we have the logic ready to be tested immediately. A refactor will ensure shortly I guess. Shall we?

Sounds good. Have you read through ADR 020 and the discussions surrounding it?

alessio
alessio commented Jun 6, 2020
View reviewed changes
x/auth/client/cli/cli_test.go Outdated Show resolved Hide resolved
@alessio
Copy link
Contributor Author

alessio commented Jun 6, 2020

@aaronc dixit:

Have you read through ADR 020 and the discussions surrounding it?

Yes, I have 👍

@alessio alessio requested review from tac0turtle and aaronc June 6, 2020 16:43
alessio
alessio commented Jun 6, 2020
View reviewed changes
x/auth/client/tx_test.go Outdated Show resolved Hide resolved
x/auth: add sign-batch command
29948a2 
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
incremnts the sequence number automatically.

Author: @jgimeno
Reviewed-by: @alessio
@alessio alessio force-pushed the aib/auth-client-sign-batch branch from 806840a to 29948a2 Compare June 6, 2020 17:14
@alessio alessio merged commit 65ea305 into master Jun 8, 2020
@alessio alessio deleted the aib/auth-client-sign-batch branch June 8, 2020 15:19
@alessio alessio mentioned this pull request Jun 8, 2020
Merged
8 tasks
@jgimeno jgimeno mentioned this pull request Jun 9, 2020
Closed
@clevinson
Copy link
Contributor

@aaronc @alessio Did we get around to updating this to use Tx and client/tx ?

@alessio
Copy link
Contributor Author

alessio commented Sep 29, 2020

This is currently not on my plate

@clevinson
Copy link
Contributor

clevinson commented Oct 2, 2020 •
edited
Loading

@alessio Actually, it looks like this was already taken care of by @anilcse in #6717

See blame: https://github.com/cosmos/cosmos-sdk/blame/89097a00d7f6d6339c377f6c87bea8fa5068d125/x/auth/client/cli/tx_sign.go

So i think no action is needed anymore.

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
x/auth: add sign-batch command (cosmos#6350)
80c7e87 
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
increments the sequence number automatically.

Author: @jgimeno
Reviewed-by: @alessio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgimeno jgimeno jgimeno approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@clevinson clevinson Awaiting requested review from clevinson

@fedekunze fedekunze Awaiting requested review from fedekunze

@aaronc aaronc Awaiting requested review from aaronc

Assignees
No one assigned
Labels
C:CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alessio @aaronc @clevinson @jgimeno @tac0turtle