-
Notifications
You must be signed in to change notification settings - Fork 285
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
docs: update specs for v2 #3661
Conversation
because it is another list of pages that can be inconsistent with SUMMARY.md and SUMMARY.md is the only thing that matters for creating the specs book.
@@ -1,19 +0,0 @@ | |||
# Celestia App Specifications |
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.
Deleted this file b/c it's not necessary and a maintenance burden. It frequently becomes out of sync with SUMMARY.md which is the only thing that matters for generating the specs book
- 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 = 256`](<https://github.com/cosmos/cosmos-sdk/blob/a429238fc267da88a8548bfebe0ba7fb28b82a13/x/auth/README.md?plain=1#L230>). | ||
- The tx's [gas_limit](https://github.com/cosmos/cosmos-sdk/blob/22c28366466e64ebf0df1ce5bec8b1130523552c/proto/cosmos/tx/v1beta1/tx.proto#L211-L213) is > the gas consumed based on the tx's size where [`TxSizeCostPerByte = 10`](https://github.com/cosmos/cosmos-sdk/blob/a429238fc267da88a8548bfebe0ba7fb28b82a13/x/auth/README.md?plain=1#L232). | ||
- The tx's feepayer has enough funds to pay fees for the tx. The tx's feepayer is the feegranter (if specified) or the tx's first signer. Note the [feegrant](https://github.com/cosmos/cosmos-sdk/blob/v0.46.15/x/feegrant/README.md) module is enabled. | ||
- The tx's gas price is >= the network minimum gas price where [`NetworkMinGasPrice = 0.000001` utia](https://github.com/celestiaorg/celestia-app/blob/8caa5807df8d15477554eba953bd056ae72d4503/pkg/appconsts/v2/app_consts.go#L9). |
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.
new for v2
|
||
The AnteHandler chains together several decorators to ensure the following criteria are met for app version 2: | ||
|
||
- The tx does not contain any messages that are unsupported by the current app version. See `MsgVersioningGateKeeper`. |
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.
new for v2
| `v1.Version` | `uint64` | `1` | | First version of the application. Breaking changes (hard forks) must update this parameter. | | ||
| `v2.Version` | `uint64` | `2` | | Second version of the application. Breaking changes (hard forks) must update this parameter. | |
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.
The only change here is I got rid of VERSION_APP
and added v1.Version and v2.Version
| ibc.Transfer.SendEnabled | true | Enable sending tokens via IBC. | True | | ||
| icahost.HostEnabled | True | Enables or disables the Inter-Chain Accounts host module. | True | | ||
| icahost.AllowMessages | [icaAllowMessages] | Defines a list of sdk message typeURLs allowed to be executed on a host chain. | True | | ||
| minfee.NetworkMinGasPrice | 0.000001 utia | All transactions must have a gas price greater than or equal to this value. | True | |
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.
new for v2
## `celestia-app` modules | ||
|
||
- [blob](https://github.com/celestiaorg/celestia-app/blob/main/x/blob/README.md) | ||
- [minfee](https://github.com/celestiaorg/celestia-app/blob/main/x/minfee/README.md) |
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.
new for v2
- [minfee](https://github.com/celestiaorg/celestia-app/blob/main/x/minfee/README.md) | ||
- [mint](https://github.com/celestiaorg/celestia-app/blob/main/x/mint/README.md) | ||
- [paramfilter](https://github.com/celestiaorg/celestia-app/blob/main/x/paramfilter/README.md) | ||
- [signal](https://github.com/celestiaorg/celestia-app/blob/main/x/signal/README.md) |
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.
new for v2
- [interchain accounts](https://github.com/cosmos/ibc/blob/2921c5cec7b18e4ef77677e16a6b693051ae3b35/spec/app/ics-027-interchain-accounts/README.md) | ||
- [packetforwardmiddleware](https://github.com/cosmos/ibc-apps/blob/main/middleware/packet-forward-middleware/README.md) |
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.
new for v2
@@ -370,6 +369,32 @@ func (suite *GovParamsTestSuite) TestModifiableParams() { | |||
assert.Equal(want, got) | |||
}, | |||
}, | |||
{ |
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.
added tests to verify that the ICA params are indeed modifiable
WalkthroughWalkthroughThe changes introduce versioning to the Celestia App Specifications and refine the Changes
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (8)
specs/src/specs/state_machine_modules.md (2)
2-2
: Add missing article "the".The article "the" is missing before "Celestia app".
- Celestia app is built using the cosmos-sdk, and follows standard cosmos-sdk module structure. + The Celestia app is built using the cosmos-sdk, and follows standard cosmos-sdk module structure.Tools
LanguageTool
[uncategorized] ~2-~2: You might be missing the article “the” here.
Context: # State Machine Modules Celestia app is built using the cosmos-sdk, and ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
3-3
: Add missing article "the".The article "the" is missing before "modules".
- follows standard cosmos-sdk module structure. The modules used in the app vary based on app version: + follows the standard cosmos-sdk module structure. The modules used in the app vary based on app version:Tools
LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...built using the cosmos-sdk, and follows standard cosmos-sdk module structure. The module...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
specs/src/specs/ante_handler.md (2)
3-3
: Consider simplifying the phrase.The phrase "in order to" can be simplified to "to".
- in order to reject decodable sdk.Txs that do not meet certain criteria. + to reject decodable sdk.Txs that do not meet certain criteria.Tools
LanguageTool
[style] ~3-~3: Consider a shorter alternative to avoid wordiness.
Context: ...0.46.15/x/auth/spec/03_antehandlers.md) in order to reject decodable sdk.Txs that do not me...(IN_ORDER_TO_PREMIUM)
5-5
: Consider simplifying the phrase.The phrase "prior to" can be simplified to "before".
- `CheckTx` prior to the transaction entering the mempool + `CheckTx` before the transaction enters the mempoolTools
LanguageTool
[style] ~5-~5: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...he transaction lifecycle: 1.CheckTx
prior to the transaction entering the mempool 1....(EN_WORDINESS_PREMIUM_PRIOR_TO)
specs/src/SUMMARY.md (2)
13-13
: Possible duplicated word.It looks like "AnteHandler" is repeated.
- [AnteHandler](./specs/ante_handler.md) - - [AnteHandler v1](./specs/ante_handler_v1.md) + [AnteHandler](./specs/ante_handler.md) - [AnteHandler v1](./specs/ante_handler_v1.md)Tools
LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...](./specs/block_validity_rules.md) - AnteHandler - AnteHandler v1 - [...(ENGLISH_WORD_REPEAT_RULE)
25-25
: Possible duplicated word.It looks like "Parameters" is repeated.
- [Parameters](./specs/parameters.md) - - [Parameters v1](./specs/parameters_v1.md) + [Parameters](./specs/parameters.md) - [Parameters v1](./specs/parameters_v1.md)Tools
LanguageTool
[duplication] ~25-~25: Possible typo: you repeated a word
Context: ..../specs/state_machine_modules_v2.md) - Parameters - Parameters v1 - [Para...(ENGLISH_WORD_REPEAT_RULE)
specs/src/specs/state_machine_modules_v1.md (1)
22-22
: Add missing punctuation.A punctuation mark might be missing after "README.md".
- [feegrant](https://github.com/celestiaorg/cosmos-sdk/blob/v1.14.0-sdk-v0.46.11/x/feegrant/spec/README.md) + [feegrant](https://github.com/celestiaorg/cosmos-sdk/blob/v1.14.0-sdk-v0.46.11/x/feegrant/spec/README.md),Tools
LanguageTool
[uncategorized] ~22-~22: A punctuation mark might be missing here.
Context: ...1/x/evidence/spec/README.md) - feegrant - [genutil](https://github.com/celestiaorg/...(AI_EN_LECTOR_MISSING_PUNCTUATION)
specs/src/specs/state_machine_modules_v2.md (1)
23-23
: Add missing punctuation.A punctuation mark might be missing after "README.md".
- [feegrant](https://github.com/celestiaorg/cosmos-sdk/blob/v1.14.0-sdk-v0.46.11/x/feegrant/spec/README.md) + [feegrant](https://github.com/celestiaorg/cosmos-sdk/blob/v1.14.0-sdk-v0.46.11/x/feegrant/spec/README.md),Tools
LanguageTool
[uncategorized] ~23-~23: A punctuation mark might be missing here.
Context: ...1/x/evidence/spec/README.md) - feegrant - [genutil](https://github.com/celestiaorg/...(AI_EN_LECTOR_MISSING_PUNCTUATION)
|
||
## Parameters | ||
Note that not all of these parameters are changeable via governance. This list | ||
also includes parameter that require a hardfork to change due to being manually |
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.
Fix grammatical issue.
Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
- also includes parameter that require a hardfork to change due to being manually hardcoded in the application or they are blocked by the `x/paramfilter` module.
+ also includes parameters that require a hardfork to change due to being manually hardcoded in the application, or they are blocked by the `x/paramfilter` module.
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.
also includes parameter that require a hardfork to change due to being manually | |
also includes parameters that require a hardfork to change due to being manually hardcoded in the application, or they are blocked by the `x/paramfilter` module. |
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.
very nice, thanks for remembering these
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
specs/src/specs/state_machine_modules.md (1)
3-3
: Missing article: theConsider adding the article "the" before "cosmos-sdk" for grammatical correctness.
- Celestia app is built using the cosmos-sdk, and follows standard cosmos-sdk module structure. + Celestia app is built using the cosmos-sdk, and follows the standard cosmos-sdk module structure.Tools
LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...built using the cosmos-sdk, and follows standard cosmos-sdk module structure. The module...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
Closes #3273 by adding pages that are specific to v2