chore: clean-up main #22787
chore: clean-up main #22787
Conversation
julienrbrt
requested review from
auricom,
tac0turtle,
a team and
facundomedica
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications to the build and testing workflows for the SimApp project, including the transition from a legacy build process to a versioned approach. The build configuration now supports only 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: 0
🧹 Outside diff range and nitpick comments (3)
simapp/CHANGELOG.md (3)
50-50: Address the TODO comment about moving changelog elements.The TODO comment suggests moving changelog elements, but it's unclear which elements should be moved and where they should go.
Would you like me to help create a GitHub issue to track this task and provide a detailed plan for reorganizing the changelog elements?
Line range hint
1-50: Fix typos in the changelog documentation.There are a few typos in the changelog:
- "exautive" should be "exhaustive" in the introduction paragraph
- "UpradeKeeper" should be "UpgradeKeeper" in PR refactor(x/upgrade): migrate to appmodule.VersionMap #20485 description
Line range hint
1-50: Improve changelog structure and consistency.The changelog structure could be improved:
- The v0.50 to v0.52 section contains a mix of bullet points with and without PR references
- Older versions (v0.47 to v0.50, v0.46 to v0.47, v0.45 to v0.46) lack detailed changelog entries
Consider:
- Consistently formatting all entries with PR references
- Adding detailed changelog entries for older versions instead of just referring to UPGRADING.md
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
.github/workflows/build.yml(1 hunks).github/workflows/test.yml(0 hunks)scripts/build/build.mk(0 hunks)scripts/go-lint-all.bash(0 hunks)simapp/CHANGELOG.md(1 hunks)simapp/app.go(0 hunks)simapp/app_di.go(0 hunks)simapp/go.mod(1 hunks)simapp/simd/cmd/root.go(0 hunks)simapp/simd/cmd/root_di.go(0 hunks)
💤 Files with no reviewable changes (7)
- simapp/simd/cmd/root_di.go
- scripts/go-lint-all.bash
- simapp/app_di.go
- .github/workflows/test.yml
- simapp/simd/cmd/root.go
- scripts/build/build.mk
- simapp/app.go
🧰 Additional context used
📓 Path-based instructions (1)
simapp/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (4)
.github/workflows/build.yml (2)
53-54: LGTM: Build v2 step addition
The addition of the v2 build step with COSMOS_BUILD_OPTIONS=v2 aligns with the PR objectives of transitioning away from legacy components. This change supports the migration path to SDK v2.
Line range hint 18-19: Verify impact of dropping 32-bit ARM support
The removal of 32-bit ARM support could affect users still running on older ARM devices. Please ensure this change is documented in the release notes and migration guide.
✅ Verification successful
32-bit ARM support removal is a deliberate change with limited impact
The codebase shows this is an intentional architectural change where 32-bit ARM support is being dropped while maintaining ARM64 support. The impact is minimal because:
- ARM64 (64-bit ARM) support is still maintained in the build matrix
- The project's release artifacts (.goreleaser.yml) already support ARM64 builds
- No critical ARM32-specific configurations or dependencies were found in the codebase
- The change is clearly documented in the build.yml comment as "drop 32 bit support for now (and maybe forever)"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any ARM-specific build configurations or dependencies
rg -i "arm32|armv7" --type yaml
Length of output: 33
Script:
#!/bin/bash
# Let's check for any ARM-related configurations in the entire codebase
rg -i "arm|aarch" --type yaml
# Also check the build.yml file specifically to understand the changes
cat .github/workflows/build.yml
# Look for any architecture-related configurations
rg -i "architecture|platform|arch" --type yaml
Length of output: 3660
simapp/go.mod (2)
35-35: LGTM: Dependency cleanup
Appropriate cleanup of dependencies by marking tx and cast packages as indirect, as they are not directly imported by the module.
Also applies to: 42-42
Line range hint 7-8: Verify stability of pre-release dependencies
The module depends on pre-release versions:
collections v0.4.1-0.20241128094659-bd76b47e1d8bcometbft v1.0.0-rc2.0.20241127125717-4ce33b646ac9
Please ensure these versions are stable enough for production use.
✅ Verification successful
Based on the results from checking the repositories, I can now provide a final assessment:
Pre-release dependencies appear to be actively maintained and stable
The pre-release versions in use are from actively maintained repositories:
- CometBFT v1.0.0-rc2 was released in November 2024 and is a release candidate (RC) version, indicating it's in the final stages of testing before a stable release. The repository shows active development with recent commits and releases.
- The collections module version is tied to a specific commit from November 2024, which is a common practice for ensuring dependency stability while waiting for the next official release.
No critical issues were found related to these specific versions in the repositories. The pre-release status is acceptable in this context as these are part of the broader Cosmos SDK ecosystem where coordinated releases are common.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any reported issues with these dependencies
gh api graphql -f query='
{
repository(owner: "cosmos", name: "cosmos-sdk") {
issues(first: 5, states: OPEN, labels: ["bug"], searchQuery: "collections v0.4.1 in:body OR cometbft v1.0.0-rc2 in:body") {
nodes {
title
url
}
}
}
}'
Length of output: 604
Script:
#!/bin/bash
# Let's try a different approach to check for issues
# First, let's check if these dependencies are being actively maintained
gh api repos/cosmos/collections/commits?per_page=5
echo "---"
gh api repos/cometbft/cometbft/commits?per_page=5
# Also check for open issues mentioning these versions
gh api search/issues?q="collections+v0.4.1+repo:cosmos/cosmos-sdk+is:issue+is:open+label:bug"
echo "---"
gh api search/issues?q="cometbft+v1.0.0-rc2+repo:cosmos/cosmos-sdk+is:issue+is:open+label:bug"
# Let's also check the go.mod file for any additional context
cat go.mod
Length of output: 63907
Script:
#!/bin/bash
# Let's check the stability of these dependencies by looking at their recent releases and activity
# Check collections repository
gh api repos/cosmos/cosmos-sdk/commits?path=collections&per_page=5
echo "---"
# Check CometBFT repository releases
gh api repos/cometbft/cometbft/releases?per_page=5
# Also check if there are any recent discussions or issues about these versions
gh api search/issues?q="v1.0.0-rc2+repo:cometbft/cometbft+is:issue+state:open"
Length of output: 69845
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/integration/v2/app.go (2)
Line range hint
448-467: Improve error handling before state commitThe function should verify handler execution success before committing state changes. Consider this refactor:
func (app *App) RunMsg(t *testing.T, ctx context.Context, handler handler, option ...Option) (resp transaction.Msg, err error) { t.Helper() // set options cfg := &RunMsgConfig{} for _, opt := range option { opt(cfg) } // need to have integration context integrationCtx, ok := ctx.Value(contextKey).(*integrationContext) require.True(t, ok) resp, err = handler(ctx) + // Only commit if handler execution was successful + if err == nil && cfg.Commit { + if commitErr := app.Commit(integrationCtx.state); commitErr != nil { + return resp, fmt.Errorf("failed to commit state: %w", commitErr) + } + app.lastHeight++ + } - if cfg.Commit { - app.lastHeight++ - _, err := app.Commit(integrationCtx.state) - require.NoError(t, err) - } return resp, err }
Line range hint
448-467: Add unit tests for RunMsg functionThis test helper is critical for integration testing but lacks its own test coverage. Consider adding tests for:
- Success and failure scenarios
- Commit behavior with different options
- Error handling cases
Would you like me to help generate comprehensive test cases for this function?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
schema/indexer/start.go(1 hunks)tests/integration/v2/app.go(1 hunks)tests/integration/v2/evidence/infraction_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
schema/indexer/start.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/evidence/infraction_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/app.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
tests/integration/v2/evidence/infraction_test.go (1)
252-252: LGTM: Added essential consensus parameter validation
The addition of error checking for consensus parameters retrieval is crucial for the test setup, ensuring that the evidence validation will work with the correct parameters.
schema/indexer/start.go (1)
123-123: 🛠️ Refactor suggestion
Consider modernizing error handling and improving error message.
The comment indicates maintaining Go 1.12 compatibility, which seems to conflict with the PR's objective of cleaning up legacy components. Additionally, the error message could be more descriptive.
Let's verify the minimum Go version requirement:
If Go 1.12 compatibility is no longer required, consider these improvements:
- return IndexingTarget{}, fmt.Errorf("failed to unmarshal indexer config for target %q: %v", targetName, err) //nolint:errorlint // we support go 1.12, so no error wrapping
+ return IndexingTarget{}, fmt.Errorf("failed to unmarshal indexer config for target %q: config type %T: %w", targetName, targetCfg.Config, err)The suggested changes:
- Use error wrapping (
%w) for better error handling - Include the config type in the error message for better debugging
- Remove the legacy Go 1.12 comment
tests/integration/v2/app.go (1)
448-449: Good addition of t.Helper()
The addition of t.Helper() improves test failure reporting by showing the correct line numbers in test output. This is a good practice for test helper functions.
|
lint is failing |
julienrbrt
force-pushed
the
julien/cleanup-main
branch
from
1a7d0df to
d230ee2
Compare
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
math/dec_test.go (1)
1285-1285: LGTM with a minor suggestion for test naming.The test execution block follows Go testing best practices with proper error handling and validation. Consider making the test names more descriptive by using the test case values in a formatted string:
-t.Run(fmt.Sprint(tc.src), func(t *testing.T) { +t.Run(fmt.Sprintf("input=%s exp=%s", tc.src, tc.exp), func(t *testing.T) {server/v2/cometbft/abci_test.go (1)
823-834: Add error handling for handler registration.While the implementation is correct, it would be beneficial to handle potential errors from the handler registration to ensure robustness.
-_ = queryRouterBuilder.RegisterHandler( +if err := queryRouterBuilder.RegisterHandler( gogoproto.MessageName(&testdata.SayHelloRequest{}), helloFooHandler, - ) +); err != nil { + t.Fatalf("failed to register handler: %v", err) +}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
math/dec_test.go(1 hunks)server/v2/cometbft/abci_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
math/dec_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (4)
server/v2/cometbft/abci_test.go (4)
67-67: LGTM: Interface constraint changes are correct.
The transition from proto.Message to gogoproto.Message aligns with the modernization goals and provides better performance characteristics.
Also applies to: 71-71
79-79: LGTM: Message name retrieval updated correctly.
The change to gogoproto.MessageName maintains consistency with the new gogoproto-based message handling system.
109-109: LGTM: Message name retrieval updated correctly.
The change to gogoproto.MessageName maintains consistency with the new gogoproto-based message handling system.
897-897: LGTM: Proto registry initialization updated correctly.
The change to gogoproto.MergedRegistry with sync.OnceValues ensures thread-safe and consistent proto registry initialization.
Description
Removes simapp legacy from main. Additionally, all baseapp related stuff are being deleted from main (see #22392), so this starts that.
We advice users to use
runtime, it will make the migration to v2 easier, as runtime -> runtime/v2 is 3 line change.This won't be backported, legacy wiring will always be available on release/v0.52.x and lower branches.
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
go.modfile to ensure compatibility.