-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(collections): add a time.Time key codec #19879
Conversation
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{}
) inFormatTimeBytes
andFormatTimeString
to 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
ParseTimeBytes
andParseTime
. 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
Decode
andDecodeNonTerminal
to 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
TimeKey
andTimeValue
to 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
StartTime
field, such as querying or updating, is compatible with the newcollections.TimeKey
codec. Consider adding or reviewing tests to confirm this compatibility.x/auth/go.mod (1)
- 177-177: The
replace
directive forcosmossdk.io/collections
is correctly added and follows best practices for Go module management.x/staking/go.mod (1)
- 177-177: The
replace
directive forcosmossdk.io/collections
is 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/collections
is correctly implemented, pointing to the localcollections
directory. Ensure that the relative path../../collections
accurately reflects the directory structure within the Cosmos SDK repository.x/evidence/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collections
is correctly added, pointing to the localcollections
directory. As with the previous file, ensure the relative path../../collections
is accurate within the Cosmos SDK repository structure.x/protocolpool/go.mod (1)
- 178-178: The addition of the replace directive for
cosmossdk.io/collections
is consistent with the changes in other modules, correctly pointing to the localcollections
directory. Please verify the relative path../../collections
for accuracy in the repository's structure.x/authz/go.mod (1)
- 179-179: The replace directive for
cosmossdk.io/collections
is appropriately added, pointing to the localcollections
directory. Ensure the relative path../../collections
accurately 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/collections
is correctly pointing to the localcollections
directory. This ensures that the local changes to thecollections
package are utilized across the SDK, facilitating the integration of the newtime.Time
key codec.x/nft/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collections
correctly points to the localcollections
directory, ensuring that the module uses the local changes. This is a necessary step for integrating the newtime.Time
key codec within the SDK.x/circuit/go.mod (1)
- 178-178: The inclusion of the replace directive for
cosmossdk.io/collections
is consistent with the changes in other modules, ensuring the use of the local version of thecollections
package. This facilitates the integration of the newtime.Time
key codec across the SDK.x/accounts/go.mod (1)
- 178-178: The replace directive for
cosmossdk.io/collections
correctly points to the localcollections
directory, ensuring that the module uses the local changes. This consistency across the SDK is crucial for the integration of the newtime.Time
key codec.x/gov/go.mod (1)
- 181-181: The addition of the replace directive for
cosmossdk.io/collections
is correctly pointing to the localcollections
directory. This is necessary for local development and ensures that changes to thecollections
package are immediately reflected across the SDK.x/slashing/go.mod (1)
- 180-180: The addition of the replace directive for
cosmossdk.io/collections
is correctly pointing to the localcollections
directory. This is necessary for local development and ensures that changes to thecollections
package are immediately reflected across the SDK.x/mint/go.mod (1)
- 179-179: The addition of the replace directive for
cosmossdk.io/collections
is correctly pointing to the localcollections
directory. This is necessary for local development and ensures that changes to thecollections
package are immediately reflected across the SDK.x/accounts/defaults/lockup/go.mod (1)
- 183-183: The addition of the replace directive for
cosmossdk.io/collections
is correctly pointing to the localcollections
directory, considering the nested location of this module. This is necessary for local development and ensures that changes to thecollections
package are immediately reflected across the SDK.x/feegrant/go.mod (1)
- 183-183: The addition of the replace directive for
cosmossdk.io/collections
is appropriate for local development and testing. Please ensure the relative path../../collections
accurately 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/collections
is appropriate for local development and testing. Please ensure the relative path../../collections
accurately 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/collections
is appropriate for local development and testing. Please ensure the relative path../../collections
accurately 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/collections
is appropriate for local development and testing. Please ensure the relative path../../collections
accurately reflects the intended directory structure within the Cosmos SDK repository.x/gov/keeper/keeper.go (1)
- 130-131: The update from
sdk.TimeKey
tocollections.TimeKey
forActiveProposalsQueue
andInactiveProposalsQueue
aligns 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/collections
to./collections
is appropriate for integrating the newtime.Time
key codec. Ensure that the local./collections
directory 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/collections
pointing to../../collections
is appropriate for ensuring the newtime.Time
key codec is used in the upgrade module. Verify that the local../../collections
directory 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.TimeKey
instead ofsdk.TimeKey
aligns 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.TimeKey
for theStartTime
field 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/collections
pointing to../collections
is correctly implemented. It ensures that the tests use the local version of thecollections
module, facilitating testing against the latest changes.tests/starship/tests/go.mod (1)
- 39-39: The addition of the replace directive for
cosmossdk.io/collections
is 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/collections
is 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.TimeKey
withcollections.TimeKey
in theUnbondingQueue
map initialization aligns with the PR's objectives to handle time-based keys more effectively within the Cosmos SDK.- 240-240: The replacement of
sdk.TimeKey
withcollections.TimeKey
in theRedelegationQueue
map initialization is consistent with the PR's goal of enhancing the handling of time-based keys within the SDK.- 258-258: Using
collections.TimeKey
for theValidatorQueue
map initialization is appropriate for managing time-based keys, supporting the PR's objectives.- 270-270: The use of
collections.TimeKey
in theValidatorConsensusKeyRotationRecordIndexKey
supports the PR's aim of improving time-based key handling within the SDK.- 277-277: The adoption of
collections.TimeKey
for theValidatorConsensusKeyRotationRecordQueue
map 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.TimeKey
for theEndTime
declaration in thenewBaseLockup
function aligns with the PR's objectives to improve handling oftime.Time
keys 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/proto
andgoogle.golang.org/protobuf/runtime/protoiface
is 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
ProtoMsg
andgogoProtoPlusV2
are well-defined and crucial for the functionality of the proto v2 to gogo protobuf wrapper. The use of bothproto.Message
andProtoMsg
in thegogoProtoPlusV2
interface 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
protoV2GogoWrapper
and its methods is well-structured and serves the intended purpose of wrapping proto v2 messages into a gogo-compatible format. The methodsmakeMsgSend
,makeMsgDelegate
, andmakeMsgUndelegate
effectively demonstrate how to create specific message types using this wrapper. However, it's important to ensure that the conversion betweensdk.Coins
andv1beta1.Coin
(or its singular form formakeMsgDelegate
andmakeMsgUndelegate
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto imho it should stay
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want more of a redesign on how we save time keys in state
b42f646
to
049a355
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's time, the time parameter is not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
"time" | ||
) | ||
|
||
var timeSize = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's collections/colltest package u can use to test the keys if they work properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
Reviewers 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