-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add multisign batch command #7787
Conversation
…to sahith/add-batch-ms
…th/add-batch-test
…into sahith/add-batch-ms
…into sahith/add-batch-ms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nihil obstat.
@robert-zaremba Please review again ASAP. I believe all your comments were addressed. This is a non-breaking addition requested by various Cosmos SDK clients. Unless serious concerns are raised, I intend to merge this by today EOD. |
Why removing copy paste in separate PR? It's not complicated. |
Because it looks more important to merge this in and keep the scope of this PR small. Then we iterate. Agree? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to not block. However would be happy if the author create a follow-up issue to:
- break the command function (150 lines) into sub-functions (especially the transaction building part)
- add unit tests for combining signatures (both for happy and failing path)
- Add documentation with more details (somewhere in
/docs
)
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
* initial commit * update signing data * Update signature * code cleanup * code cleanup * Add test for ms batch * update test * add build flag * update flags * update tests * add test for signbatch multisig * update test * fix sign batch multisig * add test * update offline usage * update with sign batch fix * fix lint * update tests * update test * update tests * fix signature only * update seq * fix conflicts * update multisign * revert unintended * fix tests * rename flags * code refactor * fix typo * update docs * update test * Update x/auth/client/cli/tx_multisign.go * use named return values and explicit return * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Jonathan Gimeno <jonathan@tendermint.com> Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 77668a3)
@sahith-narahari how do docs fit in with multisign? Is there an issue we provide more details in? Would love to learn more. |
I created a followup issue, to address some of robert's concerns and documentation #8711 |
Thank you |
* initial commit * update signing data * Update signature * code cleanup * code cleanup * Add test for ms batch * update test * add build flag * update flags * update tests * add test for signbatch multisig * update test * fix sign batch multisig * add test * update offline usage * update with sign batch fix * fix lint * update tests * update test * update tests * fix signature only * update seq * fix conflicts * update multisign * revert unintended * fix tests * rename flags * code refactor * fix typo * update docs * update test * Update x/auth/client/cli/tx_multisign.go * use named return values and explicit return * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Jonathan Gimeno <jonathan@tendermint.com> Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
Multisign-batch adds functionality similar to multisign but for batch multisig transactions which are signed using sign-batch
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes