docs: remaining docs from versioned gas scheduler variables #3914
Conversation
|
ninabarbakadze
requested review from
cmwaters and
staheri14
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces new documentation entries for "AnteHandler v3" and "Parameters v3" in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (6)
specs/src/parameters.md (1)
7-7: Consider adding brief context about Parameters v3While the addition of the v3 link is good, it might be helpful to provide a brief description of what's new or different in v3 compared to v1 and v2. This context could help users understand when to refer to which version.
Consider adding a short description, for example:
- [Parameters v2](./parameters_v2.md) - - [Parameters v3](./parameters_v3.md) + +- [Parameters v3](./parameters_v3.md) (Includes updated gas scheduler variables)specs/src/SUMMARY.md (2)
27-27: LGTM! Consider adding a brief description.The addition of the "Parameters v3" entry is well-placed and follows the existing structure and formatting conventions. This change aligns with the PR objective of adding a new parameters page to the specifications.
To improve clarity, consider adding a brief description of what's new or different in Parameters v3. This could be done by adding a short comment next to the link, for example:
- [Parameters v3](./parameters_v3.md) <!-- New gas scheduler variables -->This would provide users with a quick overview of what to expect in the new document without having to navigate to it.
Inconsistent Versioning Detected Across Sections
The verification shows that only "State Machine Modules" includes versioning (v2), while other sections like "Parameters" have different version numbers. To maintain consistency and clarity:
- Update all sections to follow a uniform versioning scheme.
- Add a brief explanation of the versioning strategy at the beginning of the document.
🔗 Analysis chain
Line range hint
1-27: Verify consistency of versioning across all sectionsThe addition of "Parameters v3" maintains the versioning approach used in other sections of the document. To ensure overall consistency and completeness:
- Verify if other sections (e.g., AnteHandler, State Machine Modules) require updates to include their latest versions.
- Consider adding a brief note at the beginning of the document explaining the versioning system used throughout the specifications.
To assist in this verification, you can run the following script to check for inconsistencies in versioning across sections:
This script will help identify any sections that may need updating to maintain consistency with the newly added "Parameters v3".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistencies in versioning across sections # Test: List all versioned entries and their latest versions echo "Versioned entries and their latest versions:" rg -n '(\w+) v\d+' specs/src/SUMMARY.md | awk -F':' '{print $2}' | sort | awk '{a[$1]=$0} END{for(i in a) print a[i]}' echo -e "\nSections with only one version:" rg -n '(\w+) v1' specs/src/SUMMARY.md | awk -F':' '{print $2}' | awk '{print $1}' | sort | uniq -u echo -e "\nSections with multiple versions:" rg -n '(\w+) v\d+' specs/src/SUMMARY.md | awk -F':' '{print $2}' | awk '{print $1}' | sort | uniq -dLength of output: 641
🧰 Tools
🪛 LanguageTool
[duplication] ~24-~24: Possible typo: you repeated a word
Context: ...s v2](./state_machine_modules_v2.md) - Parameters - Parameters v1 - [Parameters...(ENGLISH_WORD_REPEAT_RULE)
specs/src/ante_handler_v3.md (3)
1-19: Excellent documentation of AnteHandler v3 criteriaThe introduction and list of criteria provide a clear and comprehensive overview of the AnteHandler for app version 3. The inclusion of links to relevant code is particularly helpful for developers.
On line 15, consider changing "a.k.a nonce" to "a.k.a. nonce" for correct abbreviation punctuation.
To ensure long-term maintainability, consider replacing hardcoded constant values with references to the actual constants in the code. This approach would help keep the documentation synchronized with any future changes to these values. For example:
- The tx's [memo](https://github.com/cosmos/cosmos-sdk/blob/22c28366466e64ebf0df1ce5bec8b1130523552c/proto/cosmos/tx/v1beta1/tx.proto#L110-L113) is <= the max memo characters where [`MaxMemoCharacters`](https://github.com/cosmos/cosmos-sdk/blob/a429238fc267da88a8548bfebe0ba7fb28b82a13/x/auth/README.md?plain=1#L230) is defined.This way, if the constant value changes, only the link would need to be updated, reducing the risk of inconsistencies between the code and documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: The abbreviation/initialism is missing a period after the last letter.
Context: ...e that the signature's sequence number (a.k.a nonce) matches the account sequence num...(ABBREVIATION_PUNCTUATION)
18-19: Provide more context for additional criteriaThe inclusion of criteria for MsgSubmitProposal and IBC packets is valuable. However, these points could benefit from additional context or explanation.
The criteria are clear and concise.
Consider expanding on these points to provide more context:
- For the MsgSubmitProposal criterion, explain why zero proposal messages are not allowed and what the implications are.
- For the IBC packet criterion, provide a brief explanation of why preventing double-processing is important and how it relates to the overall system integrity.
This additional information would help developers understand the rationale behind these criteria and their importance in the system.
21-25: Clear explanation of AnteHandler side effectsThe side effects section provides valuable information about the additional behaviors of the AnteHandler.
The three listed side effects are clear and informative.
On line 21, "side-effects" should be "side effects" without a hyphen, according to standard English usage.
- In addition to the above criteria, the AnteHandler also has a number of side-effects: + In addition to the above criteria, the AnteHandler also has a number of side effects:This minor correction will improve the document's adherence to standard English grammar.
🧰 Tools
🪛 LanguageTool
[misspelling] ~21-~21: Did you mean “side effects” (=adverse effect, unintended consequence)? Open compounds are not hyphenated.
Context: ...a, the AnteHandler also has a number of side-effects: - Tx fees are deducted from the tx's ...(AFFECT_EFFECT)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- specs/src/SUMMARY.md (1 hunks)
- specs/src/ante_handler_v3.md (1 hunks)
- specs/src/parameters.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/src/ante_handler_v3.md
[uncategorized] ~15-~15: The abbreviation/initialism is missing a period after the last letter.
Context: ...e that the signature's sequence number (a.k.a nonce) matches the account sequence num...(ABBREVIATION_PUNCTUATION)
[misspelling] ~21-~21: Did you mean “side effects” (=adverse effect, unintended consequence)? Open compounds are not hyphenated.
Context: ...a, the AnteHandler also has a number of side-effects: - Tx fees are deducted from the tx's ...(AFFECT_EFFECT)
🔇 Additional comments (2)
specs/src/parameters.md (1)
7-7: LGTM: Addition of Parameters v3 linkThe addition of the link to Parameters v3 is in line with the PR objectives and the linked issue #3762. This update expands the documentation to include the latest version of parameters.
specs/src/ante_handler_v3.md (1)
16-17: Clarify maximum blob share count criterionThe explanation of gas consumption for blobs is detailed and clear. However, the maximum blob share count criterion could benefit from additional clarification.
The blob gas consumption explanation is comprehensive and well-documented.
Could you provide more details on how the maximum blob share count is derived from the maximum valid square size? Specifically:
- What are
GovMaxSquareSizeandSquareSizeUpperBound?- How is the maximum blob share count calculated from these values?
- Are there any relevant constants or formulas that should be included?
Adding this information would make the criterion more understandable and actionable for developers.
| @@ -4,3 +4,4 @@ The parameters in the application depend on the app version: | |||
|
|
|||
| - [Parameters v1](./parameters_v1.md) | |||
| - [Parameters v2](./parameters_v2.md) | |||
| - [Parameters v3](./parameters_v3.md) | |||
There was a problem hiding this comment.
💡 Codebase verification
Parameters v3 File Missing Non-Governance Modifiability
- The
blob.GasPerBlobByteandauth.TxSizeCostPerByteparameters inspecs/src/parameters_v3.mdare not marked as non-governance modifiable as required by issue Add a params v3 page to the specs #3762.
🔗 Analysis chain
Verify the existence of parameters_v3.md file
Ensure that the parameters_v3.md file exists and contains the necessary information, including the blob.GasPerBlobByte and auth.TxSizeCostPerByte parameters mentioned in issue #3762.
Let's verify the existence and content of the file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of parameters_v3.md file
# Test 1: Check if the file exists
if fd -t f "parameters_v3.md" specs/src; then
echo "parameters_v3.md file exists."
else
echo "Error: parameters_v3.md file not found in specs/src directory."
exit 1
fi
# Test 2: Check if the file contains the required parameters
if rg -q "blob\.GasPerBlobByte" specs/src/parameters_v3.md &&
rg -q "auth\.TxSizeCostPerByte" specs/src/parameters_v3.md; then
echo "Required parameters found in parameters_v3.md"
else
echo "Warning: One or both required parameters not found in parameters_v3.md"
fi
# Test 3: Check if the parameters are marked as non-governance modifiable
if rg -q "non-governance modifiable" specs/src/parameters_v3.md; then
echo "Parameters are marked as non-governance modifiable"
else
echo "Warning: Parameters are not marked as non-governance modifiable"
fi
Length of output: 552
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
specs/src/ante_handler.md (1)
14-14: LGTM! Consider adding a brief description of AnteHandler v3.The addition of the link to AnteHandler v3 is appropriate and consistent with the existing structure. This change aligns well with the PR objectives to update the documentation.
To provide more context for users, consider adding a brief description of what's new in AnteHandler v3, such as the introduction of
blob.GasPerBlobByteandauth.TxSizeCostPerByteparameters. This could be done inline or by adding a short note after the list. For example:- [AnteHandler v3](./ante_handler_v3.md) AnteHandler v3 introduces new non-governance modifiable parameters: `blob.GasPerBlobByte` and `auth.TxSizeCostPerByte`.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- specs/src/SUMMARY.md (2 hunks)
- specs/src/ante_handler.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/src/SUMMARY.md
|
Shouldn't we hold out on this until we know if there will be a tx size capping antehandler in v3? |
We changed some gas scheduler variable related stuff which is used in ante handler so I updated that and NetworkMinGasPrice reference as well. tx size capping antehandler pr already adds another rule so we can just merge this and then add the tx size capping section if we do end up pushing it out. |
Overview