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

Incorrect use of ErrNoProposalHandlerExists when the handler returns an error. #13030

Closed
SpicyLemon opened this issue Aug 24, 2022 · 2 comments · Fixed by #13051
Closed

Incorrect use of ErrNoProposalHandlerExists when the handler returns an error. #13030

SpicyLemon opened this issue Aug 24, 2022 · 2 comments · Fixed by #13051
Assignees
Labels

Comments

@SpicyLemon
Copy link
Collaborator

Summary of Bug

In x/gov/keeper/proposal.go -> SubmitProposal, for legacy messages, if the handler call returns an error, it is erroneously wrapped as a ErrNoProposalHandlerExists. For example, if the handler returns a code 5, insufficient funds, that gets changed into a code 9, "no handler exists for proposal type".

In one of Provenance's unit tests on a governance proposal endpoint, the endpoint/handler returns the error 0stake is smaller than 1stake: insufficient funds, code 5. But now, the governance module wraps that with ErrNoProposalHandlerExists and it ends up as 0stake is smaller than 1stake: insufficient funds: invalid proposal content: no handler exists for proposal type, code 9.

I feel like the error shouldn't be wrapped again. The handler should be trusted to return wrapped errors; if not, it's a bug in that handler. But wrapping them again wipes out any utility there was in returning different codes from the handlers.

This is the offending code:

		// Only if it's a MsgExecLegacyContent do we try to execute the
		// proposal in a cached context.
		// For other Msgs, we do not verify the proposal messages any further.
		// They may fail upon execution.
		// ref: https://github.com/cosmos/cosmos-sdk/pull/10868#discussion_r784872842
		if msg, ok := msg.(*v1.MsgExecLegacyContent); ok {
			cacheCtx, _ := ctx.CacheContext()
			if _, err := handler(cacheCtx, msg); err != nil {
				return v1.Proposal{}, sdkerrors.Wrap(types.ErrNoProposalHandlerExists, err.Error())
			}
		}

Version

First noticed in v0.46.0.
Still exists in main as of commit 6af89d6ce3f43e8a67ea2ee93c5a49e395ebd09d Wed Aug 24 23:13:11 2022 +0200.

Steps to Reproduce

Submit a governance proposal that will fail when it's handler is called.

@SpicyLemon SpicyLemon self-assigned this Aug 25, 2022
@SpicyLemon
Copy link
Collaborator Author

Upon further inspection, it looks like the ErrInvalidProposalContent error is applicable here (and fits with what used to happen).

@SpicyLemon
Copy link
Collaborator Author

Upon even further inspection, the thing that wraps the legacy handler endpoints returns a ErrNoProposalHandlerExists if there isn't a handler found. So this line is currently wrapping the error as a ErrNoProposalHandlerExists a second time.

So if it's a ErrNoProposalHandlerExists, we should keep the error as is, if it's something else, wrap it as ErrInvalidProposalContent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant