chore: add spellchecker #303
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Codespell
participant Codebase
User->>Codespell: Run spell-check
Codespell->>Codebase: Check for terms in .codespellignore
Codebase-->>Codespell: Ignore specified terms
Codespell-->>User: Return spell-check results
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@Vritra4 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
x/genutil/collect.go (2)
101-105: Maintain consistent indentation.While the logic is correct, the indentation of the conditions appears inconsistent. Consider aligning the indentation of both if-continue blocks for better readability.
for _, entry := range entries { if entry.IsDir() { continue } -if !strings.HasSuffix(entry.Name(), ".json") { - continue -} + if !strings.HasSuffix(entry.Name(), ".json") { + continue + }
Line range hint
149-153: Remove debug print statements.The code contains temporary debugging statements using
runtime.Caller. These should be removed or replaced with proper logging.- _, file, no, ok := runtime.Caller(1) - if ok { - fmt.Printf("CollectTxs-1, called from %s#%d\n", file, no) - } - _, file, no, ok := runtime.Caller(1) - if ok { - fmt.Printf("CollectTxs-2, called from %s#%d - %s\n", file, no, valAccAddrStr) - }Also applies to: 160-164
x/move/client/cli/utils_test.go (1)
Line range hint
1-1: Update .codespellignore to preserve common test abbreviationsSince this PR introduces spell checking, please add
expResto.codespellignoreto preserve this common test abbreviation. This will prevent unintended changes to established testing conventions while still catching actual spelling errors.x/move/keeper/balancer.go (1)
Line range hint
209-219: LGTM! Consider consistent naming for paired variables.The variable renaming from
metadataAtometadataimproves clarity. However, for better symmetry withmetadataB, consider either:
- Keep both variables with suffixes:
metadataAandmetadataB- Remove suffixes from both:
metadata1andmetadata2This is a minor stylistic suggestion, the current implementation is functionally correct.
x/move/keeper/staking.go (2)
143-143: LGTM! Consider enhancing the function documentation.The spelling correction from "consequentially" to "consequently" is accurate. Consider enhancing the documentation to better describe the operation flow and potential error cases.
Add more details to the comment:
-// consequently delegate the deposited coins to a validator. +// consequently delegate the deposited coins to a validator. +// +// Flow: +// 1. Withdraws staking coins from move module account +// 2. Sends coins to delegator module account +// 3. Delegates coins to the specified validator +// +// Returns the delegation shares and any error encountered during the process. +// Possible errors: +// - If the module account already exists and is not empty +// - If coin transfer fails +// - If validator does not exist +// - If delegation fails
Line range hint
149-152: Enhance error message for better debugging.The error message for address collision could be more descriptive to aid in debugging.
- return sdk.NewDecCoins(), types.ErrAddressAlreadyTaken.Wrapf("module account %s is already taken", delModuleAddr.String()) + return sdk.NewDecCoins(), types.ErrAddressAlreadyTaken.Wrapf( + "module account %s is already taken by a non-module account type %T", + delModuleAddr.String(), + macc, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
.github/workflows/spellcheck.ymlis excluded by!**/*.ymlapi/initia/move/v1/query_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goclient/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlx/distribution/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc-hooks/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/mstaking/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/mstaking/types/staking.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/reward/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (41)
.github/config/.codespellignore(1 hunks)api/initia/distribution/v1/genesis.pulsar.go(1 hunks)api/initia/gov/v1/genesis.pulsar.go(1 hunks)api/initia/ibchooks/v1/genesis.pulsar.go(1 hunks)api/initia/move/v1/types.pulsar.go(2 hunks)api/initia/mstaking/v1/genesis.pulsar.go(1 hunks)api/initia/mstaking/v1/staking.pulsar.go(1 hunks)api/initia/reward/v1/genesis.pulsar.go(1 hunks)app/upgrade.go(1 hunks)cmd/move/move.go(1 hunks)contrib/devtools/Makefile(2 hunks)images/node/entrypoint.sh(1 hunks)proto/initia/distribution/v1/genesis.proto(1 hunks)proto/initia/gov/v1/genesis.proto(1 hunks)proto/initia/ibchooks/v1/genesis.proto(1 hunks)proto/initia/move/v1/query.proto(1 hunks)proto/initia/move/v1/types.proto(2 hunks)proto/initia/mstaking/v1/genesis.proto(1 hunks)proto/initia/mstaking/v1/staking.proto(1 hunks)proto/initia/reward/v1/genesis.proto(1 hunks)shared.Dockerfile(1 hunks)x/authz/client/cli/tx.go(1 hunks)x/bank/keeper/msg_server.go(1 hunks)x/distribution/types/dec_pool.go(1 hunks)x/distribution/types/pool.go(1 hunks)x/genutil/collect.go(2 hunks)x/gov/types/params.go(1 hunks)x/ibc/nft-transfer/client/cli/query.go(1 hunks)x/ibc/testing/README.md(2 hunks)x/ibc/testing/endpoint.go(1 hunks)x/intertx/keeper/msg_server_test.go(1 hunks)x/move/client/cli/utils_test.go(5 hunks)x/move/keeper/balancer.go(2 hunks)x/move/keeper/handler.go(1 hunks)x/move/keeper/keeper.go(1 hunks)x/move/keeper/staking.go(1 hunks)x/move/keeper/whitelist_test.go(1 hunks)x/move/types/authz.go(1 hunks)x/mstaking/keeper/slash.go(1 hunks)x/mstaking/types/authz.go(1 hunks)x/slashing/keeper/signing_info.go(1 hunks)
🔥 Files not summarized due to errors (1)
- api/initia/mstaking/v1/staking.pulsar.go: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (32)
- .github/config/.codespellignore
- api/initia/distribution/v1/genesis.pulsar.go
- api/initia/gov/v1/genesis.pulsar.go
- api/initia/ibchooks/v1/genesis.pulsar.go
- api/initia/move/v1/types.pulsar.go
- api/initia/mstaking/v1/genesis.pulsar.go
- api/initia/reward/v1/genesis.pulsar.go
- cmd/move/move.go
- contrib/devtools/Makefile
- images/node/entrypoint.sh
- proto/initia/distribution/v1/genesis.proto
- proto/initia/gov/v1/genesis.proto
- proto/initia/ibchooks/v1/genesis.proto
- proto/initia/move/v1/query.proto
- proto/initia/move/v1/types.proto
- proto/initia/mstaking/v1/genesis.proto
- proto/initia/mstaking/v1/staking.proto
- proto/initia/reward/v1/genesis.proto
- shared.Dockerfile
- x/authz/client/cli/tx.go
- x/distribution/types/dec_pool.go
- x/distribution/types/pool.go
- x/gov/types/params.go
- x/ibc/nft-transfer/client/cli/query.go
- x/ibc/testing/README.md
- x/ibc/testing/endpoint.go
- x/move/keeper/keeper.go
- x/move/keeper/whitelist_test.go
- x/move/types/authz.go
- x/mstaking/keeper/slash.go
- x/mstaking/types/authz.go
- x/slashing/keeper/signing_info.go
🔇 Additional comments (12)
app/upgrade.go (3)
Line range hint 51-82: Critical: Ensure data migration reliability and validation.
The migration logic appears sound but has several critical aspects that need attention:
- The descending order walk is correct to prevent overwriting, but consider adding a comment explaining this choice.
- Key cloning is properly implemented with
bytes.Clone(). - Consider adding validation steps:
- Count of migrated entries
- Verification of checksum computation
- Post-migration integrity check
Let's verify the migration logic:
#!/bin/bash
# Description: Check for similar migration patterns in the codebase
# Look for other Walk implementations to compare patterns
ast-grep --pattern 'Walk($_, $_, func($_, $_) ($_, $_) {'
# Search for similar upgrade handlers
rg -A 5 'SetUpgradeHandler'Consider adding these safety measures:
func (app *InitiaApp) RegisterUpgradeHandlers(cfg module.Configurator) {
+ // Track migration statistics
+ var migratedCount int
+ var checksumCount int
+
app.UpgradeKeeper.SetUpgradeHandler(
upgradeName,
func(ctx context.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
+ // Log start of migration
+ logger := app.Logger()
+ logger.Info("Starting VM store migration", "upgrade", upgradeName)
+
// ... existing code ...
+
err = app.MoveKeeper.VMStore.Walk(ctx, new(collections.Range[[]byte]).Descending(), func(key, value []byte) (stop bool, err error) {
// ... existing code ...
if separator == movetypes.ModuleSeparator {
checksum := movetypes.ModuleBzToChecksum(value)
value = checksum[:]
+ checksumCount++
}
// ... existing code ...
err = app.MoveKeeper.VMStore.Set(ctx, key, value)
if err != nil {
return true, err
}
+ migratedCount++
return false, nil
})
+
+ // Log migration statistics
+ logger.Info("Completed VM store migration",
+ "migrated_entries", migratedCount,
+ "checksum_entries", checksumCount)Line range hint 1-100: Verify upgrade handler test coverage.
This upgrade handler performs critical database migrations. Please ensure:
- Comprehensive test coverage including:
- Migration of different entry types
- Error scenarios
- Edge cases with various key lengths
- Validation of checksums
- Integration tests with actual VM store data
- Rollback scenarios
Let's check for existing tests:
#!/bin/bash
# Description: Check for upgrade handler tests
# Look for test files
fd -e go -e _test.go
# Search for upgrade handler tests
rg -l "TestUpgrade.*Handler" Line range hint 37-50: Document version information and verify migration completeness.
The comment block clearly explains the separator changes, but consider adding:
- The version where this change was introduced (0.6.0)
- Any backward compatibility implications
Let's verify that all separator constants are updated correctly in the codebase:
Consider extracting these separator constants into a versioned configuration to make future upgrades more maintainable.
✅ Verification successful
Separator constants are correctly defined and used in the upgrade handler
The verification confirms that:
-
The separator constants are properly defined in
x/move/types/keys.gowith the new values:- ModuleSeparator = byte(0)
- ChecksumSeparator = byte(1)
- ResourceSeparator = byte(2)
- TableEntrySeparator = byte(3)
- TableInfoSeparator = byte(4)
-
The upgrade handler in
app/upgrade.gocorrectly handles the migration by:- Preserving ModuleSeparator at byte(0)
- Adding ChecksumSeparator at byte(1)
- Shifting other separators accordingly
- Walking through the store and updating the data structure
The implementation matches the documented changes and handles the migration appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded separator values that might need updating
# Search for separator constant definitions
ast-grep --pattern 'const $_ = byte($_)'
# Search for direct byte comparisons that might be using old separator values
rg -A 2 'byte\([0-3]\)'
Length of output: 17554
Script:
#!/bin/bash
# Search for separator-related constants and their usage in the codebase
# Look for separator constants in move module
rg -A 5 "ModuleSeparator|ResourceSeparator|TableEntrySeparator|TableInfoSeparator|ChecksumSeparator" --type go
# Look for byte separator usage in move module
rg -A 5 "byte\([0-4]\)" --type go
# Look for separator-related code in move types
ast-grep --pattern 'const $_ = byte($_)' x/move/types/
Length of output: 11513
x/bank/keeper/msg_server.go (1)
105-105: LGTM! The comment clarification improves readability.
The updated comment "total In == total Out" better reflects the validation check performed by ValidateInputOutputs. The spacing makes it easier to read while maintaining technical accuracy.
Let's verify the validation logic referenced in the comment:
x/intertx/keeper/msg_server_test.go (1)
38-38: LGTM! Typo fix looks good.
The correction from "faliure" to "failure" improves code clarity and maintains consistency with other test case names.
Let's verify the consistency of "failure" spelling across test cases:
✅ Verification successful
Spelling consistency verified across the codebase
The search results confirm that "failure" is consistently spelled correctly throughout the codebase, including all test case names and comments. No instances of the misspelling "faliure" were found, indicating that this was an isolated typo that has been properly fixed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining misspellings of "failure" in test files
# Expected: No occurrences of "faliure" should be found
rg -i "faliure" --type go
Length of output: 25
Script:
#!/bin/bash
# Let's check for both "failure" and "faliure" with context to better understand the test case naming patterns
rg -i "fail[ui]re" --type go -A 2 -B 2
Length of output: 6608
x/genutil/collect.go (2)
81-82: LGTM! Improved variable naming.
The change from fos to entries makes the code more readable by using a more descriptive variable name that better represents its purpose.
110-110: LGTM! Secure file path handling.
The code correctly uses filepath.Join for path construction and maintains proper error handling.
x/move/keeper/handler.go (2)
Line range hint 1-600: Document JSON format expectations for new JSON-based methods
The migration from byte-based to JSON-based methods introduces potential compatibility concerns. Consider adding documentation that specifies:
- Expected JSON format for different argument types
- Examples of valid JSON inputs
- Handling of special characters and escape sequences
Review unsafe string conversions for potential risks
The code uses unsafe.String and unsafe.Slice for performance optimization. While this can be beneficial for performance, it needs careful consideration:
- Document the assumptions about input data
- Add validation checks before unsafe conversions
- Consider adding runtime checks in debug builds
Let's check for potential issues with unsafe conversions:
#!/bin/bash
# Search for patterns that might cause issues with unsafe string conversions
rg -g '*.go' 'unsafe\.(String|Slice).*json' .
# Look for any existing validation before unsafe conversions
rg -g '*.go' -B 5 'unsafe\.(String|Slice)' .Line range hint 424-437: Consider using proper JSON marshaling for callback arguments
While the current string formatting approach works for simple types, it may not handle all JSON edge cases correctly. Consider using proper JSON marshaling for better reliability and maintainability.
Here's a suggested improvement:
- fmt.Sprintf("\"%d\"", message.Callback.Id),
- fmt.Sprintf("%v", success),
+ string(must(json.Marshal(message.Callback.Id))),
+ string(must(json.Marshal(success))),Where must is a helper function:
func must[T any](v T, err error) T {
if err != nil {
panic(err)
}
return v
}Let's verify the callback execution behavior:
x/move/keeper/staking.go (1)
Line range hint 144-148: Verify validator address handling across the codebase.
The function handles validator addresses in multiple places. Let's verify that all validator address validations are consistent across the codebase.
Also applies to: 171-173
api/initia/mstaking/v1/staking.pulsar.go (2)
13000-13000: Consider the impact on existing validators
If this is indeed a new field, adding it to the validator struct could affect:
- Existing validator state migrations
- API compatibility
- Database schema changes
Please ensure:
- State migration handlers are in place
- API version is properly incremented if this is a breaking change
- Documentation is updated to reflect this new identifier
Let's check for migration handling:
#!/bin/bash
# Description: Look for state migration code and documentation updates
# Check for migration handlers
fd -e .go | xargs rg -l "RegisterMigration|MigrateState"
# Look for documentation updates
fd -e .md | xargs rg -i "unbonding[_-]id"13000-13000: Verify if this is a new field addition rather than a spelling fix
While this PR is marked as a spell-checking effort, the addition of UnbondingId appears to be a functional change that introduces a new field to track validator unbonding processes. This seems to go beyond the scope of a spell-checking PR.
Let's verify if this is indeed a new field:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
=======================================
Coverage 41.04% 41.04%
=======================================
Files 268 268
Lines 25728 25728
=======================================
Hits 10559 10559
Misses 13532 13532
Partials 1637 1637
|
Description
add a workflow to check spell
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 changeReviewers 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...