fix(oracle): timestamps for exchange rates #2117
Conversation
WalkthroughThe changes in this pull request involve multiple updates across various files related to the Nibiru EVM. Key modifications include the introduction of new features, such as enabling WASM light clients and enhancements to gas management. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 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. 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: 1
🧹 Outside diff range and nitpick comments (7)
x/evm/precompile/oracle.go (1)
97-101: LGTM! Consider adding documentation.The return value packing correctly includes all necessary block information with proper timestamp precision.
Consider adding a comment to document the return values:
+// Pack the exchange rate with block information: +// - exchange rate as big.Int +// - block timestamp in milliseconds +// - block height return method.Outputs.Pack( priceAtBlock.ExchangeRate.BigInt(), uint64(priceAtBlock.BlockTimestampMs), priceAtBlock.CreatedBlock, )x/evm/precompile/oracle_test.go (1)
Line range hint
61-75: Consider parameterizing the runQuery helper functionWhile the helper function effectively reduces code duplication, consider making it more flexible by accepting parameters for the trading pair and gas limit. This would make it more reusable for different test scenarios.
-runQuery := func(ctx sdk.Context) ( +runQuery := func(ctx sdk.Context, pair string, gasLimit uint64) ( resp *evm.MsgEthereumTxResponse, err error, ) { return deps.EvmKeeper.CallContract( ctx, embeds.SmartContract_Oracle.ABI, deps.Sender.EthAddr, &precompile.PrecompileAddr_Oracle, false, - OracleGasLimitQuery, + gasLimit, "queryExchangeRate", - "unibi:uusd", + pair, ) }x/oracle/keeper/grpc_query_test.go (1)
152-155: Enhanced test observability with detailed loggingThe addition of logging statements improves test debugging and maintenance by clearly showing the progression of block times and heights.
Consider using a test helper function to reduce the repetition of logging statements:
+func logBlockInfo(t *testing.T, msg string, blockTime time.Time, blockHeight int64) { + t.Logf("%s\nblockTimeSeconds: %d, blockHeight: %d", + msg, blockTime.Unix(), blockHeight) +} -t.Logf("Advance time and block height\nblockTimeSeconds: %d, blockHeight: %d", - blockTime.Unix(), - blockHeight, -) +logBlockInfo(t, "Advance time and block height", blockTime, blockHeight)Also applies to: 179-187, 201-209
CHANGELOG.md (2)
Line range hint
1-24: Consider standardizing changelog formattingWhile the changelog follows the Keep a Changelog format, there are some inconsistencies in the formatting:
- Consider standardizing the use of periods at the end of changelog entries
- Consider consistently hyperlinking all PR numbers for better navigation
Line range hint
31-40: Consider refining change categorizationWhile the current categorization is helpful, consider:
- Being more explicit about breaking changes by adding a "!" suffix to feature titles that include breaking changes
- Adding migration guides or upgrade notes for breaking changes
- Consistently using "State Machine Breaking" vs "Breaking Changes" categories
x/oracle/keeper/grpc_query.go (2)
Line range hint
19-115: Renamequeriertogrpc_queryfor consistencyTo maintain consistency with the naming conventions used throughout the codebase, consider renaming the
querierstruct togrpc_query. This aligns with the refactoring mentioned in the PR objectives and enhances code clarity.Apply this diff to rename
queriertogrpc_query:-type querier struct { +type grpc_query struct { Keeper } -func NewQuerier(keeper Keeper) types.QueryServer { - return &querier{Keeper: keeper} +func NewQuerier(keeper Keeper) types.QueryServer { + return &grpc_query{Keeper: keeper} } -var _ types.QueryServer = querier{} +var _ types.QueryServer = grpc_query{} -// Params queries params of distribution module -func (q querier) Params(c context.Context, _ *types.QueryParamsRequest) (*types.QueryParamsResponse, error) { +// Params queries params of distribution module +func (q grpc_query) Params(c context.Context, _ *types.QueryParamsRequest) (*types.QueryParamsResponse, error) { ... -// ExchangeRate queries exchange rate of a pair -func (q querier) ExchangeRate(c context.Context, req *types.QueryExchangeRateRequest) (*types.QueryExchangeRateResponse, error) { +// ExchangeRate queries exchange rate of a pair +func (q grpc_query) ExchangeRate(c context.Context, req *types.QueryExchangeRateRequest) (*types.QueryExchangeRateResponse, error) {
53-61: Consider renaming variableoutto improve readabilityThe variable
outcan be renamed to a more descriptive name likeexchangeRateDataorrateInfoto enhance code readability and maintainability.Apply this diff to rename the variable:
-out, err := q.Keeper.ExchangeRates.Get(ctx, req.Pair) +exchangeRateData, err := q.Keeper.ExchangeRates.Get(ctx, req.Pair) if err != nil { return nil, err } return &types.QueryExchangeRateResponse{ - ExchangeRate: out.ExchangeRate, - BlockTimestampMs: out.BlockTimestampMs, - BlockHeight: out.CreatedBlock, + ExchangeRate: exchangeRateData.ExchangeRate, + BlockTimestampMs: exchangeRateData.BlockTimestampMs, + BlockHeight: exchangeRateData.CreatedBlock, }, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/oracle/types/oracle.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)app/wasmext/stargate_query.go(0 hunks)proto/nibiru/oracle/v1/oracle.proto(1 hunks)proto/nibiru/oracle/v1/query.proto(2 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/oracle_test.go(4 hunks)x/oracle/genesis_test.go(1 hunks)x/oracle/keeper/grpc_query.go(1 hunks)x/oracle/keeper/grpc_query_test.go(9 hunks)x/oracle/keeper/keeper.go(4 hunks)x/oracle/keeper/test_utils.go(4 hunks)
💤 Files with no reviewable changes (1)
- app/wasmext/stargate_query.go
🔇 Additional comments (25)
x/evm/precompile/oracle.go (1)
92-95: LGTM! Verify exchange rate retrieval behavior.
The change to use ExchangeRates.Get provides a more structured approach to retrieve exchange rates with accurate block information, which aligns with the PR's objective to fix timestamp issues.
Let's verify the exchange rate retrieval behavior:
✅ Verification successful
Based on the search results, I can see that ExchangeRates.Get is consistently used across the codebase, particularly in test cases that verify exchange rate behavior. The test cases demonstrate that:
- Exchange rates are properly stored and retrieved with block height context
- Exchange rates have expiration logic based on block height
- The method handles both success and error cases appropriately
Since the change aligns with the established pattern in the codebase and is well-tested, I can generate the final response:
Exchange rate retrieval change is verified and correct
The switch to ExchangeRates.Get is consistent with the codebase's established patterns for exchange rate handling, with proper block height context and error handling. The test cases demonstrate that this approach correctly manages exchange rate storage, retrieval, and expiration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the exchange rate retrieval implementation
# Look for any test cases or related code that validates timestamp accuracy
# Search for related test cases
rg -A 5 "ExchangeRates.Get" --type go
# Look for any timestamp-related assertions in tests
ast-grep --pattern 'func Test$_($_) {
$$$
BlockTimestampMs
$$$
}'
Length of output: 5925
x/evm/precompile/oracle_test.go (2)
4-4: LGTM: Import changes are appropriate
The added imports support the new test structure and string formatting requirements.
Also applies to: 13-16
77-116: Test cases effectively verify timestamp handling
The test cases thoroughly validate the exchange rate queries across different block times, which directly addresses the PR's objective of fixing timestamp issues. The assertions properly verify:
- Exchange rate precision
- Block timestamp correlation
- Block height correlation
- Data consistency across time changes
Let's verify that these are the only timestamp-related test cases:
✅ Verification successful
Test cases are complete and correctly verify timestamp handling
The test cases in oracle_test.go are sufficient and properly verify the timestamp-related functionality. After analyzing other timestamp-related tests in the codebase:
- The oracle precompile implementation in
oracle.goconfirms that the test correctly verifies all timestamp-related outputs: exchange rate, block timestamp in milliseconds, and block height. - Other timestamp usages in test files (
epochs,inflation,oracle/keeper) are for different features and don't overlap with the precompile's timestamp functionality. - The test cases cover both immediate and future block times, ensuring consistent behavior across time changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other timestamp-related tests that might need similar updates
rg -l "WithBlockTime|UnixMilli|BlockTime" --type go
Length of output: 680
Script:
#!/bin/bash
# Let's check the actual usage of WithBlockTime in test files to see if they involve timestamp-related assertions
rg "WithBlockTime" -B 2 -A 2 --type go x/oracle/keeper/grpc_query_test.go x/inflation/keeper/hooks_test.go x/devgas/v1/keeper/msg_server_test.go x/epochs/abci_test.go
# Also check oracle.go to understand the timestamp handling implementation
rg "BlockTime|UnixMilli" -B 2 -A 2 x/evm/precompile/oracle.go
Length of output: 6241
proto/nibiru/oracle/v1/oracle.proto (2)
133-141: LGTM! Improved naming and consistent field definitions.
The rename from DatedPrice to ExchangeRateAtBlock better reflects the message's purpose, and the field definitions maintain consistency with the codebase's protobuf patterns.
142-145: Verify timestamp conversion accuracy.
The new block_timestamp_ms field appropriately uses int64 for Unix millisecond timestamps. However, given that this PR aims to fix issues with ctx.WithBlock* timestamps:
- Please ensure that the conversion from block time to Unix milliseconds is properly handled in the implementation.
- Consider adding validation to prevent potential overflow or precision loss during conversion.
Let's verify the timestamp handling in the implementation:
✅ Verification successful
Let me search for the actual timestamp conversion implementation since the previous results only showed the protobuf definitions.
Let me search for the actual implementation of the oracle price setting to see how the timestamp is handled.
Based on the search results, I can now generate the final response since I found the timestamp conversion implementation in the keeper.
Timestamp conversion is correctly implemented
The implementation in x/oracle/keeper/keeper.go shows that the block_timestamp_ms field is properly set using ctx.BlockTime().UnixMilli(), which:
- Gets the block time from the context
- Correctly converts it to Unix milliseconds using the standard Go time package
The conversion is safe from overflow since:
BlockTime()returns atime.TimeUnixMilli()is a built-in Go method that safely handles the conversion to milliseconds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timestamp conversion logic
rg -A 5 "block_timestamp_ms"
# Look for potential time conversion functions
ast-grep --pattern 'func $_($$) {
$$$
WithBlock$$$
$$$
}'
Length of output: 2004
Script:
#!/bin/bash
# Search for timestamp conversion implementations
rg -A 10 "func.*ExchangeRateAtBlock"
# Look for code that sets the block_timestamp_ms field
rg -A 5 "SetBlock.*Time|block_timestamp_ms.*="
# Search for time conversion utilities
rg -A 5 "time\.(Now|Unix)"
Length of output: 28360
Script:
#!/bin/bash
# Search for oracle price setting implementation
rg -A 10 "func.*SetPrice.*Context"
# Look for oracle keeper implementation files
fd "keeper.*\.go" --exec rg -A 5 "block.*time"
Length of output: 1266
proto/nibiru/oracle/v1/query.proto (3)
13-14: LGTM: Documentation accurately reflects the response structure.
The updated documentation clearly communicates that the response includes both block height and timestamp information, which aligns with the PR's objective to fix timestamp-related issues.
122-125: LGTM: Empty request message is appropriate for this use case.
The empty QueryExchangeRatesRequest message is correctly defined as it's used to fetch all exchange rates without any filtering parameters.
Line range hint 1-1: Verify timestamp usage across the codebase.
The changes align well with the PR objectives by adding proper block timestamp and height fields. Let's verify the timestamp usage across the codebase to ensure consistency.
✅ Verification successful
Timestamp usage is consistent across the codebase
The verification confirms that:
- All timestamp fields in the oracle module consistently use milliseconds (block_timestamp_ms)
- The DatedExchangeRate has been completely removed from proto files
- Other modules using timestamps (e.g., epochs, evm) properly use the standard google.protobuf.Timestamp type with appropriate annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent timestamp usage across the codebase
# Look for other timestamp-related fields to ensure consistent units (milliseconds)
rg -n "timestamp|time" --type proto
# Check for any remaining references to DatedExchangeRate
rg -n "DatedExchangeRate"
Length of output: 180
Script:
#!/bin/bash
# Let's try a different approach to find proto files and check timestamp usage
# Find proto files first
fd '\.proto$' | while read -r file; do
echo "=== $file ==="
grep -n "timestamp\|time" "$file" || true
done
# Check for DatedExchangeRate in proto files specifically
fd '\.proto$' | while read -r file; do
echo "=== $file ==="
grep -n "DatedExchangeRate" "$file" || true
done
Length of output: 14829
🧰 Tools
🪛 GitHub Check: break-check
[failure] 12-12:
Previously present RPC "DatedExchangeRate" on service "Query" was deleted.
x/oracle/keeper/test_utils.go (5)
8-8: LGTM: Correct import of sdkmath package
The import statement correctly adds the sdkmath package from cosmossdk.io/math, which is part of the migration from the math package.
225-225: LGTM: Correct usage of sdkmath for CommunityTax
The change correctly uses sdkmath.LegacyNewDecWithPrec for setting the community tax parameter to 2%.
284-289: LGTM: Proper migration to sdkmath in validator creation
The changes correctly update all decimal and integer operations to use sdkmath, maintaining the original test behavior while using the new package.
313-313: LGTM: Correct update to sdkmath for test exchange rate
The change appropriately updates the test exchange rate to use sdkmath while maintaining the original test value.
Line range hint 1-377: Consider adding test coverage for timestamp handling
While the sdkmath migration is well implemented, given that the PR objectives mention fixing issues with timestamps for exchange rates, consider adding test cases to verify the correct handling of block timestamps in the oracle's exchange rate functionality.
Would you like help implementing test cases for the timestamp functionality?
x/oracle/keeper/grpc_query_test.go (5)
13-13: LGTM: Import addition for set package
The addition of the set package aligns with the refactoring to use sets for managing target pairs, improving code clarity.
41-49: LGTM: Updated exchange rate storage with block timestamp
The exchange rate storage now correctly includes the block timestamp, addressing the PR's objective of fixing timestamp-related issues.
249-252: LGTM: Comprehensive timestamp and block height assertions
The test now properly verifies both the exchange rate and its associated metadata (timestamp and block height), ensuring the correctness of the oracle's historical data.
Also applies to: 260-263
265-269: LGTM: Added error case test
Good addition of a test case for querying non-existent pairs, improving error handling coverage.
361-374: LGTM: Improved active pairs management using Set
The refactoring to use set.New[asset.Pair]() improves code clarity and efficiency for managing target pairs. The use of a set data structure is more idiomatic for this use case.
Also applies to: 378-379
CHANGELOG.md (2)
110-115: Verify changelog entry matches PR objectives
The changelog entry accurately reflects the PR objectives, correctly capturing:
- The timestamp discrepancy issue
- The impact on oracle exchange rates
- The EVM precompile integration
- Additional code simplification in x/oracle module
Line range hint 25-30: LGTM: Semantic versioning is properly implemented
The changelog correctly implements semantic versioning with:
- Clear version numbers (MAJOR.MINOR.PATCH)
- Proper marking of breaking changes
- Appropriate version increments based on change types
x/oracle/keeper/keeper.go (5)
83-83: Confirm ExchangeRates initialization aligns with the new type.
The initialization of ExchangeRates in NewKeeper has been correctly updated to use types.ExchangeRateAtBlock. This ensures consistency with the updated data structure.
171-172: Ensure comment accuracy and clarity in GetExchangeRateTwap.
The added comments enhance the understanding of how the last snapshot is handled when computing the TWAP. They accurately describe that the price is considered ongoing until the current block time.
187-193: Confirm consistent usage of block timestamps in SetPrice.
The blockTimestampMs is derived using ctx.BlockTime().UnixMilli(). Verify that this method consistently provides the correct block timestamp in milliseconds across different environments and that there are no time zone discrepancies.
Run the following script to confirm consistent usage of ctx.BlockTime():
✅ Verification successful
Let me gather more context about the timestamp usage by checking the type definitions and any potential configuration related to timestamps.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Block timestamps are consistently handled using UnixMilli across the codebase
The implementation is correct and consistent because:
- The
BlockTimestampMsfield inExchangeRateAtBlockis explicitly documented as "a conventional Unix millisecond time, i.e. the number of milliseconds elapsed since January 1, 1970 UTC" - All usages of
ctx.BlockTime().UnixMilli()in the codebase are consistent and only used for timestamp operations in milliseconds - The timestamp handling is uniform across different components:
- Price updates in
keeper.go - TWAP calculations
- Event emissions
- Query responses
- Price updates in
- Test cases confirm correct millisecond precision handling with explicit time conversions using
UnixMilli()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `ctx.BlockTime()` to ensure consistent timestamp retrieval.
# Expected: All usages should correctly handle time in milliseconds.
rg 'ctx\.BlockTime\(\)\.Unix(Milli)?'
Length of output: 614
Script:
#!/bin/bash
# Check type definitions and configurations related to timestamps
ast-grep --pattern 'type ExchangeRateAtBlock struct {
$$$
BlockTimestampMs $_
$$$
}'
# Also check for any other timestamp-related fields or configurations
rg -A 3 'BlockTimestampMs|Timestamp' --type go
Length of output: 17349
38-38: Verify all usages of ExchangeRates are updated to handle ExchangeRateAtBlock.
The ExchangeRates field in the Keeper struct has been updated to use types.ExchangeRateAtBlock. Ensure that all methods and code segments interacting with ExchangeRates are updated to accommodate the new structure, including serialization, deserialization, and access patterns.
Run the following script to identify all usages of ExchangeRates beyond keeper.go:
✅ Verification successful
Let me check the implementation details of these usages to verify they are properly handling the ExchangeRateAtBlock type.
Let me check one more thing to verify if there are any other methods that might be using ExchangeRates in a different way.
Let me check the actual usage in the query and message handlers to ensure they're properly handling the ExchangeRateAtBlock type.
Based on the comprehensive review of the codebase, I can now provide the final response:
All usages of ExchangeRates are properly handling the ExchangeRateAtBlock type
The verification shows that all interactions with ExchangeRates are correctly handling the ExchangeRateAtBlock type:
- In
keeper.go, theSetPricemethod properly inserts anExchangeRateAtBlockwith all required fields - In
update_exchange_rates.go, the code correctly accesses theCreatedBlockfield for expiration checks - In
grpc_query.go, the queries properly handle the new type, accessingExchangeRatefield when needed - In
msg_server.go, there are no direct interactions with the storage, only with the parsed exchange rate tuples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `ExchangeRates` in the codebase to verify they are updated.
# Expected: All usages should be compatible with the new `ExchangeRateAtBlock` type.
rg 'k\.ExchangeRates\.(Get|Insert|Has|Delete)'
Length of output: 293
Script:
#!/bin/bash
# Check the implementation details around the ExchangeRates usages
rg -A 5 -B 5 'k\.ExchangeRates\.(Get|Insert|Has|Delete)' x/oracle/keeper/
Length of output: 1978
Script:
#!/bin/bash
# Look for any other methods that might be using ExchangeRates
rg -l "ExchangeRates" x/oracle/keeper/
Length of output: 335
Script:
#!/bin/bash
# Check ExchangeRates usage in grpc_query.go and msg_server.go
rg -A 5 -B 5 "ExchangeRates" x/oracle/keeper/grpc_query.go x/oracle/keeper/msg_server.go
Length of output: 4883
199-204:
Verify event consumers handle the new TimestampMs field in EventPriceUpdate.
The EventPriceUpdate event now includes TimestampMs. Ensure that all subscribers or consumers of this event are updated to handle the new field appropriately to prevent potential parsing errors or data inconsistencies.
Run the following script to identify usages of EventPriceUpdate and verify they accommodate the new TimestampMs field:
✅ Verification successful
Let me gather more information about the event consumers by checking the test files to see how they handle the TimestampMs field.
Let me check if there are any other consumers of this event by looking for event handlers or subscribers in the codebase.
Let me check the event definition in the proto file to understand the complete structure and any potential breaking changes.
Based on the gathered information, I can now provide the final response:
The TimestampMs field addition to EventPriceUpdate is safe
The verification shows that:
- The
EventPriceUpdateis properly defined in the proto file withtimestamp_msas field number 3, making it a non-breaking change - The test files correctly handle the new field in all test cases
- No external event consumers were found that directly parse or handle this event
- The event is only emitted within the oracle module and used in a type-safe manner through the generated protobuf code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to `EventPriceUpdate` to confirm handling of `TimestampMs`.
# Expected: Consumers should be updated to handle the new `TimestampMs` field.
rg 'EventPriceUpdate'
Length of output: 2619
Script:
#!/bin/bash
# Check test files for EventPriceUpdate usage and TimestampMs handling
cat x/oracle/keeper/grpc_query_test.go | grep -A 5 "EventPriceUpdate{"
Length of output: 847
Script:
#!/bin/bash
# Search for potential event handlers or subscribers
rg -i "handle.*event"
rg -i "subscribe.*event"
rg -i "process.*event"
Length of output: 3785
Script:
#!/bin/bash
# Check the proto definition of EventPriceUpdate
cat proto/nibiru/oracle/v1/event.proto | grep -A 10 "message EventPriceUpdate"
Length of output: 400
| input.OracleKeeper.ExchangeRates.Insert(input.Ctx, "pair1:pair2", | ||
| types.ExchangeRateAtBlock{ | ||
| ExchangeRate: math.LegacyNewDec(123), | ||
| CreatedBlock: 0, | ||
| BlockTimestampMs: 0, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for timestamp handling
Given that this PR focuses on fixing timestamp-related issues, the test should validate proper timestamp handling more thoroughly. Currently, BlockTimestampMs is set to 0, which might mask potential issues.
Consider:
- Adding test cases with non-zero timestamps
- Validating that timestamps are correctly preserved during export/import
- Adding assertions to verify timestamp accuracy
Here's a suggested enhancement:
input.OracleKeeper.ExchangeRates.Insert(input.Ctx, "pair1:pair2",
types.ExchangeRateAtBlock{
ExchangeRate: math.LegacyNewDec(123),
CreatedBlock: 0,
- BlockTimestampMs: 0,
+ BlockTimestampMs: input.Ctx.BlockTime().UnixMilli(),
})
+
+// Add assertion to verify timestamp preservation
+exported := oracle.ExportGenesis(input.Ctx, input.OracleKeeper)
+rate, found := exported.ExchangeRates.Get("pair1:pair2")
+require.True(t, found)
+require.Equal(t, input.Ctx.BlockTime().UnixMilli(), rate.BlockTimestampMs)Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
=======================================
Coverage 64.66% 64.67%
=======================================
Files 273 273
Lines 21616 21601 -15
=======================================
- Hits 13979 13970 -9
+ Misses 6659 6655 -4
+ Partials 978 976 -2
|
Unique-Divine
force-pushed
the
ud/oracle-time
branch
from
dffca3d to
80e4637
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
eth/rpc/backend/blocks_test.go (1)
19-21: LGTM! Consider enhancing the comment.The change to allow for a 2-block difference is a good improvement that makes the test more resilient against false failures due to network latency.
Consider making the comment more precise:
-// Rather than checking exact equality, which might not be true due to -// latency. Add a cushion of 2 blocks. +// Allow for a difference of up to 2 blocks between latest height and block height +// to account for potential network latency between nodes.x/oracle/keeper/keeper.go (1)
187-204: Consider adding price validationThe method accepts any decimal value as price without validation. Consider adding basic sanity checks for the price parameter.
func (k Keeper) SetPrice(ctx sdk.Context, pair asset.Pair, price sdk.Dec) { + if price.IsNegative() { + panic("price cannot be negative") + } + if price.GT(sdk.NewDec(1000000)) { // Example threshold + panic("price exceeds maximum allowed value") + } blockTimestampMs := ctx.BlockTime().UnixMilli() k.ExchangeRates.Insert(ctx, pair,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/oracle/types/oracle.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)app/wasmext/stargate_query.go(0 hunks)eth/rpc/backend/blocks_test.go(1 hunks)proto/nibiru/oracle/v1/oracle.proto(1 hunks)proto/nibiru/oracle/v1/query.proto(2 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/oracle_test.go(4 hunks)x/oracle/genesis_test.go(1 hunks)x/oracle/keeper/grpc_query.go(1 hunks)x/oracle/keeper/grpc_query_test.go(9 hunks)x/oracle/keeper/keeper.go(4 hunks)x/oracle/keeper/test_utils.go(4 hunks)
💤 Files with no reviewable changes (1)
- app/wasmext/stargate_query.go
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- proto/nibiru/oracle/v1/oracle.proto
- proto/nibiru/oracle/v1/query.proto
- x/evm/precompile/oracle.go
- x/evm/precompile/oracle_test.go
- x/oracle/genesis_test.go
- x/oracle/keeper/grpc_query.go
- x/oracle/keeper/grpc_query_test.go
🔇 Additional comments (8)
x/oracle/keeper/keeper.go (4)
83-83: LGTM: Correctly updated collection encoder
The ExchangeRates initialization is properly updated to use the new type encoder while maintaining the same collection key for data compatibility.
171-172: LGTM: Clear documentation of timestamp handling
The comment clearly explains the edge case handling for the last snapshot's timestamp in TWAP calculations.
187-193: LGTM: Consistent timestamp handling
The implementation correctly captures and uses the block timestamp consistently across the ExchangeRateAtBlock structure.
38-38: Verify the impact of ExchangeRates type change
The type change from DatedPrice to ExchangeRateAtBlock is aligned with the PR objectives. However, we should verify that all consumers of this field have been updated accordingly.
✅ Verification successful
Let me gather more context about the type change and its usage.
Based on the comprehensive search results, I can now generate the final response:
Type change from DatedPrice to ExchangeRateAtBlock is properly implemented
The codebase shows consistent implementation of the ExchangeRateAtBlock type:
- All references to the old
DatedPricetype have been removed - The
ExchangeRateAtBlocktype is properly defined with timestamp and block height fields - All usages of
k.ExchangeRateshave been updated to work with the new type - Test files have been updated to use the new type structure
- The
SetPricemethod correctly uses the new type when inserting exchange rates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to DatedPrice that might need updating
rg "DatedPrice" --type go
# Search for all uses of ExchangeRates field to verify they've been updated
ast-grep --pattern 'k.ExchangeRates.$$$'
Length of output: 462
Script:
#!/bin/bash
# Check the definition and usage of ExchangeRateAtBlock
rg "ExchangeRateAtBlock" -A 3 --type go
# Check the implementation of SetPrice and related methods
ast-grep --pattern 'func (k Keeper) SetPrice($$$) {
$$$
}'
# Check for any test files that might need updates
fd ".*_test.go$" --exec rg "ExchangeRates" {}
Length of output: 12605
x/oracle/keeper/test_utils.go (4)
8-8: LGTM: Correct import for sdkmath package
The import follows the standard Cosmos SDK convention for importing the sdkmath package.
225-225: LGTM: Correct migration to sdkmath for community tax
The change correctly updates the package reference while maintaining the same decimal precision and value for community tax.
284-289: LGTM: Consistent migration to sdkmath in validator creation
All numeric types have been correctly migrated to use the sdkmath package while maintaining the original behavior of zero commission rates and minimum self delegation.
313-313: LGTM: Correct migration to sdkmath for test exchange rate
The test exchange rate constant has been properly migrated to use sdkmath while maintaining its original value.
correspond to the block header information from specified blocks in the chain's history, so the oracle exchange rates need a way to correctly retrieve this information. This change fixes that discrepency, giving the expected block timesamp for the EVM's oracle precompiled contract. The change also simplifies and corrects the code in x/oracle. refactor(oralce): rename querier -> grpc_query for consistency with the rest of the codebase
Unique-Divine
force-pushed
the
ud/oracle-time
branch
from
80e4637 to
8327275
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
x/oracle/keeper/keeper.go (2)
171-172: Consider adding more detailed documentationThe logic for handling the last snapshot's time period is correct, but consider adding more documentation about why the current block time is used as the endpoint.
Add a comment explaining that this approach ensures the TWAP calculation includes the most recent price data up to the current block:
// if we're at the last snapshot, then consider that price as ongoing // until the current blocktime +// This ensures the TWAP includes the most recent price impact up until +// the current block, providing the most up-to-date average price nextTimestampMs = ctx.BlockTime().UnixMilli()
187-193: LGTM: Consistent timestamp handling implementedThe implementation correctly uses
BlockTimestampMsacross all operations, fixing the timestamp issues mentioned in the PR objectives.Consider adding error handling for the ExchangeRates.Insert operation:
+var err error -k.ExchangeRates.Insert(ctx, pair, +if err = k.ExchangeRates.Insert(ctx, pair, types.ExchangeRateAtBlock{ ExchangeRate: price, CreatedBlock: uint64(ctx.BlockHeight()), BlockTimestampMs: blockTimestampMs, - }) + }); err != nil { + ctx.Logger().Error("failed to insert exchange rate", "pair", pair, "error", err) +}x/oracle/keeper/test_utils.go (1)
225-225: Consider planning for future removal of Legacy math functions.While the current usage of
LegacyNewDecWithPrec,LegacyZeroDec, andLegacyNewDecis appropriate for test code, these functions are marked as "Legacy". Consider creating a tracking issue to migrate away from legacy math functions in the future when the SDK provides stable alternatives.Would you like me to help create a GitHub issue to track the future migration from legacy math functions?
Also applies to: 284-289, 313-313
x/oracle/keeper/grpc_query_test.go (1)
Line range hint
152-269: LGTM! Well-structured test with good debugging capabilitiesThe test improvements provide clear temporal progression and comprehensive validation of exchange rate timestamps. However, consider enhancing the test coverage.
Consider converting this into a table-driven test to cover more time-related edge cases:
tests := []struct { name string timeOffsets []time.Duration expectedRates []math.LegacyDec expectedError error }{ { name: "sequential updates within same block", timeOffsets: []time.Duration{0, time.Second, 2 * time.Second}, expectedRates: []math.LegacyDec{ math.LegacyNewDec(1700), math.LegacyNewDec(1800), }, }, // Add more test cases }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/oracle/types/oracle.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)app/wasmext/stargate_query.go(0 hunks)eth/rpc/backend/blocks_test.go(1 hunks)proto/nibiru/oracle/v1/oracle.proto(1 hunks)proto/nibiru/oracle/v1/query.proto(2 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/oracle_test.go(4 hunks)x/oracle/genesis_test.go(1 hunks)x/oracle/keeper/grpc_query.go(1 hunks)x/oracle/keeper/grpc_query_test.go(9 hunks)x/oracle/keeper/keeper.go(4 hunks)x/oracle/keeper/test_utils.go(4 hunks)
💤 Files with no reviewable changes (1)
- app/wasmext/stargate_query.go
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- eth/rpc/backend/blocks_test.go
- proto/nibiru/oracle/v1/oracle.proto
- proto/nibiru/oracle/v1/query.proto
- x/evm/precompile/oracle.go
- x/evm/precompile/oracle_test.go
- x/oracle/genesis_test.go
- x/oracle/keeper/grpc_query.go
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/keeper.go (1)
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2097
File: x/oracle/keeper/keeper.go:195-195
Timestamp: 2024-11-12T10:02:53.398Z
Learning: In `x/oracle/keeper/keeper.go`, within the `GetDatedExchangeRate` method, it's not possible for `exchangeRate.CreatedBlock` to be in the future relative to `ctx.BlockHeight()`, so additional validation before time calculation is unnecessary.
🔇 Additional comments (8)
x/oracle/keeper/keeper.go (3)
83-83: LGTM: Encoder initialization properly updated
The encoder initialization is correctly updated to use ExchangeRateAtBlock type, maintaining consistency with the type change.
199-204: LGTM: Consistent timestamp usage in snapshots and events
The implementation maintains consistency by using the same timestamp value across price snapshots and events.
38-38: LGTM: Type change enhances timestamp precision
The change from DatedPrice to ExchangeRateAtBlock aligns with the PR's objective to fix timestamp handling. The new type includes explicit block timestamp tracking.
Let's verify the type definition is consistent:
✅ Verification successful
Type change correctly implements timestamp tracking
The change to ExchangeRateAtBlock is verified to be correct and well-documented. The new type includes block_timestamp_ms field that explicitly tracks Unix millisecond timestamps for price consensus, which is a more precise approach than the previous implementation. The field is properly defined with clear documentation explaining its purpose and format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the ExchangeRateAtBlock type definition
ast-grep --pattern 'message ExchangeRateAtBlock {
$$$
int64 block_timestamp_ms = $_;
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's try finding the type definition in proto files
fd "\.proto$" | xargs rg "ExchangeRateAtBlock" -A 5
# Also search for the old DatedPrice type for comparison
fd "\.proto$" | xargs rg "DatedPrice" -A 5
Length of output: 531
Script:
#!/bin/bash
# Let's check the complete ExchangeRateAtBlock message definition
rg "message ExchangeRateAtBlock" -A 15 proto/nibiru/oracle/v1/oracle.proto
# Also check for any references to block timestamp in this type
rg "block_timestamp" proto/nibiru/oracle/v1/oracle.proto
Length of output: 891
x/oracle/keeper/test_utils.go (2)
8-8: LGTM! Good practice using the official SDK math package.
The change from math to sdkmath aligns with the recommended practice of using the official Cosmos SDK math package.
Line range hint 1-377: Verify completeness of PR objectives.
While the math package updates look good, I notice that this file doesn't contain any changes related to:
- Fixing timestamp issues for exchange rates
- Renaming
queriertogrpc_query
Let's verify if these changes exist in other files:
x/oracle/keeper/grpc_query_test.go (3)
13-13: LGTM! Good use of the set package
The addition of the set package improves the code's readability and efficiency in managing unique pairs.
41-49: LGTM! Good test coverage for the new ExchangeRateAtBlock structure
The test correctly validates both the block timestamp and height, ensuring accurate temporal tracking of exchange rates.
361-374: LGTM! Excellent refactor using Set for target pairs
The changes improve both efficiency and readability:
- Using Set for managing unique pairs
- Consistent temporal tracking with BlockTimestampMs
- Clear block height progression
Purpose / Abstract
fix(oracle): The timestamps resulting from ctx.WithBlock* don't actually correspond to the block header information from specified blocks in the chain's history, so the oracle exchange rates need a way to correctly retrieve this information. This change fixes that discrepency, giving the expected block timesamp for the EVM's oracle precompiled contract. The change also simplifies and corrects the code in x/oracle.
refactor(oracle): rename querier -> grpc_query for consistency with the rest of the codebase
Related Audit Ticket: https://github.com/code-423n4/2024-11-nibiru-findings/issues/2
Part of [evm] Oracle precompile UX would be improved by update time information #2075
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Deprecations