fix(x/gov): limit execution in gov #20348
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
|
|
||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| ) | ||
|
|
||
| // EndBlocker is called every block. | ||
|
|
@@ -35,6 +36,11 @@ | |
| return err | ||
| } | ||
|
|
||
| params, err := k.Params.Get(ctx) | ||
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, prop := range inactiveProps { | ||
| proposal, err := k.Proposals.Get(ctx, prop.Key.K2()) | ||
| if err != nil { | ||
|
|
@@ -61,10 +67,6 @@ | |
| return err | ||
| } | ||
|
|
||
| if !params.BurnProposalDepositPrevote { | ||
| err = k.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal | ||
| } else { | ||
|
|
@@ -77,9 +79,9 @@ | |
|
|
||
| // called when proposal become inactive | ||
| // call hook when proposal become inactive | ||
| if err = k.BranchService.Execute(ctx, func(ctx context.Context) error { | ||
| return k.Hooks().AfterProposalFailedMinDeposit(ctx, proposal.Id) | ||
| }); err != nil { | ||
|
Check warning Code scanning / CodeQL Panic in BeginBock or EndBlock consensus methods Warning |
||
| // purposely ignoring the error here not to halt the chain if the hook fails | ||
| k.Logger.Error("failed to execute AfterProposalFailedMinDeposit hook", "error", err) | ||
| } | ||
|
|
@@ -193,26 +195,34 @@ | |
| // Messages may mutate state thus we use a cached context. If one of | ||
| // the handlers fails, no state mutation is written and the error | ||
| // message is logged. | ||
| _, err = k.BranchService.ExecuteWithGasLimit(ctx, params.ProposalExecutionGas, func(ctx context.Context) error { | ||
| // execute all messages | ||
| for idx, msg = range messages { | ||
| if _, err := safeExecuteHandler(ctx, msg, k.MsgRouterService); err != nil { | ||
| // `idx` and `err` are populated with the msg index and error. | ||
| proposal.Status = v1.StatusFailed | ||
| proposal.FailedReason = err.Error() | ||
| tagValue = types.AttributeValueProposalFailed | ||
| logMsg = fmt.Sprintf("passed, but msg %d (%s) failed on execution: %s", idx, sdk.MsgTypeURL(msg), err) | ||
|
|
||
| return err | ||
| } | ||
| } | ||
|
|
||
| proposal.Status = v1.StatusPassed | ||
| tagValue = types.AttributeValueProposalPassed | ||
| logMsg = "passed" | ||
|
|
||
| return nil | ||
| }) | ||
|
||
| if err != nil { | ||
| // update proposal if error is out of gas | ||
| if errors.Is(err, sdkerrors.ErrOutOfGas) { | ||
|
There was a problem hiding this comment. problem is branch service implementation of stf doesn't return this error There was a problem hiding this comment. should it be in all cases we set the proposal to fail? if not then we need to see how to make this work correctly There was a problem hiding this comment. yes, we can just remove this https://github.com/cosmos/cosmos-sdk/pull/20348/files/12ad1f9e4f4600950f089e358215d5fc68bcae6d#diff-4bb7b38066b637ade8ae58c5d5bf4050e08f2af0f89aa791baee39cb008187b9R206-R212 (but store the proposal idx), and handle for all error here instead. There was a problem hiding this comment. pushed what i think you mean, not sure where to store the proposal idx There was a problem hiding this comment. we need to remove L214-L219 and make the error log more generic (as it can be out of gas or proposal execution error (at handling)). As it is only used in the log, let's skip it? |
||
| proposal.Status = v1.StatusFailed | ||
| proposal.FailedReason = err.Error() | ||
| tagValue = types.AttributeValueProposalFailed | ||
| logMsg = "passed proposal failed due to out of gas" | ||
|
There was a problem hiding this comment. There is a break missing or this is overwritten in L224. Is there a way for people to find out the required gas amount? If the original error msg contains this information it feels like the better choice. There was a problem hiding this comment. That whole if condition above should be removed (L214-L219) |
||
| } | ||
| break // We do not anything with the error. Returning an error halts the chain, and proposal struct is already updated. | ||
| } | ||
| case !burnDeposits && (proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED || | ||
|
|
@@ -222,10 +232,6 @@ | |
| // once the regular voting period expires again, the tally is repeated | ||
| // according to the regular proposal rules. | ||
| proposal.ProposalType = v1.ProposalType_PROPOSAL_TYPE_STANDARD | ||
| endTime := proposal.VotingStartTime.Add(*params.VotingPeriod) | ||
| proposal.VotingEndTime = &endTime | ||
|
|
||
|
|
||
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.
Correct the article usage for better grammatical accuracy.
Committable suggestion
Tools
LanguageTool