docs(specs): add missing versioned consts #4054
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to several files related to application constants and parameters across different versions. Key changes include the addition of clarifying comments for timeout constants in versions 1 and 2, updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 0
🧹 Outside diff range and nitpick comments (6)
specs/src/parameters_v2.md (3)
14-14: Enhance documentation for SubtreeRootThresholdThe current description only references ADR 13 without providing any context. Consider adding a brief explanation of what this threshold represents directly in the parameters document for better readability.
16-16: Maintain consistent unit notationThe spacing in "100 MiB" is inconsistent with other size units in the codebase. Consider standardizing to "100MiB" to match the format used elsewhere.
15-15: Fix hyphenation in compound modifier"Height based" should be hyphenated as "Height-based" since it's a compound modifier preceding the noun "delay".
-| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v3.md (2)
14-14: Enhance SubtreeRootThreshold documentationThe current description only references ADR 13 without providing direct context. Consider adding a brief explanation of the parameter's purpose and impact.
Would you like me to help draft a more detailed description that includes the key points from ADR 13?
18-18: Fix hyphenation in "Height based"The phrase "Height based" should be hyphenated as "Height-based" according to standard English grammar rules.
-| UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 100800 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v1.md (1)
14-14: Add link to ADR 13 for SubtreeRootThreshold.The reference to ADR 13 should be a clickable link to help readers find more details easily.
-| SubtreeRootThreshold | 64 | See ADR 13 for more details. | False | +| SubtreeRootThreshold | 64 | See [ADR 13](../architecture/adr-013-subtree-based-data-square-construction.md) for more details. | False |
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/appconsts/v1/app_consts.go(1 hunks)pkg/appconsts/v2/app_consts.go(1 hunks)pkg/appconsts/v3/app_consts.go(1 hunks)specs/src/parameters_v1.md(1 hunks)specs/src/parameters_v2.md(1 hunks)specs/src/parameters_v3.md(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/appconsts/v2/app_consts.go
- pkg/appconsts/v3/app_consts.go
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (7)
pkg/appconsts/v1/app_consts.go (2)
9-13: Verify the impact of potential constant removal
The comment indicates these timeout values weren't actually constants in v1 and suggests their removal. Before proceeding with removal, we should verify their usage across the codebase.
Let's check for references to these constants:
14-19: Consider relocating UpgradeHeightDelay to appropriate version
The TODO correctly questions the presence of this constant in v1 since it lacks the signal module. This suggests an architectural inconsistency.
Let's verify the signal module integration:
Consider moving this constant to the appropriate version package where the signal module is first introduced to maintain better version boundaries.
specs/src/parameters_v3.md (2)
29-29: LGTM! Module parameters are well documented
The parameters TxSizeCostPerByte and GasPerBlobByte are clearly documented with appropriate values, descriptions, and governance changeability settings.
Also applies to: 31-31
Line range hint 1-19: LGTM! Document structure aligns with versioning requirements
The parameters documentation is well-structured and properly versioned, which aligns with the PR objectives of adding missing versioned constants. The tables provide clear organization of parameters with their values, descriptions, and governance changeability status.
Also applies to: 29-31
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v1.md (3)
11-12: LGTM! Clear and consistent table format.
The table headers are well-structured and accurately represent the parameter documentation.
15-15: LGTM! Consistent unit formatting.
The formatting change for MaxBlockSizeBytes from 100MiB to 100 MiB improves readability and maintains consistency with other size parameters in the document.
13-14: Verify the relationship between SquareSizeUpperBound and GovMaxSquareSize.
The parameter change from MaxSquareSize to SquareSizeUpperBound looks good, but we should verify its relationship with the blob.GovMaxSquareSize parameter mentioned later in the module parameters section.
✅ Verification successful
Let me analyze the relationship between SquareSizeUpperBound and GovMaxSquareSize based on the search results.
The key findings from the codebase show:
-
SquareSizeUpperBoundis a versioned constant that imposes a hard upper bound on the max effective square size (currently 128 as seen in v1, v2, and v3 constants). -
GovMaxSquareSizeis a governance-modifiable parameter with a default value of 64 (as seen inDefaultGovMaxSquareSize). -
The actual maximum square size used is determined by taking the minimum of these two values, as seen in
app/square_size.go:
govMax := int(app.BlobKeeper.GovMaxSquareSize(ctx))
hardMax := appconsts.SquareSizeUpperBound(app.AppVersion())
return min(govMax, hardMax)The relationship is clear: SquareSizeUpperBound acts as a hard cap that GovMaxSquareSize cannot exceed.
SquareSizeUpperBound(128) defines the absolute maximum limitGovMaxSquareSize(default 64) is a governance-adjustable parameter that must stay within this limit- The effective maximum square size is the minimum of these two values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to these parameters in the codebase
rg --type go "SquareSizeUpperBound|MaxSquareSize|GovMaxSquareSize" -A 3
Length of output: 32317
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
specs/src/parameters_v2.md (1)
15-15: Add hyphen to improve readabilityThe phrase "Height based" should be hyphenated as "Height-based" to improve readability.
-| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v3.md (1)
18-18: Add hyphen to "height-based"The description should use a hyphenated form for the compound adjective.
-| UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 100800 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v1.md (1)
14-14: Add a brief description of the parameter's purpose.While linking to ADR-013 is good for detailed information, consider adding a brief summary of what this threshold controls. This would help readers understand the parameter's purpose without having to navigate to the ADR.
-| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False | +| SubtreeRootThreshold | 64 | Threshold value for subtree root calculation in the zero-padding rules. See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
specs/src/parameters_v1.md(1 hunks)specs/src/parameters_v2.md(1 hunks)specs/src/parameters_v3.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (10)
specs/src/parameters_v2.md (4)
11-12: LGTM: Clear and well-structured table header
The table structure effectively organizes the parameter documentation with appropriate column headers.
13-13: LGTM: Clear documentation for SquareSizeUpperBound
The parameter is well-documented with a clear explanation of its purpose and limitations.
14-14: Add more context to the ADR link description
16-16: LGTM: Clear documentation for MaxBlockSizeBytes
The parameter documentation is clear and follows proper notation for binary size units.
specs/src/parameters_v3.md (4)
13-15: LGTM: Clear and well-documented new parameters
The addition of SquareSizeUpperBound, SubtreeRootThreshold, and MaxTxSize parameters with their descriptions and governance modifiability status enhances the documentation clarity.
16-17: LGTM: Improved documentation with spec links
The addition of CometBFT specification links for TimeoutPropose and TimeoutCommit parameters provides valuable context for understanding the consensus process.
29-29: LGTM: Well-documented module parameters
The TxSizeCostPerByte and GasPerBlobByte parameters are clearly documented with their values, descriptions, and governance modifiability status.
Also applies to: 31-31
Line range hint 1-90: LGTM: Comprehensive parameter documentation
The document provides a well-structured and thorough documentation of all parameters, including:
- Clear organization of global and module parameters
- Consistent table format with values, summaries, and governance modifiability
- Helpful note about mint module parameters being hardcoded constants
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v1.md (2)
13-13: LGTM! Improved parameter naming.
The rename from MaxSquareSize to SquareSizeUpperBound better reflects its purpose as a hard limit on shares per row or column. The description is clear and accurate.
15-15: LGTM! Improved value formatting.
The spacing in 100 MiB improves readability compared to the previous 100MiB format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
specs/src/parameters_v2.md (1)
15-16: Fix hyphenation in UpgradeHeightDelay descriptionThe changes to include units and improve formatting are good. However, there's a minor grammatical issue to fix.
-| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v3.md (1)
18-18: Add hyphen to "Height-based"For better readability and correct grammar, modify the description to use "Height-based delay" instead of "Height based delay".
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
pkg/appconsts/v1/app_consts.go(1 hunks)pkg/appconsts/v2/app_consts.go(1 hunks)specs/src/parameters_v1.md(1 hunks)specs/src/parameters_v2.md(1 hunks)specs/src/parameters_v3.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/appconsts/v1/app_consts.go
- pkg/appconsts/v2/app_consts.go
- specs/src/parameters_v1.md
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (5)
specs/src/parameters_v2.md (2)
11-12: LGTM! Header changes improve clarity
The change from "Default" to "Value" in the column header is more precise and better reflects the content.
13-14: Improve formatting of SubtreeRootThreshold description
The content is correct, but let's maintain consistent formatting with the suggestion from the previous review.
-| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |
+| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |specs/src/parameters_v3.md (3)
16-17: LGTM! Well-documented timeout parameters
The TimeoutPropose and TimeoutCommit parameters are well documented with clear descriptions and helpful links to the CometBFT specifications.
29-29: LGTM! Clear documentation of gas-related parameters
The TxSizeCostPerByte and GasPerBlobByte parameters are well documented with clear values and descriptions. The governance modifiability is correctly specified as False for both parameters.
Also applies to: 31-31
11-19: LGTM! Comprehensive documentation of versioned constants
The documentation successfully captures all the necessary versioned constants with clear specifications, values, and governance modifiability status. The inclusion of references to ADRs and specifications enhances the documentation quality.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
specs/src/parameters_v2.md (1)
15-15: Add hyphen to "height-based"For better readability and correct grammar.
-| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v3.md (1)
18-18: Fix hyphenation in parameter descriptionThe phrase "Height based" should be hyphenated as "Height-based" for correct compound adjective usage.
-| UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 100800 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
pkg/appconsts/v1/app_consts.go(1 hunks)pkg/appconsts/v2/app_consts.go(1 hunks)specs/src/parameters_v1.md(1 hunks)specs/src/parameters_v2.md(1 hunks)specs/src/parameters_v3.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/appconsts/v1/app_consts.go
- pkg/appconsts/v2/app_consts.go
- specs/src/parameters_v1.md
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (4)
specs/src/parameters_v2.md (2)
13-14: LGTM! Well-documented parameter additions
The new parameters SquareSizeUpperBound and SubtreeRootThreshold are well-documented with clear descriptions and appropriate values. The ADR link provides excellent context for the SubtreeRootThreshold parameter.
15-16: LGTM! Improved value formatting
The updated formatting for UpgradeHeightDelay (adding "blocks") and MaxBlockSizeBytes (space between value and unit) improves clarity and consistency.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md (2)
29-31: LGTM: Parameters well documented
The gas-related parameters are clearly documented with appropriate values and governance modifiability flags.
11-19: LGTM: Well-structured parameter documentation
The global parameters section is well-organized with clear values, units, and governance modifiability flags. The new parameters (SquareSizeUpperBound, SubtreeRootThreshold, MaxTxSize) are properly documented with appropriate context and references.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
specs/src/parameters_v2.md (2)
15-15: Add hyphen to compound adjectiveThe phrase "Height based" should be hyphenated as it's used as a compound adjective.
-| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
15-15: Enhance UpgradeHeightDelay descriptionThe current description could be clearer about the purpose and impact of this delay.
-| UpgradeHeightDelay | 50400 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 50400 blocks | Number of blocks to wait after a successful upgrade proposal (`MsgTryUpgrade`) before applying the upgrade. This delay allows nodes to prepare for the upgrade. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
specs/src/parameters_v3.md (1)
18-18: Add hyphen in "Height-based"Minor grammatical improvement needed in the description.
-| UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | +| UpgradeHeightDelay | 100800 blocks | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...(BASED_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
pkg/appconsts/v1/app_consts.go(1 hunks)pkg/appconsts/v2/app_consts.go(1 hunks)specs/src/parameters_v1.md(1 hunks)specs/src/parameters_v2.md(1 hunks)specs/src/parameters_v3.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/appconsts/v1/app_consts.go
- pkg/appconsts/v2/app_consts.go
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ...| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~18-~18: This expression is usually spelled with a hyphen.
Context: ... UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (3)
specs/src/parameters_v3.md (2)
13-17: LGTM! Well-documented parameters with helpful spec links.
The new parameters are clearly documented, and the added CometBFT specification links provide valuable context for timeout parameters.
29-29: LGTM! Clear documentation of gas-related parameters.
The gas cost parameters are well-documented with appropriate governance restrictions.
Also applies to: 31-31
specs/src/parameters_v1.md (1)
11-12: LGTM: Improved column header terminology
The change from "Default" to "Value" better reflects that these are fixed parameters rather than default values that can be changed.
| | SquareSizeUpperBound | 128 | Hardcoded maximum square size which limits the number of shares per row or column for the original data square (not yet extended). | False | | ||
| | SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False | | ||
| | MaxBlockSizeBytes | 100 MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False | |
There was a problem hiding this comment.
💡 Codebase verification
Parameter naming inconsistency detected - MaxSquareSize vs SquareSizeUpperBound
The codebase shows inconsistency between the documented parameter name and its implementation:
- The parameter is documented as
SquareSizeUpperBoundin the specs - However, the codebase extensively uses
MaxSquareSizeandGovMaxSquareSizein the implementation - The
SubtreeRootThresholdparameter is consistently used and matches the documentation - The
MiBunit formatting is consistently used across the spec files
🔗 Analysis chain
LGTM: Clear and well-documented parameter definitions
The changes improve clarity and completeness:
SquareSizeUpperBoundis a more descriptive name than the previousMaxSquareSize- Added the missing
SubtreeRootThresholdparameter with proper ADR reference - Improved formatting consistency for binary units in
MaxBlockSizeBytes
Let's verify the consistency of these parameter names across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify parameter naming consistency across the codebase
# Check for any remaining references to the old MaxSquareSize name
rg "MaxSquareSize" --type go
# Verify SubtreeRootThreshold usage
rg "SubtreeRootThreshold" --type go
# Check for consistent binary unit formatting
rg "MiB" --type md specs/
Length of output: 16613
pkg/appconsts/v1/app_consts.go
Outdated
| // TODO: why does this constant exist in v1? v1 does not contain the signal | ||
| // module. |
There was a problem hiding this comment.
Yeah I think it should be removed
There was a problem hiding this comment.
I just marked them deprecated in the most recent commit on this PR. I don't really want to remove them b/c it's breaking.
If you want them to be removed, I can do it in a follow-up PR.
Not yet. See #3995 |
Closes #4010