fix(x/gov): limit execution in gov #20348
Conversation
|
Warning Rate limit exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 37 minutes and 22 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates introduce a new gas limit parameter for governance proposal execution, ensuring proposals adhere to a maximum gas usage. This involves modifications to error handling and context usage within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GovModule
participant Keeper
participant Context
participant ProposalExecution
User->>GovModule: Submit Proposal
GovModule->>Keeper: Process Proposal
Keeper->>Context: Pass Context
Context->>ProposalExecution: Execute Proposal
ProposalExecution-->>Context: Return Execution Result
Context-->>Keeper: Return Context with Result
Keeper->>GovModule: Update Proposal Status
GovModule->>User: Notify Proposal Status
sequenceDiagram
participant System
participant MigrationScript
participant GovParams
System->>MigrationScript: Trigger Migration
MigrationScript->>GovParams: Add ProposalExecutionGas Field
GovParams-->>MigrationScript: Confirm Field Addition
MigrationScript-->>System: Migration Complete
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/gov/README.md
Outdated
|
|
||
| Execution is the process of executing the messages contained in a proposal. The execution phase will commence after the proposal has been accepted by the network. The messages contained in the proposal will be executed in the order they were submitted. | ||
|
|
||
| Execution has a upper limit on how many messages can be executed in a single block. This limit is defined by the `MaxMessagesPerProposal` parameter. We use the MaxBlockGas from the consensus engine, value stored in the consensus module, to calculate the limit of messages that can be executed in a block. |
There was a problem hiding this comment.
Why do we need a MaxMessagesPerProposal if we already have that block gas limit?
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (7)
x/gov/README.md (7)
Line range hint
26-26: Add a period after "a.k.a" for correct abbreviation usage.- This module is in use on the Cosmos Hub (a.k.a [gaia](https://github.com/cosmos/gaia)). + This module is in use on the Cosmos Hub (a.k.a. [gaia](https://github.com/cosmos/gaia)).Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
94-94: Replace "ie." with "i.e." to correct the abbreviation usage.- Messages without authority are message meant to be executed by users. Using the `MsgSudoExec` message in a proposal, let governance can execute any message, effectively acting as super user. + Messages without authority are message meant to be executed by users. Using the `MsgSudoExec` message in a proposal, let governance can execute any message, effectively acting as super user (i.e., no authority present).Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
95-95: Change "super user" to "superuser" for correct terminology.- Messages without authority are message meant to be executed by users. Using the `MsgSudoExec` message in a proposal, let governance can execute any message, effectively acting as super user. + Messages without authority are message meant to be executed by users. Using the `MsgSudoExec` message in a proposal, let governance can execute any message, effectively acting as superuser.Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
103-103: Change "accompanied with" to "accompanied by" for correct preposition usage.- When a proposal is submitted, it has to be accompanied with a deposit that must be strictly positive... + When a proposal is submitted, it has to be accompanied by a deposit that must be strictly positive...Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
110-110: Add "the" before "state" for correct article usage.- ...the proposal will be removed from state and the deposit will be burned... + ...the proposal will be removed from the state and the deposit will be burned...Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
136-136: Replace "that" with "who" when referring to people for correct pronoun usage.- *Participants* are users that have the right to vote on proposals. + *Participants* are users who have the right to vote on proposals.Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Line range hint
172-172: Add a comma after "No" for better readability.- Often times the entity owning that address might not be a single individual. For example, a company might have different stakeholders who want to vote differently, and so it makes sense to allow them to split their voting power. Currently, it is not possible for them to do "passthrough voting" and giving their users voting rights over their tokens. However, with this system, exchanges can poll their users for voting preferences, and then vote on-chain proportionally to the results of the poll. + Often times the entity owning that address might not be a single individual. For example, a company might have different stakeholders who want to vote differently, and so it makes sense to allow them to split their voting power. Currently, it is not possible for them to do "passthrough voting" and giving their users voting rights over their tokens. However, with this system, exchanges can poll their users for voting preferences, and then vote on-chain proportionally to the results of the poll. Often times the entity owning that address might not be a single individual. For example, a company might have different stakeholders who want to vote differently, and so it makes sense to allow them to split their voting power. Currently, it is not possible for them to do "passthrough voting" and giving their users voting rights over their tokens. However, with this system, exchanges can poll their users for voting preferences, and then vote on-chain proportionally to the results of the poll.Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/gov/go.modis excluded by!**/*.modx/gov/types/v1/gov.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- api/cosmos/gov/v1/gov.pulsar.go (16 hunks)
- x/gov/README.md (1 hunks)
- x/gov/keeper/abci.go (3 hunks)
- x/gov/migrations/v6/store.go (2 hunks)
- x/gov/proto/cosmos/gov/v1/gov.proto (2 hunks)
- x/gov/simulation/genesis.go (1 hunks)
- x/gov/types/v1/params.go (4 hunks)
Files not summarized due to errors (1)
- api/cosmos/gov/v1/gov.pulsar.go: Error: Message exceeds token limit
Additional context used
Path-based instructions (6)
x/gov/migrations/v6/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/simulation/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/keeper/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/types/v1/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"api/cosmos/gov/v1/gov.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
x/gov/README.md
[uncategorized] ~26-~26: The abbreviation/initialism is missing a period after the last letter.
Context: ...his module is in use on the Cosmos Hub (a.k.a gaia)...
[grammar] ~87-~87: Possible subject-verb agreement error detected.
Context: ...self. Modules such asx/upgrade, that want to allow certain messages to be execute...
[style] ~94-~94: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...eck. :::warning Ultimately, governance is able to execute any proposal, even if they were...
[uncategorized] ~94-~94: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...n't meant to be executed by governance (ie. no authority present). Messages without...
[grammar] ~95-~95: This is normally spelled as one word.
Context: ...cute any message, effectively acting as super user. ::: ### Deposit To prevent spam, pro...
[grammar] ~103-~103: The usual collocation for “accompany” is “by”, not “with”.
Context: ...n a proposal is submitted, it has to be accompanied with a deposit that must be strictly positiv...
[uncategorized] ~110-~110: Possible missing article found.
Context: ...oyed: the proposal will be removed from state and the deposit will be burned (see x/g...
[style] ~136-~136: Consider using “who” when you are referring to people instead of objects.
Context: ... Participants Participants are users that have the right to vote on proposals. On...
[uncategorized] ~172-~172: Possible missing comma found.
Context: ... of its voting power to vote No. Often times the entity owning that address might no...
[typographical] ~198-~198: It seems that a comma is missing.
Context: ...oposal for the result to be valid. ### Yes Quorum Yes quorum is a more restrictiv...
[typographical] ~199-~199: It seems that a comma is missing.
Context: ...he result to be valid. ### Yes Quorum Yes quorum is a more restrictive quorum tha...
[style] ~201-~201: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...minimum percentage of voting power that needs to have votedYesfor the proposal to pa...
[style] ~277-~277: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... quorum. *BurnProposalDepositPrevoteburns the proposal deposit if it does not ent...
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
[uncategorized] ~304-~304: When specifying a month and year, the comma is unnecessary.
Context: ...validators do? * Terra crash of May, 2022, saw validators choose to run a new bin...
[formatting] ~304-~304: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...that had not been approved by governance, because the governance token had been inflated ...
[style] ~310-~310: Consider shortening this phrase to strengthen your wording.
Context: ...by governance. Communities whishing to make amendments to their original constitution should use ...
[style] ~320-~320: Consider using “arise” to strengthen your wording.
Context: ...nesis, so that when difficult questions come up, the constutituon can provide guidance ...
[typographical] ~339-~339: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ta` field allows custom use for networks, however, it is expected that the field contains ...
[grammar] ~357-~357: The verb form ‘are’ does not appear to fit in this context.
Context: ...keeper as a config. The default maximum length are: for the title 255 characters, for the ...
[misspelling] ~357-~357: Make sure that ‘the one of’ is correct and that ‘one’ is a pronoun. Possibly, the ‘the’ is unnecessary or ‘of’ is better expressed with a preposition such as ‘about’ or ‘in’.
Context: ... for summary 10200 characters (40 times the one of the title). #### Writing a module that...
[style] ~362-~362: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...as changing various parameters. This is very simple to do. First, write out your message ty...
[uncategorized] ~378-~378: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...d, a new parameter set has to be created and the previous one rendered inactive. ##...
[grammar] ~445-~445: Did you mean “querying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...|'Params'toParams`. This map allows to query all x/gov params. For pseudocode p...
[uncategorized] ~450-~450: Loose punctuation mark.
Context: ...rite in stores: *load(StoreKey, Key): Retrieve item stored at keyKeyin st...
[uncategorized] ~457-~457: Loose punctuation mark.
Context: ... Store: *ProposalProcessingQueue: A queuequeue[proposalID]containing ...
[grammar] ~472-~472: Did you mean “submitting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... any messages, a legacy proposal allows to submit a set of pre-defined proposals. These p...
[uncategorized] ~509-~509: The preposition ‘to’ seems more likely in this position.
Context: ...itis reached: * PushproposalIDinProposalProcessingQueue* TransferI...
[grammar] ~521-~521: Consider using either the past participle “conformed” or the present participle “conforming” here.
Context: ...voting period * The deposited coins are conform to the accepted denom from the `MinDepo...
[style] ~539-~539: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...starts. From there, bonded Atom holders are able to sendMsgVotetransactions to cast the...
[style] ~551-~551: ‘take into account’ might be wordy. Consider a shorter alternative.
Context: ...::note Gas cost for this message has to take into account the future tallying of the vote in EndB...
[typographical] ~638-~638: Two consecutive dots
Context: ... | string (address) | "cosmos1.." or empty for burn | | propos...
[uncategorized] ~643-~643: Possible missing comma found.
Context: ...nce module contains parameters that are objects unlike other modules. If only a subset ...
[grammar] ~644-~644: Make sure the noun ‘subset’ is in agreement with the verb ‘are’. Beware that some collective nouns (like ‘police’ or ‘team’) can be treated as both singular and plural.
Context: ...modules. If only a subset of parameters are desired to be changed, only they need t...
[uncategorized] ~647-~647: This expression is usually spelled with a hyphen.
Context: ...entire parameter object structure. ### Message Based Parameters In addition to the paramete...
[uncategorized] ~2465-~2465: Did you mean: “By default,”?
Context: ...t the on-chain actions they are taking. By default all metadata fields have a 255 characte...
[style] ~2465-~2465: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ... of proposals made by all groups may be quite large. Second, that client applications such ...
[grammar] ~2489-~2489: When ‘255-character’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Vote Location: on-chain as json within 255 character limit (mirrors [group vote](../group/RE...
Markdownlint
x/gov/README.md
36-36: Expected: 2; Actual: 4
Unordered list indentation
37-37: Expected: 2; Actual: 4
Unordered list indentation
38-38: Expected: 2; Actual: 4
Unordered list indentation
40-40: Expected: 2; Actual: 4
Unordered list indentation
41-41: Expected: 2; Actual: 4
Unordered list indentation
42-42: Expected: 2; Actual: 4
Unordered list indentation
43-43: Expected: 2; Actual: 4
Unordered list indentation
44-44: Expected: 2; Actual: 4
Unordered list indentation
45-45: Expected: 2; Actual: 4
Unordered list indentation
46-46: Expected: 2; Actual: 4
Unordered list indentation
48-48: Expected: 2; Actual: 4
Unordered list indentation
49-49: Expected: 2; Actual: 4
Unordered list indentation
50-50: Expected: 2; Actual: 4
Unordered list indentation
52-52: Expected: 2; Actual: 4
Unordered list indentation
53-53: Expected: 2; Actual: 4
Unordered list indentation
56-56: Expected: 2; Actual: 4
Unordered list indentation
57-57: Expected: 2; Actual: 4
Unordered list indentation
58-58: Expected: 2; Actual: 4
Unordered list indentation
60-60: Expected: 2; Actual: 4
Unordered list indentation
61-61: Expected: 2; Actual: 4
Unordered list indentation
301-301: Expected: 2; Actual: 4
Unordered list indentation
302-302: Expected: 2; Actual: 4
Unordered list indentation
303-303: Expected: 2; Actual: 4
Unordered list indentation
304-304: Expected: 4; Actual: 8
Unordered list indentation
306-306: Expected: 2; Actual: 4
Unordered list indentation
509-509: Expected: 2; Actual: 4
Unordered list indentation
533-533: Expected: 2; Actual: 4
Unordered list indentation
312-312: null
Emphasis used instead of a heading
27-27: null
Link fragments should be valid
62-62: null
Link fragments should be valid
Additional comments not posted (24)
x/gov/migrations/v6/store.go (1)
59-59: The addition ofProposalExecutionGastogovParamsaligns with the PR's objectives to manage gas consumption effectively. Ensure that all related components that utilizegovParamsare aware of this new field.x/gov/simulation/genesis.go (1)
197-197: The inclusion of10_000_000as a parameter in theRandomizedGenStatefunction aligns with the newProposalExecutionGasparameter, ensuring consistency in simulation environments.x/gov/keeper/abci.go (2)
80-80: The updated error handling in theEndBlockerfunction for out-of-gas errors enhances the robustness of proposal execution. This is a critical update for maintaining system stability.
Line range hint
196-227: The implementation ofExecuteWithGasLimitwithin theEndBlockerfunction is a significant enhancement. It ensures that proposal execution respects the newly introduced gas limits, aligning with the PR's objectives.x/gov/types/v1/params.go (2)
51-51: The addition ofproposalExecutionGasto theNewParamsfunction ensures that all new parameter instances will include the new gas limit setting, which is crucial for the governance module's operation.
103-103: SettingDefaultProposalExecutionGasin theDefaultParamsfunction ensures that the default settings are aligned with the new governance requirements regarding gas consumption.x/gov/proto/cosmos/gov/v1/gov.proto (1)
347-347: The addition of theproposal_execution_gasfield to theParamsmessage is well-integrated and aligns with the PR's objectives to manage gas consumption in governance proposals.x/gov/README.md (1)
281-286: The addition of theProposalExecutionGasparameter is well-explained and aligns with the PR's objectives to limit gas consumption during proposal execution.Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...api/cosmos/gov/v1/gov.pulsar.go (16)
6648-6648: Field descriptor forproposal_execution_gascorrectly added.
6675-6675: Initialization ofproposal_execution_gasfield descriptor is correct.
6869-6874: Handling ofProposalExecutionGasin conditional block is implemented correctly.
6932-6933: Case forproposal_execution_gascorrectly added to switch statement.
6992-6993: Case for resettingproposal_execution_gascorrectly added to switch statement.
7082-7084: Case for retrievingproposal_execution_gasvalue correctly added to switch statement.
7153-7154: Case for settingproposal_execution_gascorrectly added to switch statement.
7238-7239: Case for handling immutability ofproposal_execution_gascorrectly added to switch statement.
7301-7302: Case for default value ofproposal_execution_gascorrectly added to switch statement.
7459-7461: Handling for size calculation ofproposal_execution_gasimplemented correctly.
7491-7497: Handling for encodingproposal_execution_gasimplemented correctly.
8408-8426: Error handling for incorrect wire type ofproposal_execution_gasduring unmarshalling is implemented correctly.
10137-10138:ProposalExecutionGasfield correctly added to theParamsmessage.
10308-10313: Getter method forProposalExecutionGascorrectly implemented.
10572-10572: Binary encoding forProposalExecutionGasappears to be handled correctly.
10674-10749: Binary encoding forProposalExecutionGasappears to be handled correctly.
|
|
||
| Execution is the process of executing the messages contained in a proposal. The execution phase will commence after the proposal has been accepted by the network. The messages contained in the proposal will be executed in the order they were submitted. | ||
|
|
||
| Execution has a upper limit on how much gas can be consumed in a single block. This limit is defined by the `ProposalExecutionGas` parameter. |
There was a problem hiding this comment.
Correct the article usage for better grammatical accuracy.
- Execution has a upper limit on how much gas can be consumed in a single block.
+ Execution has an upper limit on how much gas can be consumed in a single block.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Execution has a upper limit on how much gas can be consumed in a single block. This limit is defined by the `ProposalExecutionGas` parameter. | |
| Execution has an upper limit on how much gas can be consumed in a single block. This limit is defined by the `ProposalExecutionGas` parameter. |
Tools
LanguageTool
[misspelling] ~285-~285: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...der they were submitted. Execution has a upper limit on how much gas can be cons...
julienrbrt
changed the title
x/gov/keeper/abci.go
Outdated
| }) | ||
| 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?
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- x/accounts/go.mod (4 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/keeper/abci.go (7 hunks)
- x/gov/proto/cosmos/gov/v1/gov.proto (2 hunks)
- x/gov/types/v1/params.go (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/gov/keeper/abci.go
- x/gov/proto/cosmos/gov/v1/gov.proto
- x/gov/types/v1/params.go
Additional context used
Path-based instructions (1)
x/gov/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/gov/CHANGELOG.md
[grammar] ~42-~42: You should probably use “replicate”. (AGREEMENT_SENT_START)
Context: ...clude vote options counts. Those counts replicates the now deprecated (but not removed) ye...
[style] ~72-~72: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...` have been moved to the keeper package in order to support the custom tallying function. *...
Additional comments not posted (1)
x/accounts/go.mod (1)
10-10: Dependency updates are correct and match the specified versions.Also applies to: 96-96
x/gov/CHANGELOG.md
Outdated
| @@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * [#18762](https://github.com/cosmos/cosmos-sdk/pull/18762) Add multiple choice proposals. | |||
| * [#18856](https://github.com/cosmos/cosmos-sdk/pull/18856) Add `ProposalCancelMaxPeriod` parameters. | |||
| * [#19167](https://github.com/cosmos/cosmos-sdk/pull/19167) Add `YesQuorum` parameter. | |||
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov exectution of proposals to a max gas limit. The limit was added to parameters and can be modified | |||
There was a problem hiding this comment.
Clarify the changelog entry for better understanding.
- * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov exectution of proposals to a max gas limit. The limit was added to parameters and can be modified
+ * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Introduce a configurable maximum gas limit for governance proposal execution, enhancing control over resource usage.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov exectution of proposals to a max gas limit. The limit was added to parameters and can be modified | |
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Introduce a configurable maximum gas limit for governance proposal execution, enhancing control over resource usage. |
x/gov/keeper/abci.go
Outdated
| 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.
Status, FailedReason and tagValue are the same as for other errors. Would it make sense to move the error check below and have an if/else for the logMsg only?
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)
Given that we cannot know if it is out of gas or execution error (or anything else).
Imho, the log message should stay generic.
| @@ -56,6 +56,7 @@ func MigrateStore(ctx context.Context, storeService corestoretypes.KVStoreServic | |||
| govParams.OptimisticAuthorizedAddresses = defaultParams.OptimisticAuthorizedAddresses | |||
| govParams.OptimisticRejectedThreshold = defaultParams.OptimisticRejectedThreshold | |||
| govParams.ProposalCancelMaxPeriod = defaultParams.ProposalCancelMaxPeriod | |||
| govParams.ProposalExecutionGas = defaultParams.ProposalExecutionGas | |||
Co-authored-by: Alexander Peters <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
x/gov/CHANGELOG.md (4)
Line range hint
42-42: Correct the verb agreement for better grammatical accuracy.- Those counts replicates the now deprecated (but not removed) yes, no, abstain and veto count fields. + Those counts replicate the now deprecated (but not removed) yes, no, abstain, and veto count fields.Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Line range hint
72-72: Consider a more concise phrasing.- in order to support the custom tallying function. + to support the custom tallying function.Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Line range hint
66-69: Adjust the indentation to comply with markdown standards.- * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`. - * `Prompt` and `PromptMetadata` take an address.Codec as arguments. - * `SetProposer` takes a String as an argument instead of a `fmt.Stringer`. + * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`. + * `Prompt` and `PromptMetadata` take an address.Codec as arguments. + * `SetProposer` takes a String as an argument instead of a `fmt.Stringer`.Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Line range hint
66-66: Remove trailing spaces for cleaner markdown formatting.- * [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method: + * [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method:Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/gov/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
x/gov/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/gov/CHANGELOG.md
[grammar] ~42-~42: You should probably use “replicate”. (AGREEMENT_SENT_START)
Context: ...clude vote options counts. Those counts replicates the now deprecated (but not removed) ye...
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
[style] ~72-~72: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...` have been moved to the keeper package in order to support the custom tallying function. *...
Markdownlint
x/gov/CHANGELOG.md
67-67: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
68-68: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
69-69: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
66-66: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
| @@ -56,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * [#18762](https://github.com/cosmos/cosmos-sdk/pull/18762) Add multiple choice proposals. | |||
| * [#18856](https://github.com/cosmos/cosmos-sdk/pull/18856) Add `ProposalCancelMaxPeriod` parameters. | |||
| * [#19167](https://github.com/cosmos/cosmos-sdk/pull/19167) Add `YesQuorum` parameter. | |||
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was infinite gas. | |||
There was a problem hiding this comment.
Add a comma for better readability.
- With this version the default is set to 10 million gas.
+ With this version, the default is set to 10 million gas.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was infinite gas. | |
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version, the default is set to 10 million gas. Before it was infinite gas. |
Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Correct the pluralization for grammatical accuracy.
- Before it was infinite gas.
+ Before it was an infinite amount of gas.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was infinite gas. | |
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was an infinite amount of gas. |
Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
Refine the changelog entry for better clarity and correctness.
- * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was infinite gas.
+ * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Introduce a configurable maximum gas limit for governance proposal execution, enhancing control over resource usage. The default limit is now set to 10 million gas units, replacing the previous infinite limit.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Limit gov execution of proposals to a max gas limit. The limit was added to parameters and can be modified. With this version the default is set to 10 million gas. Before it was infinite gas. | |
| * [#20348](https://github.com/cosmos/cosmos-sdk/pull/20348) Introduce a configurable maximum gas limit for governance proposal execution, enhancing control over resource usage. The default limit is now set to 10 million gas units, replacing the previous infinite limit. |
Tools
LanguageTool
[typographical] ~58-~58: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...rameters and can be modified. With this version the default is set to 10 million gas. B...
[grammar] ~58-~58: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...ersion the default is set to 10 million gas. Before it was infinite gas. ### Clien...
* main: fix(x/staking): stop validators from rotating to the same key on the same block (#20649) perf: add cache to address codec (#20122) build(deps): Bump google.golang.org/protobuf from 1.34.1 to 1.34.2 (#20632) fix: remove recipient amount from map (#20625) fix(proto): remove conditional preventing proper generated file placement (#20650) (serverv2/cometbft) Read config from commands & handle `FlagNode` (#20621) fix(x/consensus): fix .proto file placement (#20646) fix(store): avoid nil error on not exhausted payload stream (#20644) fix (x/accounts): Fix genesis condition check (#20645) feat(accounts): add genesis account initialization (#20642) fix(x/gov): limit execution in gov (#20348)
Description
limit execution of gov proposals to infinite messages.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Enhancements
ProposalExecutionGasto governance parameters for better control over proposal execution.Dependency Updates