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

fix: check bugs from #19224 #19524

Merged
merged 7 commits into from
Feb 22, 2024
Merged

fix: check bugs from #19224 #19524

merged 7 commits into from
Feb 22, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Feb 22, 2024

Description

Noticed in #19512 that #19224 introduces a few issues.
This was unnoticed at merging #19224 as CI runs only on the diff of changes, and the issue I was doing was touching all modules, hence running CI on all modules.

Instead of fixing them there, as it is out of the scope of my PR, let's investigate and fix them here.

From what I have seen, we have the following issues:


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

  • New Features

    • Introduced significant modifications to the JSON payload structure in the auth_info section.
    • Added support for longer Cosmos addresses in MsgAuthorizeCircuitBreaker struct.
    • Enhanced transaction signing and handling mechanisms.
  • Bug Fixes

    • Removed obsolete fields in the body section to streamline operations.
  • Refactor

    • Updated various dependencies across modules to improve stability and performance.
    • Replaced several modules with more efficient alternatives or relative paths for enhanced integration.
  • Chores

    • Updated cosmossdk.io/api to version v0.7.3 and added new indirect dependencies across multiple modules to keep the project up-to-date with the latest standards and security practices.

Copy link
Contributor

coderabbitai bot commented Feb 22, 2024

Walkthrough

Walkthrough

The update includes significant modifications to the JSON payload structure in the auth_info section, removal of various fields in the body section, and updates to module versions across multiple components. New dependencies were added, especially for protocol buffers, and indirect requirements were updated. This overhaul impacts transaction handling, signing, and module interdependencies, aiming to enhance functionality and compatibility with new standards.

Changes

File(s) Change Summary
client/v2/autocli/testdata/... Modified JSON payload structure in auth_info; removed several fields in body.
client/v2/go.mod, x/.../go.mod (multiple modules) Updated cosmossdk.io/api to v0.7.3; added indirect dependencies; replaced modules.
x/authz/client/cli/tx_test.go, x/feegrant/client/cli/tx_test.go Added module imports; modified MakeTestEncodingConfig function call.
x/circuit/ante/circuit_test.go Updated Grantee and Granter fields with longer Cosmos addresses.
x/genutil/client/cli/gentx.go Refactored transaction signing and handling.
x/staking/client/cli/tx.go Updated method for converting address bytes to string representation.
Various go.mod files across modules Added and updated dependencies, especially for protocol buffers; replaced some dependencies with local or relative paths.

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.

@github-actions github-actions bot added the C:x/genutil genutil module issues label Feb 22, 2024
@julienrbrt julienrbrt marked this pull request as ready for review February 22, 2024 12:17
@julienrbrt julienrbrt requested a review from a team as a code owner February 22, 2024 12:17
Copy link
Contributor

@julienrbrt your pull request is missing a changelog!

@julienrbrt
Copy link
Member Author

@julienrbrt your pull request is missing a changelog!

Not needed.

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

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 40e7a30 and f3010aa.
Files selected for processing (33)
  • client/v2/autocli/testdata/msg-output.golden (1 hunks)
  • client/v2/go.mod (4 hunks)
  • client/v2/go.sum (1 hunks)
  • x/authz/client/cli/tx_test.go (2 hunks)
  • x/authz/go.mod (2 hunks)
  • x/bank/go.mod (3 hunks)
  • x/bank/go.sum (1 hunks)
  • x/circuit/ante/circuit_test.go (1 hunks)
  • x/circuit/go.mod (2 hunks)
  • x/circuit/go.sum (1 hunks)
  • x/distribution/go.mod (3 hunks)
  • x/distribution/go.sum (1 hunks)
  • x/evidence/go.mod (2 hunks)
  • x/evidence/go.sum (1 hunks)
  • x/feegrant/client/cli/tx_test.go (2 hunks)
  • x/feegrant/go.mod (3 hunks)
  • x/feegrant/go.sum (1 hunks)
  • x/genutil/client/cli/gentx.go (2 hunks)
  • x/gov/go.mod (2 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (3 hunks)
  • x/mint/go.sum (1 hunks)
  • x/nft/go.mod (2 hunks)
  • x/nft/go.sum (1 hunks)
  • x/params/go.mod (3 hunks)
  • x/params/go.sum (1 hunks)
  • x/protocolpool/go.mod (2 hunks)
  • x/protocolpool/go.sum (1 hunks)
  • x/staking/client/cli/tx.go (1 hunks)
  • x/staking/go.mod (3 hunks)
  • x/staking/go.sum (1 hunks)
  • x/upgrade/go.mod (3 hunks)
  • x/upgrade/go.sum (2 hunks)
Files not reviewed due to errors (3)
  • (no review received)
  • (no review received)
  • (no review received)
Additional comments: 63
x/circuit/ante/circuit_test.go (1)
  • 64-65: The update to longer Cosmos addresses for the Grantee and Granter fields in the MsgAuthorizeCircuitBreaker struct improves the realism of the test data. This change is a positive step towards reflecting more realistic use cases in tests.
x/genutil/client/cli/gentx.go (2)
  • 191-191: The error handling for authclient.SignTx is correctly implemented using errors.Wrap, providing clear context in case of failure. This aligns well with best practices for error handling in Go, ensuring that errors are wrapped with additional context to aid in debugging.
  • 203-203: The function writeSignedGenTx is correctly used to write the signed transaction to the specified output document. The error handling here is also done properly, using errors.Wrap to provide context in case of failure. This is a good practice, ensuring that any issues during the file writing process are clearly communicated.
x/evidence/go.mod (2)
  • 29-30: The addition of new indirect dependencies buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go is noted. It's important to ensure that these dependencies are necessary for the module's functionality and that their versions are compatible with the rest of the project. This change likely reflects an update in the protocol buffer definitions used by the Cosmos SDK and its dependencies on Cosmos and Tendermint proto files.
Verification successful

The verification process has confirmed that the newly added indirect dependencies buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go are indeed present in the x/evidence module's go.mod file, as well as in multiple other go.mod files throughout the project. This widespread inclusion across various modules suggests that these dependencies are necessary for the project's functionality and likely compatible with the rest of the project, given their adoption across different modules.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify the necessity and compatibility of new indirect dependencies
fd go.mod --exec grep -E "buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go|buf.build/gen/go/tendermint/tendermint/protocolbuffers/go"

Length of output: 5264



Script:

#!/bin/bash
# List unique go.mod files containing the specified dependencies
fd go.mod --exec grep -lE "buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go|buf.build/gen/go/tendermint/tendermint/protocolbuffers/go" | sort -u

Length of output: 582

* 171-171: The replacement of `cosmossdk.io/api` with a relative path (`../../api`) is a significant change. This suggests a move towards using a locally available version of the `api` module, possibly for development or to ensure compatibility with local changes. It's crucial to verify that this change does not affect the module's ability to interact with other parts of the Cosmos SDK and that it aligns with the project's dependency management strategy.
x/protocolpool/go.mod (2)
  • 29-30: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies is noted. Ensure these versions are compatible with the rest of the project and that their licenses are compliant with the project's license policy.
  • 171-171: The replacement of cosmossdk.io/api with a local path (../../api) in the replace section is a significant change. This approach is beneficial for local development and testing but ensure that this change is intended for the final merge into the main branch, as it might affect the build process in environments where the local path structure differs.
Verification successful

The replacement of cosmossdk.io/api with a local path (../../api) across multiple go.mod files suggests a deliberate and consistent practice within the project, indicating that this approach is intended for the project's structure and development process. While there was no direct evidence regarding the handling of these replacements in CI environments or during the build/test processes, the widespread and consistent use across the project implies that these local path replacements are likely compatible with the project's overall development and deployment workflows.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the local path replacements are intended and work in all expected environments
echo "Check local development and CI environments for compatibility with local path replacements."

Length of output: 192



Script:

#!/bin/bash
# Search for documentation or comments related to the local path replacement
echo "Searching for documentation or comments related to local path replacement..."
rg "cosmossdk.io/api => ../../api" -C 5

# Check for CI configuration files and analyze them for any references to the local path
echo "Checking CI configuration files for references to local path adjustments..."
fd '.github/workflows/|\.travis\.yml|\.circleci/' --exec rg "cosmossdk.io/api => ../../api"

# Look for build or test scripts that might adjust for the local path replacement
echo "Looking for build or test scripts referencing local path adjustments..."
fd 'Makefile|build.sh|test.sh' --exec rg "cosmossdk.io/api => ../../api"

Length of output: 9520

x/authz/go.mod (2)
  • 6-7: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies is consistent with the changes in the x/protocolpool module. As before, ensure these versions are compatible with the rest of the project and that their licenses are compliant with the project's license policy.
  • 15-15: The addition of cosmossdk.io/x/accounts as an indirect dependency is noted. Given that this is marked as indirect and has a placeholder version, ensure that this module is indeed used indirectly by other dependencies and that there are no version conflicts with direct dependencies.
x/distribution/go.mod (3)
  • 34-35: The addition of new indirect dependencies buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go with specific versions indicates an update to protocol buffer dependencies. Ensure these versions are compatible with the rest of the Cosmos SDK to avoid potential conflicts or unexpected behavior.
  • 54-55: The updates to github.com/cockroachdb/tokenbucket and github.com/cometbft/cometbft dependencies should be carefully reviewed for compatibility and security implications. Given that these updates can affect rate limiting and consensus mechanisms, thorough testing and validation are recommended to ensure these changes do not introduce regressions or vulnerabilities.
  • 172-172: The replacements specified for cosmossdk.io/api, cosmossdk.io/core, and other modules with local paths suggest a move towards using local versions of these modules. This is a common practice for development and testing but ensure that these paths are correctly set up in your development environment to avoid build errors. Additionally, confirm that the final production build uses the appropriate versions of these dependencies.
x/gov/go.mod (3)
  • 6-7: The addition of new indirect dependencies buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go with specific versions is consistent with updates in other modules. Ensure that these protocol buffer dependencies are compatible across the Cosmos SDK to maintain consistency and avoid potential conflicts.
  • 16-16: The addition of cosmossdk.io/x/accounts as an indirect dependency is noteworthy. Given that this is a new addition, verify that it integrates well with the existing functionality of the x/gov module and does not introduce any breaking changes or unexpected behavior.
  • 21-21: The update to github.com/cockroachdb/tokenbucket is mirrored in this module as well. As previously mentioned, ensure that this update is thoroughly tested for compatibility and does not introduce any security or performance issues, especially considering its potential impact on rate limiting functionalities.
x/nft/go.mod (2)
  • 25-26: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies is noted. Ensure these versions are compatible with the rest of the project and that their licenses are compliant with the project's licensing policy.
  • 171-171: The replacement of cosmossdk.io/api with a local path is observed. This change should be verified to ensure it does not impact the module's ability to fetch the correct version of cosmossdk.io/api for other developers or in CI/CD environments.
x/circuit/go.mod (2)
  • 25-26: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies is noted for the x/circuit module as well. As previously mentioned, ensure these versions are compatible with the rest of the project and that their licenses are compliant with the project's licensing policy.
  • 171-171: The replacement of cosmossdk.io/api with a local path in the x/circuit module is observed. This change should be verified to ensure it does not impact the module's ability to fetch the correct version of cosmossdk.io/api for other developers or in CI/CD environments, similar to the x/nft module.
x/mint/go.mod (3)
  • 14-15: The addition of new indirect dependencies cosmossdk.io/x/accounts and github.com/cockroachdb/tokenbucket is noted. Ensure that these dependencies are necessary for the x/mint module's functionality and that their inclusion does not introduce any version conflicts or unnecessary bloat to the module's dependency graph.
  • 29-30: The update to indirect dependencies versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go is observed. It's important to verify that these updates are compatible with the rest of the project and do not introduce any breaking changes, especially since they are related to protocol buffers which are critical for data serialization and deserialization.
  • 172-172: The replacement of modules cosmossdk.io/api, cosmossdk.io/core, cosmossdk.io/depinject, and cosmossdk.io/x/accounts with relative paths is a significant change. This approach is typically used to facilitate local development or when preparing to spin out modules into separate repositories. Ensure that this change is intentional and that the relative paths correctly point to the local versions of these modules. Additionally, confirm that this does not affect the module's ability to be used in projects that depend on it from outside the local development environment.
x/feegrant/go.mod (4)
  • 6-6: The update to cosmossdk.io/api to version v0.7.3 is noted. Ensure that this version is compatible with the rest of the project and that it includes any necessary features or fixes that prompted the update. Compatibility with other modules that depend on cosmossdk.io/api should also be verified.
  • 14-17: The addition of new indirect dependencies, including cosmossdk.io/x/accounts and github.com/cockroachdb/tokenbucket, is observed. Similar to the x/mint module, ensure that these dependencies are necessary for the x/feegrant module's functionality and that their inclusion does not introduce any version conflicts or unnecessary bloat to the module's dependency graph.
  • 34-37: The update to indirect dependencies versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go, as well as the addition of cosmossdk.io/x/protocolpool as an indirect dependency, is noted. Verify that these updates and the new dependency are compatible with the rest of the project and do not introduce any breaking changes, especially since they relate to critical aspects of the project such as protocol buffers and transaction handling.
  • 175-175: The replacement of modules with relative paths in the x/feegrant module mirrors the changes observed in the x/mint module. Ensure that this change is intentional and that the relative paths correctly point to the local versions of these modules. Additionally, confirm that this does not affect the module's ability to be used in projects that depend on it from outside the local development environment.
x/params/go.mod (3)
  • 6-6: The update of cosmossdk.io/api to v0.7.3 is a positive change, ensuring the module uses the latest stable version. This should help with compatibility and access to the latest features or bug fixes.
  • 29-30: Adding new dependencies buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies indicates an enhancement in protocol buffer support. Ensure these additions are necessary for the module's functionality and that their versions are compatible with the rest of the project.
  • 172-172: The modifications in the replace directives are crucial for local development and testing, ensuring that local paths are used instead of fetching the modules from the remote repository. This is particularly useful for simultaneous development across modules. Ensure that these paths correctly point to the local copies of the modules.
x/group/go.mod (1)
  • 6-6: Updating cosmossdk.io/api to v0.7.3 in the x/group module aligns with the update in the x/params module, promoting consistency across modules. This update should be beneficial for maintaining compatibility and leveraging the latest features or bug fixes from the api module.
client/v2/go.mod (4)
  • 6-6: The update of cosmossdk.io/api from a specific commit version to v0.7.3 is a good practice as it moves to a stable version. This should help ensure compatibility and stability within the module dependencies.
  • 31-31: Adding cosmossdk.io/x/accounts as a new indirect requirement is appropriate if it's being used indirectly by other dependencies. It's important to ensure that this module is indeed needed to avoid unnecessary dependencies.
Verification successful

The verification process has confirmed that the addition of cosmossdk.io/x/accounts as an indirect requirement in the go.mod file is justified. The module is indeed used throughout the codebase, as evidenced by the numerous references found in various files, including implementation details, tests, and specific modules. This widespread usage supports the necessity of including this module to ensure the project's dependencies are accurately represented.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify if cosmossdk.io/x/accounts is used indirectly by checking imports in the codebase.
rg --type go 'cosmossdk.io/x/accounts'

Length of output: 3489

* 49-49: The addition of `github.com/cockroachdb/tokenbucket` as an indirect requirement suggests new functionality or dependencies that require rate limiting. It's crucial to verify that this library is used appropriately and does not introduce performance bottlenecks. * 169-172: The inclusion of `buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go` and `buf.build/gen/go/tendermint/tendermint/protocolbuffers/go` with specific versions as indirect dependencies is notable. It's important to ensure these versions are compatible with the rest of the project and that they are necessary for the project's protobuf definitions.
x/upgrade/go.mod (2)
  • 6-6: Updating cosmossdk.io/api to v0.7.3 aligns with the changes in the client/v2/go.mod file, promoting consistency across modules. This is a positive change, ensuring that all parts of the project use the same version of this dependency.
  • 34-35: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go as indirect dependencies is consistent with the changes in the client/v2/go.mod file. It's important to ensure these versions are compatible with the project's protobuf definitions, similar to the verification needed in the client/v2/go.mod file.
x/staking/client/cli/tx.go (1)
  • 158-158: The change from AddressCodec to ValidatorAddressCodec for converting address bytes to a string representation in NewEditValidatorCmd is noted. It's crucial to ensure this change aligns with the handling of validator addresses across the application and does not introduce regressions or compatibility issues. Please verify the impact of this change, especially in areas where validator addresses are used or processed.
x/authz/client/cli/tx_test.go (2)
  • 19-19: The addition of the authz module import and the inclusion of authz.AppModule{} in the SetupSuite function are noted. These changes are essential for integrating the authz module into the test setup. Please ensure these changes are consistent with the overall testing strategy and do not introduce unintended side effects in the test environment.
  • 54-54: The modification to include authz.AppModule{} in the MakeTestEncodingConfig function call within SetupSuite is aimed at enhancing test coverage for the authz module. It's important to verify that this inclusion correctly configures the test environment for authz module tests and aligns with the objectives of comprehensive and accurate test coverage.
x/feegrant/client/cli/tx_test.go (3)
  • 22-22: The addition of "cosmossdk.io/x/gov" import aligns with the PR's objective to address registration errors and transaction decoding issues by ensuring the gov module is included in the test setup. This change is necessary for the tests to cover functionalities related to the gov module, which might be indirectly affected by the feegrant and authz modules.
  • 64-64: The modification in the SetupSuite method to include gov.AppModule{} in the MakeTestEncodingConfig call is crucial for initializing the test suite with the necessary modules. This change ensures that the gov module is considered during encoding and decoding operations in tests, which is essential for accurately testing the integration and behavior of the feegrant module with other parts of the Cosmos SDK, especially in light of the transaction decoding errors mentioned in the PR objectives.
  • 19-25: > 📝 NOTE

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

Overall, the test suite in x/feegrant/client/cli/tx_test.go is well-structured and follows best practices for maintainability and readability. The changes made in this PR are aligned with the objectives and contribute to addressing the issues identified in previous PRs. The inclusion of the gov module in the test setup is a necessary adjustment for comprehensive testing, ensuring that the feegrant module's integration with other parts of the Cosmos SDK is accurately represented.

x/bank/go.sum (1)
  • 1-9: > 📝 NOTE

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

The updates in the go.sum file reflect the addition and updates of module versions as expected from the context provided. It's important to ensure that all dependency updates are intentional and have been verified for compatibility and security. If there are any dependencies updated or added unintentionally, it would be good to review those changes closely.

x/nft/go.sum (1)
  • 1-9: > 📝 NOTE

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

The go.sum file has been updated with new versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go, buf.build/gen/go/tendermint/tendermint/protocolbuffers/go, and cloud.google.com/go, and it has removed a specific version of cosmossdk.io/api. These changes are consistent with the typical updates in a Go project's dependencies to either upgrade to newer versions for added features, improvements, or security patches, or to remove unused dependencies. Ensure that these updates are intentional and that they have been tested to not break existing functionality.

x/params/go.sum (4)
  • 1-4: The addition of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go modules is consistent with the AI-generated summary. These additions likely support the project's need for updated or additional protocol buffer definitions from Cosmos and Tendermint.
  • 5-5: The update to cloud.google.com/go versions is mentioned in the AI-generated summary. However, without specific version numbers in the summary, it's challenging to verify the exact nature of the updates. It's assumed these updates are for maintaining compatibility or leveraging new features or fixes from the cloud.google.com/go library.
  • 1-9: > 📝 NOTE

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

The AI-generated summary mentions the removal of cosmossdk.io/api module versions, but the go.sum file provided does not show any removal actions. This might be due to the nature of go.sum which includes checksums for both current and previous versions of modules. It's important to check the go.mod file to confirm the removal of cosmossdk.io/api.

  • 1-9: > 📝 NOTE

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

The addition of cosmossdk.io/collections and cosmossdk.io/errors is noted and aligns with the AI-generated summary. These additions are likely for utilizing specific functionalities provided by these modules in the Cosmos SDK.

x/protocolpool/go.sum (3)
  • 1-4: The addition of new versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go suggests updates to these dependencies. Ensure these updates are intentional and compatible with the project's requirements.
  • 5-5: The cloud.google.com/go module version is updated. Verify that this update does not introduce breaking changes or conflicts with other dependencies.
  • 6-6: The removal of cosmossdk.io/api v0.7.3-0.20231113122742-912390d5fc4a indicates this version is no longer used. Confirm that this removal is intentional and that any necessary replacements are correctly reflected in the project's dependencies.
x/distribution/go.sum (4)
  • 1-2: Added new versions of buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go. Ensure these updates are necessary and do not introduce compatibility issues with existing code.
  • 3-4: Added new versions of buf.build/gen/go/tendermint/tendermint/protocolbuffers/go. Verify that these updates are required for new features or bug fixes and check for any breaking changes.
  • 5-6: Updated cloud.google.com/go to versions v0.26.0 and v0.34.0. Confirm that these updates address specific needs or improvements and do not break existing functionality.
  • 7-7: Removed versions of cosmossdk.io/api. Ensure the removal is intentional and does not affect the project's dependencies or functionality.
client/v2/go.sum (3)
  • 1-4: The addition of new versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go is correctly reflected in the go.sum file.
  • 5-5: The update of versions for cloud.google.com/go is not explicitly mentioned in the provided go.sum file content. However, multiple versions of cloud.google.com/go are listed, which is common as different parts of the project may depend on different versions of the same package.
  • 6-6: The removal of versions for cosmossdk.io/api is not directly observable in the provided go.sum file content since the file does not show removed lines. Assuming the removal was correctly handled by the Go toolchain, this should not be an issue.
x/upgrade/go.sum (5)
  • 1-4: The addition of new versions for buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go and buf.build/gen/go/tendermint/tendermint/protocolbuffers/go is noted. Ensure these updates are compatible with the rest of the project dependencies and that there are no breaking changes introduced by these new versions.
  • 1-4: The cloud.google.com/go package versions listed here do not represent an addition or update in this specific hunk. It's important to ensure that the versions of cloud.google.com/go mentioned are correctly updated elsewhere in the go.sum file to maintain compatibility and leverage any bug fixes or new features.
  • 1-4: The removal of cosmossdk.io/api versions is mentioned in the summary but not shown in the provided code snippets. If cosmossdk.io/api versions are indeed removed, verify that this does not introduce any compatibility issues or missing dependencies in the project. Ensure that any functionality previously relying on cosmossdk.io/api is either no longer needed or has been replaced with an equivalent solution.
  • 1-4: The updates to cloud.google.com/go packages, as mentioned in the AI-generated summary, are not directly shown in the provided code snippets. However, it's crucial to ensure that these updates are in line with the project's requirements and do not introduce any compatibility issues. Additionally, verify that all necessary updates to cloud.google.com/go are reflected across the project to maintain consistency and leverage improvements or bug fixes.
  • 1-4: The addition of cosmossdk.io/collections v0.4.0 and cosmossdk.io/errors v1.0.1 indicates an update or introduction of these dependencies. Ensure that these versions are compatible with the project's existing dependencies and that their inclusion addresses specific needs or improvements outlined in the PR objectives. It's also important to review the impact of these additions on the project's overall dependency graph to avoid potential conflicts or issues.

x/staking/go.sum Show resolved Hide resolved
@@ -1 +1 @@
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","to_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","amount":[{"denom":"foo","amount":"1"}]}],"memo":"","timeout_height":"0","unordered":false,"extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","to_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","amount":[{"denom":"foo","amount":"1"}]}]},"auth_info":{"fee":{"gas_limit":"200000"}}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The significant modification to the JSON payload structure, specifically the removal of several fields from the auth_info section, could impact backward compatibility and the behavior of existing clients that rely on these fields. It's important to ensure that these changes are clearly communicated to developers and that any necessary migration paths or adjustments are provided.

x/staking/go.mod Show resolved Hide resolved
x/bank/go.mod Show resolved Hide resolved
x/authz/go.mod Show resolved Hide resolved

replace github.com/cosmos/cosmos-sdk => ./../../

replace (
cosmossdk.io/api => ../../api
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The verification process has confirmed that the specified local paths in the replacements section of the go.mod file are missing or incorrect. This indicates an issue with the directory structure not aligning with the expectations for local development and testing. It's important to correct these paths to ensure that local modules are used as intended during development.

Analysis chain

The replacements section is crucial for local development and testing, ensuring that local paths are used instead of fetching modules from remote repositories. This is particularly important for modules under active development. Ensure that all local paths specified in the replacements are correct and point to the intended directories.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check if local paths specified in replacements exist and are correct.
for path in api core depinject x/accounts x/auth x/bank x/distribution x/gov x/mint x/protocolpool x/slashing x/staking x/tx; do
  if [ ! -d "../../$path" ]; then
    echo "Missing or incorrect path: ../../$path"
  fi
done

Length of output: 3263

x/upgrade/go.mod Show resolved Hide resolved
Copy link
Member

@facundomedica facundomedica left a comment

Choose a reason for hiding this comment

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

Thank you

img

@julienrbrt julienrbrt enabled auto-merge February 22, 2024 13:47
@julienrbrt julienrbrt added this pull request to the merge queue Feb 22, 2024
@@ -1 +1 @@
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","to_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","amount":[{"denom":"foo","amount":"1"}]}],"memo":"","timeout_height":"0","unordered":false,"extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Point 1, empty fields are omitted

Copy link
Member

Choose a reason for hiding this comment

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

hmm, does this introduce malleability, cc @aaronc @kocubinski @testinginprod

Copy link
Member

Choose a reason for hiding this comment

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

This is on the CLI side, I don't think it changes how we are verifying signatures, but yeah it's worth to look into it

Merged via the queue into main with commit 4d5b140 Feb 22, 2024
60 of 61 checks passed
@julienrbrt julienrbrt deleted the julien/19224 branch February 22, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants