-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(gov): handle panics when executing x/gov proposals #17780
Changes from 2 commits
bbcc29f
8f6110d
e08fc65
1c1555e
08ea4d0
9ca4e86
4369935
f4f5c89
7c074ba
fa528f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,9 @@ | |
"time" | ||
|
||
"cosmossdk.io/collections" | ||
"google.golang.org/protobuf/proto" | ||
Check failure on line 8 in x/gov/abci.go GitHub Actions / dependency-review
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (01)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (01)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
Check failure on line 8 in x/gov/abci.go GitHub Actions / tests (02)
|
||
robert-zaremba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
"github.com/cosmos/cosmos-sdk/baseapp" | ||
"github.com/cosmos/cosmos-sdk/telemetry" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/cosmos-sdk/x/gov/keeper" | ||
|
@@ -133,9 +135,8 @@ | |
// execute all messages | ||
for idx, msg = range messages { | ||
handler := keeper.Router().Handler(msg) | ||
|
||
var res *sdk.Result | ||
res, err = handler(cacheCtx, msg) | ||
res, err = safeExecuteHandler(cacheCtx, msg, handler) | ||
robert-zaremba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
break | ||
} | ||
|
@@ -223,3 +224,16 @@ | |
} | ||
return nil | ||
} | ||
|
||
// executes handle(msg) and recovers from panic. | ||
func safeExecuteHandler(ctx sdk.Context, msg sdk.Msg, handler baseapp.MsgServiceHandler, | ||
) (res *sdk.Result, err error) { | ||
|
||
defer func() { | ||
if r := recover(); r != nil { | ||
err = fmt.Errorf("handling x/gov poposal msg [%s] PANICED: %v", msg, r) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is another typo I think |
||
} | ||
}() | ||
res, err = handler(ctx, msg) | ||
return | ||
} |
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.
Recover panics and turn them into errors when executing x/gov proposals
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.
@odeke-em thanks for suggestion. I will update the changelog. However I don't think this kind of comment should be blocking, because now we need to wait for you to unblock.