Skip to content
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: add a synopsis upgrade doc for 52 #23097

Merged
merged 9 commits into from
Jan 8, 2025
Merged

docs: add a synopsis upgrade doc for 52 #23097

merged 9 commits into from
Jan 8, 2025

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Dec 26, 2024

Description

This pr adds a short step by step upgrade doc to the documentation. its smaller than the upgrading with some slight other information .


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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Added a comprehensive upgrade guide for transitioning from Cosmos SDK version 0.50 to 0.52.
    • Introduced a new category for upgrade tutorials in the documentation.
  • Documentation

    • Enhanced navigation with a structured table of contents in the upgrade guide.
    • Clarified the description of the Environment struct within the context of keepers to specify "kvstore services".
    • Updated links for better accessibility in various documentation sections.

Copy link
Contributor

coderabbitai bot commented Dec 26, 2024

📝 Walkthrough

Walkthrough

This pull request introduces an upgrade guide for transitioning from Cosmos SDK version 0.50 to version 0.52. The document outlines significant changes, including the removal of the x/params module and migration of parameter management to respective modules. It details the introduction of a simplified module manager, refactoring of server components, and the addition of new modules like x/accounts and x/protocolpool. Additionally, a new category for "Upgrade Tutorials" is included to improve documentation organization.

Changes

File Change Summary
docs/build/building-apps/upgrades/0.52.md New upgrade guide document detailing Cosmos SDK transition from v0.50 to v0.52
docs/build/building-apps/upgrades/_category_.json Added category configuration for "Upgrade Tutorials" with position 0
docs/build/building-modules/06-keeper.md Updated description of Environment struct to clarify "kvstore services"
docs/architecture/adr-040-storage-and-smt-state-commitments.md Proposal for transitioning from IAVL to Sparse Merkle Tree for state commitments and data storage, including new types and interfaces
docs/build/build.md Updated link for "Building Apps" section to point to 00-runtime.md
docs/build/building-apps/00-runtime.md Updated reference to next documentation section from 02-app-building.md to 02-app-go-di.md
docs/build/building-modules/16-testing.md Updated URL reference for integration test helpers
docs/learn/advanced/00-baseapp.md Clarified description of CheckTx function regarding gas tracking
docs/learn/advanced/04-store.md Removed section on default behavior of KVStores being wrapped in GasKv.Stores
docs/user/run-node/06-run-production.md Corrected URL for CometBFT to absolute format

Possibly related PRs

  • docs: use the right link for app.go[Supplementary Omission] #21616: This PR addresses documentation changes related to the app.go file, which is relevant as the main PR also discusses updates to app.go and related migration processes.
  • docs(x/accounts): improve comments #22339: This PR improves comments in the x/accounts module, which is relevant to the main PR's focus on the upgrade guide that includes changes to account management.
  • docs: learn/advance #22367: This PR updates various documentation sections, including those related to building applications, which aligns with the main PR's focus on upgrade documentation.
  • docs: fix dead link in UPGRADING.md #23059: This PR fixes a dead link in the UPGRADING.md file, which is directly related to the main PR's focus on the upgrade process from version 0.50 to 0.52.
  • chore(docs): STF readme #23150: This PR updates the STF documentation, which is relevant as it discusses changes in the Cosmos SDK that may impact the upgrade process outlined in the main PR.
  • docs: fix typos #23152: This PR fixes typos in the documentation, which is relevant as it contributes to the overall clarity of the upgrade guide in the main PR.
  • docs: update modules list doc #23172: This PR updates the modules list in the documentation, which is relevant to the main PR's focus on changes in module management during the upgrade process.

Suggested Reviewers

  • sontrinh16
  • julienrbrt

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@tac0turtle tac0turtle marked this pull request as ready for review December 26, 2024 12:41
@tac0turtle tac0turtle requested a review from a team as a code owner December 26, 2024 12:41
@tac0turtle tac0turtle added T:Docs Changes and features related to documentation. backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release labels Dec 26, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (5)
docs/build/building-apps/upgrades/0.52.md (5)

171-172: Fix grammatical error in the instructions

The sentence structure needs improvement.

-For the same instructions than for legacy app wiring to enable sign mode textual
+For the same instructions as for legacy app wiring to enable sign mode textual
🧰 Tools
🪛 LanguageTool

[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)


135-136: Add missing comma for better readability

-The embedding of gRPC-Web was removed. if you would like to continue using gRPC-Web you will need to use [Envoy](https://www.envoyproxy.io/)
+The embedding of gRPC-Web was removed. If you would like to continue using gRPC-Web, you will need to use [Envoy](https://www.envoyproxy.io/)
🧰 Tools
🪛 LanguageTool

[uncategorized] ~135-~135: Possible missing comma found.
Context: ...f you would like to continue using gRPC-Web you will need to use [Envoy](https://ww...

(AI_HYDRA_LEO_MISSING_COMMA)


407-408: Clarify x/validate module requirements

The instructions about x/validate module usage are ambiguous. Please specify when exactly this module is required and when it can be skipped.

-`x/validate` is a module for antehandlers.  If you are using runtime/depinject or v2 this module is required. Skip this step or define your own custom ante/post handlers, see `x/validate` documentation for more details.
+`x/validate` is a module for antehandlers. If you are using runtime/depinject or v2, this module is required. If you're not using these features, you can either skip this step or define your own custom ante/post handlers. See `x/validate` documentation for more details.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~407-~407: Possible missing comma found.
Context: ... If you are using runtime/depinject or v2 this module is required. Skip this step...

(AI_HYDRA_LEO_MISSING_COMMA)


368-369: Fix typo in protocolpool description

-This new module allows teams and the community to create funding opportunities fot the development team and the community.
+This new module allows teams and the community to create funding opportunities for the development team and the community.

513-519: Enhance local testing instructions

Consider adding specific validation steps for critical features:

  1. Verify parameter migration from x/params to module-managed parameters
  2. Test unordered transactions if enabled
  3. Validate protocolpool functionality if x/distribution is used
  4. Verify account number migration if using x/accounts
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aabd4b1 and 78e7f3c.

📒 Files selected for processing (2)
  • docs/build/building-apps/upgrades/0.52.md (1 hunks)
  • docs/build/building-apps/upgrades/_category_.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • docs/build/building-apps/upgrades/category.json
🧰 Additional context used
📓 Path-based instructions (1)
docs/build/building-apps/upgrades/0.52.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🪛 LanguageTool
docs/build/building-apps/upgrades/0.52.md

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)


[uncategorized] ~135-~135: Possible missing comma found.
Context: ...f you would like to continue using gRPC-Web you will need to use [Envoy](https://ww...

(AI_HYDRA_LEO_MISSING_COMMA)


[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)


[grammar] ~197-~197: Probably a preposition is missing after ‘corresponding’.
Context: ...ml configuration files by adjusting the corresponding opt option to Mgoogle/protobuf/any.proto=gi...

(ATD_VERBS_TO_COLLOCATION)


[style] ~227-~227: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...sdk.io/x/gov/types" ``` ### Adopt Core API Interfaces Core API has been introduced for modul...

(ACRONYM_TAUTOLOGY)


[grammar] ~229-~229: The verb ‘recommend’ is used with the gerund form.
Context: ...deprecation of sdk.Context, we strongly recommend to use the cosmossdk.io/core/appmodule interfa...

(ADMIT_ENJOY_VB)


[style] ~309-~309: Consider a shorter alternative to avoid wordiness.
Context: ...emain, but you cannot create new ones. In order to create new vesting accounts you need to...

(IN_ORDER_TO_PREMIUM)


[typographical] ~310-~310: It seems that a comma is missing.
Context: ...w ones. In order to create new vesting accounts you need to add the accounts module to ...

(IN_ORDER_TO_VB_COMMA)


[style] ~331-~331: Consider a shorter alternative to avoid wordiness.
Context: ...` ### TX Decoder Setup for x/accounts In order to support x/accounts, you need to initial...

(IN_ORDER_TO_PREMIUM)


[grammar] ~356-~356: Did you mean “be removed”?
Context: ... module was removed entirely and should removed from your application. * Remove code...

(WILL_BASED_ON)


[uncategorized] ~407-~407: Possible missing comma found.
Context: ... If you are using runtime/depinject or v2 this module is required. Skip this step...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~411-~411: Consider a shorter alternative to avoid wordiness.
Context: ... appmodule is the single entry point. In order to make the necessary changes the appmodul...

(IN_ORDER_TO_PREMIUM)


[uncategorized] ~509-~509: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...g accounts, etc. If your chain is small or you do not rely on x/upgrade, you can d...

(COMMA_COMPOUND_SENTENCE)

🔇 Additional comments (1)
docs/build/building-apps/upgrades/0.52.md (1)

1-47: Well-structured documentation with clear navigation!

The document follows documentation best practices with proper markdown formatting, clear headings, and a comprehensive table of contents.

🧰 Tools
🪛 LanguageTool

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)

@julienrbrt julienrbrt changed the title chore(docs): add a synopsis upgrade doc for 52 docs: add a synopsis upgrade doc for 52 Dec 28, 2024
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, but users should use this + changelog + release notes + upgrading.md?
I am failing to see the difference between this and the upgrading.md. This one is better worded and that would be beneficial to have the UPGRADING.md adopt this style.

docs/build/building-apps/upgrades/0.52.md Outdated Show resolved Hide resolved
docs/build/building-apps/upgrades/0.52.md Show resolved Hide resolved
docs/build/building-apps/upgrades/0.52.md Outdated Show resolved Hide resolved
docs/build/building-apps/upgrades/0.52.md Outdated Show resolved Hide resolved
docs/build/building-apps/upgrades/0.52.md Outdated Show resolved Hide resolved
docs/build/building-apps/upgrades/0.52.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (9)
docs/build/building-apps/upgrades/0.52.md (9)

30-30: Remove redundant word "Interfaces"

The phrase "Core API Interfaces" is redundant since "API" already stands for "Application Programming Interface". Consider using just "Core API" instead.

Also applies to: 227-227

🧰 Tools
🪛 LanguageTool

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)


171-171: Fix grammatical error in comparison

The phrase "same instructions than" should be "same instructions as".

-For the same instructions than for legacy app wiring
+For the same instructions as for legacy app wiring
🧰 Tools
🪛 LanguageTool

[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)


229-229: Fix verb usage with gerund form

The phrase "strongly recommend to use" should be "strongly recommend using".

-we strongly recommend to use
+we strongly recommend using
🧰 Tools
🪛 LanguageTool

[grammar] ~229-~229: The verb ‘recommend’ is used with the gerund form.
Context: ...precation of sdk.Context, we strongly recommend to use the cosmossdk.io/core/appmodule interfa...

(ADMIT_ENJOY_VB)


309-311: Improve sentence structure and fix comma

The phrases starting with "In order to" can be simplified, and a comma is missing after "accounts".

-In order to create new vesting accounts you need to
+To create new vesting accounts, you need to
🧰 Tools
🪛 LanguageTool

[style] ~309-~309: Consider a shorter alternative to avoid wordiness.
Context: ...emain, but you cannot create new ones. In order to create new vesting accounts you need to...

(IN_ORDER_TO_PREMIUM)


[typographical] ~310-~310: It seems that a comma is missing.
Context: ...w ones. In order to create new vesting accounts you need to add the accounts module to ...

(IN_ORDER_TO_VB_COMMA)


356-356: Fix missing word in sentence

There's a missing word "be" in the sentence.

-should removed from your application
+should be removed from your application
🧰 Tools
🪛 LanguageTool

[grammar] ~356-~356: Did you mean “be removed”?
Context: ... module was removed entirely and should removed from your application. * Remove code...

(WILL_BASED_ON)


368-368: Fix typo in word

The word "fot" should be "for".

-fot the development team
+for the development team
🧰 Tools
🪛 GitHub Actions: Spell Check

[warning] 368-368: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'


291-293: Add code block formatting for better readability

The InitGenesis and ExportGenesis examples should be formatted as a diff block for better clarity. Also, consider mentioning common error types that developers might have been relying on.

-func (am AppModule) InitGenesis(ctx context.Context, data json.RawMessage) error {
-    // ...
-}
+func (am AppModule) InitGenesis(ctx context.Context, data json.RawMessage) error {
+    var genesisState types.GenesisState
+    if err := am.cdc.UnmarshalJSON(data, &genesisState); err != nil {
+        return fmt.Errorf("failed to unmarshal genesis state: %w", err)
+    }
+    return am.keeper.InitGenesis(ctx, genesisState)
+}

-func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) {
-    // ...
-}
+func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) {
+    genesisState := am.keeper.ExportGenesis(ctx)
+    return am.cdc.MarshalJSON(genesisState)
+}

Note: If you were previously using sdk.ErrNotFound or types.ErrNotFound, these should now be replaced with collections.ErrNotFound.

Also applies to: 304-304


175-175: Add missing link to environment documentation

The sentence "Learn more about environment here" is missing the actual link to the documentation.

-Learn more about environment here
+Learn more about environment [here](https://docs.cosmos.network/main/build/building-modules/module-manager#environment)

509-509: Add missing comma in compound sentence

Add a comma before "or" as it connects two independent clauses.

-If your chain is small or you do not rely on x/upgrade
+If your chain is small, or you do not rely on x/upgrade
🧰 Tools
🪛 LanguageTool

[uncategorized] ~509-~509: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...g accounts, etc. If your chain is small or you do not rely on x/upgrade, you can d...

(COMMA_COMPOUND_SENTENCE)

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78e7f3c and fc4a722.

📒 Files selected for processing (1)
  • docs/build/building-apps/upgrades/0.52.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/build/building-apps/upgrades/0.52.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🪛 LanguageTool
docs/build/building-apps/upgrades/0.52.md

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)


[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)


[grammar] ~197-~197: Probably a preposition is missing after ‘corresponding’.
Context: ...ml configuration files by adjusting the corresponding opt option to Mgoogle/protobuf/any.proto=gi...

(ATD_VERBS_TO_COLLOCATION)


[style] ~227-~227: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...sdk.io/x/gov/types" ``` ### Adopt Core API Interfaces Core API has been introduced for modul...

(ACRONYM_TAUTOLOGY)


[grammar] ~229-~229: The verb ‘recommend’ is used with the gerund form.
Context: ...precation of sdk.Context, we strongly recommend to use the cosmossdk.io/core/appmodule interfa...

(ADMIT_ENJOY_VB)


[style] ~309-~309: Consider a shorter alternative to avoid wordiness.
Context: ...emain, but you cannot create new ones. In order to create new vesting accounts you need to...

(IN_ORDER_TO_PREMIUM)


[typographical] ~310-~310: It seems that a comma is missing.
Context: ...w ones. In order to create new vesting accounts you need to add the accounts module to ...

(IN_ORDER_TO_VB_COMMA)


[style] ~331-~331: Consider a shorter alternative to avoid wordiness.
Context: ...` ### TX Decoder Setup for x/accounts In order to support x/accounts, you need to initial...

(IN_ORDER_TO_PREMIUM)


[grammar] ~356-~356: Did you mean “be removed”?
Context: ... module was removed entirely and should removed from your application. * Remove code...

(WILL_BASED_ON)


[style] ~411-~411: Consider a shorter alternative to avoid wordiness.
Context: ... appmodule is the single entry point. In order to make the necessary changes the appmodul...

(IN_ORDER_TO_PREMIUM)


[uncategorized] ~509-~509: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...g accounts, etc. If your chain is small or you do not rely on x/upgrade, you can d...

(COMMA_COMPOUND_SENTENCE)

🪛 GitHub Actions: Spell Check
docs/build/building-apps/upgrades/0.52.md

[warning] 368-368: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
docs/build/building-apps/upgrades/0.52.md (2)

71-71: Minor grammatical improvement needed

Consider rephrasing for better clarity:

- Genesis & ADR-070 – Constructors now take `context.Context` and do not pass around a codec.
+ Genesis & ADR-070 – Constructors now accept `context.Context` and no longer pass around a codec.
🧰 Tools
🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'


528-534: Enhance local testing instructions

Consider adding specific validation steps for critical features:

  1. Verify parameter migration from x/params
  2. Test x/accounts functionality with both new and existing accounts
  3. Validate protocolpool operations if using x/distribution
🧰 Tools
🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc4a722 and 4b6a4ce.

📒 Files selected for processing (1)
  • docs/build/building-apps/upgrades/0.52.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/build/building-apps/upgrades/0.52.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🪛 LanguageTool
docs/build/building-apps/upgrades/0.52.md

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)


[uncategorized] ~119-~119: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...changed** • The “basic module manager” is removed. You should directly use module...

(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)


[uncategorized] ~119-~119: You might be missing the article “the” here.
Context: ...er” is removed. You should directly use module.Manager. Action • Remove referenc...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)


[uncategorized] ~175-~175: You might be missing the article “the” here.
Context: ...nment for all modules. Learn more about environment here . Given the fields of environment,...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[grammar] ~197-~197: Probably a preposition is missing after ‘corresponding’.
Context: ...ml configuration files by adjusting the corresponding opt option to Mgoogle/protobuf/any.proto=gi...

(ATD_VERBS_TO_COLLOCATION)


[style] ~227-~227: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...sdk.io/x/gov/types" ``` ### Adopt Core API Interfaces Core API has been introduced for modul...

(ACRONYM_TAUTOLOGY)


[uncategorized] ~228-~228: You might be missing the article “the” here.
Context: ...es" ``` ### Adopt Core API Interfaces Core API has been introduced for modules sin...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[grammar] ~229-~229: The verb ‘recommend’ is used with the gerund form.
Context: ...precation of sdk.Context, we strongly recommend to use the cosmossdk.io/core/appmodule interfa...

(ADMIT_ENJOY_VB)


[uncategorized] ~273-~273: A comma might be missing here.
Context: ... Invoke, Provide, Register → appconfig Previously cosmossdk.io/core/appconfig held functi...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[grammar] ~305-~305: Future tense may not be necessary in this context (after ‘when’).
Context: ...th no error. When migrating collections will return collections.ErrNotFound. Checking fo...

(WHEN_I_WILL)


[style] ~324-~324: Consider a shorter alternative to avoid wordiness.
Context: ...emain, but you cannot create new ones. In order to create new vesting accounts you need to...

(IN_ORDER_TO_PREMIUM)


[typographical] ~325-~325: It seems that a comma is missing.
Context: ...w ones. In order to create new vesting accounts you need to add the accounts module to ...

(IN_ORDER_TO_VB_COMMA)


[style] ~346-~346: Consider a shorter alternative to avoid wordiness.
Context: ...` ### TX Decoder Setup for x/accounts In order to support x/accounts, you need to initial...

(IN_ORDER_TO_PREMIUM)


[grammar] ~371-~371: Did you mean “be removed”?
Context: ... module was removed entirely and should removed from your application. * Remove code...

(WILL_BASED_ON)


[style] ~426-~426: Consider a shorter alternative to avoid wordiness.
Context: ... appmodule is the single entry point. In order to make the necessary changes the appmodul...

(IN_ORDER_TO_PREMIUM)


[uncategorized] ~426-~426: A comma might be missing here.
Context: ...y point. In order to make the necessary changes the appmodulebasic struct in the module...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[uncategorized] ~524-~524: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...g accounts, etc. If your chain is small or you do not rely on x/upgrade, you can d...

(COMMA_COMPOUND_SENTENCE)

🪛 GitHub Actions: Spell Check
docs/build/building-apps/upgrades/0.52.md

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'

⏰ Context from checks skipped due to timeout of 90000ms (20)
  • GitHub Check: tests (03)
  • GitHub Check: tests (02)
  • GitHub Check: tests (01)
  • GitHub Check: tests (00)
  • GitHub Check: test-x-mint
  • GitHub Check: test-x-epochs
  • GitHub Check: test-x-consensus
  • GitHub Check: test-tools-benchmark
  • GitHub Check: test-x-staking
  • GitHub Check: test-x-authz
  • GitHub Check: test-x-bank
  • GitHub Check: test-x-slashing
  • GitHub Check: test-x-group
  • GitHub Check: test-x-upgrade
  • GitHub Check: test-x-protocolpool
  • GitHub Check: build (arm64)
  • GitHub Check: build (amd64)
  • GitHub Check: markdown-link-check
  • GitHub Check: dependency-review
  • GitHub Check: Summary
🔇 Additional comments (1)
docs/build/building-apps/upgrades/0.52.md (1)

1-47: Well-structured documentation layout!

The document has a clear hierarchy with a comprehensive table of contents that makes it easy to navigate through the upgrade process.

🧰 Tools
🪛 LanguageTool

[style] ~30-~30: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...rts-to-cosmossdkiox) 11. Adopt Core API Interfaces 12. [Si...

(ACRONYM_TAUTOLOGY)

🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'


### Update Depinject app_config.go / app.yml if Applicable

With the introduction of environment in modules, depinject automatically creates the environment for all modules. Learn more about environment here . Given the fields of environment, this means runtime creates a kv store service for all modules by default. It can happen that some modules do not have a store necessary (such as x/auth/tx for instance). In this case, the store creation should be skipped in app_config.go:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add missing link to environment documentation

The line "Learn more about environment here" is missing its hyperlink.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~175-~175: You might be missing the article “the” here.
Context: ...nment for all modules. Learn more about environment here . Given the fields of environment,...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'


### Add x/protocolpool if Using x/distribution

The community pool was moved from distribution to a new module called protocolpool. This new module allows teams and the community to create funding opportunities fot the development team and the community.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo in protocol pool description

- This new module allows teams and the community to create funding opportunities fot the development team and the community.
+ This new module allows teams and the community to create funding opportunities for the development team and the community.
📝 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.

Suggested change
The community pool was moved from distribution to a new module called protocolpool. This new module allows teams and the community to create funding opportunities fot the development team and the community.
The community pool was moved from distribution to a new module called protocolpool. This new module allows teams and the community to create funding opportunities for the development team and the community.
🧰 Tools
🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'


### Handle Sign Mode Textual

With the split of x/auth/tx/config in two (`x/auth/tx/config` as depinject module for txconfig and tx options) and `x/validate`, sign mode textual is no more automatically configured when using runtime (it was previously the case). For the same instructions than for legacy app wiring to enable sign mode textual (see in v0.50 UPGRADING documentation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve clarity of sign mode textual instructions

- For the same instructions than for legacy app wiring to enable sign mode textual (see in v0.50 UPGRADING documentation).
+ Follow the same instructions as for legacy app wiring to enable sign mode textual (see the v0.50 UPGRADING documentation).
📝 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.

Suggested change
With the split of x/auth/tx/config in two (`x/auth/tx/config` as depinject module for txconfig and tx options) and `x/validate`, sign mode textual is no more automatically configured when using runtime (it was previously the case). For the same instructions than for legacy app wiring to enable sign mode textual (see in v0.50 UPGRADING documentation).
With the split of x/auth/tx/config in two (`x/auth/tx/config` as depinject module for txconfig and tx options) and `x/validate`, sign mode textual is no more automatically configured when using runtime (it was previously the case). Follow the same instructions as for legacy app wiring to enable sign mode textual (see the v0.50 UPGRADING documentation).
🧰 Tools
🪛 LanguageTool

[grammar] ~171-~171: Comparison is written ‘the same … as’.
Context: ...ly the case). For the same instructions than for legacy app wiring to enable sign mo...

(THE_SAME_AS)

🪛 GitHub Actions: Spell Check

[warning] 383: Spelling error: 'fot' should be 'for', 'fit', 'dot', 'rot', 'cot', 'got', 'tot', or 'fog'

@aljo242
Copy link
Contributor

aljo242 commented Jan 7, 2025

Looks like we have some markdown link fails in unrelated files

https://github.com/cosmos/cosmos-sdk/actions/runs/12634159334/job/35201313253?pr=23097#step:5:500

@tac0turtle
Copy link
Member Author

markdown links some times appear broken but after running the

Looks like we have some markdown link fails in unrelated files

https://github.com/cosmos/cosmos-sdk/actions/runs/12634159334/job/35201313253?pr=23097#step:5:500

fixed

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
docs/learn/advanced/00-baseapp.md (1)

312-312: Consider adding a comma for better readability.

Add a comma before "and" to improve the flow of this compound sentence.

-The [`Core pkg`](../advanced/02-core.md), which includes a `GasService` that tracks how much gas is used during the execution of `Tx`, is initialized at the beginning of `CheckTx`. The user-provided amount of gas for `Tx` is referred to as `GasWanted`. If `GasConsumed`, the amount of gas used during execution, exceeds `GasWanted`, the execution is halted and the changes made to the cached copy of the state are not committed.
+The [`Core pkg`](../advanced/02-core.md), which includes a `GasService` that tracks how much gas is used during the execution of `Tx`, is initialized at the beginning of `CheckTx`. The user-provided amount of gas for `Tx` is referred to as `GasWanted`. If `GasConsumed`, the amount of gas used during execution, exceeds `GasWanted`, the execution is halted, and the changes made to the cached copy of the state are not committed.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~312-~312: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...eds GasWanted, the execution is halted and the changes made to the cached copy of ...

(COMMA_COMPOUND_SENTENCE_2)

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8ff404 and e8d5729.

📒 Files selected for processing (7)
  • docs/architecture/adr-040-storage-and-smt-state-commitments.md (0 hunks)
  • docs/build/build.md (1 hunks)
  • docs/build/building-apps/00-runtime.md (1 hunks)
  • docs/build/building-modules/16-testing.md (1 hunks)
  • docs/learn/advanced/00-baseapp.md (1 hunks)
  • docs/learn/advanced/04-store.md (0 hunks)
  • docs/user/run-node/06-run-production.md (1 hunks)
💤 Files with no reviewable changes (2)
  • docs/learn/advanced/04-store.md
  • docs/architecture/adr-040-storage-and-smt-state-commitments.md
✅ Files skipped from review due to trivial changes (4)
  • docs/user/run-node/06-run-production.md
  • docs/build/build.md
  • docs/build/building-apps/00-runtime.md
  • docs/build/building-modules/16-testing.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/learn/advanced/00-baseapp.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🪛 LanguageTool
docs/learn/advanced/00-baseapp.md

[uncategorized] ~312-~312: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...eds GasWanted, the execution is halted and the changes made to the cached copy of ...

(COMMA_COMPOUND_SENTENCE_2)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: test-x-consensus
  • GitHub Check: test-tools-benchmark
  • GitHub Check: test-x-slashing
  • GitHub Check: build (arm64)
  • GitHub Check: build (amd64)
  • GitHub Check: dependency-review
  • GitHub Check: markdown-link-check
  • GitHub Check: Summary
🔇 Additional comments (1)
docs/learn/advanced/00-baseapp.md (1)

312-312: LGTM! Accurate update of gas tracking implementation.

The change correctly reflects that gas tracking is now handled by the GasService in the Core pkg rather than directly in the Context.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~312-~312: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...eds GasWanted, the execution is halted and the changes made to the cached copy of ...

(COMMA_COMPOUND_SENTENCE_2)

@tac0turtle tac0turtle enabled auto-merge January 8, 2025 13:23
@tac0turtle tac0turtle added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 9afe2f7 Jan 8, 2025
71 of 72 checks passed
@tac0turtle tac0turtle deleted the marko/52_tut branch January 8, 2025 13:32
@julienrbrt
Copy link
Member

@Mergifyio backport release/v0.52.x

Copy link
Contributor

mergify bot commented Jan 8, 2025

backport release/v0.52.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release T:Docs Changes and features related to documentation. Type: ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants