feat(collections): add Address and Integer codecs #22517
Conversation
cool-develope
requested review from
aaronc,
testinginprod,
a team and
facundomedica
as code owners
|
Warning Rate limit exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 15 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces enhancements to key and codec handling across multiple files in the Cosmos SDK. The changes focus on improving schema type conversions, error handling, and codec functionality. In 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: 1
🧹 Outside diff range and nitpick comments (6)
collections/indexing.go (1)
153-155: Consider refactoring decoder handling to reduce duplicationThe key and value decoder logic follows similar patterns. Consider extracting the common decoder handling logic into a helper function to improve maintainability and reduce code duplication.
Example refactor:
func decodeWithSchemaType(decoder codec.SchemaCodec, data []byte, decode func([]byte) (any, error)) (any, error) { x, err := decode(data) if err != nil { return nil, err } if decoder.ToSchemaType == nil { return x, nil } return decoder.ToSchemaType(x) }This helper could then be used for both key and value decoding:
res.keyDecoder = func(i []byte) (any, error) { return decodeWithSchemaType(keyDecoder, i, func(data []byte) (any, error) { _, x, err := c.m.kc.Decode(data) return x, err }) } res.valueDecoder = func(i []byte) (any, error) { return decodeWithSchemaType(valueDecoder, i, c.m.vc.Decode) }types/collections.go (2)
124-139: LGTM with a minor suggestion for error messagesThe implementation correctly handles schema encoding/decoding for address types with proper error handling and type assertions.
Consider enhancing the error message to include the actual value:
- return t, fmt.Errorf("expected string, got %T", s) + return t, fmt.Errorf("expected string, got %T: %v", s, s)
235-253: LGTM with suggestions for error handling and performanceThe implementation correctly handles schema encoding/decoding for math.Int with proper validation.
Consider these improvements:
- Enhance error messages to include the actual value:
- return math.Int{}, fmt.Errorf("expected string, got %T", s) + return math.Int{}, fmt.Errorf("expected string, got %T: %v", s, s)
- Optimize the error path by combining the parsing check and conversion:
- t, ok := math.NewIntFromString(sz) - if !ok { - return math.Int{}, fmt.Errorf("failed to parse Int from string: %s", sz) - } - return t, nil + if t, ok := math.NewIntFromString(sz); ok { + return t, nil + } + return math.Int{}, fmt.Errorf("failed to parse Int from string: %s", sz)collections/pair.go (3)
248-257: Refactor duplicated code for obtaining schema codecsThe code blocks for obtaining
codec1andcodec2fromcodec.KeySchemaCodecand handling errors are nearly identical. Refactoring this duplication into a helper function can improve maintainability and reduce redundancy.Here's how you might refactor:
func (p pairKeyCodec[K1, K2]) SchemaCodec() (codec.SchemaCodec[Pair[K1, K2]], error) { field1, err := getNamedKeyField(p.keyCodec1, p.key1Name) if err != nil { return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key1 field: %w", err) } field2, err := getNamedKeyField(p.keyCodec2, p.key2Name) if err != nil { return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key2 field: %w", err) } - codec1, err := codec.KeySchemaCodec(p.keyCodec1) - if err != nil { - return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key1 schema codec: %w", err) - } - - codec2, err := codec.KeySchemaCodec(p.keyCodec2) - if err != nil { - return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key2 schema codec: %w", err) - } + codec1, err := getKeySchemaCodec(p.keyCodec1, "key1") + if err != nil { + return codec.SchemaCodec[Pair[K1, K2]]{}, err + } + + codec2, err := getKeySchemaCodec(p.keyCodec2, "key2") + if err != nil { + return codec.SchemaCodec[Pair[K1, K2]]{}, err + }And define the helper function
getKeySchemaCodecto encapsulate the error handling.
261-269: Refactor duplicated code when converting to schema typesThe conversion of
k1andk2usingtoKeySchemaTypeis duplicated. Refactoring this into a loop or helper function can reduce redundancy and enhance code readability.Here's an example of how you might refactor:
- k1, err := toKeySchemaType(codec1, pair.K1()) - if err != nil { - return nil, err - } - k2, err := toKeySchemaType(codec2, pair.K2()) - if err != nil { - return nil, err - } - return []interface{}{k1, k2}, nil + keys := make([]interface{}, 2) + codecs := []codec.SchemaCodec{codec1, codec2} + pairValues := []any{pair.K1(), pair.K2()} + for i, cdc := range codecs { + k, err := toKeySchemaType(cdc, pairValues[i]) + if err != nil { + return nil, err + } + keys[i] = k + } + return keys, nil
276-284: Refactor duplicated code when converting from schema typesSimilarly, the code for converting
k1andk2from schema types is duplicated. Consider refactoring to reduce repetition and improve maintainability.Here's how you might adjust the code:
- k1, err := fromKeySchemaType(codec1, aSlice[0]) - if err != nil { - return Pair[K1, K2]{}, err - } - k2, err := fromKeySchemaType(codec2, aSlice[1]) - if err != nil { - return Pair[K1, K2]{}, err - } - return Join(k1, k2), nil + keys := make([]interface{}, 2) + codecs := []codec.SchemaCodec{codec1, codec2} + for i, cdc := range codecs { + k, err := fromKeySchemaType(cdc, aSlice[i]) + if err != nil { + return Pair[K1, K2]{}, err + } + keys[i] = k + } + return Join(keys[0].(K1), keys[1].(K2)), nil
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
collections/indexing.go(1 hunks)collections/pair.go(2 hunks)indexer/postgres/params.go(1 hunks)schema/kind.go(0 hunks)types/collections.go(3 hunks)
💤 Files with no reviewable changes (1)
- schema/kind.go
🧰 Additional context used
📓 Path-based instructions (4)
collections/indexing.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/pair.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/postgres/params.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/collections.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
indexer/postgres/params.go (1)
111-115: LGTM! The changes look good but require verification.
The modification to handle address values as strings instead of byte slices is well-implemented with proper error handling and clear error messages.
Let's verify that all callers have been updated to pass string values:
✅ Verification successful
Verified: All callers have been updated to pass string values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential callers of bindParam that might be affected by the AddressKind change
# Look for any code that might still be passing []byte for AddressKind fields
# Search for struct field declarations of AddressKind type
rg -l "schema\.AddressKind" | xargs -I {} rg -A 5 "type.*struct" {}
# Search for direct usage of bindParam with byte slices
ast-grep --pattern 'bindParam($_, []byte{$$$})'
Length of output: 1406
collections/indexing.go (1)
153-155: LGTM: Consistent nil check for schema type conversion
The added nil check for valueDecoder.ToSchemaType is a good defensive programming practice that:
- Maintains consistency with the key decoder's behavior
- Prevents potential nil pointer dereferences
- Provides a clean fallback to return the original decoded value
|
@cool-develope your pull request is missing a changelog! |
indexer/postgres/params.go
Outdated
| param, err = tm.options.addressCodec.BytesToString(value.([]byte)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("address encoding failed for field %q: %w", field.Name, err) | ||
| t, ok := value.(string) |
There was a problem hiding this comment.
Where are passed the address codec then now? cc @facundomedica
There was a problem hiding this comment.
We now have a difference between how are read the values (readCol uses the address codec) and how they are rendered (expecting the field being a string). I think we should use the address codec for both. They are in the indexing options and now properly wired: 0cda5c3
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
julienrbrt
added
the
backport/v0.52.x
github-actions
bot
added
C:server/v2
julienrbrt
assigned julienrbrt and unassigned aaronc, kocubinski, facundomedica, sontrinh16 and testinginprod
julienrbrt
force-pushed
the
schema/codec_missing
branch
from
77c2d50 to
9506c52
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
types/collections.go (1)
236-254: LGTM with a minor improvement suggestion.The implementation is thorough and handles all edge cases correctly. Consider extracting error messages into constants for better maintainability.
+const ( + errExpectedString = "expected string, got %T" + errFailedIntParse = "failed to parse Int from string: %s" +) func (i intValueCodec) SchemaCodec() (collcodec.SchemaCodec[math.Int], error) { return collcodec.SchemaCodec[math.Int]{ Fields: []schema.Field{{Kind: schema.IntegerKind}}, ToSchemaType: func(t math.Int) (any, error) { return t.String(), nil }, FromSchemaType: func(s any) (math.Int, error) { sz, ok := s.(string) if !ok { - return math.Int{}, fmt.Errorf("expected string, got %T", s) + return math.Int{}, fmt.Errorf(errExpectedString, s) } t, ok := math.NewIntFromString(sz) if !ok { - return math.Int{}, fmt.Errorf("failed to parse Int from string: %s", sz) + return math.Int{}, fmt.Errorf(errFailedIntParse, sz) } return t, nil }, }, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
types/collections.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
types/collections.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
🔇 Additional comments (1)
types/collections.go (1)
12-12: LGTM!The schema import is correctly added and necessary for the new SchemaCodec implementations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Julien Robert <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 261f0b9) # Conflicts: # collections/pair.go
…23202) Co-authored-by: cool-developer <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #XXXX
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
Deprecation Notices
Improvements