chore: cleanup/minor refactoring #69
Conversation
|
Warning Rate limit exceeded@Vritra4 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 1 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. WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on enhancing build configurations, error handling, and pagination functionalities. The Changes
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
CodeRabbit Configuration File (
|
![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 (6)
submodules/move-nft/collect.go (1)
Line range hint
1-400: Consider standardizing error handling patterns across the fileWhile the improved error handling at line 98 is good, there's an opportunity to apply similar improvements throughout the file. Many error messages are generic (e.g., "failed to unmarshal mint event") and could benefit from including more context or preserving original errors.
Consider:
- Preserving original errors instead of wrapping them with generic messages
- Including relevant context in error messages (e.g., addresses, IDs)
- Standardizing the logging and error handling pattern across all event handlers
Example improvement pattern:
- return errors.New("failed to unmarshal mint event") + return fmt.Errorf("failed to unmarshal mint event: %w", err)collection/option.go (1)
8-15: Enhance documentation with usage examplesWhile the documentation explains the purpose, it would be more helpful to include:
- Example usage
- Description of the generic type parameters K1, K2, K3
- Return value explanation
Consider adding documentation like this:
// WithCollectionPaginationTriplePrefix applies a prefix to a collection, whose key is a collections.Triple, // being paginated that needs prefixing. // // Type Parameters: // - K1: Type of the first key component // - K2: Type of the second key component // - K3: Type of the third key component // // Example: // // opts := query.CollectionsPaginateOptions[collections.Triple[string, uint64, []byte]]{} // WithCollectionPaginationTriplePrefix("prefix")(opts)config/types.go (1)
16-16: Consider future-proofing the backend configurationThe comment indicates "goleveldb" is the only supported type currently. Consider:
- Making this limitation more visible in the code through validation
- Planning for future backend support through an interface
Would you like help designing a more extensible backend configuration system?
x/kvindexer/keeper/handler.go (2)
Line range hint
68-82: Improve error handling and logging in HandleFinalizeBlockWhile the panic recovery is good, consider these improvements:
- The recovered panic error is not propagated
- Submodule failures are only logged as warnings
Consider this approach:
func (k *Keeper) HandleFinalizeBlock(ctx context.Context, req abci.RequestFinalizeBlock, res abci.ResponseFinalizeBlock) (err error) { defer func() { if r := recover(); r != nil { - k.Logger(ctx).Error("panic in HandleFinalizeBlock", "err", err) + stack := string(debug.Stack()) + k.Logger(ctx).Error("panic in HandleFinalizeBlock", "err", r, "stack", stack) + err = fmt.Errorf("panic in HandleFinalizeBlock: %v", r) debug.PrintStack() } }() + var errs []error for name, svc := range k.submodules { if err = svc.FinalizeBlock(ctx, req, res); err != nil { - k.Logger(ctx).Warn("failed to handle finalize block event", "submodule", name) + k.Logger(ctx).Error("failed to handle finalize block event", "submodule", name, "error", err) + errs = append(errs, fmt.Errorf("submodule %s: %w", name, err)) } } + if len(errs) > 0 { + return fmt.Errorf("multiple submodules failed: %v", errs) + } return nil }
Line range hint
83-101: Apply consistent error handling in HandleCommitThe same error handling improvements should be applied to HandleCommit for consistency.
Apply similar changes as suggested for HandleFinalizeBlock:
func (k *Keeper) HandleCommit(ctx context.Context, res abci.ResponseCommit, changeSet []*storetypes.StoreKVPair) (err error) { defer func() { - if err := recover(); err != nil { - k.Logger(ctx).Error("panic in HandleCommit", "err", err) + if r := recover(); r != nil { + stack := string(debug.Stack()) + k.Logger(ctx).Error("panic in HandleCommit", "err", r, "stack", stack) + err = fmt.Errorf("panic in HandleCommit: %v", r) debug.PrintStack() } }() + var errs []error for name, svc := range k.submodules { if err := svc.Commit(ctx, res, changeSet); err != nil { - k.Logger(ctx).Warn("failed to handle commit event", "submodule", name) + k.Logger(ctx).Error("failed to handle commit event", "submodule", name, "error", err) + errs = append(errs, fmt.Errorf("submodule %s: %w", name, err)) } } k.store.Write() + if len(errs) > 0 { + return fmt.Errorf("multiple submodules failed: %v", errs) + } return nil }submodules/move-nft/grpc_query.go (1)
232-232: Consider enhancing error logging for pagination failuresThe migration to
kvcollection.WithCollectionPaginationTriplePrefix2is correct. However, consider adding pagination-specific error details to the warning logs for better debugging.if err != nil { - sm.Logger(ctx).Warn("index mismatch found", "account", ownerSdkAddr, "collection", colSdkAddr, "action", "GetTokensByAccountAndCollection", "error", err) + sm.Logger(ctx).Warn("index mismatch found", + "account", ownerSdkAddr, + "collection", colSdkAddr, + "action", "GetTokensByAccountAndCollection", + "pagination_key", req.Pagination.Key, + "error", err) if cosmoserr.IsOf(err, collections.ErrNotFound) { pageRes.Total-- } continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
Makefile(0 hunks)collection/option.go(1 hunks)config/types.go(1 hunks)indexer.go(0 hunks)submodules/evm-nft/grpc_query.go(3 hunks)submodules/move-nft/collect.go(1 hunks)submodules/move-nft/grpc_query.go(3 hunks)submodules/move-nft/store.go(0 hunks)submodules/pair/collect.go(1 hunks)submodules/wasm-nft/grpc_query.go(3 hunks)submodules/wasm-nft/types/types.go(1 hunks)types.go(0 hunks)x/kvindexer/keeper/handler.go(1 hunks)x/kvindexer/module.go(0 hunks)
💤 Files with no reviewable changes (5)
- types.go
- x/kvindexer/module.go
- indexer.go
- submodules/move-nft/store.go
- Makefile
🧰 Additional context used
🪛 golangci-lint (1.62.2)
submodules/pair/collect.go
169-169: func PairSubmodule.generateCw721FromIcs721PortInfo is unused
(unused)
🔇 Additional comments (13)
submodules/move-nft/collect.go (1)
98-98: LGTM! Improved error handling transparency
The change to return the raw error instead of wrapping it with a generic message is a good improvement. This preserves the original error context and makes debugging easier by maintaining the complete error chain.
submodules/wasm-nft/types/types.go (2)
Line range hint 1-54: Verify impact of removed ObjectAddr field
The AI summary mentions removal of ObjectAddr field from CollectionResource, but this change isn't visible in the provided diff. This could impact existing code that might be using this field.
✅ Verification successful
Let me analyze the search results. I can see that:
-
The
ObjectAddrfield is commented out in multiple type files:submodules/move-nft/types/types.gosubmodules/evm-nft/types/types.gosubmodules/wasm-nft/types/types.go(the file under review)
-
However,
ObjectAddris still actively used in various places:- In VM utility files (
vm_util.go) across different modules - In collection handling code (
collect.go) - In store operations (
store.go) - In protobuf definitions (
nft/types/types.pb.go)
- In VM utility files (
Let me verify if these usages are properly handled with a more specific search:
The removal of ObjectAddr field from CollectionResource is intentional and safe
The commented out ObjectAddr field in the types files (//ObjectAddr string json:"object_addr,omitempty") represents a deliberate removal, while the actual object addressing functionality has been properly moved to dedicated index types (IndexedCollection and IndexedToken) in the protobuf definitions. This is a clean separation of concerns where:
CollectionResourcerepresents the pure resource dataIndexedCollection/IndexedTokenhandle the addressing and indexing aspects
The existing usages in vm_util.go and collect.go files are working with these index types, not with the removed field from CollectionResource.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to ObjectAddr
rg -i "ObjectAddr"
# Check if there are any migrations or database schema changes related to this removal
fd -e sql -e go -e json . | xargs rg -i "object.?addr"
Length of output: 6369
Script:
#!/bin/bash
# Check the actual struct definitions where ObjectAddr is used
ast-grep --pattern 'type $_ struct {
$$$
ObjectAddr $$$
$$$
}'
# Check the git history for recent changes related to ObjectAddr
git log --oneline -p --since="1 month ago" | grep -B5 -A5 "ObjectAddr"
Length of output: 1207
52-52: Verify if this line actually changed
The AI summary indicates that the Expiration field remained unchanged, yet this line is marked as modified. Let's verify if this is a meaningful change or just a formatting difference.
collection/option.go (2)
10-15: Implementation looks good!
The function is well-structured and correctly uses generics with the collections.Triple type. The closure pattern for modifying options is a clean approach.
17-23: Implementation looks good!
The function correctly extends the single prefix functionality to handle two prefixes, maintaining consistency with the first function's pattern.
config/types.go (1)
9-16: Documentation improvements look good!
The added comments clearly explain each field's purpose and constraints.
x/kvindexer/keeper/handler.go (1)
Line range hint 99-99: Verify error handling for store.Write()
The store.Write() call should be protected against potential errors.
Let's check if the store.Write() method can return errors:
submodules/evm-nft/grpc_query.go (1)
15-15: LGTM! Good refactoring to use centralized collection pagination
The changes correctly migrate pagination functionality to the centralized kvcollection package, which promotes code reuse and maintainability.
Also applies to: 182-182, 225-225
submodules/wasm-nft/grpc_query.go (1)
15-15: LGTM! Consistent refactoring of pagination logic
The changes maintain consistency with the evm-nft module by using the same centralized kvcollection package for pagination functionality.
Also applies to: 186-186, 231-231
submodules/move-nft/grpc_query.go (2)
15-15: LGTM: Clean import addition for pagination refactoring
The addition of the kvcollection import aligns with the PR's objective to centralize pagination functionality.
187-187: Verify consistent pagination behavior after refactoring
The migration to kvcollection.WithCollectionPaginationTriplePrefix maintains the same functionality while centralizing pagination logic.
✅ Verification successful
The search results show that the pagination function has been consistently refactored across all NFT submodules (wasm-nft, evm-nft, and move-nft). Let me verify the implementation details to ensure complete consistency.
Pagination refactoring verified as consistent and correct
The migration to kvcollection.WithCollectionPaginationTriplePrefix has been consistently implemented across all NFT submodules (wasm-nft, evm-nft, and move-nft) with:
- Identical type parameters
[sdk.AccAddress, sdk.AccAddress, string] - Same usage context within
CollectionPaginatecalls - Consistent error handling
- Uniform callback function implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new pagination functions across the codebase
# Check for any remaining old pagination function usage
rg "WithCollectionPaginationTriplePrefix[^2]" --type go
# Verify the new function is used consistently
rg "kvcollection\.WithCollectionPaginationTriplePrefix[^2]" --type go
Length of output: 1409
Script:
#!/bin/bash
# Check the implementation and usage context in all files
rg -B 5 -A 5 "WithCollectionPaginationTriplePrefix\[.*?\]" --type go
Length of output: 3249
submodules/pair/collect.go (2)
Line range hint 1-167: Well-structured error handling and logging
The code demonstrates good practices with:
- Consistent error handling patterns
- Proper use of error wrapping
- Contextual logging
- Clear separation of concerns
🧰 Tools
🪛 golangci-lint (1.62.2)
169-169: func PairSubmodule.generateCw721FromIcs721PortInfo is unused
(unused)
169-171: Verify if this function is needed
The static analysis indicates that this function is unused. Since this PR is focused on cleanup and removing unused definitions, we should either:
- Remove this function if it's truly unused
- Document why we're keeping it if it's needed for future use
✅ Verification successful
Function is used in wasm-pair submodule
The function generateCw721FromIcs721PortInfo is actively used in submodules/wasm-pair/collect.go for constructing collection names. Since this is a shared functionality between pair and wasm-pair submodules, the function should be retained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of this function in the codebase
rg "generateCw721FromIcs721PortInfo" --type go
# Search for potential future usage patterns
rg "cw721.*ics721" --type go
Length of output: 471
🧰 Tools
🪛 golangci-lint (1.62.2)
169-169: func PairSubmodule.generateCw721FromIcs721PortInfo is unused
(unused)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
go.mod(1 hunks)
🔇 Additional comments (2)
go.mod (2)
Line range hint 155-183: Review replace directives for necessity and security
The replace directives include important security fixes:
github.com/dgrijalva/jwt-go→github.com/golang-jwt/jwt/v4(security update)github.com/gin-gonic/ginpinned to v1.9.1 (GHSA-h395-qcrw-5vmq fix)- Custom fork for cometbft
Let's verify if these replacements are still necessary:
Consider:
- Tracking the TODO items (issues #13134 and #10409) in your issue tracker
- Documenting the security implications of using the custom cometbft fork
✅ Verification successful
Replace directives remain necessary for security and functionality
Based on the verification results:
- The
gin-gonic/ginreplacement to v1.9.1 is still necessary as newer critical vulnerabilities have been discovered (latest from June 2024). - The codebase actively uses
cometbftin multiple modules (24 files), making the custom fork replacement essential for functionality. - The
goleveldbpackage is used in the store implementation, justifying its pinned version.
Recommendations:
- Keep all current replace directives as they serve important security and functionality purposes
- Consider upgrading
gin-gonic/ginbeyond v1.9.1 once compatibility is verified, as newer vulnerabilities have been reported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the security issues in original packages are still relevant
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/dgrijalva/jwt-go") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
# Check if TODO comments are addressed in cosmos-sdk
rg "13134|10409" $(fd -t f -e go -e md)
Length of output: 3536
Script:
#!/bin/bash
# Check for gin-gonic/gin vulnerability status
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/gin-gonic/gin") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
# Check if we're using the replaced packages
rg -l "github.com/(dgrijalva/jwt-go|gin-gonic/gin|syndtr/goleveldb)" $(fd -t f -e go)
# Check cometbft import usage
rg -l "github.com/cometbft/cometbft" $(fd -t f -e go)
# Check if we're using the cosmos keyring fork
rg -l "github.com/99designs/keyring" $(fd -t f -e go)
Length of output: 11117
Line range hint 8-24: Verify security implications of dependency updates
Several critical dependencies have been updated:
- cometbft v0.38.12
- cosmos-sdk v0.50.5
- grpc v1.67.1
- protobuf v1.5.4
Let's check for any known vulnerabilities in these versions:
The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar.
just a minor cleanup
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Refactor
kvcollectionpackage across multiple components.