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(x/accounts): improve comments #22339

Merged
merged 4 commits into from
Oct 23, 2024
Merged

docs(x/accounts): improve comments #22339

merged 4 commits into from
Oct 23, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Oct 23, 2024

Description

Noticed while doing the workshop: https://github.com/julienrbrt/workshop-cosmos-sdk-cosmoverse-2024


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

    • Introduced new message types for multisig account initialization and proposal management.
    • Added functionality for querying multisig account configurations and proposals.
  • Documentation

    • Enhanced comments for clarity in lockup and multisig message definitions.
    • Updated README to provide comprehensive guidance on account functionalities and implementations.
    • Improved structure and readability of the lockup account tutorial.
  • Bug Fixes

    • Improved comment clarity in existing messages without altering functionality.
  • Configuration Updates

    • Updated telemetry server address from localhost:1318 to localhost:1327 in configuration files.

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Oct 23, 2024
Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several modifications across multiple files related to the Cosmos SDK. Key changes include updates to comments in the lockup and multisig message definitions for clarity, the addition of new message types for multisig account functionalities, and enhancements to the README documentation for the x/accounts module. Additionally, the go.mod file has been updated to manage dependencies more effectively, reflecting an expansion of account-related functionalities. Overall, these changes aim to improve documentation and structure without altering the core functionality.

Changes

File Path Change Summary
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go - Updated comments for EndTime and StartTime in MsgInitLockupAccount for clarity.
- Updated comment for MsgWithdraw to clarify its purpose.
- Added MsgWithdrawResponse message type.
api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go - Introduced MsgInit for initializing multisig accounts with members and config fields.
- Added message types for proposals and configuration updates.
simapp/go.mod - Added dependencies for cosmossdk.io/x/accounts/defaults/base and cosmossdk.io/x/accounts/defaults/multisig.
- Moved google.golang.org/protobuf to active requirement.
x/accounts/README.md - Enhanced documentation regarding account functionalities, including initialization and query handlers.
- Defined new protobuf messages related to account operations.
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto - Updated comments for end_time and start_time fields for clarity.
- Standardized formatting across option declarations.
x/accounts/proto/cosmos/accounts/defaults/multisig/v1/multisig.proto - Added comments to MsgInit, Config, Proposal, and QueryConfigResponse messages for better understanding of fields.

Possibly related PRs

Suggested reviewers

  • tac0turtle
  • sontrinh16
  • akhilkumarpilli

📜 Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 962ea09 and 0e9272e.

📒 Files selected for processing (2)
  • server/v2/api/telemetry/config.go (1 hunks)
  • tools/confix/data/v2-app.toml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/telemetry/config.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (3)
server/v2/api/telemetry/config.go (2)

Line range hint 1-57: LGTM! Well-documented configuration

The telemetry configuration is well-structured with comprehensive documentation for each field. The port change aligns with the v2-app.toml update.


6-6: Verify port change consistency across the codebase

The telemetry address port has been changed from 1318 to 1327. Let's verify this change is consistent across all configuration files and documentation.

tools/confix/data/v2-app.toml (1)

88-88: Verify the telemetry port change impact

This changes the default metrics server port from 1318 to 1327, which is a breaking change that could affect monitoring systems. While both ports are secure (non-privileged and localhost-bound), this change requires careful consideration:

  1. The PR is marked as docs-only but contains a functional change
  2. Migration documentation may need updates

Let's verify the related documentation and changes:


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

🧹 Outside diff range and nitpick comments (10)
x/accounts/README.md (5)

363-365: Include the actual protobuf definition instead of a reference link.

For better readability and self-containment of the documentation, consider replacing the GitHub reference link with the actual protobuf definition of MsgAuthenticate. This ensures that readers don't need to navigate to a different page to understand the interface.


362-365: Enhance authentication documentation with common patterns.

Consider adding examples of common authentication patterns (e.g., multisig, time-based, threshold) to help developers understand how to implement secure authentication mechanisms.


Line range hint 566-567: Add more genesis configuration examples.

Consider expanding the genesis section with examples for different account types to demonstrate the variety of initialization possibilities. This would help developers understand how to properly configure different types of accounts at genesis.


Line range hint 1-567: Maintain consistent capitalization for technical terms.

Consider standardizing the capitalization of technical terms throughout the document. For example:

  • "transaction (TX)" vs "Tx" vs "tx"
  • "AnteHandler" vs "ante handler"

Line range hint 401-402: Enhance security considerations for authentication.

Consider expanding the security considerations section with:

  • Best practices for nonce management
  • Signature verification guidelines
  • Rate limiting recommendations
  • Recovery mechanisms
api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (5)

8875-8878: Clarify the purpose of the Members field.

The comment for the Members field should provide more details on what constitutes a member of the multisig account and how they are used in the account's operations.


8877-8878: Clarify the purpose of the Config field.

The comment for the Config field should provide more details on what configuration options are available for the multisig account and how they impact the account's behavior.


9280-9283: Provide more details in the comments for the Threshold and Quorum fields.

The comments should clarify:

  • How the Threshold and Quorum values are determined
  • What happens if the Threshold is not met
  • What happens if the Quorum is not met
  • How these fields interact with the Members of the multisig account

9353-9362: Enhance the comments for the Proposal struct fields.

The comments for the Proposal struct fields should provide more context on:

  • Expectations for the Title and Summary content and length
  • What types of Messages can be included
  • How the VotingPeriodEnd is determined
  • Details on each of the possible Status values a proposal can have

9517-9520: Clarify the relationship between Members and Config in the response.

The comment should specify if the Members and Config returned in the QueryConfigResponse represent the current state of the multisig account. If they can be different from the current state, explain when and why that can occur.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8fa3090 and c4f66b5.

⛔ Files ignored due to path filters (2)
  • x/accounts/defaults/lockup/v1/tx.pb.go is excluded by !**/*.pb.go
  • x/accounts/defaults/multisig/v1/multisig.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (6)
  • api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1 hunks)
  • api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (4 hunks)
  • simapp/go.mod (2 hunks)
  • x/accounts/README.md (1 hunks)
  • x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (7 hunks)
  • x/accounts/proto/cosmos/accounts/defaults/multisig/v1/multisig.proto (4 hunks)
✅ Files skipped from review due to trivial changes (2)
  • x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto
  • x/accounts/proto/cosmos/accounts/defaults/multisig/v1/multisig.proto
🧰 Additional context used
📓 Path-based instructions (3)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/accounts/README.md (1)

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

🔇 Additional comments (3)
simapp/go.mod (2)

17-19: LGTM: Account module dependencies properly configured

The new dependencies for base and multisig account defaults are correctly added with placeholder versions and corresponding replace directives for local development.


49-49: LGTM: Protobuf dependency properly configured

The google.golang.org/protobuf dependency is correctly moved to active requirements with a pinned version v1.35.1.

api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)

Line range hint 1-1: Generated code - skip detailed review.

This is an auto-generated protobuf file with a "DO NOT EDIT" header. The changes should be reviewed in the source .proto file instead.

Let's verify the proto file changes:

#!/bin/bash
# Description: Verify the documentation changes in the proto file
# Test: Check if the proto file contains the updated field comments
rg -A 2 'end_time|start_time' 'cosmos/accounts/defaults/lockup/v1/tx.proto'

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 (11)
x/accounts/proto/cosmos/accounts/defaults/multisig/v1/multisig.proto (4)

12-15: Enhance field documentation with more context

While the comments are accurate, they could be more informative for developers implementing multisig accounts.

Consider expanding the comments like this:

-  // members are the members of the multisig account.
+  // members defines the list of addresses and their corresponding voting weights
+  // that are authorized to manage this multisig account.
   repeated Member members = 1;

-  // config is the configuration of the multisig account.
+  // config specifies the voting rules and operational parameters
+  // that govern this multisig account's decision-making process.
   Config config = 2;

73-76: Clarify threshold and quorum definitions

The current comments could be more precise about how these parameters affect voting outcomes.

Consider this improved documentation:

-  // threshold is the minimum weight required for a proposal to pass.
+  // threshold specifies the minimum sum of member weights required for a proposal
+  // to pass. For example, if threshold is 7, the sum of weights of all 'yes'
+  // votes must be at least 7 for the proposal to pass.
   int64 threshold = 1;

-  // quorum is the minimum number of members that need to vote for a proposal to pass.
+  // quorum defines the minimum number of members that must vote (regardless of
+  // their vote option) for a proposal to be considered valid. This ensures
+  // sufficient participation in decision-making.
   int64 quorum = 2;

91-103: Expand proposal field documentation

The comments should provide more guidance on field usage and expectations.

Consider enhancing the documentation:

-  // title is the title of the proposal.
+  // title is a brief, descriptive name for the proposal that uniquely identifies
+  // its purpose to multisig members.
   string title = 1;

-  // summary is the summary of the proposal.
+  // summary provides a detailed description of the proposal's intent, including
+  // the rationale for the proposed actions and expected outcomes.
   string summary = 2;

-  // status is the current status of the proposal.
+  // status tracks the proposal's current state in the voting process
+  // (unspecified, voting period, passed, or rejected). This field is managed
+  // by the system and should not be set during proposal creation.
   ProposalStatus status = 5;

121-124: Improve response field documentation

The comments should better describe the response context.

Consider this enhanced documentation:

-  // members are the current members of the account.
+  // members returns the current list of authorized members and their respective
+  // voting weights for this multisig account at the time of query.
   repeated Member members = 1;

-  // config is the current config of the account.
+  // config returns the active voting rules and operational parameters that
+  // currently govern this multisig account's decision-making process.
   Config config = 2;
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (1)

Line range hint 86-96: Consider enhancing message documentation

While the formatting is good, consider adding a more detailed description for the MsgSend message, similar to other messages in the file. The description should explain when and why this message would be used in the context of a lockup account.

+// MsgSend defines a message that enables the lockup account owner to send locked tokens
+// to another address, subject to lockup constraints and vesting schedules.
 message MsgSend {
x/accounts/README.md (4)

363-365: Include the actual MsgAuthenticate code instead of external reference.

Rather than linking to an external source, consider including the actual protobuf definition in the documentation. This ensures the documentation remains self-contained and readers don't need to navigate away to understand the interface.

Replace the reference with the actual protobuf definition:

message MsgAuthenticate {
    // signer is the account that is trying to authenticate the transaction
    string signer = 1;
    // signature_data contains the signature data that needs to be verified
    bytes signature_data = 2;
}

message MsgAuthenticateResponse {}

Line range hint 391-402: Consider using a sequence diagram for better flow visualization.

The current top-down graph, while informative, could be improved by using a sequence diagram to better illustrate the temporal flow of the authentication process.

Consider replacing with:

sequenceDiagram
    participant C as Client
    participant AH as AnteHandler
    participant AM as x/accounts Module
    participant A as Account

    C->>AH: Submit Transaction
    AH->>AH: Execute Signature Verification
    alt is x/accounts signer
        AH->>AM: Delegate Authentication
        AM->>A: MsgAuthenticate
        alt implements Authentication
            A-->>AM: Success/Failure
        else
            A-->>AM: Non-externally owned account
        end
        AM-->>AH: Authentication Result
    else
        AH->>AH: Continue Standard Verification
    end
    AH-->>C: Final Result
Loading

Line range hint 516-547: Enhance the Genesis section with a complete example.

The current Genesis section could be improved in several ways:

  1. The CLI command example lacks concrete parameter values
  2. The JSON example uses placeholder values ("..")
  3. The section ends abruptly without explaining the expected outcome

Consider enhancing the section with:

  1. A concrete CLI command example:
simd tx accounts init lockup \
  '{"owner":"cosmos1...", "end_time":"2024-12-31T23:59:59Z", "start_time":"2024-01-01T00:00:00Z"}' \
  --from cosmos1... --genesis
  1. A complete JSON example with actual values:
{
  "app_state": {
    "accounts": {
      "init_account_msgs": [
        {
          "sender": "cosmos1xyz...",
          "account_type": "lockup",
          "message": {
            "@type": "cosmos.accounts.defaults.lockup.MsgInitLockupAccount",
            "owner": "cosmos1abc...",
            "end_time": "2024-12-31T23:59:59Z",
            "start_time": "2024-01-01T00:00:00Z"
          },
          "funds": [
            {
              "denom": "stake",
              "amount": "1000"
            }
          ]
        }
      ]
    }
  }
}
  1. Add a section explaining the expected outcome:
After genesis, you can verify the account creation using:
```simd query accounts list-accounts```
The command should show the newly created lockup account with the specified parameters.

Line range hint 1-3: Add table of contents and improve cross-references.

The documentation would benefit from:

  1. A table of contents at the beginning for easier navigation
  2. Cross-references between related sections (e.g., linking Authentication section from Account Constructor section)

Consider adding a table of contents after the introduction:

## Table of Contents

1. [Basics](#basics)
   - [Example Account Creation](#example-account-creation)
   - [State Isolation](#state-isolation)
2. [Account Implementation](#account-implementation)
   - [Init](#init)
   - [Execute Handlers](#execute-handlers)
   - [Query Handlers](#query-handlers)
   - [Account Constructor](#the-account-constructor)
3. [App Wiring](#app-wiring)
4. [Authentication](#the-authentication-interface)
5. [Genesis](#genesis)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)

5854-5864: Consider enhancing field comments for better clarity.

The comments for end_time and start_time fields could be more descriptive to better explain their purpose and constraints.

Consider updating the comments to:

-  // end_time is end of lockup
+  // end_time defines the timestamp when the lockup period ends and tokens become withdrawable
-  // start_time is start of lockup  
+  // start_time defines the timestamp when the lockup period begins and tokens become locked
api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (1)

Line range hint 1-9999: The multisig account architecture looks well-designed.

The message structures follow good practices:

  • Clear separation between initialization, proposal management, and configuration
  • Proper use of protobuf types for serialization
  • Well-defined enums for proposal status and vote options
  • Comprehensive query interfaces for account state

However, consider adding rate limiting or gas costs for proposal creation to prevent spam attacks.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8fa3090 and c4f66b5.

⛔ Files ignored due to path filters (2)
  • x/accounts/defaults/lockup/v1/tx.pb.go is excluded by !**/*.pb.go
  • x/accounts/defaults/multisig/v1/multisig.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (6)
  • api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1 hunks)
  • api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (4 hunks)
  • simapp/go.mod (2 hunks)
  • x/accounts/README.md (1 hunks)
  • x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (7 hunks)
  • x/accounts/proto/cosmos/accounts/defaults/multisig/v1/multisig.proto (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/accounts/README.md (1)

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

🔇 Additional comments (9)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto (5)

15-27: Documentation improvements look good!

The enhanced comments for end_time and start_time fields provide better clarity about their purpose in the lockup account context. The message description accurately describes the purpose of MsgInitLockupAccount.


53-55: Option formatting looks good!

The formatting of options follows a consistent style and improves readability.


65-67: Option formatting is consistent!

The formatting matches the style used in other messages, maintaining consistency throughout the file.


76-78: Option formatting maintains consistency!

The formatting aligns with the established pattern in the file.


107-111: Documentation and formatting improvements look good!

The message description clearly explains the purpose of MsgWithdraw, and the option formatting maintains consistency with the rest of the file.

simapp/go.mod (2)

17-17: LGTM: New account module dependencies.

The addition of base and multisig account defaults modules aligns with the documentation improvements in the x/accounts module.

Also applies to: 19-19


49-49: LGTM: Protobuf dependency requirement.

Moving google.golang.org/protobuf to active requirements is appropriate as it's needed for the protobuf message definitions in the accounts module.

api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)

Line range hint 1-6000: Message definitions and validations look good.

The protobuf message definitions are well-structured with proper validation tags, correct field types, and appropriate interfaces implementation. The generated code follows best practices for protobuf code generation.

api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go (1)

8875-8878: Documentation improvements look good!

The added comments are clear, descriptive and follow Go documentation standards. They provide good context about:

  • Member fields and their purpose
  • Configuration parameters and their effects
  • Proposal structure and field usage

Also applies to: 9280-9283, 9353-9362, 9517-9520

@@ -38,8 +33,7 @@ message MsgInitLockupAccountResponse {}
// MsgInitPeriodicLockingAccount defines a message that enables creating a periodic locking
// account.
message MsgInitPeriodicLockingAccount {
option (amino.name) = "cosmos-sdk/MsgInitPeriodLockupAccount";

option (amino.name) = "cosmos-sdk/MsgInitPeriodLockupAccount";
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 amino name option

The amino name contains a typo: MsgInitPeriodLockupAccount should be MsgInitPeriodicLockupAccount to match the message name.

-  option (amino.name)      = "cosmos-sdk/MsgInitPeriodLockupAccount";
+  option (amino.name)      = "cosmos-sdk/MsgInitPeriodicLockupAccount";
📝 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
option (amino.name) = "cosmos-sdk/MsgInitPeriodLockupAccount";
option (amino.name) = "cosmos-sdk/MsgInitPeriodicLockupAccount";

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 (2)
x/accounts/defaults/lockup/TUTORIAL.md (2)

211-211: Improve command syntax clarity.

The command syntax has been simplified by removing the --from owner flag, which is an improvement. However, consider adding a note about the expected output format to make it more user-friendly.

Add a comment about the expected output format:

 simd tx accounts query <account_address> <query-request-type-url> $querycontents
+# The command returns the query result in JSON format

Line range hint 1-256: General documentation improvements needed.

Several areas of the documentation could be enhanced for clarity and correctness:

  1. The JSON examples contain syntax errors (missing commas after amount fields)
  2. Inconsistent use of "continuous" vs "continous" (spelling)
  3. Some warning blocks could be more informative by explaining the consequences

Would you like me to provide specific fixes for these issues?

🧰 Tools
🪛 LanguageTool

[duplication] ~12-~12: Possible typo: you repeated a word
Context: ...n) * Send coins * Query * Query account info ...

(ENGLISH_WORD_REPEAT_RULE)

🪛 Markdownlint

4-4: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


5-5: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


6-6: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


7-7: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


8-8: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


9-9: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


10-10: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


11-11: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


12-12: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


13-13: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


14-14: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c4f66b5 and 962ea09.

📒 Files selected for processing (1)
  • x/accounts/defaults/lockup/TUTORIAL.md (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/accounts/defaults/lockup/TUTORIAL.md (1)

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

🪛 LanguageTool
x/accounts/defaults/lockup/TUTORIAL.md

[duplication] ~12-~12: Possible typo: you repeated a word
Context: ...n) * Send coins * Query * Query account info ...

(ENGLISH_WORD_REPEAT_RULE)

🪛 Markdownlint
x/accounts/defaults/lockup/TUTORIAL.md

4-4: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


5-5: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


6-6: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


7-7: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


8-8: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


9-9: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


10-10: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


11-11: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


12-12: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

Comment on lines +3 to +12
* [Using lockup account on Cosmos sdk](#using-lockup-account-on-cosmos-sdk)
* [Setup](#setup)
* [Init](#init)
* [Execution](#execution)
* [Delegate](#delegate)
* [Undelegate](#undelegate)
* [Withdraw reward](#withdraw-reward)
* [Withdraw unlocked token](#withdraw-unlocked-token)
* [Send coins](#send-coins)
* [Query](#query)
* [Query](#query)
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 table of contents indentation for better readability.

The table of contents structure needs consistent indentation to properly reflect the document hierarchy.

Apply these changes to fix the indentation:

- * [Using lockup account on Cosmos sdk](#using-lockup-account-on-cosmos-sdk)
-   * [Setup](#setup)
-   * [Init](#init)
-   * [Execution](#execution)
-     * [Delegate](#delegate)
-     * [Undelegate](#undelegate)
-     * [Withdraw reward](#withdraw-reward)
-     * [Withdraw unlocked token](#withdraw-unlocked-token)
-     * [Send coins](#send-coins)
-   * [Query](#query)
-     * [Query account info](#query-account-info)
-     * [Query periodic lockup account locking periods](#query-periodic-lockup-account-locking-periods)
+ * [Using lockup account on Cosmos sdk](#using-lockup-account-on-cosmos-sdk)
+     * [Setup](#setup)
+     * [Init](#init)
+     * [Execution](#execution)
+         * [Delegate](#delegate)
+         * [Undelegate](#undelegate)
+         * [Withdraw reward](#withdraw-reward)
+         * [Withdraw unlocked token](#withdraw-unlocked-token)
+         * [Send coins](#send-coins)
+     * [Query](#query)
+         * [Query account info](#query-account-info)
+         * [Query periodic lockup account locking periods](#query-periodic-lockup-account-locking-periods)

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 LanguageTool

[duplication] ~12-~12: Possible typo: you repeated a word
Context: ...n) * Send coins * Query * Query account info ...

(ENGLISH_WORD_REPEAT_RULE)

🪛 Markdownlint

4-4: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


5-5: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


6-6: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)


7-7: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


8-8: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


9-9: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


10-10: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


11-11: Expected: 8; Actual: 4
Unordered list indentation

(MD007, ul-indent)


12-12: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

@github-actions github-actions bot added C:Confix Issues and PR related to Confix C:server/v2 Issues related to server/v2 C:server/v2 api labels Oct 23, 2024
@julienrbrt julienrbrt added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 98be2b8 Oct 23, 2024
77 of 78 checks passed
@julienrbrt julienrbrt deleted the julien/accounts branch October 23, 2024 10:38
mergify bot pushed a commit that referenced this pull request Oct 23, 2024
(cherry picked from commit 98be2b8)

# Conflicts:
#	api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go
#	api/cosmos/accounts/defaults/multisig/v1/multisig.pulsar.go
#	server/v2/api/telemetry/config.go
#	simapp/go.mod
julienrbrt added a commit that referenced this pull request Oct 23, 2024
alpe added a commit that referenced this pull request Oct 25, 2024
* main: (157 commits)
  feat(core): add ConfigMap type (#22361)
  test: migrate e2e/genutil to systemtest (#22325)
  feat(accounts): re-introduce bundler (#21562)
  feat(log): add slog-backed Logger type (#22347)
  fix(x/tx): add feePayer as signer (#22311)
  test: migrate e2e/baseapp to integration tests (#22357)
  test: add x/circuit system tests (#22331)
  test: migrate e2e/tx tests to systemtest (#22152)
  refactor(simdv2): allow non-comet server components (#22351)
  build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352)
  fix(server/v2): respect context cancellation in start command (#22346)
  chore(simdv2): allow overriding the --home flag (#22348)
  feat(server/v2): add SimulateWithState to AppManager (#22335)
  docs(x/accounts): improve comments (#22339)
  chore: remove unused local variables (#22340)
  test: x/accounts systemtests (#22320)
  chore(client): use command's configured output (#22334)
  perf(log): use sonic json lib  (#22233)
  build(deps): bump x/tx (#22327)
  fix(x/accounts/lockup): fix proto path (#22319)
  ...
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 C:Confix Issues and PR related to Confix C:server/v2 api C:server/v2 Issues related to server/v2 C:x/accounts/lockup C:x/accounts/multisig C:x/accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants