revert: upgrade cometbft and cosmos-sdk #2956
Conversation
This reverts commit 5d981aa.
lumtis
requested review from
a team,
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple configuration files and scripts. Key changes include updates to the CI workflow to enhance testing commands and failure handling, adjustments to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
scripts/gen-docs-zetacored.sh (1)
Line range hint
1-45: Enhance script security, performance, and maintainability.While the script functions as intended, there are several areas where it can be improved:
Security:
- Handle filenames with spaces and special characters to prevent unexpected behavior.
Performance:
- Combine multiple sed commands into a single operation per file.
- Avoid creating and removing temporary .bak files for each operation.
Maintainability:
- Add comments explaining the purpose of each sed operation.
- Implement version checks for required tools (zetacored, sed).
Consider the following improvements:
#!/bin/bash set -euo pipefail # Check for required tools command -v zetacored >/dev/null 2>&1 || { echo >&2 "zetacored is required but not installed. Aborting."; exit 1; } command -v sed >/dev/null 2>&1 || { echo >&2 "sed is required but not installed. Aborting."; exit 1; } DIR="docs/cli/zetacored" rm -rf "$DIR" if ! go install ./cmd/zetacored; then echo "Error: Failed to install zetacored. Aborting documentation generation." exit 1 fi zetacored docs --path "$DIR" # Recursive function to process files process_files() { local dir="$1" while IFS= read -r -d '' file; do # Combine all sed operations into a single command sed -i.bak -e 's/<\([^<>]*\)>/[\1]/g' \ -e 's/\[appd\]/zetacored/g' \ -e 's/(default ".*")//g' \ -e 's/\(\](#[^)]*\)\.md)/\1)/g' "$file" # Remove the backup file rm -f "$file.bak" done < <(find "$dir" -type f -print0) } # Start processing from the given directory process_files "$DIR" echo "Documentation generation complete."This refactored version addresses the identified issues and improves overall script quality.
zetaclient/context/chain.go (1)
68-68: Approve the simplification with a suggestion for using the standard library.The simplification of the sorting logic is commendable, improving both readability and potentially performance. However, the use of the experimental
slicespackage fromgolang.org/x/expmight raise concerns in a production environment.Consider using the standard library's
sort.Slicefunction for improved stability:sort.Slice(items, func(i, j int) bool { return items[i].ID() < items[j].ID() })This alternative maintains the simplicity of the current implementation while leveraging the stable standard library.
Makefile (1)
Line range hint
17-32: Simplification of build flags may impact build processThe changes to the build flags and variable definitions have simplified the Makefile, but they may have unintended consequences:
- The removal of
-extldflags=-Wl,--allow-multiple-definitionfromBUILD_FLAGScould potentially lead to linking errors if there are multiple definitions of symbols in the codebase.- The simplification of
TEST_BUILD_FLAGSandHSM_BUILD_FLAGSby removing the-ldflagsoption might affect the build process for tests and HSM-related builds.Please verify that these changes do not introduce any build issues, especially for tests and HSM-related builds. Consider running a comprehensive test suite to ensure all builds complete successfully with these new flag configurations.
go.mod (4)
Line range hint
1-5: Consider aligning Go version with toolchain versionThe Go version (1.22.2) and the toolchain version (1.22.5) are mismatched. It is recommended to align these versions to ensure consistency across development environments and avoid potential compatibility issues.
Consider updating the Go version to match the toolchain:
-go 1.22.2 +go 1.22.5
205-205: Note the changes in indirect dependenciesSeveral indirect dependencies have been updated or downgraded. Most notably:
- github.com/klauspost/compress: v1.17.2 -> v1.16.7
While changes to indirect dependencies are generally less critical, the downgrade of
klauspost/compressmight affect compression performance in the project.Consider monitoring the project's performance, particularly in areas that involve data compression, to ensure that this downgrade doesn't negatively impact your application.
Also applies to: 263-263
Line range hint
371-385: Ensure maintenance of forked dependenciesThe
go.modfile includes severalreplacedirectives, particularly for ZetaChain maintained forks:replace ( github.com/bnb-chain/tss-lib => github.com/zeta-chain/tss-lib v0.0.0-20240916163010-2e6b438bd901 github.com/ethereum/go-ethereum => github.com/zeta-chain/go-ethereum v1.10.26-spc github.com/libp2p/go-libp2p => github.com/zeta-chain/go-libp2p v0.0.0-20240710192637-567fbaacc2b4 gitlab.com/thorchain/tss/go-tss => github.com/zeta-chain/go-tss v0.0.0-20240916173049-89fee4b0ae7f )While these forks are likely necessary for project-specific modifications, they can lead to divergence from upstream packages over time.
To mitigate potential issues:
- Implement a regular schedule for syncing these forks with their upstream repositories.
- Document the reasons for each fork and the specific modifications made.
- Consider contributing non-project-specific improvements back to the original repositories when possible.
- Set up automated checks to alert when significant updates occur in the original repositories that might need to be incorporated into the forks.
Line range hint
1-385: Summary of go.mod changes and recommendationsThis update to
go.modprimarily involves downgrading several dependencies to resolve build issues. While this addresses the immediate problem, it introduces potential risks:
- Security: Downgraded versions might reintroduce resolved vulnerabilities.
- Performance: Some changes, like downgrading
klauspost/compress, might affect performance.- Maintenance: The use of forked dependencies requires ongoing synchronization with upstream repositories.
To ensure long-term stability and security:
- Implement a comprehensive test suite to verify that the downgrades haven't reintroduced bugs or vulnerabilities.
- Set up automated dependency scanning to alert on known security issues.
- Establish a clear timeline and process for re-upgrading these dependencies once the build issues are resolved.
- Regularly review and update forked dependencies to minimize divergence from upstream.
These steps will help balance the immediate need to fix build issues with the long-term health and security of the project.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- .github/workflows/ci.yml (1 hunks)
- .golangci.yml (0 hunks)
- Makefile (3 hunks)
- contrib/rpcimportable/test.sh (0 hunks)
- go.mod (12 hunks)
- scripts/gen-docs-zetacored.sh (1 hunks)
- zetaclient/context/chain.go (1 hunks)
💤 Files with no reviewable changes (2)
- .golangci.yml
- contrib/rpcimportable/test.sh
🧰 Additional context used
📓 Path-based instructions (2)
scripts/gen-docs-zetacored.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.zetaclient/context/chain.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
Makefile (1)
Line range hint
1-499: Comprehensive testing required for build process changesThe changes in this Makefile appear to be reverting some previous modifications, potentially addressing the reported issues with the
makecommand. While these simplifications may resolve the immediate build problems, they could also introduce new challenges or regressions in other areas of the build process.To ensure the integrity of the build process across all use cases, please run the following comprehensive test suite:
Please review the output of these tests carefully to ensure that all build processes complete successfully and that no regressions have been introduced by these changes.
go.mod (1)
13-13: Verify security implications of dependency downgradesSeveral major dependencies have been downgraded:
- github.com/btcsuite/btcd/btcec/v2: v2.3.4 -> v2.3.2
- github.com/cometbft/cometbft: v0.37.11 -> v0.37.4
- github.com/cosmos/cosmos-sdk: v0.47.14 -> v0.47.10
- github.com/fatih/color: v1.14.1 -> v1.13.0
While these downgrades align with the PR objective to resolve build issues, it's crucial to ensure that they don't reintroduce any known vulnerabilities or bugs that were fixed in the newer versions.
Please run the following script to check for any known vulnerabilities in the downgraded versions:
This script will search for security-related issues in the GitHub repositories of the downgraded dependencies. Review any returned URLs for potential security implications.
Also applies to: 19-19, 22-22, 28-28
✅ Verification successful
Dependency downgrades do not introduce known vulnerabilities
After verifying the downgraded dependencies, no known security vulnerabilities were found in the specified versions:
- github.com/btcsuite/btcd/btcec/v2: v2.3.2
- github.com/cometbft/cometbft: v0.37.4
- github.com/cosmos/cosmos-sdk: v0.47.10
- github.com/fatih/color: v1.13.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in downgraded dependencies echo "Checking for vulnerabilities in btcd/btcec v2.3.2" gh api -X GET search/issues -f q="repo:btcsuite/btcd is:issue label:security merged:>2022-01-01" --jq '.items[].html_url' echo "Checking for vulnerabilities in cometbft v0.37.4" gh api -X GET search/issues -f q="repo:cometbft/cometbft is:issue label:security merged:>2022-01-01" --jq '.items[].html_url' echo "Checking for vulnerabilities in cosmos-sdk v0.47.10" gh api -X GET search/issues -f q="repo:cosmos/cosmos-sdk is:issue label:security merged:>2022-01-01" --jq '.items[].html_url' echo "Checking for vulnerabilities in fatih/color v1.13.0" gh api -X GET search/issues -f q="repo:fatih/color is:issue label:security merged:>2022-01-01" --jq '.items[].html_url'Length of output: 943
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2956 +/- ##
========================================
Coverage 66.39% 66.39%
========================================
Files 389 389
Lines 21758 21758
========================================
Hits 14447 14447
Misses 6584 6584
Partials 727 727
|
Reverts #2934
Seems there is an issue building from MacOS with the new flags added
Summary by CodeRabbit
New Features
zetacoredbefore generating documentation.Bug Fixes
Refactor
ChainRegistrystruct for better performance and clarity.Chores