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: Update build/abci docs #22067

Merged
merged 2 commits into from
Oct 3, 2024
Merged

docs: Update build/abci docs #22067

merged 2 commits into from
Oct 3, 2024

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Oct 3, 2024

Description

Ref: #21429


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

  • Documentation
    • Clarified the functionality of ABCI methods in the introduction and various sections.
    • Updated references to the latest version of the Cosmos SDK for improved accuracy.
    • Streamlined explanations for PrepareProposal, ProcessProposal, and vote extensions to enhance user understanding.
    • Minor grammatical adjustments made for better readability.

@hieuvubk hieuvubk requested a review from a team as a code owner October 3, 2024 08:51
Copy link
Contributor

coderabbitai bot commented Oct 3, 2024

📝 Walkthrough

Walkthrough

The pull request includes updates to several documentation files related to the ABCI methods in version 2.0 of the CometBFT framework. The modifications clarify the functionality of various processes such as PrepareProposal, ProcessProposal, and vote extensions. Key changes involve refining descriptions, updating version references, and ensuring that the documentation accurately reflects the current implementation and requirements. No alterations were made to the declarations of exported or public entities.

Changes

File Path Change Summary
docs/build/abci/00-introduction.md Clarified ABCI functionality, streamlined "PrepareProposal" section, emphasized non-deterministic nature of vote extensions, and succinctly described "FinalizeBlock".
docs/build/abci/01-prepare-proposal.md Updated to clarify PrepareProposal functionality, refined mempool description, deprecated priority mempool, and corrected code example.
docs/build/abci/02-process-proposal.md Clarified ProcessProposal requirements, updated link to implementation, and refined instructions for custom handler implementation.
docs/build/abci/03-vote-extensions.md Updated version references for vote extensions, clarified handler requirements, and maintained guidance on vote extension management.

Possibly related PRs

Suggested labels

backport/v0.52.x, C:CLI

Suggested reviewers

  • sontrinh16
  • julienrbrt
  • tac0turtle

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

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: 1

🧹 Outside diff range and nitpick comments (3)
docs/build/abci/02-process-proposal.md (1)

22-23: Excellent clarification, consider minor wording adjustment.

The added clarity regarding the requirements for custom ProcessProposal handlers is valuable. The explicit mention of checking both maximum block gas and maxtxbytes is an important addition.

To further improve readability, consider this minor wording adjustment:

-your own `ProcessProposal` handler, you must ensure that the transactions
-provided in the proposal DO NOT exceed the maximum block gas and `maxtxbytes` (if set).
+your own `ProcessProposal` handler, you must ensure that the transactions
+provided in the proposal do not exceed the maximum block gas and `maxtxbytes` (if set).

This change maintains the emphasis while adhering to standard documentation style.

docs/build/abci/00-introduction.md (1)

22-22: Approve with suggestions for clarity and grammar.

The added information about custom handlers is valuable. However, I suggest the following improvements for clarity and grammar:

  1. Consider rephrasing "in conjunction with" to a more concise alternative.
  2. Add the article "an" before "application developer".
  3. Restructure the sentence for better flow.

Here's a suggested revision:

-To perform this manipulation on the application side, a custom handler must be implemented. By default, the Cosmos SDK provides `PrepareProposalHandler`, used in conjunction with an application specific mempool. A custom handler can be written by application developer, if a noop handler provided, all transactions are considered valid.
+To perform this manipulation on the application side, a custom handler must be implemented. By default, the Cosmos SDK provides `PrepareProposalHandler`, which works with an application-specific mempool. An application developer can write a custom handler; if a no-op handler is provided, all transactions are considered valid.

This revision addresses the wordiness, adds the missing article, and improves the overall sentence structure for better readability.

🧰 Tools
🪛 LanguageTool

[style] ~22-~22: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...provides PrepareProposalHandler, used in conjunction with an application specific mempool. A cust...

(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)


[uncategorized] ~22-~22: You might be missing the article “an” here.
Context: ...ool. A custom handler can be written by application developer, if a noop handler provided, ...

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

docs/build/abci/03-vote-extensions.md (1)

Line range hint 1-139: Approve overall changes and suggest minor improvement for clarity.

The document has been successfully updated to reflect the changes in the Cosmos SDK version (v0.52.0-beta.1). The content remains informative and well-structured, providing comprehensive information about vote extensions in ABCI++.

To enhance clarity for readers, consider adding a brief note at the beginning of the document mentioning the version update. For example:

# Vote Extensions

:::note Synopsis
This section describes how the application can define and use vote extensions
defined in ABCI++. (Updated for Cosmos SDK v0.52.0-beta.1)
:::

This addition would help readers immediately understand the context of the document and its relevance to the current SDK version.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4e6a43b and 0329cab.

📒 Files selected for processing (4)
  • docs/build/abci/00-introduction.md (1 hunks)
  • docs/build/abci/01-prepare-proposal.md (2 hunks)
  • docs/build/abci/02-process-proposal.md (1 hunks)
  • docs/build/abci/03-vote-extensions.md (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/build/abci/00-introduction.md (1)

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

docs/build/abci/01-prepare-proposal.md (1)

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

docs/build/abci/02-process-proposal.md (1)

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

docs/build/abci/03-vote-extensions.md (1)

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

🪛 LanguageTool
docs/build/abci/00-introduction.md

[style] ~22-~22: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...provides PrepareProposalHandler, used in conjunction with an application specific mempool. A cust...

(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)


[uncategorized] ~22-~22: You might be missing the article “an” here.
Context: ...ool. A custom handler can be written by application developer, if a noop handler provided, ...

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

🔇 Additional comments (7)
docs/build/abci/02-process-proposal.md (1)

Line range hint 1-37: Overall improvements to documentation clarity and accuracy.

The changes made to this document significantly enhance its quality:

  1. The updated link to the implementation ensures readers are directed to the most recent version.
  2. The clarification regarding custom ProcessProposal handlers provides valuable guidance for developers.
  3. The overall refinement of the text improves readability and understanding of the ProcessProposal functionality.

These updates contribute to more accurate and helpful documentation for the Cosmos SDK.

docs/build/abci/01-prepare-proposal.md (4)

Line range hint 1-30: LGTM: Clear and accurate explanation of PrepareProposal

The updated text provides a comprehensive and clear explanation of the PrepareProposal process. It accurately describes the relationship between CometBFT's mempool and the application's control over block construction. The document also correctly mentions the deprecation of the priority mempool, which aligns with recent changes in the Cosmos SDK.


32-32: LGTM: Updated reference link

The reference link has been correctly updated to point to the newer version (v0.52.0-beta.1) of the Cosmos SDK repository. This change ensures that users are directed to the most recent and relevant documentation.


Line range hint 36-45: LGTM: Corrected code snippet

The code snippet has been updated to remove an extra parenthesis from the SetPrepareProposal method call. This correction improves the accuracy of the example and ensures that users can copy and use the code without syntax errors.


Line range hint 1-45: Excellent updates to the PrepareProposal documentation

The changes made to this document significantly improve its clarity and accuracy. The explanation of the PrepareProposal process is now more comprehensive, clearly distinguishing between CometBFT's mempool and the application's control over block construction. The mention of the deprecated priority mempool and the updated code references ensure that the documentation remains current with the latest Cosmos SDK developments.

These updates will help developers better understand the PrepareProposal process and how to implement custom handlers if needed. Great job on improving the documentation!

docs/build/abci/03-vote-extensions.md (2)

38-39: Verify the updated GitHub link and ensure version consistency.

The link has been updated to reflect the new version (v0.52.0-beta.1). Please verify that this link is valid and points to the correct documentation.

To maintain consistency, consider adding a version note here as well, similar to the suggestion for the previous link update. For example:

this process MUST be deterministic. The Cosmos SDK defines [`sdk.VerifyVoteExtensionHandler`](https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.1/types/abci.go#L29-L31) (updated for v0.52.0-beta.1):
✅ Verification successful

GitHub link is valid and points to the correct version (v0.52.0-beta.1).

The provided link returns a 200 status code, confirming its validity and correctness.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if the GitHub link is valid
curl -s -o /dev/null -w "%{http_code}" https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.1/types/abci.go

Length of output: 115


11-12: Verify the updated GitHub link and consider adding a note about version changes.

The link has been updated to reflect the new version (v0.52.0-beta.1). Please ensure that this link is valid and points to the correct documentation.

Consider adding a note about version changes to help users understand the context of the update. For example:

validator process. The Cosmos SDK defines [`baseapp.ExtendVoteHandler`](https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.1/types/abci.go#L26-L27) (updated for v0.52.0-beta.1):
✅ Verification successful

GitHub link has been validated successfully.

The updated link points to the correct documentation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if the GitHub link is valid
curl -s -o /dev/null -w "%{http_code}" https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.1/types/abci.go

Length of output: 115

@@ -14,12 +14,12 @@ and `ProcessProposal` for the new proposal.
Here is the implementation of the default implementation:

```go reference
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/baseapp/abci_utils.go#L153-L159
https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.1/baseapp/abci_utils.go#L224-L231
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The linked implementation does not contain the DefaultProcessProposal function.

The function DefaultProcessProposal was not found in the specified lines of abci_utils.go or anywhere else in the v0.52.0-beta.1 version of the repository. This suggests that the documentation may be outdated or referencing a non-existent implementation.

  • Verify if the function has been renamed or relocated in a newer version.
  • Update the documentation to reflect the current state of the codebase accurately.
🔗 Analysis chain

Verify the updated implementation link and consider adding a version note.

The link to the implementation has been updated to a newer version, which is good for keeping the documentation current. However, please verify that the linked code still accurately represents the default implementation of ProcessProposal.

Consider adding a note that the link may change with future SDK versions, to help readers understand that they might need to adjust for their specific version.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the linked code represents the default ProcessProposal implementation

# Test: Check if the linked file and lines exist and contain the expected content
gh api repos/cosmos/cosmos-sdk/contents/baseapp/abci_utils.go?ref=v0.52.0-beta.1 | jq -r '.content' | base64 -d | sed -n '224,231p' | grep -q "func DefaultProcessProposal"

if [ $? -eq 0 ]; then
    echo "The linked code appears to contain the default ProcessProposal implementation."
else
    echo "The linked code may not accurately represent the default ProcessProposal implementation. Please verify."
fi

Length of output: 480


Script:

#!/bin/bash
# Description: Verify the linked code represents the default ProcessProposal implementation without using base64

# Fetch the raw content of the specified file and lines
gh api repos/cosmos/cosmos-sdk/contents/baseapp/abci_utils.go?ref=v0.52.0-beta.1 \
  -H "Accept: application/vnd.github.v3.raw" | sed -n '224,231p' | grep -q "func DefaultProcessProposal"

if [ $? -eq 0 ]; then
    echo "The linked code appears to contain the default ProcessProposal implementation."
else
    echo "The linked code may not accurately represent the default ProcessProposal implementation. Please verify."
fi

Length of output: 420


Script:

#!/bin/bash
# Description: Search the entire abci_utils.go file for the DefaultProcessProposal function

# Search for the function in the specified file
if gh api repos/cosmos/cosmos-sdk/contents/baseapp/abci_utils.go?ref=v0.52.0-beta.1 \
    -H "Accept: application/vnd.github.v3.raw" | grep -q "func DefaultProcessProposal"; then
    echo "The function DefaultProcessProposal exists in baseapp/abci_utils.go."
else
    echo "The function DefaultProcessProposal was not found in baseapp/abci_utils.go. Searching the entire repository."

    # Search the entire repository for the function
    gh api repos/cosmos/cosmos-sdk/contents?recursive=true | jq -r '.[]?.path' | grep '.go$' | while read -r file; do
        gh api repos/cosmos/cosmos-sdk/contents/"$file"?ref=v0.52.0-beta.1 \
            -H "Accept: application/vnd.github.v3.raw" | grep -H "func DefaultProcessProposal" && echo "Found in $file"
    done
fi

Length of output: 502


Script:

#!/bin/bash
# Description: Search the entire repository for the DefaultProcessProposal function using ast-grep

ast-grep --lang go --pattern 'func DefaultProcessProposal($_)' .

Length of output: 66

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.

lgtm!

@julienrbrt
Copy link
Member

@hieuvubk don't forget to assign you to be pages and tick them in the issue, so they don't get reviewed again.

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Oct 3, 2024
@tac0turtle tac0turtle added this pull request to the merge queue Oct 3, 2024
Merged via the queue into main with commit d1426c7 Oct 3, 2024
74 of 75 checks passed
@tac0turtle tac0turtle deleted the hieu/abci/docs branch October 3, 2024 10:48
mergify bot pushed a commit that referenced this pull request Oct 3, 2024
(cherry picked from commit d1426c7)
julienrbrt pushed a commit that referenced this pull request Oct 3, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants