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: fixes broken links #19295

Merged
merged 8 commits into from
Feb 2, 2024
Merged

docs: fixes broken links #19295

merged 8 commits into from
Feb 2, 2024

Conversation

samricotta
Copy link
Contributor

@samricotta samricotta commented Jan 30, 2024

Description

This PR addresses the broken links identified by the md-link-checker workflow throughout the repo. By fixing these links, we aim to ensure that the md-link-checker action passes successfully, improving the documentation's reliability and accessibility.


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

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
    • Updated URLs and corrected references across various documentation files, ensuring accuracy and consistency in the Cosmos SDK documentation.
    • Enhanced documentation for the release process, upgrade paths, and various modules, including CometBFT migration and IBC-related adjustments.
  • Refactor
    • Adjustments to error messages and removal of deprecated functions in the Cosmos SDK codebase.
  • Chores
    • Updated dependencies and handling of coin denominations in the Cosmos SDK codebase.

@samricotta samricotta added the T:Docs Changes and features related to documentation. label Jan 30, 2024
@samricotta samricotta requested a review from a team as a code owner January 30, 2024 14:44
Copy link
Contributor

coderabbitai bot commented Jan 30, 2024

Warning

Rate Limit Exceeded

@samricotta has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 4 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between c480cb5 and 3da4e68.

Walkthrough

The Cosmos SDK has undergone a series of updates aimed at refining its codebase and documentation. These changes encompass updates to URLs, the removal of deprecated functions, modifications in error messaging, and a shift towards CometBFT. Additionally, adjustments have been made to IBC modules, dependencies, and coin handling. Documentation has been streamlined by correcting links, updating file paths, and removing outdated references. These enhancements are designed to improve the SDK's usability, security, and overall functionality.

Changes

File(s) Change Summary
CHANGELOG.md Updated URLs, removed deprecated functions, adjusted error messages, migrated to CometBFT, modified IBC modules, updated dependencies, changed coin denomination handling.
CODING_GUIDELINES.md, CONTRIBUTING.md Updated documentation URLs and file paths; renamed files and folders for RFC and ADR processes; updated configurations for package extraction, SonarCloud, and Dependabot.
RELEASE_PROCESS.md, ROADMAP.md Minor modifications in URLs; updated email links to use mailto: protocol.
UPGRADING.md Updated ABCI++ 2.0 links; removed GetSigners() requirement from Msg types.
docs/architecture/..., docs/learn/... Comprehensive updates across documentation files, including URL corrections, hyperlink updates, and minor textual modifications to ensure accuracy and consistency in the documentation.
store/snapshots/README.md, x/README.md Updated URLs and references in the documentation.

This table groups similar changes across different files to provide a concise overview of the updates made to the Cosmos SDK codebase and documentation.

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>.
    • Generate unit-tests for this file.
  • 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 tests 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 generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@samricotta samricotta marked this pull request as draft January 30, 2024 14:52
@samricotta samricotta marked this pull request as ready for review January 30, 2024 15:09
@samricotta samricotta marked this pull request as draft January 30, 2024 15:16
@samricotta samricotta marked this pull request as ready for review January 30, 2024 19:08
@julienrbrt julienrbrt changed the title fix(docs): fixes broken links docs: fixes broken links Jan 30, 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.

Mostly lgtm, but there is a small UX change that may break the user flow due to those links changes.

CHANGELOG.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/architecture/adr-042-group-module.md Outdated Show resolved Hide resolved
docs/learn/advanced/08-events.md Outdated Show resolved Hide resolved
docs/learn/beginner/00-app-anatomy.md Outdated Show resolved Hide resolved
docs/learn/beginner/00-app-anatomy.md Show resolved Hide resolved
docs/learn/beginner/01-tx-lifecycle.md Outdated Show resolved Hide resolved
docs/learn/beginner/02-query-lifecycle.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.

Review Status

