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.

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 failing address to multisig validation #8518

Merged
merged 11 commits into from
Feb 19, 2021
Merged

Conversation

jackzampolin
Copy link
Member

@jackzampolin jackzampolin commented Feb 5, 2021

This PR helps users of multisig wallets debug signature issues. cc @zmanian for the hat tip

@@ -125,7 +125,7 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
for _, sig := range sigs {
err = signing.VerifySignature(sig.PubKey, signingData, sig.Data, txCfg.SignModeHandler(), txBuilder.GetTx())
if err != nil {
return fmt.Errorf("couldn't verify signature: %w", err)
return fmt.Errorf("couldn't verify signature for address %s", sig.PubKey.Address())
Copy link
Member

Choose a reason for hiding this comment

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

				addr, _ := sdktypes.AccAddressFromHex(stdSig.PubKey.Address().String())
				return fmt.Errorf("couldn't verify signature for address %s", addr)

Copy link
Member

Choose a reason for hiding this comment

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

Needs an import added too

	sdktypes "github.com/cosmos/cosmos-sdk/types"

Copy link
Member Author

Choose a reason for hiding this comment

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

danke, got both.

@jackzampolin jackzampolin added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 6, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, would be cool to have a test for this

@@ -125,7 +125,8 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
for _, sig := range sigs {
err = signing.VerifySignature(sig.PubKey, signingData, sig.Data, txCfg.SignModeHandler(), txBuilder.GetTx())
if err != nil {
return fmt.Errorf("couldn't verify signature: %w", err)
addr, _ := sdk.AccAddressFromHex(sig.PubKey.Address().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit you can drop the .String() as the %s formatter will automatically do this.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, would be cool to have a test for this

Actually there is one test, and it's failing now. OK to merge once test passes

@mergify mergify bot merged commit e577378 into master Feb 19, 2021
@mergify mergify bot deleted the jack/multisig-index branch February 19, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI T: Client UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants