feat(collections): add a time.Time key codec #19879
Conversation
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
WalkthroughWalkthroughThe primary change involves introducing a new Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (37)
- client/v2/go.mod (1 hunks)
- collections/codec/time.go (1 hunks)
- collections/codec/time_test.go (1 hunks)
- collections/codec/utils.go (1 hunks)
- collections/collections.go (3 hunks)
- go.mod (1 hunks)
- go.sum (1 hunks)
- simapp/go.mod (1 hunks)
- tests/go.mod (1 hunks)
- tests/starship/tests/go.mod (1 hunks)
- types/collections.go (1 hunks)
- types/collections_test.go (2 hunks)
- x/accounts/defaults/lockup/continuous_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/go.mod (1 hunks)
- x/accounts/defaults/lockup/lockup.go (1 hunks)
- x/accounts/defaults/lockup/periodic_locking_account.go (1 hunks)
- x/accounts/go.mod (1 hunks)
- x/auth/go.mod (1 hunks)
- x/authz/go.mod (1 hunks)
- x/bank/go.mod (1 hunks)
- x/circuit/go.mod (1 hunks)
- x/distribution/go.mod (1 hunks)
- x/evidence/go.mod (1 hunks)
- x/feegrant/go.mod (1 hunks)
- x/feegrant/keeper/keeper.go (1 hunks)
- x/gov/go.mod (1 hunks)
- x/gov/keeper/keeper.go (1 hunks)
- x/group/go.mod (1 hunks)
- x/mint/go.mod (1 hunks)
- x/nft/go.mod (1 hunks)
- x/params/go.mod (1 hunks)
- x/protocolpool/go.mod (1 hunks)
- x/slashing/go.mod (1 hunks)
- x/staking/go.mod (1 hunks)
- x/staking/keeper/keeper.go (4 hunks)
- x/staking/keeper/validator.go (1 hunks)
- x/upgrade/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- go.sum
Additional comments: 40
collections/codec/time_test.go (1)
- 9-21: Consider adding tests for edge cases, such as the zero time value (
time.Time{}), to ensure the codec handles these scenarios correctly.types/collections_test.go (1)
- 32-32: Consider adding more detailed tests for
collections.TimeKey, such as encoding/decoding specific non-zero time values, to ensure the codec accurately handles various time scenarios.collections/codec/utils.go (2)
- 12-18: Consider documenting the behavior or explicitly handling the zero time value (
time.Time{}) inFormatTimeBytesandFormatTimeStringto ensure clarity on how these cases are treated.- 21-48: Clarify the behavior or add handling for edge cases, such as parsing from an empty string or byte array, in
ParseTimeBytesandParseTime. This ensures clarity on how these cases are treated, especially regarding the zero time value.collections/codec/time.go (2)
- 18-47: Consider refining the error messages in
DecodeandDecodeNonTerminalto include more details about expected vs. actual sizes. This can help developers quickly identify and resolve issues related to buffer size mismatches.- 31-36: The JSON encoding and decoding methods are correctly implemented and follow best practices.
collections/collections.go (1)
- 52-59: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [55-79]
Consider adding documentation or comments for
TimeKeyandTimeValueto explain their intended use cases or scenarios. This can enhance understanding and maintainability for future developers.x/accounts/defaults/lockup/continuous_locking_account.go (1)
- 28-28: Ensure that all functionality related to the
StartTimefield, such as querying or updating, is compatible with the newcollections.TimeKeycodec. Consider adding or reviewing tests to confirm this compatibility.x/auth/go.mod (1)
- 177-177: The
replacedirective forcosmossdk.io/collectionsis correctly added and follows best practices for Go module management.x/staking/go.mod (1)
- 177-177: The
replacedirective forcosmossdk.io/collectionsis correctly added and follows best practices for Go module management.x/bank/go.mod (1)
- 177-177: The addition of the replace directive for
cosmossdk.io/collectionsis correctly implemented, pointing to the localcollectionsdirectory. Ensure that the relative path../../collectionsaccurately reflects the directory structure within the Cosmos SDK repository.x/evidence/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collectionsis correctly added, pointing to the localcollectionsdirectory. As with the previous file, ensure the relative path../../collectionsis accurate within the Cosmos SDK repository structure.x/protocolpool/go.mod (1)
- 178-178: The addition of the replace directive for
cosmossdk.io/collectionsis consistent with the changes in other modules, correctly pointing to the localcollectionsdirectory. Please verify the relative path../../collectionsfor accuracy in the repository's structure.x/authz/go.mod (1)
- 179-179: The replace directive for
cosmossdk.io/collectionsis appropriately added, pointing to the localcollectionsdirectory. Ensure the relative path../../collectionsaccurately reflects the directory structure within the Cosmos SDK repository.x/distribution/go.mod (1)
- 179-179: The addition of the replace directive for
cosmossdk.io/collectionsis correctly pointing to the localcollectionsdirectory. This ensures that the local changes to thecollectionspackage are utilized across the SDK, facilitating the integration of the newtime.Timekey codec.x/nft/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collectionscorrectly points to the localcollectionsdirectory, ensuring that the module uses the local changes. This is a necessary step for integrating the newtime.Timekey codec within the SDK.x/circuit/go.mod (1)
- 178-178: The inclusion of the replace directive for
cosmossdk.io/collectionsis consistent with the changes in other modules, ensuring the use of the local version of thecollectionspackage. This facilitates the integration of the newtime.Timekey codec across the SDK.x/accounts/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collectionscorrectly points to the localcollectionsdirectory, ensuring that the module uses the local changes. This consistency across the SDK is crucial for the integration of the newtime.Timekey codec.x/gov/go.mod (1)
- 181-181: The addition of the replace directive for
cosmossdk.io/collectionsis correctly pointing to the localcollectionsdirectory. This is necessary for local development and ensures that changes to thecollectionspackage are immediately reflected across the SDK.x/slashing/go.mod (1)
- 180-180: The addition of the replace directive for
cosmossdk.io/collectionsis correctly pointing to the localcollectionsdirectory. This is necessary for local development and ensures that changes to thecollectionspackage are immediately reflected across the SDK.x/mint/go.mod (1)
- 179-179: The addition of the replace directive for
cosmossdk.io/collectionsis correctly pointing to the localcollectionsdirectory. This is necessary for local development and ensures that changes to thecollectionspackage are immediately reflected across the SDK.x/accounts/defaults/lockup/go.mod (1)
- 183-183: The addition of the replace directive for
cosmossdk.io/collectionsis correctly pointing to the localcollectionsdirectory, considering the nested location of this module. This is necessary for local development and ensures that changes to thecollectionspackage are immediately reflected across the SDK.x/feegrant/go.mod (1)
- 183-183: The addition of the replace directive for
cosmossdk.io/collectionsis appropriate for local development and testing. Please ensure the relative path../../collectionsaccurately reflects the intended directory structure within the Cosmos SDK repository.x/params/go.mod (1)
- 184-184: The addition of the replace directive for
cosmossdk.io/collectionsis appropriate for local development and testing. Please ensure the relative path../../collectionsaccurately reflects the intended directory structure within the Cosmos SDK repository.x/group/go.mod (1)
- 190-190: The addition of the replace directive for
cosmossdk.io/collectionsis appropriate for local development and testing. Please ensure the relative path../../collectionsaccurately reflects the intended directory structure within the Cosmos SDK repository.client/v2/go.mod (1)
- 189-189: The addition of the replace directive for
cosmossdk.io/collectionsis appropriate for local development and testing. Please ensure the relative path../../collectionsaccurately reflects the intended directory structure within the Cosmos SDK repository.x/gov/keeper/keeper.go (1)
- 130-131: The update from
sdk.TimeKeytocollections.TimeKeyforActiveProposalsQueueandInactiveProposalsQueuealigns with the PR's objective to replace the deprecated key codec. Ensure that the integration of the new codec has been thoroughly tested, especially in terms of queuing mechanisms for proposals.go.mod (1)
- 184-184: The addition of a module mapping for
cosmossdk.io/collectionsto./collectionsis appropriate for integrating the newtime.Timekey codec. Ensure that the local./collectionsdirectory is correctly set up and that there are no module resolution issues.x/upgrade/go.mod (1)
- 214-214: The addition of a replace directive for
cosmossdk.io/collectionspointing to../../collectionsis appropriate for ensuring the newtime.Timekey codec is used in the upgrade module. Verify that the local../../collectionsdirectory is correctly set up and that there are no module resolution issues.x/feegrant/keeper/keeper.go (1)
- 55-55: The update to use
collections.TimeKeyinstead ofsdk.TimeKeyaligns with the PR's objectives and enhances the SDK's capability to handle time-based keys efficiently. This change is correctly implemented.x/accounts/defaults/lockup/periodic_locking_account.go (1)
- 30-30: The update to use
collections.TimeKeyfor theStartTimefield initialization is correctly implemented and aligns with the PR's objectives to enhance the SDK's capability to handle time-based keys efficiently.tests/go.mod (1)
- 256-256: The addition of a replace directive for
cosmossdk.io/collectionspointing to../collectionsis correctly implemented. It ensures that the tests use the local version of thecollectionsmodule, facilitating testing against the latest changes.tests/starship/tests/go.mod (1)
- 39-39: The addition of the replace directive for
cosmossdk.io/collectionsis noted. Please verify that this change is intended for local development or CI testing purposes only and ensure it is not merged into the main branch, as it could affect module users by pointing to a local path that is not accessible outside your development environment.simapp/go.mod (1)
- 260-260: The addition of the replace directive for
cosmossdk.io/collectionsis noted. Ensure this is intended for local development or testing purposes. Replace directives should be used with caution in shared or production branches.x/staking/keeper/keeper.go (5)
- 215-215: The replacement of
sdk.TimeKeywithcollections.TimeKeyin theUnbondingQueuemap initialization aligns with the PR's objectives to handle time-based keys more effectively within the Cosmos SDK.- 240-240: The replacement of
sdk.TimeKeywithcollections.TimeKeyin theRedelegationQueuemap initialization is consistent with the PR's goal of enhancing the handling of time-based keys within the SDK.- 258-258: Using
collections.TimeKeyfor theValidatorQueuemap initialization is appropriate for managing time-based keys, supporting the PR's objectives.- 270-270: The use of
collections.TimeKeyin theValidatorConsensusKeyRotationRecordIndexKeysupports the PR's aim of improving time-based key handling within the SDK.- 277-277: The adoption of
collections.TimeKeyfor theValidatorConsensusKeyRotationRecordQueuemap initialization is in line with the PR's objectives to enhance time-based key management.x/accounts/defaults/lockup/lockup.go (1)
- 55-55: The update to use
collections.TimeKeyfor theEndTimedeclaration in thenewBaseLockupfunction aligns with the PR's objectives to improve handling oftime.Timekeys within the SDK.
x/staking/keeper/validator.go
Outdated
| @@ -417,7 +417,7 @@ func (k Keeper) GetLastValidators(ctx context.Context) (validators []types.Valid | |||
| // GetUnbondingValidators returns a slice of mature validator addresses that | |||
| // complete their unbonding at a given time and height. | |||
| func (k Keeper) GetUnbondingValidators(ctx context.Context, endTime time.Time, endHeight int64) ([]string, error) { | |||
| timeSize := sdk.TimeKey.Size(endTime) | |||
| timeSize := collections.TimeKey.Size(endTime) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The search for tests specifically related to the Size method of collections.TimeKey did not yield any directly relevant results. The tests found in the broader search appear to be unrelated to the Size method, suggesting a potential lack of direct test coverage for this functionality. This could be a concern for ensuring the correctness and reliability of the Size method, especially given its importance in accurately calculating the serialized size of time.Time objects. It may be beneficial to consider adding targeted tests for this method to verify its behavior under various conditions.
Analysis chain
The usage of collections.TimeKey.Size(endTime) to calculate timeSize is a direct adaptation to the new time.Time key codec. However, it's important to ensure that the Size method accurately reflects the serialized size of the time.Time object for consistent key sizing in the database. If not already covered, adding tests to verify the correctness of the Size method in various scenarios (e.g., different times) would be beneficial.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and coverage of tests for the Size method of collections.TimeKey
fd "time_test.go" --exec grep -E "Test.*Size" {}
Length of output: 50
Script:
#!/bin/bash
# Broaden the search for tests related to the Size method of collections.TimeKey, without assuming specific file names or exact test names
fd --type f --extension go --exec grep -E "func Test.*Size" {}
Length of output: 240
chixiaowen
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- collections/codec/time_test.go (1 hunks)
- types/collections.go (3 hunks)
- x/accounts/defaults/lockup/lockup.go (3 hunks)
- x/accounts/defaults/lockup/protov2_wrapper.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- collections/codec/time_test.go
- types/collections.go
- x/accounts/defaults/lockup/lockup.go
Additional comments: 3
x/accounts/defaults/lockup/protov2_wrapper.go (3)
- 4-5: The addition of imports for
google.golang.org/protobuf/protoandgoogle.golang.org/protobuf/runtime/protoifaceis appropriate given the context of introducing a wrapper for proto v2 messages. These libraries are essential for working with proto v2 messages and their interfaces in Go.- 1-14: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-17]
The type definitions for
ProtoMsgandgogoProtoPlusV2are well-defined and crucial for the functionality of the proto v2 to gogo protobuf wrapper. The use of bothproto.MessageandProtoMsgin thegogoProtoPlusV2interface ensures compatibility with both protobuf versions.
- 1-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [19-55]
The implementation of
protoV2GogoWrapperand its methods is well-structured and serves the intended purpose of wrapping proto v2 messages into a gogo-compatible format. The methodsmakeMsgSend,makeMsgDelegate, andmakeMsgUndelegateeffectively demonstrate how to create specific message types using this wrapper. However, it's important to ensure that the conversion betweensdk.Coinsandv1beta1.Coin(or its singular form formakeMsgDelegateandmakeMsgUndelegate) correctly handles any potential discrepancies in the representation of amounts or denominations between these types.
types/collections.go
Outdated
| // Deprecated: exists only for state compatibility reasons, should not | ||
| // be used for new storage keys using time. Please use the time KeyCodec | ||
| // provided in the collections package. | ||
| TimeKey collcodec.KeyCodec[time.Time] = timeKeyCodec{} |
There was a problem hiding this comment.
Imho, we shouldn't remove this, we don't need to introduce such breaking change given that it is already deprecated.
types/collections.go
Outdated
| @@ -245,49 +238,6 @@ func (i uintValueCodec) ValueType() string { | |||
| return Uint | |||
| } | |||
|
|
|||
| type timeKeyCodec struct{} | |||
x/accounts/go.mod
Outdated
| @@ -175,4 +175,5 @@ replace ( | |||
| cosmossdk.io/x/mint => ../mint | |||
| cosmossdk.io/x/slashing => ../slashing | |||
| cosmossdk.io/x/staking => ../staking | |||
| cosmossdk.io/collections => ../../collections | |||
There was a problem hiding this comment.
Lets avoid to add replace everywhere, can you revert them all? We can see in a follow up if the implemented time key in collection is and should be state compatible with the one we had in the sdk.
chixiaowen
force-pushed
the
cxw/time-codec
branch
from
b42f646
to
049a355
Compare
github-actions
bot
removed
C:x/slashing
C:x/distribution
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- collections/codec/time.go (1 hunks)
- collections/codec/time_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- collections/codec/time.go
- collections/codec/time_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- collections/codec/time.go (1 hunks)
- collections/codec/time_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- collections/codec/time.go
- collections/codec/time_test.go
collections/codec/time.go
Outdated
|
|
||
| type timeKey[T time.Time] struct{} | ||
|
|
||
| func NewTimeKey[T time.Time]() KeyCodec[T] { return timeKey[T]{} } |
There was a problem hiding this comment.
if it's time, the time parameter is not needed here
| "time" | ||
| ) | ||
|
|
||
| var timeSize = 8 |
There was a problem hiding this comment.
idk if 8 bytes gives you the precision you're looking for, assuming the time is represented as Join(seconds,milliseconds)
There was a problem hiding this comment.
Using a 64-bit integer to directly represent a Unix timestamp with millisecond precision usually provides sufficient precision and range, eliminating the need to separately represent seconds and milliseconds.
collections/codec/time_test.go
Outdated
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestTimeKey(t *testing.T) { |
There was a problem hiding this comment.
there's collections/colltest package u can use to test the keys if they work properly
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (3)
- collections/codec/correctness_test.go (2 hunks)
- collections/codec/time.go (1 hunks)
- collections/collections.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- collections/codec/time.go
- collections/collections.go
Additional Context Used
Path-based Instructions (1)
collections/codec/correctness_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
| t.Run("time.Time", func(t *testing.T) { | ||
| colltest.TestKeyCodec(t, collections.TimeKey, time.UnixMilli(time.Now().UnixMilli()).UTC()) | ||
| }) |
There was a problem hiding this comment.
Using time.Now() in tests can introduce non-determinism, which might lead to flaky tests. Consider using a fixed time.Time value to ensure the test's predictability and reliability. For example:
- colltest.TestKeyCodec(t, collections.TimeKey, time.UnixMilli(time.Now().UnixMilli()).UTC())
+ colltest.TestKeyCodec(t, collections.TimeKey, time.Date(2024, 3, 29, 0, 0, 0, 0, time.UTC))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| t.Run("time.Time", func(t *testing.T) { | |
| colltest.TestKeyCodec(t, collections.TimeKey, time.UnixMilli(time.Now().UnixMilli()).UTC()) | |
| }) | |
| t.Run("time.Time", func(t *testing.T) { | |
| colltest.TestKeyCodec(t, collections.TimeKey, time.Date(2024, 3, 29, 0, 0, 0, 0, time.UTC)) | |
| }) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Closes: #19501
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.
I have...
Summary by CodeRabbit