Actionable comments generated: 3

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 482d0d191b8ae10231ad44f5a0293413daffbce9 and 5ff3728332f4e80f29f2b3519d282969d05673a7.
Files selected for processing (39)
  • .coderabbit.yml (1 hunks)
  • CHANGELOG.md (9 hunks)
  • UPGRADING.md (3 hunks)
  • api/cosmos/gov/v1/gov.pulsar.go (30 hunks)
  • client/cmd.go (1 hunks)
  • client/v2/internal/offchain/msgSignArbitraryData.proto (2 hunks)
  • collections/lookup_map_test.go (1 hunks)
  • docs/architecture/adr-070-unordered-transactions.md (6 hunks)
  • proto/cosmos/gov/v1/gov.proto (2 hunks)
  • scripts/init-simapp.sh (2 hunks)
  • server/util.go (1 hunks)
  • server/util_test.go (9 hunks)
  • simapp/simd/cmd/root_v2.go (1 hunks)
  • store/db/db_test.go (1 hunks)
  • store/db/goleveldb.go (4 hunks)
  • store/db/memdb.go (3 hunks)
  • store/db/prefixdb.go (1 hunks)
  • tests/go.mod (2 hunks)
  • tests/go.sum (22 hunks)
  • types/codec_test.go (1 hunks)
  • types/coin.go (1 hunks)
  • types/coin_test.go (1 hunks)
  • types/collections.go (3 hunks)
  • types/utils.go (2 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/accounts/go.sum (2 hunks)
  • x/gov/CHANGELOG.md (2 hunks)
  • x/gov/README.md (7 hunks)
  • x/gov/go.mod (1 hunks)
  • x/gov/go.sum (2 hunks)
  • x/gov/keeper/msg_server_test.go (5 hunks)
  • x/gov/keeper/tally.go (5 hunks)
  • x/gov/keeper/tally_test.go (2 hunks)
  • x/gov/migrations/v6/store.go (1 hunks)
  • x/gov/migrations/v6/store_test.go (1 hunks)
  • x/gov/simulation/genesis.go (4 hunks)
  • x/gov/simulation/genesis_test.go (2 hunks)
  • x/gov/types/v1/gov.pb.go (11 hunks)
  • x/gov/types/v1/params.go (6 hunks)
Files not summarized due to errors (3)
  • api/cosmos/gov/v1/gov.pulsar.go: Error: Message exceeds token limit
  • tests/go.sum: Error: Message exceeds token limit
  • x/gov/types/v1/gov.pb.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (6)
  • client/v2/internal/offchain/msgSignArbitraryData.proto
  • collections/lookup_map_test.go
  • scripts/init-simapp.sh
  • store/db/db_test.go
  • x/accounts/go.sum
  • x/gov/go.sum
Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.md
  • UPGRADING.md
Additional comments: 108
types/codec_test.go (1)
  • 14-17: The test function TestUintValue correctly tests the encoding and decoding of unsigned integer values using the UintValue codec. It covers different scenarios including a regular number, zero value, and a large number represented as a string. This ensures a good range of test cases for the functionality.
.coderabbit.yml (1)
  • 1-32: The configuration in .coderabbit.yml is syntactically correct and logically structured. It specifies language settings, review features, path filters, and instructions for different types of files. This setup should effectively guide the review process according to the specified instructions for Golang code, tests, and documentation.
x/gov/migrations/v6/store.go (1)
  • 43-43: The assignment of YesQuorum and other fields from defaultParams to govParams ensures that the migration process updates the governance parameters to include new fields introduced in this version. This is a necessary step for maintaining compatibility and ensuring that the governance module functions correctly after the migration.
x/gov/migrations/v6/store_test.go (1)
  • 21-49: The test function TestMigrateStore properly checks the migration logic by setting initial parameters, running the migration, and then asserting that the parameters have been updated correctly. This includes checking for the addition of new parameters like YesQuorum and ProposalCancelMaxPeriod. The use of require.NoError and require.Equal from the testify package ensures that the test will fail appropriately if the migration does not perform as expected.
types/utils.go (1)
  • 2-7: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]

The removal of SortJSON and MustSortJSON functions along with the encoding/json import indicates a shift towards relying on amino signing for byte sorting. This simplification reduces complexity and potential sources of error in byte sorting operations.

x/gov/simulation/genesis_test.go (1)
  • 47-56: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [50-64]

The update to constants and the addition of a new assertion for tallyYesQuorum in the TestRandomizedGenState function reflect changes in governance parameters. This ensures that the simulation tests are aligned with the updated governance module's behavior, which is crucial for accurately testing the module's functionality.

x/gov/CHANGELOG.md (1)
  • 39-40: The addition of entries for ProposalCancelMaxPeriod and YesQuorum parameters in the changelog provides clear documentation of the changes made to the governance module. This is important for users and developers to understand the modifications and their impact on the module's functionality.
simapp/simd/cmd/root_v2.go (1)
  • 66-66: The addition of .WithViper("") to the clientCtx assignment chain modifies how Viper is used for configuration management. While this change seems to be intended to initialize Viper with default settings, it's important to ensure that this does not inadvertently affect the configuration loading process, especially since an empty string is passed as an argument. This should be tested to verify that it behaves as expected in all scenarios where clientCtx is used.
store/db/prefixdb.go (1)
  • 197-197: The modification in the Domain method of prefixDBIterator to return two []byte slices without named return values is a minor change that simplifies the method signature. This change is syntactically correct and does not affect the logic of the method. However, it's important to ensure consistency in method signatures across the codebase.
x/accounts/go.mod (1)
  • 163-170: The addition of a new dependency mapping for cosmossdk.io/x/tx to ../tx in the go.mod file correctly specifies a local path for the tx module. This is a common practice for managing dependencies that are part of the same project or workspace. It's important to ensure that the relative path ../tx accurately reflects the location of the tx module within the project structure.
x/gov/go.mod (1)
  • 171-176: The replace directive for cosmossdk.io/x/tx is correctly pointing to ../tx, which aligns with the PR's objective to fix broken links. This change ensures that the local version of the x/tx module is used instead of fetching it from a remote repository, which is a common practice in Go modules for local development or when working with forked versions of dependencies.
x/gov/simulation/genesis.go (2)
  • 27-27: The addition of the YesQuorum constant is consistent with the Cosmos SDK's approach to managing governance parameters. This constant likely serves as a key for simulation parameters related to governance proposals that require a certain percentage of "Yes" votes to pass, excluding abstentions. It's important to ensure that this new parameter is well-documented and tested to clarify its impact on governance simulations.
  • 90-93: The GenYesQuorum function introduces randomized generation of the YesQuorum parameter for simulation purposes. This is a logical extension of the simulation framework, allowing for more diverse testing scenarios. However, it's crucial to ensure that the range of generated values (0 to 500) is appropriate and reflects realistic governance scenarios. Additionally, the precision of 3 decimal places should be justified in the context of how YesQuorum is used within the governance module.
store/db/goleveldb.go (4)
  • 28-28: The NewGoLevelDB function correctly adds a new replace directive for handling options, which is a good practice for providing customizable behavior of the GoLevelDB instance. However, ensure that the documentation clearly explains the purpose of the opts parameter and how it can be used to customize the database instance, especially the maxopenfiles option.
  • 42-42: The NewGoLevelDBWithOpts function's modification to accept two string parameters (name, dir) remains consistent with the expected functionality of creating a new GoLevelDB instance with specific options. This change is straightforward and aligns with the need for specifying the database name and directory path explicitly.
  • 79-79: The modification in the Set method to accept two byte slice parameters (key, value) instead of individual byte slice parameters is a standard approach for key-value storage functions. This change is logical and aligns with the method's purpose. Ensure that the error handling for empty keys and nil values is adequately tested to prevent data integrity issues.
  • 93-93: Similarly, the SetSync method's adjustment to accept two byte slice parameters for synchronous setting operations is appropriate and follows the same rationale as the Set method. This consistency in method signatures across asynchronous and synchronous operations is good for API usability. Again, testing around error conditions is crucial.
types/collections.go (3)
  • 36-37: The introduction of the UintValue constant and its association with the uintValueCodec type is a logical extension to support math.Uint types in the Cosmos SDK's collections package. This addition enhances the SDK's type support and aligns with the existing pattern of providing codecs for different data types. Ensure that this new codec is thoroughly tested, especially for edge cases around large Uint values.
  • 58-61: The addition of the Int and Uint constants provides a clear and concise way to refer to the value types supported by the SDK's collections codecs. This is a good practice for code readability and maintainability. Ensure that these constants are used consistently across the SDK wherever applicable.
  • 212-245: The implementation of the uintValueCodec type for encoding and decoding math.Uint values is crucial for supporting unsigned integers in the SDK's collections. This implementation appears to follow the established pattern used by other codecs, such as intValueCodec. It's important to ensure that the methods of uintValueCodec handle all possible edge cases correctly, especially regarding JSON encoding/decoding and value stringification. Additionally, confirming that math.Uint values are marshaled and unmarshaled correctly will be essential for data integrity.
docs/architecture/adr-070-unordered-transactions.md (3)
  • 6-6: The addition of a new section on deterministic transaction encoding is a significant update to this ADR. It's crucial to ensure that this section provides clear guidance on how to achieve deterministic transaction hashes to prevent duplicate unordered transactions. This addition should also reference relevant ADRs or documentation that detail the encoding process.
  • 280-288: Emphasizing the importance of deterministic transaction hashes is crucial for the security and integrity of unordered transactions. This section correctly highlights the potential issues with non-deterministic transaction encoding and references ADR-027 for a solution. It's important to ensure that developers are aware of the implications of transaction encoding on the prevention of duplicate unordered transactions and are guided to follow best practices in transaction signing and encoding.
  • 315-316: The mention of additional storage overhead and management of processed unordered transactions existing outside of consensus state is an important consideration. This highlights a potential drawback of managing unordered transactions, which developers and node operators should be aware of. It's crucial to provide guidance on how to efficiently manage this overhead to minimize its impact on node performance and storage requirements.
store/db/memdb.go (4)
  • 91-91: The modification of the Set method signature to accept separate key and value parameters enhances clarity and type safety, ensuring that the function's purpose and expected inputs are explicit.
  • 111-111: The SetSync method's signature change mirrors that of the Set method, maintaining consistency across the API. This change further reinforces the explicitness of input parameters.
  • 234-234: The update to newMemDBIterator to reflect changes in parameter signatures aligns with the modifications made to the Set and SetSync methods, ensuring consistency in how keys and values are handled across the database interface.
  • 238-238: Similarly, the newMemDBIteratorMtxChoice function's signature update is consistent with the rest of the changes, ensuring that the iterator creation functions remain aligned with the updated method signatures for setting values in the database.
tests/go.mod (2)
  • 229-229: Moving cosmossdk.io/depinject to the replace block indicates a temporary local override, likely for development or testing purposes. This is a common practice to test local changes or unreleased versions of modules.
  • 246-246: Similarly, moving cosmossdk.io/x/tx to the replace block serves the same purpose as the previous comment, ensuring that local or specific versions of these modules are used during testing. This approach is useful for testing changes in tightly coupled modules.
x/gov/types/v1/params.go (4)
  • 25-25: Introducing the DefaultYesQuorum constant provides a clear default value for the new yesQuorum parameter, enhancing the configurability and flexibility of governance parameters.
  • 44-44: Adding yesQuorum to the NewParams function signature allows for the initialization of governance parameters with a specific yesQuorum value, aligning with the introduction of the DefaultYesQuorum constant and expanding the parameters' configurability.
  • 80-80: Similarly, including yesQuorum in the DefaultParams function ensures that the default governance parameters are initialized with the DefaultYesQuorum value, maintaining consistency and clarity in the default parameter setup.
  • 129-138: The addition of yesQuorum validation in the ValidateBasic method ensures that the new parameter adheres to expected constraints, such as being non-negative and not exceeding 1. This validation is crucial for maintaining the integrity of governance parameters.
x/gov/keeper/tally.go (3)
  • 69-76: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [72-85]

The update to use YesQuorum in the tallyStandard function, including fetching it from custom message parameters when available, ensures that the new governance parameter is correctly applied in the tallying process. This change is crucial for accurately determining the outcome of proposals based on the newly introduced yesQuorum logic.

  • 101-105: The early return condition based on the yesQuorum check in the tallyStandard function ensures that proposals fail if they do not meet the specified yesQuorum threshold. This addition is a significant change that aligns with the introduction of the yesQuorum parameter, ensuring that governance decisions reflect this new criterion.
  • 145-149: The inclusion of yesQuorum logic in the tallyExpedited function mirrors the changes made in the tallyStandard function, ensuring consistency across different types of proposal tallying. This consistency is important for maintaining predictable and understandable governance behavior.
client/cmd.go (1)
  • 364-370: The implementation of checking for a nil command context and setting it to a background context if so, before setting the provided client context, is logically sound and correctly follows Go's context handling best practices.
proto/cosmos/gov/v1/gov.proto (2)
  • 353-358: The addition of the yes_quorum field to the Params message with a default value of 0 (disabled) is clear and well-documented. The comment provides sufficient explanation for its purpose and default behavior.
  • 370-375: Similarly, the addition of the yes_quorum field to the MessageBasedParams message is well-documented, including the condition for disabling it. This maintains consistency across message types and ensures clarity for developers.
server/util_test.go (9)
  • 67-67: Adding server.GetServerContextFromCmd(cmd) before test assertions is a good practice to ensure that the server context is correctly initialized and available for the tests. This enhances the reliability of the tests by ensuring they operate on a valid context.
  • 147-147: Similarly, initializing the server context before assertions in this test case is appropriate and ensures that the test environment is correctly set up. This is crucial for tests that rely on specific configurations being loaded.
  • 187-187: Ensuring the server context is initialized before proceeding with test assertions in this case is correct and necessary for the accuracy of the test. It guarantees that the test operates with the expected configuration settings.
  • 217-217: The practice of initializing the server context before test assertions is consistently applied here as well, which is good for maintaining the integrity and reliability of the test environment.
  • 255-255: Initializing the server context before test assertions in this scenario is also appropriate. It ensures that the environment variables are correctly taken into account in the server configuration for the test.
  • 364-364: The consistent use of initializing the server context before test assertions across different test cases, as seen here, is a good practice. It ensures that the tests are reliable and the configurations are correctly loaded.
  • 382-382: Again, initializing the server context before test assertions ensures that the environment variables are correctly reflected in the server configuration, which is crucial for the accuracy of this test.
  • 400-400: The initialization of the server context before test assertions in this case ensures that the configuration file settings are correctly loaded into the server configuration for the test, which is necessary for test accuracy.
  • 418-418: Ensuring the server context is initialized before test assertions, even when no specific configuration source is set, is good for testing the default configuration settings. This approach maintains the consistency and reliability of the test setup.
server/util.go (1)
  • 218-226: The modification to SetCmdServerContext to check if the command's context is nil and set it to a background context if so, is a good practice for ensuring that there is always a context available for operations that require one. This change enhances the robustness of the command execution by ensuring that the server context is always set, preventing potential nil pointer dereferences.
types/coin.go (1)
  • 71-79: The addition of the IsGT method to the Coin type is a logical extension of the existing comparison methods (IsGTE, IsLT, etc.). This method provides a clear and concise way to compare two coins of the same denomination to determine if one has a greater amount than the other. The use of a panic for mismatched denominations is consistent with the existing methods, ensuring that comparisons are only made between coins of the same type. This approach enforces strict type safety at runtime.
types/coin_test.go (1)
  • 305-327: The test function TestIsGTCoin correctly tests various scenarios for the IsGT method, including cases where a panic is expected due to differing denominations. This is a comprehensive approach to testing, ensuring that the method behaves as expected across different inputs.
x/gov/keeper/msg_server_test.go (4)
  • 1902-1915: The test case "invalid yes quorum" in TestMsgUpdateMessageParams correctly checks for a negative YesQuorum value, ensuring that the parameter validation logic is robust against invalid inputs. This addition enhances the test coverage for message parameter updates related to voting, specifically targeting scenarios where YesQuorum is negative.
  • 1921-1927: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1924-1939]

The test case "invalid threshold" in TestMsgUpdateMessageParams effectively tests for a negative Threshold value. This is crucial for ensuring that the parameter validation logic correctly handles cases where the threshold for voting is set to an invalid value, further improving the test coverage for parameter validation.

  • 1951-1957: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1954-1969]

The test case "invalid veto threshold" in TestMsgUpdateMessageParams properly tests for a negative VetoThreshold value. This addition is important for verifying that the parameter validation logic accurately rejects invalid veto threshold values, thereby enhancing the test coverage for parameter validation in voting scenarios.

  • 1969-1969: The test case "valid" in TestMsgUpdateMessageParams with a YesQuorum value of "0" is a good inclusion for testing the boundary condition of a zero YesQuorum. This ensures that the system can handle scenarios where the quorum for a "yes" vote is set to the minimum possible value, contributing to comprehensive test coverage.
x/gov/README.md (1)
  • 233-239: The addition of the "Yes Quorum" subsection under "Concepts" introduces a new concept that is crucial for understanding the governance process. This section clearly defines "Yes Quorum" and differentiates it from the "Threshold," providing valuable information for users. The explanation is concise and informative, enhancing the document's clarity on governance mechanisms.
x/gov/types/v1/gov.pb.go (7)
  • 995-999: The addition of the YesQuorum field in the Params struct is noted. Ensure that the corresponding documentation and usage logic for this new field are updated accordingly to reflect how this field impacts the governance process.
  • 1168-1173: The GetYesQuorum method has been added to the Params struct. Verify that this method is utilized appropriately in the governance logic, particularly in the calculation of quorum for proposals.
  • 1180-1193: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1183-1247]

The MessageBasedParams struct now includes a YesQuorum field. This change implies adjustments in the governance module to support message-based parameters with a distinct quorum requirement. Confirm that the implementation logic and documentation are updated to reflect this addition.

  • 1963-1971: The YesQuorum field in the Params struct is serialized with a non-standard tag (field number 20 with wire type 2). Ensure that this serialization choice is intentional and compatible with the overall data structure design and usage within the Cosmos SDK.
  • 2170-2178: The serialization of the YesQuorum field in the MessageBasedParams struct uses a non-standard tag. Verify that this is consistent with the intended design and that it does not conflict with other fields or future extensions.
  • 4926-4957: The unmarshaling logic for the YesQuorum field in the Params struct is correctly implemented. Ensure that there are corresponding tests to verify the correct unmarshaling of this field, especially considering its non-standard tag.
  • 5140-5171: The unmarshaling logic for the YesQuorum field in the MessageBasedParams struct is correctly implemented. It's important to verify through testing that this field is correctly unmarshaled, given its significance in the governance process.
tests/go.sum (17)
  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]

The updates to cloud.google.com/go/workflows, cosmossdk.io/* modules, and others in this hunk are standard version bumps. Ensure that these version updates are compatible with the project's requirements and do not introduce breaking changes.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-12]

Similar to the previous comment, this hunk updates versions for modules like cosmossdk.io/math and cosmossdk.io/store. Verify that these updates are in line with the project's dependencies and check for any breaking changes or major updates that might require additional changes in the codebase.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-18]

This hunk includes updates to github.com/bufbuild/protocompile, github.com/casbin/casbin/v2, and others. It's crucial to ensure that these updates do not conflict with the project's existing protobuf definitions and access control configurations.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [19-24]

Updates to logging and hashing libraries such as github.com/cespare/xxhash/v2 and github.com/chzyer/logex are noted. Confirm that the logging format and performance characteristics remain consistent with the project's requirements.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [25-30]

The update to github.com/fsnotify/fsnotify from v1.5.4 to v1.7.0 could impact file watching functionality. Ensure that this update does not affect the project's ability to react to file system events.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [31-36]

Version bumps for localization and validation libraries like github.com/go-playground/locales and github.com/go-playground/validator/v10 are present. Verify that these updates do not introduce changes that could affect form validation or internationalization features.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [37-42]

Updates to WebSocket and JSON libraries such as github.com/gobwas/ws and github.com/goccy/go-json are included. It's important to test WebSocket communication and JSON parsing to ensure that these library updates do not introduce regressions.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-48]

The update to github.com/klauspost/compress could impact compression performance or compatibility. Ensure that this update does not adversely affect data compression or decompression processes within the project.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [49-54]

Updates to text processing and URN handling libraries like github.com/kr/text and github.com/leodido/go-urn are noted. Confirm that these updates do not affect text manipulation or URN parsing functionalities.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [55-60]

This hunk includes updates to various libraries, including github.com/stretchr/testify for testing. Ensure that the update to v1.8.4 does not introduce any issues with the project's test suite.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [61-66]

Updates to networking and codec libraries such as github.com/tidwall/btree and github.com/ugorji/go/codec are present. It's crucial to verify that these updates do not impact the project's networking capabilities or data encoding/decoding functionalities.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [67-72]

The updates to golang.org/x/* libraries involve significant version bumps. These libraries are foundational, and their updates could have wide-ranging impacts. Thorough testing is recommended to ensure that these updates do not introduce any compatibility issues or regressions.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [73-78]

Similar to the previous comment, updates to golang.org/x/* libraries are noted. Given the foundational nature of these libraries, confirm that the updates are compatible with the project's existing code and do not introduce breaking changes.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [79-84]

This hunk includes updates to golang.org/x/* libraries, which are critical to the project. Ensure that these updates are thoroughly tested, especially golang.org/x/sys and golang.org/x/term, to confirm that system-level interactions and terminal functionalities work as expected.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [85-90]

Updates to golang.org/x/text and golang.org/x/tools are included in this hunk. These libraries are often used for internationalization and development tooling, respectively. Verify that these updates do not affect the project's i18n features or development workflows.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [91-96]

The update to google.golang.org/protobuf to v1.32.0 is significant, as it could impact how protocol buffers are used within the project. Ensure compatibility with existing protobuf definitions and test extensively to confirm that serialization and deserialization work as expected.

  • 186-191: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [97-102]

This hunk includes updates to various libraries, including pgregory.net/rapid for rapid testing and rsc.io/quote/v3 for quoting functionalities. Ensure that these updates do not introduce issues with testing or text processing.

api/cosmos/gov/v1/gov.pulsar.go (28)
  • 6390-6390: The addition of fd_Params_yes_quorum field descriptor is consistent with the PR's objective to enhance documentation and codebase clarity. However, ensure that the corresponding documentation and comments are updated to reflect this addition for maintainability.
  • 6415-6415: Initialization of fd_Params_yes_quorum using md_Params.Fields().ByName("yes_quorum") is correct and follows the standard pattern for initializing field descriptors in Go protobufs. This change aligns with the PR's goal of fixing and updating the documentation and codebase.
  • 6597-6602: The conditional block checking for a non-empty YesQuorum and its subsequent handling is implemented correctly. It adheres to the protobuf message handling pattern in Go. Ensure that unit tests cover scenarios with both empty and non-empty YesQuorum values to verify this logic.
  • 6656-6657: The case handling for cosmos.gov.v1.Params.yes_quorum in the switch statement is correctly implemented. It checks for the non-emptiness of the YesQuorum field, which is a standard approach for such checks in Go. This addition should be covered in unit tests for comprehensive validation.
  • 6712-6713: Setting x.YesQuorum to an empty string in the case of cosmos.gov.v1.Params.yes_quorum is appropriate for clearing the field. This change is consistent with the protobuf message handling conventions in Go. Ensure that there are tests to verify the behavior of clearing this field.
  • 6796-6798: Retrieving the value of YesQuorum as a protoreflect.ValueOfString is correctly implemented. This pattern is consistent with Go protobufs for handling string fields. It's important to ensure that the YesQuorum field is properly documented in the code to align with the PR's objectives.
  • 6863-6864: Setting the YesQuorum field from a protoreflect.Value is correctly implemented. This approach is standard for mutating protobuf messages in Go. Documentation around this field should be clear and concise, reflecting its purpose and usage within the system.
  • 6944-6945: The panic in the case of attempting to mutate the immutable field yes_quorum is correctly implemented. This ensures the integrity of the Params message by preventing unsupported operations. Documentation should clearly state the immutability of this field to avoid confusion.
  • 7003-7004: Returning a default protoreflect.ValueOfString("") for YesQuorum in the absence of a value is correct and aligns with the handling of optional string fields in Go protobufs. This implementation should be accompanied by clear documentation explaining the default behavior.
  • 7153-7156: The calculation of the serialized size for YesQuorum is correctly implemented, following the protobuf encoding rules. This change should be verified with tests that check the serialized size of messages with various YesQuorum values to ensure correctness.
  • 7186-7194: The serialization logic for YesQuorum is correctly implemented, adhering to the protobuf wire format specifications. It's crucial to have tests that serialize messages with and without YesQuorum to validate this logic under different scenarios.
  • 8023-8054: The unmarshalling logic for YesQuorum is correctly implemented, including error handling for incorrect wire types and length validations. This is a critical part of protobuf message handling in Go, and it should be thoroughly tested, especially the error paths.
  • 8094-8094: The addition of fd_MessageBasedParams_yes_quorum and related field descriptors is consistent with the PR's objectives. Ensure that the corresponding documentation is updated to reflect these additions, maintaining clarity and accessibility of the codebase.
  • 8104-8104: Initialization of field descriptors for MessageBasedParams, including yes_quorum, is correctly done. This aligns with the protobuf handling conventions in Go. Documentation updates should accompany these changes to ensure clarity.
  • 8186-8191: The conditional handling of YesQuorum within MessageBasedParams is implemented correctly. It's important to cover this logic with unit tests, ensuring that both cases (empty and non-empty YesQuorum) are handled as expected.
  • 8223-8224: The case handling for yes_quorum in MessageBasedParams is correctly implemented, following the standard pattern for such checks in Go. Comprehensive testing should be conducted to ensure this logic works as intended under various scenarios.
  • 8249-8250: Clearing the YesQuorum field in MessageBasedParams by setting it to an empty string is appropriate and follows Go protobuf message handling conventions. This operation should be clearly documented and tested to verify its behavior.
  • 8277-8279: Retrieving the YesQuorum value as a protoreflect.ValueOfString in MessageBasedParams is correctly implemented. This approach is consistent with Go protobufs handling of string fields. Ensure that the YesQuorum field is properly documented.
  • 8310-8311: Setting the YesQuorum field from a protoreflect.Value in MessageBasedParams is correctly implemented. This standard approach for mutating protobuf messages in Go should be accompanied by clear documentation explaining the field's purpose and usage.
  • 8343-8344: The panic for attempting to mutate the immutable field yes_quorum in MessageBasedParams is correctly implemented. This ensures the integrity of the message by preventing unsupported operations. Documentation should clearly state the immutability of this field.
  • 8367-8368: Returning a default protoreflect.ValueOfString("") for YesQuorum in MessageBasedParams is correct. This implementation should be clearly documented to explain the default behavior when the field is not set.
  • 8450-8453: The calculation of the serialized size for YesQuorum in MessageBasedParams is correctly implemented. Ensure that this logic is covered by tests to verify the correctness under various scenarios.
  • 8491-8499: The serialization logic for YesQuorum in MessageBasedParams is correctly implemented. Testing this logic thoroughly is crucial, especially to validate the behavior when YesQuorum is set and when it's not.
  • 8652-8683: The unmarshalling logic for YesQuorum in MessageBasedParams, including error handling and length validations, is correctly implemented. This critical part of protobuf message handling should be thoroughly tested, particularly the error paths.
  • 9746-9750: The documentation for the YesQuorum field in Params is clear and specifies the default behavior. This aligns with the PR's objectives to enhance documentation clarity. Ensure that all related documentation is updated to reflect this addition.
  • 9906-9911: The GetYesQuorum method for Params is correctly implemented, providing a safe way to access the YesQuorum field. This method enhances the code's maintainability and usability, aligning with the PR's goals.
  • 9925-9932: The documentation and implementation of YesQuorum in MessageBasedParams are clear and follow the protobuf conventions in Go. Ensure that the documentation accurately reflects the purpose and usage of this field, maintaining the clarity and accessibility of the codebase.
  • 9970-9975: The GetYesQuorum method for MessageBasedParams is correctly implemented. This method follows the standard pattern for accessing fields in Go protobufs, enhancing the code's maintainability and clarity.

x/gov/keeper/tally_test.go Outdated Show resolved Hide resolved
x/gov/keeper/tally_test.go Outdated Show resolved Hide resolved
x/gov/simulation/genesis.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

few comments otherwise lgtm

CHANGELOG.md Outdated Show resolved Hide resolved
docs/build/building-modules/03-msg-services.md Outdated Show resolved Hide resolved
@@ -51,13 +51,13 @@ appd tx send <recipientAddress> 1000uatom --from <senderAddress> --gas auto --ga

#### Other Transaction Creation Methods

The command-line is an easy way to interact with an application, but `Tx` can also be created using a [gRPC or REST interface](../advanced/06-grpc_rest.md) or some other entry point defined by the application developer. From the user's perspective, the interaction depends on the web interface or wallet they are using (e.g. creating `Tx` using [Lunie.io](https://lunie.io/#/) and signing it with a Ledger Nano S).
Copy link
Member

Choose a reason for hiding this comment

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

holy throw back

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.

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between a9b3cca and c480cb5.
Files selected for processing (50)
  • CHANGELOG.md (7 hunks)
  • CODING_GUIDELINES.md (2 hunks)
  • CONTRIBUTING.md (3 hunks)
  • RELEASE_PROCESS.md (1 hunks)
  • ROADMAP.md (1 hunks)
  • UPGRADING.md (3 hunks)
  • docs/Introduction.md (1 hunks)
  • docs/architecture/README.md (1 hunks)
  • docs/architecture/adr-016-validator-consensus-key-rotation.md (1 hunks)
  • docs/architecture/adr-022-custom-panic-handling.md (1 hunks)
  • docs/architecture/adr-031-msg-service.md (1 hunks)
  • docs/architecture/adr-033-protobuf-inter-module-comm.md (2 hunks)
  • docs/architecture/adr-034-account-rekeying.md (1 hunks)
  • docs/architecture/adr-038-state-listening.md (1 hunks)
  • docs/architecture/adr-040-storage-and-smt-state-commitments.md (1 hunks)
  • docs/architecture/adr-042-group-module.md (1 hunks)
  • docs/architecture/adr-048-consensus-fees.md (1 hunks)
  • docs/architecture/adr-049-state-sync-hooks.md (1 hunks)
  • docs/architecture/adr-054-semver-compatible-modules.md (1 hunks)
  • docs/architecture/adr-059-test-scopes.md (2 hunks)
  • docs/architecture/adr-062-collections-state-layer.md (2 hunks)
  • docs/architecture/adr-063-core-module-api.md (2 hunks)
  • docs/build/abci/00-introduction.md (1 hunks)
  • docs/build/abci/01-prepare-proposal.md (1 hunks)
  • docs/build/abci/02-process-proposal.md (1 hunks)
  • docs/build/abci/03-vote-extensions.md (1 hunks)
  • docs/build/building-apps/04-security-part-1.md (1 hunks)
  • docs/build/building-modules/00-intro.md (1 hunks)
  • docs/build/building-modules/01-module-manager.md (2 hunks)
  • docs/build/building-modules/03-msg-services.md (1 hunks)
  • docs/build/building-modules/06-keeper.md (1 hunks)
  • docs/build/building-modules/15-depinject.md (1 hunks)
  • docs/build/building-modules/16-testing.md (2 hunks)
  • docs/learn/advanced/00-baseapp.md (5 hunks)
  • docs/learn/advanced/01-transactions.md (3 hunks)
  • docs/learn/advanced/02-context.md (1 hunks)
  • docs/learn/advanced/03-node.md (2 hunks)
  • docs/learn/advanced/05-encoding.md (2 hunks)
  • docs/learn/advanced/06-grpc_rest.md (2 hunks)
  • docs/learn/advanced/07-cli.md (5 hunks)
  • docs/learn/advanced/08-events.md (2 hunks)
  • docs/learn/beginner/00-app-anatomy.md (5 hunks)
  • docs/learn/beginner/01-tx-lifecycle.md (4 hunks)
  • docs/learn/beginner/02-query-lifecycle.md (4 hunks)
  • docs/learn/beginner/03-accounts.md (1 hunks)
  • docs/learn/beginner/04-gas-fees.md (1 hunks)
  • docs/learn/intro/02-sdk-app-architecture.md (1 hunks)
  • docs/spec/store/README.md (1 hunks)
  • store/snapshots/README.md (1 hunks)
  • x/README.md (3 hunks)
Files skipped from review as they are similar to previous changes (49)
  • CHANGELOG.md
  • CODING_GUIDELINES.md
  • CONTRIBUTING.md
  • RELEASE_PROCESS.md
  • ROADMAP.md
  • UPGRADING.md
  • docs/Introduction.md
  • docs/architecture/adr-016-validator-consensus-key-rotation.md
  • docs/architecture/adr-022-custom-panic-handling.md
  • docs/architecture/adr-031-msg-service.md
  • docs/architecture/adr-033-protobuf-inter-module-comm.md
  • docs/architecture/adr-034-account-rekeying.md
  • docs/architecture/adr-038-state-listening.md
  • docs/architecture/adr-040-storage-and-smt-state-commitments.md
  • docs/architecture/adr-042-group-module.md
  • docs/architecture/adr-048-consensus-fees.md
  • docs/architecture/adr-049-state-sync-hooks.md
  • docs/architecture/adr-054-semver-compatible-modules.md
  • docs/architecture/adr-059-test-scopes.md
  • docs/architecture/adr-062-collections-state-layer.md
  • docs/architecture/adr-063-core-module-api.md
  • docs/build/abci/00-introduction.md
  • docs/build/abci/01-prepare-proposal.md
  • docs/build/abci/02-process-proposal.md
  • docs/build/abci/03-vote-extensions.md
  • docs/build/building-apps/04-security-part-1.md
  • docs/build/building-modules/00-intro.md
  • docs/build/building-modules/01-module-manager.md
  • docs/build/building-modules/03-msg-services.md
  • docs/build/building-modules/06-keeper.md
  • docs/build/building-modules/15-depinject.md
  • docs/build/building-modules/16-testing.md
  • docs/learn/advanced/00-baseapp.md
  • docs/learn/advanced/01-transactions.md
  • docs/learn/advanced/02-context.md
  • docs/learn/advanced/03-node.md
  • docs/learn/advanced/05-encoding.md
  • docs/learn/advanced/06-grpc_rest.md
  • docs/learn/advanced/07-cli.md
  • docs/learn/advanced/08-events.md
  • docs/learn/beginner/00-app-anatomy.md
  • docs/learn/beginner/01-tx-lifecycle.md
  • docs/learn/beginner/02-query-lifecycle.md
  • docs/learn/beginner/03-accounts.md
  • docs/learn/beginner/04-gas-fees.md
  • docs/learn/intro/02-sdk-app-architecture.md
  • docs/spec/store/README.md
  • store/snapshots/README.md
  • x/README.md
Additional comments: 1
docs/architecture/README.md (1)
  • 63-63: Corrected the filename from adr-070-unordered-account.md to adr-070-unordered-transactions.md to accurately reflect the content of the ADR.

@samricotta samricotta added this pull request to the merge queue Feb 2, 2024
Merged via the queue into main with commit 26c6436 Feb 2, 2024
55 of 56 checks passed
@samricotta samricotta deleted the sam/broken-links branch February 2, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Store T:Docs Changes and features related to documentation. Type: ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants