Change time.now to ctx.BlockTime and remove genesis time validations#190
Conversation
WalkthroughThe diff refactors context and time handling in the x/rewards module: msg_server now unwraps the Cosmos SDK context via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/rewards/keeper/msg_server.go (1)
80-90: Correct usage of SDK context throughout the method.The SDK context is properly passed to all downstream calls. The changes align correctly with the updated validation function signatures.
Consider updating fundsAvailable signature for consistency
While
fundsAvailable(line 49 in validation.go) currently doesn't requiresdk.Contextfor its operations, updating its signature to matchvalidateSchedulewould improve consistency and future-proof the codebase:-func (k Keeper) fundsAvailable(ctx context.Context, amount sdk.Coin) error { +func (k Keeper) fundsAvailable(ctx sdk.Context, amount sdk.Coin) error {This is purely optional—the current implementation works correctly since
sdk.Contextimplementscontext.Context.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/rewards/keeper/msg_server.gox/rewards/keeper/validation.gox/rewards/types/release_schedule.gox/rewards/types/release_schedule_test.go
💤 Files with no reviewable changes (2)
- x/rewards/types/release_schedule.go
- x/rewards/types/release_schedule_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
Applied to files:
x/rewards/keeper/msg_server.go
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
Applied to files:
x/rewards/keeper/msg_server.go
🧬 Code graph analysis (1)
x/rewards/keeper/msg_server.go (2)
x/rewards/keeper/keeper.go (1)
Keeper(18-32)x/rewards/types/types.pb.go (3)
ReleaseSchedule(34-45)ReleaseSchedule(49-49)ReleaseSchedule(50-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: tests
- GitHub Check: golangci-lint
- GitHub Check: test-e2e
- GitHub Check: Analyze
- GitHub Check: liveness-test
🔇 Additional comments (3)
x/rewards/keeper/msg_server.go (1)
70-71: LGTM! Context unwrapping is correctly implemented.The extraction of the SDK context is necessary for accessing
BlockTime()in the validation functions. This follows the standard Cosmos SDK pattern for message servers that need SDK-specific features.x/rewards/keeper/validation.go (2)
40-46: Excellent fix: Using BlockTime for deterministic validation.Replacing
time.Now()withctx.BlockTime()is critical for blockchain consensus. Using system time would cause validators to get different results, leading to consensus failures. Block time ensures all validators agree on the same time reference.
66-127: Correctly updated for context-aware time validation.The signature change to
sdk.Contextand the use ofctx.BlockTime()throughout the function ensure deterministic validation. All call sites are properly updated, and the validation logic remains sound.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #189 by replacing the use of time.Now() with blockchain-aware ctx.BlockTime() for time validations, and removes inappropriate time validations from genesis state validation.
Key changes:
- Replaced
time.Now()withctx.BlockTime()for deterministic, blockchain-based time checks - Removed genesis-time validations that compared against current time (which is inappropriate for genesis state)
- Updated function signatures to use
sdk.Contextwhere blockchain time access is needed
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| x/rewards/types/release_schedule.go | Removed genesis validations for EndTime and LastReleaseTime that incorrectly compared against current time |
| x/rewards/types/release_schedule_test.go | Removed test cases for the deleted genesis time validations |
| x/rewards/keeper/validation.go | Updated validateEndTime to accept sdk.Context and use ctx.BlockTime(); updated validateSchedule signature and moved currentTime declaration closer to usage |
| x/rewards/keeper/msg_server.go | Added SDK context unwrapping in ChangeSchedule to properly pass blockchain context to validation functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
This fixes issue #189
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Related tests updated