refactor(oracle): introduce FeederManager to replace aggregator and caches for transaction handling #275
Conversation
WalkthroughThis pull request makes extensive modifications to the Oracle module. It updates the business logic for price creation by enhancing validation, hashing, and aggregation of price quotes, while adjusting timeout constants and error handling. Several native token and parameter management functions are refined, including versioning support and updates to test targets in the Makefile. In addition, various aggregation-related files and tests are removed and replaced by a new feedermanagement package that centralizes feeder initialization, block processing, caching, and recovery mechanisms. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (5)
x/oracle/keeper/msg_server_create_price.go (2)
68-69: InitializepriceStroutside the condition for clarity.
While settingpriceStr := newItem.PriceTR.Priceis fine, consider grouping this initialization with the conditional block that follows to make the intent clearer.
79-79: Confirm external consumers can handle hashed prices.
Emitting the hashed price (when length ≥ 32) may break downstream systems expecting the plain price. Please verify with all consumers before merging.x/oracle/keeper/native_token.go (1)
257-258: Correct spelling in comment.
Typo: "valdiator" should be "validator" in the inline comment.x/oracle/types/params.go (1)
67-106: Consolidate repeated token definitions to reduce boilerplate.
Adding multiple similar tokens is fine for now, but a loop or configuration file could streamline future expansions and reduce duplication.local_node.sh (1)
336-336: Preserve the--oracleflag if needed.
Your learnings mention that--oracleis required to enable the price-feeder functionality. If that’s still relevant, add--oracleback to the final start command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
local_node.sh(4 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/native_token.go(2 hunks)x/oracle/types/params.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
local_node.sh (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore#252
File: local_node.sh:0-0
Timestamp: 2024-12-13T08:52:47.338Z
Learning: In `local_node.sh`, the `--oracle` flag is required in the `exocored start` command to enable the price-feeder functionality and should not be removed.
🔇 Additional comments (8)
x/oracle/keeper/msg_server_create_price.go (2)
5-9: Ensure consistent usage of newly added imports.
These imports (sha256, base64, and strings) appear correct for the new hashing and string manipulation features. Please confirm they are all necessary and not duplicated elsewhere.
70-75: Verify hashing threshold and usage of string length.
The code hashes the price only if its length is at least 32 characters. Ensure you are consistently handling all character encodings (e.g., UTF-8). If multi-byte characters are possible, consider checking byte length.
x/oracle/keeper/native_token.go (1)
300-301: Validate index increments carefully.
Since you now rely on newBalance.Index++ as a global reference across rounds, ensure the logic never overflows or misaligns if multiple token operations happen concurrently.
x/oracle/types/params.go (2)
63-63: Decimal update from 18 to 8.
Check that any existing calculations relying on 18 decimals adapt correctly to the new scale. This may affect stored balances and external tools.
138-173: Ensure new token feeders align with upstream environment.
Multiple feeders are declared with overlapping or sequenced StartBaseBlock. Confirm they don’t create conflicts or race conditions when updating prices.
local_node.sh (3)
117-122: Check total supply for newly added “nsteth.”
You're adding “nsteth” with a staking_total_amount of 5000. Confirm this value meets testing requirements and doesn't disrupt the existing deposit logic in the module.
139-149: Coordinate oracle feeders with on-chain parameters.
These lines add or adjust multiple token feeders. Ensure the code references for token IDs and chain IDs indeed match the fresh feeders declared here.
253-254: Confirm the newly added “nstid” option’s correctness.
This YAML snippet references 0xeeee..._0x65 as the nstid. Verify that the beacon chain environment and local node logic expect this identifier.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
leonz789
force-pushed
the
develop-oracle-refactor2.0
branch
from
97f8272 to
d8a5b20
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 27
🔭 Outside diff range comments (3)
x/oracle/keeper/feedermanagement/aggregator.go (1)
Line range hint
655-657: Handle potential non-determinism in validator powersWhen constructing
validatorPowers, the map iteration overvalidatorSetmay introduce non-determinism. Although validators are obtained fromGetAllExocoreValidators, ensure that the order is consistent or sort the validators before processing.Consider sorting the validators list to ensure deterministic ordering:
-validatorPowers := make(map[string]*big.Int) -for _, v := range validatorSet { +sort.SliceStable(validatorSet, func(i, j int) bool { + return bytes.Compare(validatorSet[i].Address, validatorSet[j].Address) < 0 +}) +validatorPowers := make(map[string]*big.Int, len(validatorSet)) +for _, v := range validatorSet { validatorPowers[sdk.ConsAddress(v.Address).String()] = big.NewInt(v.Power) }🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 146-148: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 156-159: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 163-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 167-173: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 172-178: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 216-218: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 272-274: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinism🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
tests/e2e/oracle/create_price.go (1)
Line range hint
277-290: Fix failing balance assertion.The test is failing because the expected balance value (28) doesn't match the actual value (32) in the BalanceInfo comparison.
This could indicate:
- Incorrect balance calculation in the test
- Bug in the underlying implementation
- Stale test data
Verify the balance calculation logic and update either the test or the implementation accordingly.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 277-277: Test failure: Expected balance value 28, but got 32 in BalanceInfo comparison
x/avs/keeper/keeper.go (1)
Line range hint
594-600: Critical timing checks added for task result submission.The added checks prevent task results from being submitted:
- Too early: Before the response period ends
- Too late: After the statistical period ends
This is a crucial security enhancement to ensure task results are submitted within the correct window.
🧹 Nitpick comments (31)
x/oracle/types/types.go (1)
66-72: Add documentation and improve error handlingWhile the function implementation is correct, it would benefit from:
- Adding godoc comments explaining the purpose and usage
- Including validation for empty input
Consider this improvement:
+// ConsAddrStrFromCreator converts a Bech32 account address to its corresponding +// consensus address string. This is typically used for validator operations. +// Returns an error if the input is invalid or empty. func ConsAddrStrFromCreator(creator string) (string, error) { + if creator == "" { + return "", fmt.Errorf("empty creator address") + } accAddress, err := sdk.AccAddressFromBech32(creator) if err != nil { return "", err } return sdk.ConsAddress(accAddress).String(), nil }x/oracle/keeper/feedermanagement/algo.go (1)
23-30: Potential precision loss when computing median inBigIntList.Median()When the number of elements is even, the median is calculated using integer division, which discards any fractional component. This may lead to precision loss. Consider using
big.Ratto handle fractions for more accurate median calculations if high precision is required.🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/module_beginblock_devmode.go (1)
17-21: Clarify Error Handling in Development ModeIn the
BeginBlockfunction (lines 17-21), the code usespanicto handle situations where theFeederManagerrecovery logic fails. While panicking is acceptable in development mode (as indicated by the build tag//go:build devmode), consider providing more informative error messages or logging additional context to aid in debugging. This can help developers quickly identify and resolve issues during testing.x/oracle/keeper/common/expected_keepers.go (3)
20-22: Review Inclusion ofSlashingKeeperinKeeperOracleInterfaceThe
SlashingKeeperinterface is embedded within theKeeperOracleinterface (lines 20-22 and 25-26). While embedding interfaces can be useful, it might blur the separation of concerns. Ensure that all implementations ofKeeperOracleare intended to include slashing functionalities. If not, consider injectingSlashingKeeperwhere needed instead of embedding it, adhering to the Interface Segregation Principle.
27-48: Standardize Method Naming ConventionsThe
KeeperOracleinterface (lines 27-48) includes methods with varying naming patterns, such as:
GetMaliciousJailDurationGetSlashFractionMaliciousGrowRoundIDFor consistency and clarity, consider standardizing the method names. For example, use the
Getprefix for retrieval methods (GetX),Setfor setters, and use consistent parameter naming. Uniform method names improve readability and maintainability.
27-48: Consider Interface SegregationThe
KeeperOracleinterface has grown extensive (lines 27-48), which can make it cumbersome to implement and maintain. According to the Interface Segregation Principle, it's better to have multiple smaller, specific interfaces. Consider splittingKeeperOracleinto several focused interfaces, such asValidatorReportKeeper,PriceManager, andCacheHandler, to enhance modularity and reusability.x/oracle/keeper/feedermanagement/types.go (4)
128-135: Rename MethodCpytoCopyfor ClarityThe method
Cpyin thethresholdstruct (lines 128-135) creates a copy of the threshold. Renaming it toCopywould improve readability and conform to standard naming conventions.Apply this diff to rename the method:
-func (t *threshold) Cpy() *threshold { +func (t *threshold) Copy() *threshold { return &threshold{ totalPower: new(big.Int).Set(t.totalPower), thresholdA: new(big.Int).Set(t.thresholdA), thresholdB: new(big.Int).Set(t.thresholdB), } }🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
87-87: Correct Typographical Error in CommentThere's a typo in the comment on line 87:
// TODO: V2: accumulatedValidPower only includes validators who prividing all sources required by rules(defined in oracle.Params)"prividing" should be "providing".
Apply this diff to fix the typo:
-// TODO: V2: accumulatedValidPower only includes validators who prividing all sources required by rules(defined in oracle.Params) +// TODO: V2: accumulatedValidPower only includes validators who providing all sources required by rules(defined in oracle.Params)🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
198-204: EnhanceremoveMethod to Indicate SuccessThe
removemethod inorderedSliceInt64(lines 197-204) doesn't indicate whether the removal was successful. Modify the method to return a boolean value to reflect the outcome, which can improve error handling in calling functions.Apply this diff to modify the method:
-func (osi *orderedSliceInt64) remove(i int64) { +func (osi *orderedSliceInt64) remove(i int64) bool { for idx, v := range *osi { if v == i { *osi = append((*osi)[:idx], (*osi)[idx+1:]...) + return true } } + return false }🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
79-79: Clarify Variable Name for ReadabilityIn the
priceValidatorstruct (line 79), the fieldvalidator stringcould be renamed tovalidatorIDorvalidatorAddressfor better clarity, indicating what the string represents.Apply this diff to rename the field:
type priceValidator struct { finalPrice *PriceResult - validator string + validatorID string power *big.Int // other fields }🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/round.go (3)
89-90: Address the TODO: Implement valid value returnThe TODO comment on lines 89-90 indicates that the function should return a valid value instead of the original
protoMsg. Implementing this will enhance the function's correctness and prevent potential misuse of unprocessed messages.Do you want me to generate a code suggestion or open a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
140-140: Typographical error in commentThere's a typo on line 140: "currentHeight euqls to baseBlock" should be "currentHeight equals to baseBlock."
Apply this diff to correct the typo:
- // currentHeight euqls to baseBlock + // currentHeight equals to baseBlock🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
236-251: Improve documentation and error handling ingetPositionThe
getPositionmethod calculates critical parameters but lacks sufficient documentation and comprehensive error handling for edge cases. Enhancing this method will improve code readability and reliability.🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/types/key_slashing.go (1)
9-11: Remove commented-out codeThe commented-out function
SlashingValidatorReportInfoPrefixappears to be unused. If it's not needed, it should be removed rather than commented out.x/oracle/types/errors.go (1)
18-19: Document error codes and maintain terminology consistencyThe new error codes are well-structured, but could benefit from documentation explaining when they occur.
Add comments explaining the scenarios where these errors occur:
// Error codes for aggregation and quote handling const ( // failedInAggregation indicates that the price aggregation process failed failedInAggregation = iota + 2 // quoteRecorded indicates that the quote was successfully recorded quoteRecorded )Also, consider updating all "price proposal" terminology to "quote" throughout the codebase for consistency with the error message change in
ErrPriceProposalIgnored.Also applies to: 32-33
x/oracle/keeper/msg_server_update_params.go (1)
68-70: Consider adding error handling forSetParamsUpdated().While the check for non-check transactions is good, the
SetParamsUpdated()call should include error handling to ensure any failures are properly propagated.if !ctx.IsCheckTx() { - ms.SetParamsUpdated() + if err := ms.SetParamsUpdated(); err != nil { + return nil, err + } }x/oracle/keeper/keeper.go (2)
31-32: Remove TODO comment or provide more context.The TODO comment "remove this" lacks context about what needs to be removed and why. Either remove the comment if it's no longer relevant or provide more detailed information about the planned changes.
68-69: Remove commented-out code.There's a commented-out duplicate initialization of the FeederManager. Clean up by removing the unused code.
- // fm: feedermanagement.NewFeederManager(nil), FeederManager: feedermanagement.NewFeederManager(nil),x/oracle/keeper/prices_test.go (1)
51-52: Enhance test setup verification.The test initializes the FeederManager but doesn't verify its state. Consider adding assertions to ensure the cache is properly cleared and the block is initialized.
keeper.FeederManager.SetNilCaches() keeper.FeederManager.BeginBlock(ctx) +// Verify cache state +require.Nil(t, keeper.FeederManager.GetCache(), "Cache should be nil after SetNilCaches") +// Verify block initialization +require.Equal(t, ctx.BlockHeight(), keeper.FeederManager.GetCurrentBlock())x/oracle/keeper/feedermanagement/helper_test.go (1)
16-17: Use a non-deterministic seed for testing.The random seed is hardcoded to 1, which makes tests deterministic but might miss edge cases.
Consider using a time-based seed and running tests multiple times:
- r = rand.New(rand.NewSource(1)) + r = rand.New(rand.NewSource(time.Now().UnixNano()))🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/feedermanager_test.go (1)
21-24: Strengthen mock expectations.The mock expectations use
AnyTimes()which is too permissive. Consider using more specific expectations withTimes(n)to catch potential issues with multiple calls.- AnyTimes() + Times(1)🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/keeper_suite_test.go (2)
78-79: Document the order dependency of FeederManager operations.The order of
SetNilCaches()andBeginBlock()calls is important. Consider adding a comment explaining why this order is necessary.
112-112: Ensure consistent state reset.The
BeginBlockcall appears in bothReset()andSetupTest(). Consider extracting this common initialization to a helper method to ensure consistency.+func (suite *KeeperSuite) initFeederManager() { + suite.k.FeederManager.SetNilCaches() + suite.k.FeederManager.BeginBlock(suite.ctx) +}tests/e2e/oracle/helper_nstconvert.go (1)
26-28: Consider using constants for magic numbers.While the overflow check is good, consider defining constants for the bounds instead of using magic numbers like
math.MaxUint16.+const ( + MaxBalanceChange = math.MaxUint16 + MinBalanceChange = 0 +) -if change > math.MaxUint16 || change < 0 { +if change > MaxBalanceChange || change < MinBalanceChange {x/oracle/keeper/prices.go (1)
206-220: Enhance error handling for NST price updates.The error from
UpdateNSTByBalanceChangeis only logged but not propagated. This could mask critical issues.Consider either:
- Propagating the error for critical failures
- Adding metrics/monitoring for these errors
- Implementing a retry mechanism
Would you like me to propose an implementation?
tests/e2e/oracle/create_price.go (1)
104-107: Consider using a test helper for timestamp assertions.The pattern of manually updating timestamps is repeated multiple times. This could be refactored into a helper function.
+ func updateTimestampAndCompare(t *testing.T, expected, actual *types.PriceTimeRound) { + expected.Timestamp = actual.Timestamp + assert.Equal(t, expected, actual) + }Also applies to: 140-143, 175-178, 197-200, 315-318
app/app.go (1)
918-926: Improved module initialization order.The oracle module's BeginBlock is now executed before the epoch module, ensuring proper state initialization sequence. This ordering is crucial as the oracle module's state should be prepared before epoch updates.
x/oracle/keeper/msg_server_create_price.go (1)
76-79: Added secure price hashing for long values.SHA-256 hashing of long price strings (>=32 chars) with base64 encoding improves event handling and prevents potential issues with very long price values.
Consider adding a constant for the minimum length threshold:
+const minPriceLengthForHashing = 32 + -if len(priceStr) >= 32 { +if len(priceStr) >= minPriceLengthForHashing {x/oracle/types/params.go (1)
631-640: Review the new parameter update check methods.The new methods
IsForceSealingUpdateandIsSlashingResetUpdateimprove the clarity of parameter updates, but they should be documented.Consider adding documentation to explain:
- When these methods should be used
- What constitutes a "force sealing update"
- The implications of a "slashing reset update"
+// IsForceSealingUpdate checks if any consensus-critical parameters have been modified func (p Params) IsForceSealingUpdate(params *Params) bool { if p.MaxNonce != params.MaxNonce || p.MaxDetId != params.MaxDetId || p.ThresholdA != params.ThresholdA || p.ThresholdB != params.ThresholdB || p.Mode != params.Mode { return true } return false } +// IsSlashingResetUpdate checks if any slashing window parameters have been modified func (p Params) IsSlashingResetUpdate(params *Params) bool { if p.Slashing.ReportedRoundsWindow != params.Slashing.ReportedRoundsWindow || p.Slashing.MinReportedPerWindow != params.Slashing.MinReportedPerWindow { return true } return false }Also applies to: 642-648
x/oracle/keeper/native_token_test.go (2)
Line range hint
15-33: Enhance workflow documentation clarity.The workflow documentation could be improved by:
- Adding information about the Index field's role in balance tracking
- Maintaining consistent comment formatting
- Including expected state validations for each step
Line range hint
183-289: Add unit tests and documentation forconvertBalanceChangeToBytes.The helper function performs complex bit manipulation but lacks:
- Dedicated unit tests for edge cases
- Documentation explaining the encoding format
- Smaller, more maintainable sub-functions
Consider:
- Adding unit tests for edge cases (e.g., zero changes, maximum values)
- Breaking down the function into smaller, testable components
- Adding documentation explaining the bit manipulation logic
Would you like me to help create a test suite for this function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/oracle/types/price.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (65)
Makefile(2 hunks)app/ante/cosmos/sigverify.go(2 hunks)app/ante/cosmos/txsize_gas.go(1 hunks)app/app.go(1 hunks)precompiles/avs/events.go(1 hunks)tests/e2e/oracle/create_price.go(8 hunks)tests/e2e/oracle/helper_nstconvert.go(4 hunks)testutil/utils.go(1 hunks)x/avs/client/cli/tx.go(1 hunks)x/avs/keeper/keeper.go(2 hunks)x/evm/keeper/grpc_query.go(2 hunks)x/operator/keeper/slash.go(1 hunks)x/operator/types/keys.go(1 hunks)x/oracle/keeper/aggregator/aggregator.go(0 hunks)x/oracle/keeper/aggregator/aggregator_test.go(0 hunks)x/oracle/keeper/aggregator/calculator.go(0 hunks)x/oracle/keeper/aggregator/calculator_test.go(0 hunks)x/oracle/keeper/aggregator/context.go(0 hunks)x/oracle/keeper/aggregator/context_test.go(0 hunks)x/oracle/keeper/aggregator/filter.go(0 hunks)x/oracle/keeper/aggregator/filter_test.go(0 hunks)x/oracle/keeper/aggregator/helper_test.go(0 hunks)x/oracle/keeper/aggregator/info_test.go(0 hunks)x/oracle/keeper/aggregator/util.go(0 hunks)x/oracle/keeper/aggregator/worker.go(0 hunks)x/oracle/keeper/cache/caches.go(4 hunks)x/oracle/keeper/common/expected_keepers.go(2 hunks)x/oracle/keeper/feedermanagement/aggregator.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(1 hunks)x/oracle/keeper/feedermanagement/algo.go(1 hunks)x/oracle/keeper/feedermanagement/caches.go(1 hunks)x/oracle/keeper/feedermanagement/date_test.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager_test.go(1 hunks)x/oracle/keeper/feedermanagement/helper_test.go(1 hunks)x/oracle/keeper/feedermanagement/mock_cachereader_test.go(1 hunks)x/oracle/keeper/feedermanagement/prices.go(1 hunks)x/oracle/keeper/feedermanagement/round.go(1 hunks)x/oracle/keeper/feedermanagement/types.go(1 hunks)x/oracle/keeper/keeper.go(4 hunks)x/oracle/keeper/keeper_suite_test.go(2 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/msg_server_create_price_test.go(0 hunks)x/oracle/keeper/msg_server_update_params.go(1 hunks)x/oracle/keeper/native_token.go(2 hunks)x/oracle/keeper/native_token_test.go(4 hunks)x/oracle/keeper/nonce.go(1 hunks)x/oracle/keeper/params.go(4 hunks)x/oracle/keeper/params_test.go(1 hunks)x/oracle/keeper/prices.go(7 hunks)x/oracle/keeper/prices_test.go(1 hunks)x/oracle/keeper/recent_msg.go(1 hunks)x/oracle/keeper/recent_params.go(2 hunks)x/oracle/keeper/single.go(0 hunks)x/oracle/keeper/slashing.go(3 hunks)x/oracle/keeper/tokens.go(1 hunks)x/oracle/keeper/validate_timestamp.go(1 hunks)x/oracle/keeper/validator_update_block.go(1 hunks)x/oracle/module.go(1 hunks)x/oracle/module_beginblock.go(1 hunks)x/oracle/module_beginblock_devmode.go(1 hunks)x/oracle/types/errors.go(1 hunks)x/oracle/types/key_slashing.go(1 hunks)x/oracle/types/params.go(8 hunks)x/oracle/types/types.go(3 hunks)
💤 Files with no reviewable changes (14)
- x/oracle/keeper/single.go
- x/oracle/keeper/aggregator/filter_test.go
- x/oracle/keeper/aggregator/helper_test.go
- x/oracle/keeper/aggregator/info_test.go
- x/oracle/keeper/aggregator/context_test.go
- x/oracle/keeper/aggregator/aggregator_test.go
- x/oracle/keeper/aggregator/calculator_test.go
- x/oracle/keeper/msg_server_create_price_test.go
- x/oracle/keeper/aggregator/filter.go
- x/oracle/keeper/aggregator/util.go
- x/oracle/keeper/aggregator/aggregator.go
- x/oracle/keeper/aggregator/worker.go
- x/oracle/keeper/aggregator/calculator.go
- x/oracle/keeper/aggregator/context.go
✅ Files skipped from review due to trivial changes (8)
- x/operator/types/keys.go
- x/oracle/keeper/nonce.go
- x/oracle/keeper/tokens.go
- app/ante/cosmos/sigverify.go
- x/oracle/keeper/params_test.go
- x/operator/keeper/slash.go
- x/evm/keeper/grpc_query.go
- x/oracle/keeper/feedermanagement/mock_cachereader_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/oracle/keeper/native_token.go
🧰 Additional context used
📓 Learnings (1)
tests/e2e/oracle/create_price.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
🪛 GitHub Actions: Tests
tests/e2e/oracle/create_price.go
[error] 277-277: Test failure: Expected balance value 28, but got 32 in BalanceInfo comparison
x/oracle/keeper/feedermanagement/feedermanager_test.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/aggregator_test.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/algo.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/date_test.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/helper_test.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/round.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/prices.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/types.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/caches.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/feedermanager.go
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/aggregator.go
[warning] Low test coverage: 31.5% of statements covered
🪛 GitHub Check: CodeQL
x/oracle/keeper/feedermanagement/caches.go
[notice] 5-5: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 26-28: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 56-70: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 90-92: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 127-127: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 138-140: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 141-147: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 188-190: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/feedermanager.go
[warning] 42-42: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
[warning] 45-45: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 50-50: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 90-95: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 109-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 121-125: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 174-183: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 243-260: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 263-263: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 309-447: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 337-337: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/oracle/keeper/feedermanagement/aggregator.go
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 146-148: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 156-159: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 163-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 167-173: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 172-178: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 216-218: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 272-274: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinism
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (41)
x/oracle/types/types.go (3)
7-7: LGTM! Import added correctly.The SDK types import is properly added and follows Cosmos SDK module conventions.
51-51:⚠️ Potential issue
⚠️ Breaking Change: NSTIDPrefix modificationChanging the prefix from "NST" to "nst" is a breaking change that could affect existing data and API consumers. Please ensure this change is documented in the changelog and that migration steps are provided if necessary.
Let's verify the impact:
53-55: Verify the consistency of Chainlink source constantsThe new constants look good, but let's ensure they're used consistently across the codebase.
✅ Verification successful
Constants are consistent in production code
The TimeLayout constant is properly used in validate_timestamp.go, and the Chainlink source constants are referenced correctly in production code. There are some inconsistencies in test files where "Chainlink" is written with different casing, but this doesn't affect production functionality.
• Consider updating test files to use SourceChainlinkName constant instead of hardcoded strings for better maintainability:
- x/oracle/keeper/testdata/info.go
- x/oracle/keeper/cache/info_test.go
- x/oracle/keeper/msg_server_update_params_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any hardcoded "Chainlink" strings that should use the constant rg -i "\"chainlink\"" # Verify the source ID usage rg "SourceID.*1[^0-9]" # Check if the time layout matches existing time parsing ast-grep --pattern 'time.Parse($layout, $_)'Length of output: 2815
Script:
#!/bin/bash # Check for time-related function calls that might use the layout rg -A 1 "time\.(Parse|Format)" # Look for other time layout constants rg "Layout.*=.*\d{4}(-|\s)\d{2}(-|\s)\d{2}" # Check if there are other source-related constants ast-grep --pattern 'Source.*=.*[0-9]+'Length of output: 559
x/oracle/keeper/feedermanagement/algo.go (1)
1-132: Low test coverage for newfeedermanagementpackageThe new
feedermanagementpackage lacks unit tests, contributing to the low overall test coverage reported in the pipeline failures (31.5% of statements covered). To ensure the correctness and reliability of the new code, it's important to add unit tests covering the various methods and scenarios, including edge cases.Do you want me to help generate unit tests for the
AggMedianmethods to improve test coverage?🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/module_beginblock_devmode.go (1)
23-30: Ensure Recovery Logic Is RobustIn the
recoveryFeederManagerOnNextBlockfunction (lines 23-30), the code panics if recovery fails when the block height is greater than 1 (lines 26-27). Verify that the conditionctx.BlockHeight() > 1accurately represents the scenarios in which recovery should not fail. If there are valid cases where recovery might not succeed, consider handling those cases gracefully rather than panicking.x/oracle/keeper/feedermanagement/types.go (1)
87-87: Address TODO Comments Before MergingThere are TODO comments in the code (lines 87 and 97):
- Line 87:
// TODO: V2: accumulatedValidPower only includes validators who providing all sources required by rules(defined in oracle.Params)- Line 97:
// TODO: remove thisConsider addressing these TODOs or creating issues to track them before merging to maintain code clarity and project management.
Also applies to: 97-97
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/round.go (2)
64-64: Verify assumption about non-concurrent ABCI requestsThe comment mentions that "there's no race condition since abci requests are not executing concurrently." Please verify that this assumption holds true under all circumstances, especially if future updates introduce concurrency.
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
Line range hint
1-351: Increase test coverage for theroundmoduleThe pipeline indicates low test coverage (31.5%). To ensure robustness, please add unit tests for the
roundmodule, covering various scenarios and edge cases.Do you want me to help generate unit tests for this module?
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/caches.go (1)
1-363: Increase test coverage for thecachesmoduleThe pipeline reports low test coverage (31.5%). Adding unit tests for the
cachesmodule will help identify issues early and improve code reliability.Do you want me to help generate unit tests for this module?
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 5-5: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 26-28: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 56-70: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 90-92: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 127-127: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 138-140: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 141-147: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 188-190: Iteration over map
Iteration over map may be a possible source of non-determinism🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/prices.go (1)
1-336: Increase test coverage for thepricesmoduleThe pipeline indicates low test coverage (31.5%). Enhancing the test suite for the
pricesmodule will improve code quality and reliability.Do you want me to help generate unit tests for this module?
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/aggregator.go (1)
109-113: Address the TODO inexceedPowerLimitmethodThe
exceedPowerLimitfunction includes a TODO comment indicating future changes for version 2. The method currently may not consider all valid validators as per the rules defined inoracle.Params. This could lead to incorrect threshold calculations.Would you like assistance in updating the implementation according to the planned changes, or should I open a new GitHub issue to ensure this enhancement is tracked?
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/keeper/feedermanagement/feedermanager.go (2)
652-658: Increase unit test coverageThe test coverage is currently at 31.5%, which is below the desired threshold. Improving unit test coverage will enhance code reliability and prevent potential bugs.
Consider adding unit tests for critical functions such as
BeginBlock,EndBlock,ProcessQuote, and others introduced in this file.🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
90-95:⚠️ Potential issueMaintain deterministic order when processing validator updates
In the
updateAndCommitCachesmethod, iterating overvalidatorUpdateswithout a deterministic order may lead to non-deterministic behavior. Ensure that the processing order is consistent across nodes.Sort
validatorUpdatesbefore processing:+sort.SliceStable(validatorUpdates, func(i, j int) bool { + return validatorUpdates[i].Address < validatorUpdates[j].Address +}) for _, vu := range validatorUpdates { // Existing logic }This ensures that all nodes process validator updates in the same order.
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 90-95: Iteration over map
Iteration over map may be a possible source of non-determinism🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
x/oracle/module_beginblock.go (1)
5-13: No issues found inBeginBlockimplementationThe
BeginBlockmethod correctly callsam.keeper.BeginBlock(ctx)and adheres to the expected interface. There are no issues identified in this segment.x/oracle/keeper/keeper.go (1)
71-72: Verify potential circular dependency.The
SetKeepercall on the returned keeper instance could create a circular dependency. Please verify that this pattern is intentional and doesn't cause initialization issues.✅ Verification successful
Dependency injection pattern verified - no circular dependency issue.
The
SetKeeperimplementation in the FeederManager is a standard dependency injection pattern where the manager stores a reference to the keeper interface it needs. Theret.SetKeeper(ret)call simply provides this required dependency, and since it's interface-based, it doesn't create an actual circular dependency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SetKeeper implementation to understand the dependency chain ast-grep --pattern 'func (k Keeper) SetKeeper($_) { $$$ }'Length of output: 62
Script:
#!/bin/bash # Search for Keeper struct definition and SetKeeper implementations with various patterns echo "=== Searching for Keeper struct definition ===" ast-grep --pattern 'type Keeper struct { $$$ }' echo -e "\n=== Searching for SetKeeper with pointer receiver ===" ast-grep --pattern 'func (k *Keeper) SetKeeper($_) { $$$ }' echo -e "\n=== Searching for any SetKeeper method ===" rg "SetKeeper" -A 3Length of output: 7522
x/oracle/keeper/params.go (3)
71-75: LGTM! Good practice for parameter updates.The deferred function ensures that parameter updates are properly tracked when not in check transaction mode.
42-42: Verify the safety of type conversions.The
#nosec G115comments are suppressing Gosec warnings about integer conversions. While these conversions appear safe in the current context, we should document why they're safe to suppress.Run the following script to verify the value ranges:
Also applies to: 53-53
✅ Verification successful
Integer conversions are safe, but documentation needed
The integer conversions are safe because they only involve array indices and lengths which are guaranteed to be non-negative and well within uint64 range. However, we should add a comment explaining why the
#nosec G115suppression is safe:// #nosec G115 -- Safe to disable gosec warning as conversion is from array index/length // which is guaranteed to be non-negative and cannot realistically exceed uint64🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential integer overflow scenarios in the codebase # Look for places where these IDs are set or modified to ensure they can't exceed uint64 # Search for assignments to chainID rg -A 3 'chainID\s*[:=]' # Search for modifications to p.Chains rg -A 3 'p\.Chains.*append'Length of output: 13938
100-104: Document the assumptions about token array length.The comment states that
len(p.Tokens)must be positive, but this assumption should be verified.Run the following script to verify the assumption:
✅ Verification successful
The assumption about token array length is valid and safe.
The array access is safe because:
- The TokenID assignment happens immediately after appending to p.Tokens
- All operations on p.Tokens are append-only with no removals
- The pattern used is a common way to get the index of a just-appended element
The existing comment accurately documents this invariant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any places where p.Tokens could be empty # Look for places where tokens are removed or the array is reset # Search for modifications to p.Tokens rg -A 3 'p\.Tokens\s*=' # Search for removals from p.Tokens rg -A 3 'p\.Tokens.*remove'Length of output: 959
Script:
#!/bin/bash # Check initialization of Params struct and p.Tokens ast-grep --pattern 'type Params struct { $$$ }' # Check for NewParams or default params initialization rg -A 5 'func New.*Params'Length of output: 15602
x/oracle/keeper/recent_params.go (1)
9-30: Add error handling and validation.The function modifies state without proper error handling and validation:
- Block height conversion to uint64 should be checked for overflow
- The assumption about force sealing of rounds should be validated
- Consider adding error return for state modifications
Run this script to verify the assumption about force sealing:
app/ante/cosmos/txsize_gas.go (1)
79-86: LGTM! Proper security annotations for simulation mode.The changes correctly handle gas estimation for signatures in simulation mode with appropriate security annotations.
x/avs/client/cli/tx.go (1)
119-119: Verify impact of phase type change.The change from
uint32toint32for phase could affect existing commands and stored data.Run this script to check for potential impacts:
✅ Verification successful
Phase type change is consistent with codebase
The change from
uint32toint32aligns with the existing type system wherePhaseis already defined asint32in the protobuf generated code. The validation ensures safe value ranges, and the change maintains compatibility with the contract interface where phase values are small (1 and 2).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for phase usage in commands and types rg -A 3 "Phase.*uint32|uint32.*Phase"Length of output: 39
Script:
#!/bin/bash # Search for phase-related types and usage with broader patterns echo "=== Searching for phase type declarations ===" ast-grep --pattern 'type $_ struct { $$$ Phase $_ $$$ }' echo -e "\n=== Searching for phase usage in functions and methods ===" rg -i "phase.*int32|int32.*phase" -A 2 echo -e "\n=== Searching for phase in command handlers ===" rg -i "func.*phase" -A 3 echo -e "\n=== Checking phase in storage or state ===" fd "store|keeper|state" --type f --exec rg -l "phase" {}Length of output: 20592
x/oracle/keeper/cache/caches.go (2)
125-127: Verify the impact of the condition change.The condition for retaining recent parameters has been changed from
>to>=. This change includes the boundary case whereb == block-uint64(common.MaxNonce), which could affect parameter caching behavior.Consider documenting why this boundary case needs to be included and verify that this change doesn't cause any issues with parameter retention.
56-56: Security linting suppressions need justification.Multiple
#nosec G115comments have been added to suppress security linting warnings about potential negative values. While the comments indicate that these values cannot be negative, it would be better to add explicit validation to ensure this assumption holds true.Also applies to: 73-73, 119-119, 125-125
✅ Verification successful
Security linting suppressions are justified.
The
#nosec G115comments are correctly suppressing false positives. The block height fromsdk.Context.BlockHeight()is guaranteed to be non-negative as it starts from 0, and the conversion touint64is safe in this context. The values are used appropriately in comparisons with otheruint64values for tracking historical data.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing validation of block height # Test: Search for validation of block height before usage rg -A 5 'BlockHeight\(\)' | rg 'if.*< 0|if.*>= 0'Length of output: 51
Script:
#!/bin/bash # Find the BlockHeight method definition and usage ast-grep --pattern 'func $_($_) BlockHeight() $_' # Find where BlockHeight is used in conditions or assignments rg -B 2 -A 2 'BlockHeight\(\)' # Look for the specific file content to understand the context cat x/oracle/keeper/cache/caches.goLength of output: 54168
x/oracle/keeper/slashing.go (3)
29-37: LGTM! Well-structured cleanup method.The new
ClearAllValidatorReportInfomethod properly handles cleanup with appropriate store prefix and iterator management.
Line range hint
131-141: Improved prefix specificity in iterator usage.The change to use
SlashingMissedBitArrayValidatorPrefixinstead of the previous prefix provides better namespacing and clarity.
Line range hint
165-173: LGTM! Comprehensive cleanup implementation.The new
ClearAllValidatorMissedRoundBitArraymethod properly handles the cleanup of all missed round data with appropriate iterator management.x/oracle/keeper/feedermanagement/date_test.go (1)
5-52: 🛠️ Refactor suggestionDocument test data structures and improve test coverage.
The test data is well-structured but lacks documentation explaining the purpose of each test case. Additionally, there appears to be some redundancy in the test data (e.g., similar price sources with minor variations).
The pipeline reports low test coverage (31.5%). Consider:
- Adding documentation for each test case
- Removing redundant test cases
- Adding more test cases to improve coverage
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
precompiles/avs/events.go (1)
274-276: 🛠️ Refactor suggestionConsider addressing type mismatch at definition level.
Instead of casting
Phasetouint8at usage and adding a TODO comment, it would be better to:
- Define
Phaseasuint8in the type definition- Update all related code to use the correct type
- Add validation for the Phase value range
This would prevent potential issues with type conversion and make the code more type-safe.
x/oracle/keeper/prices.go (2)
Line range hint
229-242: Breaking change in GrowRoundID return type.The function signature has changed from returning
*types.PriceTimeRoundtostring. This is a breaking change that could affect dependent code.
37-44: Verify the bounds check for MaxSizePrices.The security comment
#nosec G11suggests a false positive from static analysis. However, we should still verify the bounds check logic.✅ Verification successful
Bounds check for MaxSizePrices is properly implemented
The implementation is safe because:
- MaxSizePrices is a small constant (100)
- Proper validation ensures MaxSizePrices > 1
- Arithmetic operations are guarded by bounds checking
The #nosec G11 comments are justified, and the static analysis warning can be safely ignored.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify MaxSizePrices usage and potential integer overflow ast-grep --pattern 'if $_ <= uint64($_) { $$$ }'Length of output: 557
Script:
#!/bin/bash # Find MaxSizePrices definition and usage rg "MaxSizePrices" -A 2Length of output: 4346
x/oracle/keeper/feedermanagement/aggregator_test.go (1)
1-313: Improve test coverage.While the test cases are well-structured using goconvey, the overall test coverage is only 31.5%. Additional test cases are needed.
Key areas needing coverage:
- Error paths
- Edge cases
- Concurrent operations
Would you like me to help generate additional test cases to improve coverage?
🧰 Tools
🪛 GitHub Actions: Tests
[warning] Low test coverage: 31.5% of statements covered
testutil/utils.go (1)
507-507: LGTM! Proper initialization of FeederManager in tests.The addition of
FeederManager.BeginBlockensures proper initialization of the oracle module's feeder management system during test setup.Let's verify the test coverage for FeederManager initialization:
✅ Verification successful
✅ Verified: FeederManager.BeginBlock initialization is consistent with existing test patterns
The addition of
FeederManager.BeginBlockinDoSetupTestaligns with existing test patterns, as evidenced by similar initializations inprices_test.goandkeeper_suite_test.go. This change ensures consistent FeederManager initialization across all tests using the common setup utility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for FeederManager initialization # Test: Search for FeederManager.BeginBlock test cases rg -A 5 "FeederManager.*BeginBlock.*test"Length of output: 43
Script:
#!/bin/bash # Search for FeederManager usage in test files echo "=== FeederManager usage in tests ===" rg -l "FeederManager" --type-add 'test:*_test.go' -t test echo -e "\n=== BeginBlock patterns in oracle tests ===" rg "BeginBlock" x/oracle/**/*_test.go echo -e "\n=== Test setup patterns ===" fd -e go -E vendor --exec grep -l "DoSetupTest" {}Length of output: 2229
x/oracle/keeper/msg_server_create_price.go (4)
31-34: Enhanced logging for better observability.Improved logging with structured fields provides better context for debugging and monitoring.
41-44: Added message validation before processing.Early validation prevents unnecessary processing of invalid messages.
48-54: Improved error handling with specific cases.Better error handling distinguishes between:
- Successfully recorded quotes (even if window is closed)
- Processing errors
This provides clearer feedback and better error tracking.
82-88: Updated event emission with more context.The event now includes:
- Round ID as a separate attribute
- Final price with token ID, round ID, price, and decimal combined
- Success indicator
This provides more comprehensive information for event consumers.
x/oracle/module.go (1)
150-150: Verify the keeper's EndBlock implementation.The complex validator performance tracking logic has been moved from the module's
EndBlockto the keeper. While this improves modularity, we need to ensure that all the previous functionality is preserved.Run the following script to verify the keeper's
EndBlockimplementation:✅ Verification successful
Verification successful: Validator performance tracking logic properly migrated to keeper
The validator performance tracking logic has been successfully migrated from the module's EndBlock to the keeper layer, specifically within the FeederManager component. The implementation preserves all critical functionality:
- Validator report info tracking and performance windows
- Missed rounds counting and thresholds
- Slashing and jailing mechanisms for missed reports and malicious behavior
- Proper state management and genesis handling
The migration improves modularity by encapsulating the logic within the keeper while maintaining all the original functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the keeper's EndBlock implementation preserves all the validator performance tracking logic. # Test: Search for the keeper's EndBlock implementation ast-grep --pattern $'func \(k \*Keeper\) EndBlock\(ctx sdk.Context\) { $$$ }' # Test: Search for any validator performance tracking logic in the keeper rg -A 5 'validator|performance|tracking|slashing|jailing'Length of output: 68473
Script:
#!/bin/bash # Find the keeper's EndBlock implementation fd endblock --type f --exec rg -l 'func.*EndBlock' # Check git history for the migration changes git log -p --all --full-history -S "EndBlock" -- "x/oracle/module.go" "x/oracle/keeper/"Length of output: 52
Script:
#!/bin/bash # Search for EndBlock implementations ast-grep --pattern 'func ($k *Keeper) EndBlock($ctx sdk.Context) { $$$ }' ast-grep --pattern 'func ($am AppModule) EndBlock($ctx sdk.Context) $ret { $$$ }' # Search for validator performance tracking code rg -B 5 -A 10 'ValidatorReportInfo|MissedRounds|performance|tracking' x/oracle/Length of output: 68197
x/oracle/types/params.go (2)
98-98: Review the significant reduction in StartBaseBlock.The
StartBaseBlockhas been reduced from 1,000,000 to 20. This is a substantial change that could affect the initialization timing of token feeders.Please confirm:
- Why was this value reduced so significantly?
- Are there any implications for existing token feeders?
- How does this affect the synchronization of price feeds?
63-63:⚠️ Potential issueVerify the impact of ETH decimal change.
The decimal places for ETH have been reduced from 18 to 8. This is a significant change that could affect price calculations and existing integrations.
Run the following script to verify the impact:
✅ Verification successful
Decimal change from 18 to 8 is safe
The codebase properly handles decimal precision changes through:
- Dynamic decimal handling in price calculations
- Automatic price normalization across different decimals
- Proper decimal scaling in USD value computations
- No hardcoded assumptions about 18 decimals in core logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded assumptions about ETH decimals in the codebase. # Test: Search for any hardcoded ETH decimal values rg -A 5 '18|1e18|10\*\*18|10\^\{?18\}?' # Test: Search for price calculation logic that might be affected ast-grep --pattern $'func $$$Price($$$) { $$$ }'Length of output: 69152
Script:
#!/bin/bash # Search for price conversion and decimal-dependent calculations # Look for price conversion/scaling logic rg -A 5 'convert|scale|multiply|divide|decimals?' --type go # Search for potential decimal usage in calculations rg -A 5 '10\s*\*\*|[Pp]ow|[Mm]ul|[Dd]iv|[Ss]cale' --type go # Look for integration points and configurations rg -A 5 'decimal|precision|places?' --type goLength of output: 69173
Makefile (1)
315-315: Document the purpose of the devmode tag.The
devmodetag has been added to test targets. This could affect test behavior, but its purpose is not documented.Please clarify:
- What functionality does the
devmodetag enable/disable?- Should this tag be used in production builds?
- Are there any test cases that specifically depend on this tag?
Run the following to check its usage:
Also applies to: 322-322
✅ Verification successful
The
devmodetag enables development-time validation and safety checksThe
devmodetag enables additional validation logic in the oracle module's BeginBlock operation:
- Verifies FeederManager recovery functionality
- Includes extra consistency checks
- Fails fast with panics when inconsistencies are detected
This tag should not be used in production builds as it includes development-specific validation code with panic conditions. It's specifically designed for catching potential issues during development and testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the devmode tag is used in the codebase # Test: Search for build tag declarations rg -A 5 '//\s*\+build\s+.*devmode' # Test: Search for conditional compilation blocks rg -A 5 'if\s+.*devmode'Length of output: 63
Script:
#!/bin/bash # Search for any mention of devmode in Go files and documentation echo "=== Searching for devmode in Go files ===" fd -e go . -x rg -i "devmode" echo -e "\n=== Searching for devmode in documentation ===" fd -e md -e txt . -x rg -i "devmode" echo -e "\n=== Searching for build tags in Go files ===" fd -e go . -x rg "^//\s*\+build" echo -e "\n=== Searching specifically in test files ===" fd -e go -g "*_test.go" . -x rg -i "devmode"Length of output: 597
Script:
#!/bin/bash # Get more context around build tags with file paths echo "=== Files and context for devmode build tags ===" rg -C 10 "^//go:build.*devmode" --with-filename echo -e "\n=== Files and context for build tags (alternative syntax) ===" rg -C 10 "^// \+build.*devmode" --with-filenameLength of output: 1668
Script:
#!/bin/bash # Get the complete implementation of both files echo "=== Development mode implementation ===" cat x/oracle/module_beginblock_devmode.go echo -e "\n=== Production implementation ===" cat x/oracle/module_beginblock.goLength of output: 1560
x/oracle/keeper/native_token_test.go (1)
86-86: LGTM! Sequential balance tracking implementation.The new Index field correctly tracks balance changes in sequential order, providing a clear history of balance modifications.
Let's verify that the Index values are always sequential and never reset:
Also applies to: 125-125, 145-145, 166-166
✅ Verification successful
Index values are correctly implemented to be sequential
The codebase ensures Index values are always sequential by incrementing the previous balance's Index value. The implementation in
x/oracle/keeper/native_token.goguarantees this by:newBalance.Index++🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any Index assignments in balance-related code rg -A 2 'Index.*=.*\d+' --type goLength of output: 16718
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
x/oracle/keeper/feedermanagement/caches.go (3)
7-7:⚠️ Potential issueAvoid importing the
reflectpackage in production codeUsing the
reflectpackage in production blockchain code can lead to non-deterministic behavior, which is undesirable in consensus-critical applications. It's recommended to implement manual field-by-field comparisons to ensure deterministic execution.Note: This recommendation is based on previous learnings specific to the Exocore project regarding the use of the
reflectpackage.
114-115:⚠️ Potential issueAvoid using
reflect.DeepEqualfor equality checks in production codeThe use of
reflect.DeepEqualcan introduce non-deterministic behavior in blockchain applications. Consider implementing manual field-by-field comparisons for theEqualsmethod to ensure deterministic execution.Note: This recommendation aligns with the project's guidelines to avoid reflection in production code.
229-229:⚠️ Potential issueReplace
reflect.DeepEqualwith manual comparisonsIn the
Equalsmethod, usingreflect.DeepEqualon parameters may lead to non-deterministic behavior. Implementing manual field-by-field comparisons for parameters helps maintain deterministic execution in the blockchain environment.Note: Consistent with previous advice, avoiding reflection ensures better determinism.
x/oracle/keeper/feedermanagement/aggregator.go (3)
83-85:⚠️ Potential issueImplement the
RecordMsgmethodThe
RecordMsgmethod currently contains a TODO comment and is not fully implemented. This could lead to issues when this method is called.Do you want me to help implement this method or open a new GitHub issue to track this task?
142-146:⚠️ Potential issueEnsure deterministic iteration over maps in
EqualsmethodIn the
Equalsmethod ofrecordsValidators, iterating over the maprv.recordswithout ordering may lead to non-deterministic behavior in a blockchain context.Consider sorting the keys before iteration to ensure deterministic execution. Here's a suggested refactor:
for k, v := range rv.records { + keys := make([]string, 0, len(rv.records)) + for k := range rv.records { + keys = append(keys, k) + } + slices.Sort(keys) + for _, k := range keys { + v := rv.records[k] if v2, ok := rv2.records[k]; !ok || !v.Equals(v2) { return false } }Please review other instances of map iteration in this file.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
438-454:⚠️ Potential issueEnsure deterministic aggregation in
GetFinalPriceofrecordsDSIn
recordsDS.GetFinalPrice, iterating overrds.recordsin reverse order may still result in non-deterministic behavior ifrds.recordswas constructed from map data.Verify that
rds.recordsis built deterministically. If not, consider sorting the slice based on a consistent criterion before iteration.
🧹 Nitpick comments (13)
x/oracle/keeper/feedermanagement/prices.go (3)
43-60: Document security suppression reasonsThe function contains multiple
#nosec G115comments suppressing security linting without explaining why these suppressions are safe. Please add comments explaining the rationale for each suppression.Example:
- // #nosec G115 + // #nosec G115 -- Safe to use int64 for sourceID as it's validated by the protocol deterministic, err := checker.IsDeterministic(int64(ps.SourceID))
154-154: Address TODO comment about validator source rulesThe TODO comment indicates missing validation for required sources. This validation is important for ensuring data integrity.
Would you like me to help implement the validation logic or create a GitHub issue to track this task?
162-166: Ensure deterministic map iterationThe code correctly handles non-deterministic map iteration by collecting keys into a slice and sorting them. However, consider extracting this pattern into a helper function for reuse.
Example:
+func getSortedKeys[K constraints.Ordered, V any](m map[K]V) []K { + keys := make([]K, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} func (pv *priceValidator) GetFinalPrice(algo AggAlgorithm) (*PriceResult, bool) { - keySlice := make([]int64, 0, len(pv.priceSources)) - for sourceID := range pv.priceSources { - keySlice = append(keySlice, sourceID) - } - slices.Sort(keySlice) + keySlice := getSortedKeys(pv.priceSources)🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 162-164: Iteration over map
Iteration over map may be a possible source of non-determinismx/oracle/keeper/feedermanagement/caches.go (2)
259-259: Fix typos in error messageThe error message contains typos: "unsuppported caceh type" should be "unsupported cache type".
Apply this diff to correct the typos:
- return fmt.Errorf("unsuppported caceh type: %T", i) + return fmt.Errorf("unsupported cache type: %T", i)
338-338: Correct typo in commentIn the comment, "updage" should be "update". Also, consider improving the phrasing for clarity.
Apply this diff to correct the typo and enhance readability:
-// SkipCommit skip real commit by setting the updage flag to false +// SkipCommit skips the real commit by setting the update flag to falsex/oracle/keeper/feedermanagement/types.go (3)
87-87: Fix typo in comment: 'prividing' should be 'providing'The comment in line 87 contains a typo. 'prividing' should be 'providing'.
Apply this diff to correct the typo:
-// TODO: V2: accumulatedValidPower only includes validators who prividing all sources required by rules(defined in oracle.Params) +// TODO: V2: accumulatedValidPower only includes validators who providing all sources required by rules(defined in oracle.Params)
94-94: Fix typo in comment: 'deteministic' should be 'deterministic'There's a typo in the comment on line 94. 'deteministic' should be 'deterministic'.
Apply this diff to correct the typo:
-// price records for deteministic source +// price records for deterministic source
128-134: Consider renaming methodCpytoCopyfor clarityThe method
Cpyin thethresholdstruct could be renamed toCopyto improve readability and adhere to standard naming conventions.Apply this diff to rename the method:
-func (t *threshold) Cpy() *threshold { +func (t *threshold) Copy() *threshold {Remember to update all references to this method throughout the codebase.
x/oracle/keeper/feedermanagement/round.go (3)
64-64: Typographical error in commentIn the comment on line 64, the word "concurrntly" should be corrected to "concurrently".
109-109: Typographical error in commentIn the comment on line 109, "handlQuotingMisBehavior" should be corrected to "handleQuotingMisbehavior".
97-97: Address the TODO comment inTallymethodThere is a TODO comment indicating that a valid value should be used instead of the original
protoMsgin the return statement of theTallymethod. Addressing this will improve the correctness and clarity of the method's output.Do you want me to help implement this modification or open a GitHub issue to track this task?
x/oracle/keeper/feedermanagement/feedermanager.go (2)
564-568: Address TODO comment in ValidateMsgThe TODO comment indicates incomplete implementation of message validation.
Would you like me to help implement the missing validation logic or create a GitHub issue to track this task?
727-805: Consider additional safeguards in recovery processThe recovery process handles complex state restoration but could benefit from:
- Transaction isolation during recovery
- Progress logging for long-running recovery
- Timeout mechanism for safety
Consider implementing these improvements to enhance recovery reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/test.yml(1 hunks)Makefile(3 hunks)tests/e2e/oracle/create_price.go(13 hunks)tests/e2e/oracle/data.go(1 hunks)tests/e2e/oracle/helper_nstconvert.go(4 hunks)x/oracle/keeper/feedermanagement/aggregator.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(1 hunks)x/oracle/keeper/feedermanagement/algo.go(1 hunks)x/oracle/keeper/feedermanagement/caches.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager_test.go(1 hunks)x/oracle/keeper/feedermanagement/helper_test.go(1 hunks)x/oracle/keeper/feedermanagement/mock_cachereader_test.go(1 hunks)x/oracle/keeper/feedermanagement/prices.go(1 hunks)x/oracle/keeper/feedermanagement/round.go(1 hunks)x/oracle/keeper/feedermanagement/types.go(1 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/validate_timestamp.go(1 hunks)x/oracle/module_beginblock_devmode.go(1 hunks)x/oracle/types/params.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/oracle/helper_nstconvert.go
- x/oracle/keeper/validate_timestamp.go
- tests/e2e/oracle/create_price.go
- x/oracle/keeper/feedermanagement/aggregator_test.go
- x/oracle/module_beginblock_devmode.go
- x/oracle/keeper/feedermanagement/algo.go
- x/oracle/keeper/feedermanagement/helper_test.go
- x/oracle/keeper/feedermanagement/mock_cachereader_test.go
- x/oracle/types/params.go
🧰 Additional context used
📓 Learnings (2)
tests/e2e/oracle/data.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#233
File: tests/e2e/oracle/create_price.go:37-39
Timestamp: 2024-11-16T11:07:01.278Z
Learning: In `tests/e2e/oracle/create_price.go`, variables like `price1` and `price2` may be defined in other files such as `tests/e2e/oracle/data.go` within the same PR. Ensure to check for variable definitions across all related files in the PR before flagging them as undefined.
x/oracle/keeper/feedermanagement/caches.go (4)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/caches.go:6-6
Timestamp: 2025-01-13T11:19:43.134Z
Learning: In the Exocore project, avoid using the `reflect` package in production blockchain code as it can lead to non-deterministic behavior. Instead, implement manual field-by-field comparisons for equality checks.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/caches.go:6-6
Timestamp: 2025-01-13T11:19:43.134Z
Learning: In the Exocore project, code that uses potentially non-deterministic features like reflection should be guarded with `//go:build devmode` build tags to ensure it's only included in development/testing builds.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/caches.go:27-29
Timestamp: 2025-01-13T11:19:20.811Z
Learning: When copying maps in Go, the iteration order doesn't need to be deterministic as long as all key-value pairs are preserved in the copy operation. The final map state will be identical regardless of iteration order.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/caches.go:27-29
Timestamp: 2025-01-13T11:19:21.379Z
Learning: Map iteration order only needs to be deterministic when it affects consensus, state transitions, or outputs that must be consistent across nodes. For operations like map copying where each key-value pair is handled independently (e.g., deep copying values), the iteration order does not matter.
🪛 actionlint (1.7.4)
.github/workflows/test.yml
50-50: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Check: CodeQL
x/oracle/keeper/feedermanagement/aggregator.go
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 156-159: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 163-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 172-178: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 216-218: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 223-225: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 248-254: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 272-274: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/prices.go
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 162-164: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/feedermanager.go
[warning] 45-45: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 50-50: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 90-95: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 109-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 121-125: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 174-183: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 243-260: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 263-263: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 309-447: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 378-382: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 385-389: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 400-403: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 453-457: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 603-605: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 623-625: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 671-671: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 717-723: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (24)
.github/workflows/test.yml (1)
47-64: LGTM! Well-structured end-to-end test jobThe new
test-unit-e2ejob is well integrated with the existing workflow, following the same patterns for consistency and reusing the same environment setup and file pattern filtering.🧰 Tools
🪛 actionlint (1.7.4)
50-50: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Makefile (2)
189-189: LGTM! Correctly added build to .PHONY targetsAdding
buildto.PHONYensures the target always runs regardless of file presence, which is the correct behavior for a build target.
308-309: LGTM! Well-structured test configurationThe changes effectively:
- Separate unit and e2e test packages
- Consistently apply the
devmodetag across test targets- Maintain uniform timeout values
Also applies to: 316-316, 323-323, 326-328
x/oracle/keeper/feedermanagement/prices.go (4)
217-228: Replace reflect.DeepEqual with explicit comparisonsUsing
reflect.DeepEqualcan lead to non-deterministic behavior. Implement explicit comparison functions.
307-309: Use stable sorting for deterministic resultsReplace
sort.Slicewithsort.SliceStableto ensure consistent ordering of equal elements.
320-320: Replace reflect.DeepEqual with explicit price comparisonUsing
reflect.DeepEqualfor price comparison can lead to non-deterministic behavior.
334-345: LGTM! Well-implemented deep copyThe deep copy implementation correctly handles all fields including the big.Int power value.
x/oracle/keeper/feedermanagement/caches.go (2)
66-71: Verify the correctness offeederIDbounds checkingIn the function
GetDecimalFromFeederID, the bounds check usesfeederID <= 0 || feederID > uint64(len(p.TokenFeeders)). Ensure thatfeederIDaligns correctly with the zero-based indexing of theTokenFeedersslice to prevent off-by-one errors. IffeederIDis intended to be 1-based, verify that all references and usages are consistent across the codebase.
331-333: ConfirmfeederIDindexing inTokenFeederssliceIn
GetTokenFeederForFeederID, the conditionint64(len(c.params.params.TokenFeeders)) > feederIDprecedes accessingc.params.params.TokenFeeders[feederID]. To avoid potential out-of-bounds panics, ensure thatfeederIDis within the valid range and that the indexing correctly matches the slice's zero-based indexing. Consistency infeederIDusage is crucial for reliable operation.x/oracle/keeper/feedermanagement/types.go (1)
216-216: Verify the use of self-referential type infCheckTxThe
FeederManagerstruct has a fieldfCheckTxof type*FeederManager, which is a pointer to the same struct type. Please verify that this self-referential type is necessary and does not introduce unintended complexities, such as circular dependencies or memory leaks.x/oracle/keeper/feedermanagement/aggregator.go (4)
13-13: Handle errors returned byIsDeterministicmethodThe
IsDeterministicmethod in thesourceCheckerinterface now returns an error. Ensure that all implementations handle this error appropriately to prevent unexpected behavior.
6-6: Review import ofreflectpackage for non-determinismImporting
"reflect"may introduce non-determinism due to its introspective capabilities.Ensure that all usages of
reflectdo not lead to non-deterministic behavior critical in a blockchain environment. If possible, minimize or eliminate reliance onreflectfor operations that affect consensus.🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
248-254:⚠️ Potential issueEnsure deterministic processing of validators in
GetFinalPriceForValidatorsIn
GetFinalPriceForValidators, iterating overrv.recordswithout a defined order may cause non-deterministic behavior.Modify the code to process validators in a sorted order:
ret := make(map[string]*PriceResult) -for validator, pv := range rv.records { +keys := make([]string, 0, len(rv.records)) +for validator := range rv.records { + keys = append(keys, validator) +} +slices.Sort(keys) +for _, validator := range keys { + pv := rv.records[validator] finalPrice, ok := pv.GetFinalPrice(algo) if !ok { return nil, false } ret[validator] = finalPrice }This ensures consistent behavior across nodes.
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 248-254: Iteration over map
Iteration over map may be a possible source of non-determinism
289-293:⚠️ Potential issueEnsure deterministic iteration over
dsMapinEqualsmethod ofrecordsDSsIn
recordsDSs.Equals, iterating over the maprdss.dsMapwithout ordering can lead to non-deterministic behavior.Refactor to iterate over sorted keys:
if len(rdss.dsMap) != len(rdss2.dsMap) { return false } -for k, v := range rdss.dsMap { +keys := maps.Keys(rdss.dsMap) +slices.Sort(keys) +for _, k := range keys { + v := rdss.dsMap[k] if v2, ok := rdss2.dsMap[k]; !ok || !v.Equals(v2) { return false } }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinismtests/e2e/oracle/data.go (1)
64-65: Verify changes toPricevalues andDecimalprecisionThe
Pricevalues forprice1andprice2have been changed to "19" and "29" respectively, and theDecimalhas been reduced from 18 to 8. This significantly alters the actual price values. Please verify that this change is intentional and update any dependent calculations or tests accordingly to prevent inaccuracies.Also applies to: 69-70
x/oracle/keeper/msg_server_create_price.go (4)
41-44: LGTM! Improved validation with clear error handlingThe addition of
ValidateMsgwith specific error logging enhances the robustness of the price creation process.
48-53: LGTM! Enhanced error handling for recorded quotesThe code now properly distinguishes between a recorded quote and processing errors, with appropriate logging for each case.
82-87: LGTM! Clear and consistent event emissionThe event emission is well-structured with all necessary attributes, maintaining backward compatibility.
18-18: Verify the impact of increased maxFutureOffsetThe
maxFutureOffsethas been increased significantly from 5 seconds to 30 seconds. This change could impact the timing validation of price quotes.Run this script to check for any timing-related issues in the codebase:
✅ Verification successful
Increased maxFutureOffset is safe and well-implemented
The change from 5s to 30s only affects the future timestamp validation in
checkTimestamp, providing more tolerance for network latency and clock skew between nodes. The validation logic remains robust and properly enforced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timing-related constants and validations rg -A 3 "maxFutureOffset|checkTimestamp|ValidateTimestamp"Length of output: 1353
x/oracle/keeper/feedermanagement/feedermanager.go (5)
19-26: LGTM! Clean initialization of FeederManagerThe constructor properly initializes all required fields with appropriate default values.
625-666: LGTM! Robust error handling in ProcessQuoteThe function implements comprehensive error handling with proper validation and state management.
316-322:⚠️ Potential issueEnsure deterministic iteration over validators
The iteration over validators in
handleQuotingMisBehaviorcould be non-deterministic.Apply this diff:
+ // Get sorted list of validators for deterministic iteration + validatorsList := make([]string, 0, len(validators)) + for v := range validators { + validatorsList = append(validatorsList, v) + } + sort.Strings(validatorsList) - for _, validator := range validators { + for _, validator := range validatorsList { // Existing logic }Likely invalid or redundant comment.
734-738:⚠️ Potential issueReplace panic with graceful error handling
Panicking in consensus methods can halt the blockchain. Instead, handle the error gracefully.
Apply this diff:
validatorUpdateBlock, found := f.k.GetValidatorUpdateBlock(ctx) if !found { - panic("validator update block not found in recovery mode for feeder manager") + f.k.Logger(ctx).Error("validator update block not found in recovery mode for feeder manager") + return false }Likely invalid or redundant comment.
716-725:⚠️ Potential issueEnsure deterministic initialization of validatorPowers
The initialization of
validatorPowersmay be non-deterministic due to map iteration. This could lead to consensus issues.Apply this diff to ensure deterministic initialization:
func (f *FeederManager) initCaches(ctx sdk.Context) { f.cs = newCaches() params := f.k.GetParams(ctx) validatorSet := f.k.GetAllExocoreValidators(ctx) + // Sort validators for deterministic iteration + sort.SliceStable(validatorSet, func(i, j int) bool { + return bytes.Compare(validatorSet[i].Address, validatorSet[j].Address) < 0 + }) validatorPowers := make(map[string]*big.Int) for _, v := range validatorSet { validatorPowers[sdk.ConsAddress(v.Address).String()] = big.NewInt(v.Power) } f.cs.Init(f.k, ¶ms, validatorPowers) }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 717-723: Iteration over map
Iteration over map may be a possible source of non-determinism
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
x/oracle/keeper/feedermanagement/prices.go (1)
216-239:⚠️ Potential issueEnsure deterministic behavior in map comparisons
Replace
reflect.DeepEqualwith explicit field comparisons to ensure deterministic behavior. This is especially important in blockchain contexts where consistency across nodes is crucial.
🧹 Nitpick comments (15)
x/oracle/keeper/feedermanagement/types.go (6)
12-25: Add interface documentation.While the interfaces are well-designed, they lack documentation. Consider adding GoDoc comments to describe the purpose and contract of each interface and its methods.
Add documentation like this:
+// Submitter defines the interface for caching oracle state updates type Submitter interface { SetValidatorUpdateForCache(sdk.Context, oracletypes.ValidatorUpdateBlock) SetParamsForCache(sdk.Context, oracletypes.RecentParams) SetMsgItemsForCache(sdk.Context, oracletypes.RecentMsg) } +// CacheReader defines the interface for reading cached oracle state type CacheReader interface { GetPowerForValidator(validator string) (*big.Int, bool) GetTotalPower() (totalPower *big.Int) GetValidators() []string IsRuleV1(feederID int64) bool IsDeterministic(sourceID int64) (bool, error) GetThreshold() *threshold }
87-89: Address TODO comment regarding validator power accumulation.The TODO comment indicates a planned change for V2 regarding accumulated validator power. This should be tracked and implemented.
Would you like me to create a GitHub issue to track this V2 enhancement?
112-138: Add documentation for threshold calculations.The threshold type and its methods are well-implemented but would benefit from documentation explaining:
- The purpose of thresholdA and thresholdB
- The mathematical relationship in the Exceeds method
Add documentation like this:
+// threshold represents power thresholds for oracle validation type threshold struct { + // totalPower represents the total voting power totalPower *big.Int + // thresholdA and thresholdB define the ratio for power threshold checks thresholdA *big.Int thresholdB *big.Int }
148-155: Enhance type safety for round status.Consider using a custom type for round status to prevent invalid values and improve type safety.
Apply this diff:
-type roundStatus int32 +type roundStatus uint8 const ( - // define closed as default value 0 - roundStatusClosed roundStatus = iota - roundStatusOpen - roundStatusCommittable + roundStatusClosed roundStatus = 1 + iota + roundStatusOpen + roundStatusCommittable + + roundStatusInvalid roundStatus = 0 )
192-198: Optimize slice operations.The
addmethod could be optimized by pre-allocating the slice and using binary search for insertion.Consider this optimization:
func (osi *orderedSliceInt64) add(i int64) { - result := append(*osi, i) - sort.Slice(result, func(i, j int) bool { - return result[i] < result[j] - }) - *osi = result + idx := sort.Search(len(*osi), func(j int) bool { + return (*osi)[j] >= i + }) + if idx < len(*osi) && (*osi)[idx] == i { + return // already exists + } + *osi = append(*osi, 0) + copy((*osi)[idx+1:], (*osi)[idx:]) + (*osi)[idx] = i }
215-226: Improve FeederManager organization and documentation.The FeederManager struct would benefit from:
- Field grouping by purpose
- Documentation for each field
- Explanation of the relationship between flags
Consider reorganizing like this:
type FeederManager struct { + // Core dependencies k common.KeeperOracle fCheckTx *FeederManager + + // State management sortedFeederIDs orderedSliceInt64 rounds map[int64]*round cs *caches + + // State flags paramsUpdated bool validatorsUpdated bool forceSeal bool resetSlashing bool }x/oracle/keeper/feedermanagement/round.go (8)
9-32: Add input validation and documentation.The
newRoundfunction should validate its input parameters and include documentation for the struct fields.Consider adding:
- Input validation:
func newRound(feederID int64, tokenFeeder *oracletypes.TokenFeeder, quoteWindowSize int64, cache CacheReader, algo AggAlgorithm) *round { + if tokenFeeder == nil { + panic("tokenFeeder cannot be nil") + } + if cache == nil { + panic("cache cannot be nil") + } + if algo == nil { + panic("algo cannot be nil") + } + if quoteWindowSize <= 0 { + panic("quoteWindowSize must be positive") + }
- Add documentation for the struct fields:
// round represents a price quoting round with the following fields: // startBaseBlock: The starting block height for this round // startRoundID: The initial round identifier // endBlock: The final block height for this round // interval: The interval between rounds // quoteWindowSize: The size of the quoting window // feederID: The identifier for the feeder // tokenID: The identifier for the token // cache: Interface for reading cached data // status: Current status of the round // a: Aggregator instance // roundBaseBlock: Current round's base block // roundID: Current round identifier // algo: Algorithm used for aggregation type round struct { // ... existing fields ... }
63-64: Improve concurrency documentation.The comment about race conditions could be more explicit about the concurrency guarantees.
- // cache does not need to be copied since it's a readonly interface, - // and there's no race condition since abci requests are not executing concurrntly + // Cache is safe to share without copying because: + // 1. It's a read-only interface + // 2. ABCI requests are processed sequentially, not concurrently + // 3. CheckTx operations are isolated from DeliverTx
97-97: Address TODO comment about using valid value.The TODO comment indicates a need to use a valid value instead of the original protoMsg in return.
Would you like me to help implement this improvement? I can create a GitHub issue to track this task.
98-133: Improve error handling structure in Tally method.The error messages could be more structured and consistent.
Consider using error wrapping consistently:
func (r *round) Tally(protoMsg *oracletypes.MsgItem) (*PriceResult, *oracletypes.MsgItem, error) { if !r.IsQuotingWindowOpen() { - return nil, nil, fmt.Errorf("quoting window is not open, feederID:%d", r.feederID) + return nil, nil, fmt.Errorf("quoting window not open for feeder %d", r.feederID) } // ... rest of the function if err != nil { - return nil, nil, fmt.Errorf("failed to add quote for aggregation of feederID:%d, roundID:%d, error:%w", r.feederID, r.roundID, err) + return nil, nil, fmt.Errorf("failed to aggregate quote: feeder=%d round=%d: %w", r.feederID, r.roundID, err) }
151-151: Fix typo in comment.The comment contains a spelling error.
- // currentHeight euqls to baseBlock + // currentHeight equals to baseBlock
146-175: Add logging for state transitions.The
PrepareForNextBlockmethod handles critical state transitions that should be logged for debugging and monitoring.Consider adding structured logging:
func (r *round) PrepareForNextBlock(currentHeight int64) (open bool) { + logger := r.cache.Logger().With("method", "PrepareForNextBlock", "height", currentHeight) if currentHeight < r.roundBaseBlock && r.IsQuoting() { + logger.Debug("closing quoting window: height below round base block") r.closeQuotingWindow() return open } // ... rest of the function if baseBlock > r.roundBaseBlock { + logger.Debug("moving to next round", + "old_base", r.roundBaseBlock, + "new_base", baseBlock, + "round_id", roundID) r.roundBaseBlock = baseBlock
210-236: Document PerformanceReview behavior.The
PerformanceReviewfunction's behavior and return values need better documentation.Add detailed documentation:
+// PerformanceReview evaluates a validator's performance for the current round. +// It returns two boolean values: +// - miss: true if the validator failed to submit a quote matching the final price +// - malicious: true if the validator submitted a quote with matching DetID but different price, +// indicating potential manipulation func (r *round) PerformanceReview(validator string) (miss, malicious bool) {
238-239: Document unparam linter directive.The
//nolint:unparamdirective needs explanation for maintainability.-//nolint:unparam +// nolint:unparam // sourceID is currently always SourceChainlinkID but may support multiple sources in future func (r *round) getFinalDetIDForSourceID(sourceID int64) string {x/oracle/keeper/msg_server_create_price.go (1)
86-92: Consider adding version indicator for encoded pricesWhen emitting events with base64 encoded prices, consider adding a prefix or indicator to distinguish between raw and encoded values. This would help consumers of these events handle the values appropriately.
- sdk.NewAttribute(types.AttributeKeyFinalPrice, strings.Join([]string{tokenIDStr, roundIDStr, priceStr, decimalStr}, "_")), + sdk.NewAttribute(types.AttributeKeyFinalPrice, strings.Join([]string{tokenIDStr, roundIDStr, isEncoded ? "b64:" + priceStr : priceStr, decimalStr}, "_")),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/oracle/keeper/feedermanagement/aggregator.go(1 hunks)x/oracle/keeper/feedermanagement/prices.go(1 hunks)x/oracle/keeper/feedermanagement/round.go(1 hunks)x/oracle/keeper/feedermanagement/types.go(1 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/feedermanagement/aggregator.go (3)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:216-218
Timestamp: 2025-01-14T02:34:18.361Z
Learning: In the oracle module's feedermanagement package, map iteration is made deterministic by extracting keys into a slice, sorting them with `slices.Sort`, and then iterating over the sorted keys. This pattern is demonstrated in the `GetFinalPrice` method of `recordsValidators`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:458-486
Timestamp: 2025-01-14T02:28:24.408Z
Learning: In the oracle module's feedermanagement package, input validation in recordsValidators ensures no duplicate detID values can reach the AddPrice method of recordsDS, maintaining deterministic behavior.
🪛 GitHub Check: CodeQL
x/oracle/keeper/feedermanagement/aggregator.go
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 156-159: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 163-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 172-178: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 216-218: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 223-225: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 248-254: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 272-274: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/prices.go
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 162-164: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (7)
x/oracle/keeper/feedermanagement/types.go (2)
1-10: LGTM! Well-organized package structure.The package name and imports are appropriate and follow Go best practices.
27-48: Add documentation and consider cache invalidation strategy.The cache types are well-structured but would benefit from:
- Documentation explaining the caching strategy
- Comments describing when the
updateflags are set/usedLet's verify if cache invalidation is handled properly:
x/oracle/keeper/feedermanagement/round.go (1)
1-8: LGTM! Clean package structure with minimal imports.x/oracle/keeper/msg_server_create_price.go (3)
77-83: LGTM! Well-documented price string optimization.The implementation of base64 encoding for long price strings is well-documented and properly implemented. The comments clearly explain the rationale for the 32-character threshold and its relation to NST balance changes.
32-35: LGTM! Improved logging with context.The logging implementation has been enhanced with proper context, including feederID, basedBlock, proposer, and other relevant information for better debugging and traceability.
18-19: Verify the impact of increasedmaxFutureOffsetThe
maxFutureOffsethas been increased from 5 seconds to 30 seconds. This significant increase in the allowed future offset could impact the system's behavior and timing assumptions.Run this script to analyze the usage and potential impact:
✅ Verification successful
maxFutureOffsetincrease is safeThe increase from 5s to 30s only affects the maximum allowed future timestamp in new price proposals. The validation logic remains unchanged and the 30-second window is reasonable for accommodating network latency and clock drift.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for maxFutureOffset usage and related timing checks rg -B 2 -A 2 'maxFutureOffset|checkTimestamp|future.*offset' # Look for any timing-related tests ast-grep --pattern 'func Test$$$Time$$$($$$) { $$$ }'Length of output: 1800
x/oracle/keeper/feedermanagement/aggregator.go (1)
82-86: Implement theRecordMsgmethodThe
RecordMsgmethod contains a TODO comment but appears to be partially implemented. Please complete the implementation or clarify if the current implementation is sufficient.Would you like me to help implement this method or create a GitHub issue to track this task?
leonz789
changed the title
leonz789
changed the title
leonz789
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
x/oracle/types/key_native_token.go (1)
Line range hint
38-39: Avoid usingpanic; handle errors gracefully.Using
paniccan cause the application to crash unexpectedly. It's better to return an error and let the caller handle it appropriately.Apply this diff to handle the error:
-func ParseNativeTokenStakerKey(key []byte) (assetID, stakerAddr string) { +func ParseNativeTokenStakerKey(key []byte) (assetID, stakerAddr string, err error) { parsed := strings.Split(string(key), "/") if len(parsed) != 2 { - panic("key of stakerInfo must be construct by 2 infos: assetID/stakerAddr") + return "", "", errors.New("key of stakerInfo must be constructed by 2 parts: assetID/stakerAddr") } - return parsed[0], parsed[1] + return parsed[0], parsed[1], nil }Please update any callers of this function to handle the error accordingly.
x/oracle/keeper/feedermanagement/prices.go (1)
Line range hint
462-462: Remove debug print statements.Debug print statements should be removed from production code:
-fmt.Println("debug--->before.AddPrice", rds.records)Also applies to: 492-492
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 162-164: Iteration over map
Iteration over map may be a possible source of non-determinism
♻️ Duplicate comments (2)
x/oracle/keeper/feedermanagement/feedermanager.go (2)
344-347:⚠️ Potential issueReplace panic with error handling to prevent chain halts.
The panic on invalid consensus address could halt the chain. Handle the error gracefully instead.
consAddr, err := sdk.ConsAddressFromBech32(validator) if err != nil { - panic("invalid consAddr string") + f.k.Logger(ctx).Error("invalid consensus address", "validator", validator, "error", err) + continue }
719-724:⚠️ Potential issueEnsure deterministic initialization of validator powers.
The initialization of validator powers map may be non-deterministic due to map iteration.
+ // Sort validators for deterministic iteration + sort.SliceStable(validatorSet, func(i, j int) bool { + return bytes.Compare(validatorSet[i].Address, validatorSet[j].Address) < 0 + }) validatorPowers := make(map[string]*big.Int) for _, v := range validatorSet { validatorPowers[sdk.ConsAddress(v.Address).String()] = big.NewInt(v.Power) }
🧹 Nitpick comments (19)
x/oracle/keeper/feedermanagement/types.go (3)
94-94: Fix typo in comment: 'deteministic' should be 'deterministic'There's a typographical error in the comment on line 94. Correcting it enhances code readability.
Apply this diff to fix the typo:
-// price records for deteministic source +// price records for deterministic source
127-133: Rename methodCpytoCopyfor clarity and conventionThe method
Cpylikely stands for "copy." Renaming it toCopyimproves clarity and aligns with Go naming conventions.Apply this diff to rename the method:
-func (t *threshold) Cpy() *threshold { +func (t *threshold) Copy() *threshold {Also, update all references to this method accordingly.
22-24: Ensure consistent error handling in interface methodsIn the
CacheReaderinterface, there is inconsistency in error handling:
IsRuleV1(feederID int64) boolreturns only abool.IsDeterministic(sourceID int64) (bool, error)returns abooland anerror.Consider standardizing the method signatures by either:
- Adding an
errorreturn value toIsRuleV1if it can fail.- Removing the
errorfromIsDeterministicif errors are not expected.x/oracle/keeper/native_token_parser.go (3)
9-10: Improve comment clarity and grammar for better understanding.The comments contain grammatical errors and are difficult to understand. Please consider rephrasing them for clarity.
Apply this diff to improve the comments:
-// TODO: This conversion has limited length for balance change, it suites for beaconchain currently, If we extend to other changes, this method need to be upgrade -// for value that might be too big leading too long length of the change value, many related changes need to be done since the message size might be too big then +// TODO: This conversion has a limited length for balance changes. It suits the Beacon Chain currently. If we extend to other changes, this method needs to be upgraded. +// For values that might be too big leading to a long length of the change value, many related changes need to be made since the message size might become too big.
12-79: Consider refactoring theparseBalanceChangeCappedfunction to improve readability.The function is quite complex with nested loops and intricate bit manipulations. Breaking it down into smaller helper functions would enhance readability and maintainability.
For example, you could extract the parsing of
lenValueandstakerChangeinto separate functions:func parseLenValue(changes []byte, byteIndex *int, bitOffset *int, lengthBits int) (int, error) { // Implementation of lenValue parsing } func parseStakerChange(changes []byte, byteIndex *int, bitOffset *int, lenValue int) (int, error) { // Implementation of stakerChange parsing }
30-31: Improve variable naming for clarity.Consider renaming
lenValuetolengthValueto enhance readability and avoid confusion with the built-inlen()function.Apply this diff:
- lenValue := changes[byteIndex] << bitOffset + lengthValue := changes[byteIndex] << bitOffsetx/oracle/types/types.go (2)
54-59: Consider using hex package for chain IDsThe chain IDs are defined as string literals with hex values. Consider using
strconv.ParseUintor theencoding/hexpackage to ensure these values are valid hexadecimal numbers.Example:
-ETHMainnetChainID = "0x7595" +ETHMainnetChainID = fmt.Sprintf("0x%x", uint64(30101))
71-82: Consider adding validation for chain mappingsThe maps establish relationships between chains, IDs, and asset addresses. Consider adding validation functions to ensure consistency across these maps.
Example validation function:
func ValidateNSTMappings() error { // Verify all chains in NSTChains have corresponding entries in NSTAssetAddr for chain := range NSTChains { if _, exists := NSTAssetAddr[chain]; !exists { return fmt.Errorf("missing asset address for chain %s", chain) } } return nil }x/oracle/keeper/common/expected_keepers.go (1)
27-49: Consider grouping related methods in theKeeperOracleinterface.The interface has grown significantly with many new methods. Consider organizing them into logical groups using comments, such as:
- Validator management methods
- Price handling methods
- Cache management methods
Example grouping:
type KeeperOracle interface { KeeperDogfood SlashingKeeper Logger(ctx sdk.Context) log.Logger + // Validator Management AddZeroNonceItemWithFeederIDForValidators(...) InitValidatorReportInfo(...) ClearAllValidatorReportInfo(...) // ... other validator methods + // Price Handling GrowRoundID(...) AppendPriceTR(...) // ... other price methods + // Cache Management SetValidatorUpdateForCache(...) SetParamsForCache(...) // ... other cache methods }x/oracle/keeper/feedermanagement/date_test.go (1)
6-52: Consider using test data builders for better maintainability.The test data setup with multiple price sources could be more maintainable using builder patterns. This would:
- Make test data creation more flexible
- Reduce duplication
- Make test intentions clearer
Example implementation:
type priceSourceBuilder struct { source *priceSource } func newPriceSourceBuilder() *priceSourceBuilder { return &priceSourceBuilder{ source: &priceSource{ deterministic: true, prices: make([]*PriceInfo, 0), }, } } func (b *priceSourceBuilder) withSourceID(id int) *priceSourceBuilder { b.source.sourceID = id return b } func (b *priceSourceBuilder) withPrice(price string, decimal int, detID string) *priceSourceBuilder { b.source.prices = append(b.source.prices, &PriceInfo{ Price: price, Decimal: uint8(decimal), DetID: detID, }) return b } func (b *priceSourceBuilder) build() *priceSource { return b.source }Usage:
ps1 := newPriceSourceBuilder(). withSourceID(1). withPrice("999", 8, "1"). build()x/oracle/keeper/prices.go (1)
35-42: Consider pre-allocating slice with make instead of append.The current implementation creates an empty slice and grows it with append. For better performance, consider pre-allocating the slice with the exact size needed since we know the size upfront.
-val.PriceList = make([]*types.PriceTimeRound, 0, nextRoundID) +val.PriceList = make([]*types.PriceTimeRound, nextRoundID-1)x/oracle/keeper/native_token.go (1)
27-34: Consider implementing dynamic chain ID validation.The TODO comments indicate that hardcoding chain IDs is a bad practice. Consider:
- Moving chain IDs to configuration
- Implementing proper validation logic
- Adding documentation for supported chain IDs
Would you like me to propose a design for dynamic chain ID validation?
x/oracle/keeper/feedermanagement/feedermanager.go (1)
626-667: Add structured logging for quote processing.Add detailed logging to help with monitoring and debugging of quote processing.
func (f *FeederManager) ProcessQuote(ctx sdk.Context, msg *oracletypes.MsgCreatePrice, isCheckTx bool) (*oracletypes.PriceTimeRound, error) { + logger := f.k.Logger(ctx) + logger.Debug("processing quote", + "feeder_id", msg.FeederID, + "based_block", msg.BasedBlock, + "is_check_tx", isCheckTx) if isCheckTx { f = f.getCheckTx() }x/oracle/keeper/feedermanagement/aggregator.go (3)
82-86: Implement theRecordMsgmethodThe
RecordMsgmethod contains a TODO comment and is not fully implemented. This could lead to unexpected behavior or errors when this method is called.Do you want me to help implement this method or open a new GitHub issue to track this task?
462-462: Remove debug print statementsDebug print statements should be removed before committing to production:
- fmt.Println("debug--->before.AddPrice", rds.records)Also applies to: 492-492
110-110: Document the V2 accumulated power calculationThe TODO comment indicates a future improvement for accumulated power calculation. Consider documenting the planned changes and creating a tracking issue.
Do you want me to help create a GitHub issue to track this improvement?
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinismtests/e2e/oracle/create_price.go (2)
85-85: Replace magic numbers with named constantsThe test cases use magic numbers for block heights. Consider defining named constants to improve readability and maintainability:
+ const ( + initialBlockHeight = 10 + firstCheckHeight = 11 + secondCheckHeight = 12 + // ... more constants + )Also applies to: 97-97, 105-105, 110-110, 135-135, 139-139, 164-164, 173-173, 272-272, 311-311, 332-332
225-237: Improve test case documentationThe test cases would benefit from better documentation explaining:
- The purpose of each test case
- The expected behavior and why
- The significance of the block heights used
Example:
// TestCreatePriceNST verifies the price creation for Native Staking Token (NST) // Test cases: // 1. Block 7: Submit prices from validators 0,1,2 // Expected: Price should be accepted as it meets the threshold // 2. ...x/oracle/keeper/native_token_test.go (1)
87-87: LGTM! Consider adding edge cases for index handling.The addition of sequential indexing for balance updates improves auditability. The test verifies the basic flow, but consider adding test cases for:
- Concurrent updates
- Index overflow scenarios
- Index consistency after failed operations
Also applies to: 126-126, 146-146, 167-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/oracle/types/genesis.pb.gois excluded by!**/*.pb.gox/oracle/types/native_token.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
proto/exocore/oracle/v1/genesis.proto(2 hunks)proto/exocore/oracle/v1/native_token.proto(1 hunks)proto/exocore/oracle/v1/query.proto(3 hunks)tests/e2e/oracle/create_price.go(13 hunks)x/oracle/genesis.go(1 hunks)x/oracle/keeper/common/expected_keepers.go(2 hunks)x/oracle/keeper/feedermanagement/aggregator.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(1 hunks)x/oracle/keeper/feedermanagement/date_test.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager.go(1 hunks)x/oracle/keeper/feedermanagement/prices.go(1 hunks)x/oracle/keeper/feedermanagement/types.go(1 hunks)x/oracle/keeper/msg_server_create_price.go(2 hunks)x/oracle/keeper/native_token.go(9 hunks)x/oracle/keeper/native_token_parser.go(1 hunks)x/oracle/keeper/native_token_test.go(4 hunks)x/oracle/keeper/prices.go(5 hunks)x/oracle/keeper/query_native_token.go(3 hunks)x/oracle/types/key_native_token.go(2 hunks)x/oracle/types/params.go(10 hunks)x/oracle/types/types.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/oracle/keeper/feedermanagement/aggregator_test.go
- x/oracle/types/params.go
🧰 Additional context used
📓 Learnings (2)
x/oracle/keeper/feedermanagement/types.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/types.go:0-0
Timestamp: 2025-01-14T03:14:39.425Z
Learning: The `finalDetID` field in the `recordsDS` struct in `x/oracle/keeper/feedermanagement/types.go` should be kept despite the TODO comment suggesting its removal.
x/oracle/keeper/feedermanagement/aggregator.go (4)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:248-254
Timestamp: 2025-01-14T03:14:27.939Z
Learning: In the oracle module's price validation, when a process follows an "all-or-nothing" pattern where all items must be valid for success, the order of map iteration does not need to be deterministic as it doesn't affect the final outcome.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:216-218
Timestamp: 2025-01-14T02:34:18.361Z
Learning: In the oracle module's feedermanagement package, map iteration is made deterministic by extracting keys into a slice, sorting them with `slices.Sort`, and then iterating over the sorted keys. This pattern is demonstrated in the `GetFinalPrice` method of `recordsValidators`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:458-486
Timestamp: 2025-01-14T02:28:24.408Z
Learning: In the oracle module's feedermanagement package, input validation in recordsValidators ensures no duplicate detID values can reach the AddPrice method of recordsDS, maintaining deterministic behavior.
🪛 GitHub Check: break-check
proto/exocore/oracle/v1/query.proto
[failure] 124-124:
Field "1" with name "version" on message "QueryStakerListResponse" changed option "json_name" from "stakerList" to "version".
[failure] 124-124:
Field "1" on message "QueryStakerListResponse" changed type from "message" to "int64".
[failure] 124-124:
Field "1" on message "QueryStakerListResponse" changed name from "staker_list" to "version".
[failure] 140-140:
Field "1" with name "version" on message "QueryStakerInfoResponse" changed option "json_name" from "stakerInfo" to "version".
[failure] 140-140:
Field "1" on message "QueryStakerInfoResponse" changed type from "message" to "int64".
[failure] 140-140:
Field "1" on message "QueryStakerInfoResponse" changed name from "staker_info" to "version".
[failure] 154-154:
Field "1" with name "version" on message "QueryStakerInfosResponse" changed option "json_name" from "stakerInfos" to "version".
[failure] 154-154:
Field "1" on message "QueryStakerInfosResponse" changed label from "repeated" to "optional".
[failure] 154-154:
Field "1" on message "QueryStakerInfosResponse" changed type from "message" to "int64".
[failure] 154-154:
Field "1" on message "QueryStakerInfosResponse" changed name from "staker_infos" to "version".
🪛 GitHub Check: CodeQL
x/oracle/keeper/feedermanagement/aggregator.go
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 110-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-146: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 156-159: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 163-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 172-178: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 216-218: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 223-225: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 248-254: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 272-274: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 289-293: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/feedermanager.go
[warning] 45-45: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 50-50: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
[warning] 90-95: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 109-112: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 121-125: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 174-183: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 243-260: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 263-263: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 309-447: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 378-382: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 385-389: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 400-403: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 453-457: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 603-605: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 623-625: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 671-671: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 717-723: Iteration over map
Iteration over map may be a possible source of non-determinism
x/oracle/keeper/feedermanagement/prices.go
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 162-164: Iteration over map
Iteration over map may be a possible source of non-determinism
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (24)
x/oracle/keeper/feedermanagement/types.go (1)
214-225: Verify the necessity of self-referential fieldfCheckTxThe
FeederManagerstruct contains a fieldfCheckTx *FeederManager, which is a pointer to anotherFeederManager. Ensure that this self-referential field is intentional and does not introduce unintended complexity or risk of nil pointer dereferences.x/oracle/keeper/native_token_parser.go (1)
12-79: Add unit tests forparseBalanceChangeCappedfunction.To ensure the correctness of the bit manipulation logic and error handling, consider adding comprehensive unit tests for this function.
Do you want me to help generate unit tests for this function or open a GitHub issue to track this task?
x/oracle/types/key_native_token.go (1)
46-48: LGTM!The addition of
NativeTokenVersionKeyfunction enhances the key management for native token versions.x/oracle/keeper/query_native_token.go (4)
28-29: Handle potential errors when retrieving the version.Ensure that
GetNSTVersiondoes not return an unexpected value or error that could affect the response.If
GetNSTVersioncan fail or return a default value, consider handling that scenario.
28-29: LGTM!Including the
Versionfield inStakerInfosresponse provides clients with valuable versioning information.
41-42: LGTM!Adding the
Versionfield toStakerInforesponse enhances consistency and context for consumers of the API.
54-55: LGTM!The inclusion of the
Versionfield inStakerListresponse is a good addition for version tracking.x/oracle/genesis.go (1)
42-42: LGTM: NST version initialization addedThe addition of
SetNSTVersionensures proper initialization of NST versions during genesis state setup.Let's verify that the version is properly handled in the keeper:
✅ Verification successful
Verified: NST version management is properly implemented
The
SetNSTVersioncall in genesis is properly supported by the keeper implementation, which correctly manages NST versions through KVStore operations and is consistently used across the codebase for tracking validator list changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if SetNSTVersion is properly implemented in the keeper ast-grep --pattern 'func $_(ctx $_, assetID string, version int64) { $$$ SetNSTVersion($$$) $$$ }'Length of output: 106
Script:
#!/bin/bash # Search for NST version-related functions with a broader pattern ast-grep --pattern 'func $_($_) { $$$ NST$_Version$_ $$$ }' # Also search for any NST version-related store operations rg -A 3 "NST.*Version" --type goLength of output: 10058
proto/exocore/oracle/v1/native_token.proto (1)
54-60: LGTM: Well-structured NSTVersion message definitionThe NSTVersion message is well-defined with appropriate fields for tracking validator list changes:
asset_id: string type for NST identificationversion: int64 type for version trackingx/oracle/types/types.go (1)
47-48: LGTM: Well-defined NSTType and basic constantsGood use of type alias for NSTType and clear constant naming.
Also applies to: 50-52
proto/exocore/oracle/v1/genesis.proto (1)
58-58: LGTM: Consistent version field additionsThe addition of
nst_versionfields to both StakerInfosAssets and StakerListAssets messages maintains consistency in version tracking.Let's verify the version field usage in queries:
Also applies to: 67-67
✅ Verification successful
Version field implementation is correct
The
nst_versionfield is appropriately implemented in the genesis state messages and doesn't require presence in query responses as it's specific to genesis state tracking. The implementation is consistent across bothStakerInfosAssetsandStakerListAssetsmessages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if version field is properly handled in queries ast-grep --pattern 'message $Response { $$$ int64 version = $_; $$$ }'Length of output: 78
Script:
#!/bin/bash # Find all proto files and check for nst_version usage echo "=== Proto files ===" fd -e proto echo -e "\n=== NST version usage ===" rg "nst_version" -A 2 -B 2 echo -e "\n=== Version field patterns ===" rg "version.*=\s*\d+" -A 2 -B 2 --type protoLength of output: 5899
x/oracle/keeper/msg_server_create_price.go (2)
42-45: LGTM! Improved error handling with detailed logging.The enhanced error handling with specific logging and early validation checks improves the code's robustness and debuggability.
77-83: LGTM! Well-documented price string optimization.The implementation efficiently handles long price strings by:
- Using SHA-256 hashing for strings >= 32 characters
- Applying base64 encoding for safe event emission
- Including clear documentation explaining the rationale
The optimization is particularly relevant for NST balance changes which are at least 32 bytes.
x/oracle/keeper/common/expected_keepers.go (1)
20-22: LGTM! Clear interface definition for slashing functionality.The
SlashingKeeperinterface provides a clean abstraction for validator jailing operations.x/oracle/keeper/prices.go (2)
195-227: LGTM! Good error handling and logging.The changes to include detID and NST version handling are well-implemented. The error logging will help with debugging, and the validation checks are thorough.
235-248: LGTM! Good simplification of the return type.The change from returning
*types.PriceTimeRoundtostringis a good simplification that reduces unnecessary object creation while providing the necessary information.x/oracle/keeper/feedermanagement/prices.go (2)
82-86: Implement the TODO in RecordMsg.The function is incomplete with a TODO comment. Please implement the missing functionality or provide details about what needs to be implemented.
Would you like me to help implement this method or create a GitHub issue to track this task?
324-326: 🛠️ Refactor suggestionUse sort.SliceStable for deterministic sorting.
Replace
sort.Slicewithsort.SliceStableto ensure deterministic ordering:-sort.Slice(ps.prices, func(i, j int) bool { +sort.SliceStable(ps.prices, func(i, j int) bool {Likely invalid or redundant comment.
x/oracle/keeper/native_token.go (1)
416-426: LGTM! Good error handling.The implementation properly validates the detID format and provides clear error messages.
x/oracle/keeper/feedermanagement/feedermanager.go (3)
735-739:⚠️ Potential issueReplace panic with graceful error handling in recovery.
The panic on missing validator update block could halt the chain during recovery.
- if !found { - panic("validator update block not found in recovery mode for feeder manager") + if !found { + f.k.Logger(ctx).Error("validator update block not found in recovery mode") + return false }Likely invalid or redundant comment.
317-324:⚠️ Potential issueEnsure deterministic iteration over validators.
Map iteration over validators could lead to non-deterministic behavior across nodes.
+ // Create a sorted slice of validators for deterministic iteration + sortedValidators := make([]string, 0, len(validators)) + for validator := range validators { + sortedValidators = append(sortedValidators, validator) + } + sort.Strings(sortedValidators) - for _, validator := range validators { + for _, validator := range sortedValidators {Likely invalid or redundant comment.
19-26: 🛠️ Refactor suggestionAdd validation for keeper parameter.
The constructor should validate that the keeper parameter is not nil to prevent potential panics.
func NewFeederManager(k common.KeeperOracle) *FeederManager { + if k == nil { + panic("keeper cannot be nil") + } return &FeederManager{ k: k, sortedFeederIDs: make([]int64, 0), rounds: make(map[int64]*round), cs: nil, } }Likely invalid or redundant comment.
x/oracle/keeper/native_token_test.go (2)
139-139: Verify version parameter consistency.The version parameter (2) differs from the previous update which used version 1. This inconsistency might indicate:
- A potential bug in version handling
- Missing documentation about version progression
Please clarify the version progression rules and ensure they are documented.
81-82: Verify version parameter change and document version semantics.The version parameter has changed from 9 to 1, which could be a breaking change. Please:
- Verify that this version change is intentional
- Document the version semantics in the function documentation
✅ Verification successful
Version changes are intentional and reflect test progression
The version parameter changes (9→1→2) in the test file are intentional and reflect the sequential progression of balance change scenarios. The version is properly handled throughout the codebase with appropriate error checking and state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of UpdateNSTByBalanceChange to ensure version consistency rg -A 2 "UpdateNSTByBalanceChange.*version: *[0-9]+"Length of output: 54
Script:
#!/bin/bash # Search for all usages of UpdateNSTByBalanceChange rg -B 2 -A 2 "UpdateNSTByBalanceChange" # Find the function definition ast-grep --pattern 'func $_ UpdateNSTByBalanceChange($_) { $$$ }' # Look for any version-related constants rg -B 1 -A 1 "Version.*=.*[0-9]+" x/oracleLength of output: 9169
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/bank/tx.go (2)
26-27: Consider documenting the reason for double block wait.While adding an extra block wait can help ensure transaction finality, it would be helpful to document why two consecutive block waits are necessary here. This pattern differs from typical blockchain testing practices where a single block confirmation is usually sufficient.
Consider adding a comment explaining the rationale:
// wait to next block for tx to be included err = s.network.WaitForNextBlock() s.Require().NoError(err) +// wait for an additional block to ensure state is fully updated err = s.network.WaitForNextBlock() s.Require().NoError(err)
Line range hint
1-57: Consider refactoring test for better maintainability.The test structure could be improved to reduce duplication and increase maintainability.
Consider these improvements:
- Extract the repeated block wait logic into a helper function
- Create a reusable function for balance verification
- Use test table patterns for different transfer scenarios
Example refactor:
func (s *E2ETestSuite) waitForConfirmation() error { // Wait for two blocks to ensure state updates if err := s.network.WaitForNextBlock(); err != nil { return err } return s.network.WaitForNextBlock() } func (s *E2ETestSuite) verifyBalance(addr sdk.AccAddress, expected sdkmath.Int) { res, err := e2e.QueryNativeCoinBalance(addr, s.network) s.Require().NoError(err) s.Require().Equal( e2e.NewNativeCoin(expected, s.network), *res.Balance, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/bank/tx.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (1)
tests/e2e/bank/tx.go (1)
26-27: Verify if this change is related to oracle refactoring.Since this PR primarily focuses on oracle module refactoring, please clarify if these test changes in the bank module are:
- Addressing test stability issues discovered during oracle testing
- Required for oracle price updates to propagate correctly
- An unrelated improvement that should be in a separate PR
Let's check if there are any dependencies between bank and oracle modules:
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
x/oracle/keeper/feedermanagement/aggregator.go (1)
6-6:⚠️ Potential issueReplace reflect package with custom equality methods
Using the
reflectpackage for equality checks can be a source of non-determinism in a blockchain context. Consider implementing customEqualmethods for each type.Add an
Equalinterface and implement it for all types:type Equal interface { Equal(other interface{}) bool }x/oracle/keeper/feedermanagement/feedermanager.go (3)
57-57:⚠️ Potential issueAvoid panics in
BeginBlockto prevent chain haltsAt line 57, the code invokes
panic(err)within theBeginBlockmethod. Panicking in consensus-critical methods likeBeginBlockcan halt the entire blockchain network, leading to service disruption. Instead, handle the error gracefully by logging it and taking appropriate action without panicking.Apply this diff to handle the error without panicking:
- panic(err) + f.k.Logger(ctx).Error("Error during recovery", "error", err) + return false🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 57-57: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 57-57: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
564-566:⚠️ Potential issueImplement missing validation logic in
ValidateMsgThe TODO comment at line 565 indicates that
ValidateMsglacks full implementation. Proper validation of incoming messages is crucial for security and correctness. Implement the necessary validations forMsgCreatePrice, including checks onnonce,feederID, andcreator.Do you want me to help implement the validation logic for
ValidateMsgor open a GitHub issue to track this task?
717-723:⚠️ Potential issueEnsure deterministic construction of
validatorPowersWhen initializing
validatorPowersin theinitCachesmethod, iterating overvalidatorSetwithout sorting can lead to non-deterministic maps. This can cause consensus issues in the blockchain.Apply this refactor:
validatorSet := f.k.GetAllExocoreValidators(ctx) + sort.SliceStable(validatorSet, func(i, j int) bool { + return bytes.Compare(validatorSet[i].Address, validatorSet[j].Address) < 0 + }) validatorPowers := make(map[string]*big.Int) for _, v := range validatorSet { validatorPowers[sdk.ConsAddress(v.Address).String()] = big.NewInt(v.Power) }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 717-723: Iteration over map
Iteration over map may be a possible source of non-determinism
🧹 Nitpick comments (9)
x/oracle/keeper/feedermanagement/aggregator.go (1)
473-481: Document the insertion sorting logicThe insertion logic maintains sorted order based on
DetID, but this isn't immediately clear from the code. Consider adding comments to explain:
- Why sorting by
DetIDis important- How this sorting contributes to deterministic behavior
Add this comment above the loop:
// Maintain sorted order by DetID to ensure deterministic processing // Find the correct insertion point where DetID is less than or equal to the next recordx/oracle/keeper/slashing.go (4)
29-37: Potential Resource Impact: Consider Batch ProcessingThe method iterates through all validator report entries and deletes them in a single transaction. For chains with many validators, this could impact block processing time.
Consider:
- Adding batch processing with a limit parameter
- Implementing pagination for large datasets
- Adding logging or metrics for monitoring the cleanup process
30-30: Remove or Uncomment Commented CodeThe commented line appears to be a valid operation that should either be executed or removed.
-// k.ClearAllValidatorMissedRoundBitArray(ctx) + k.ClearAllValidatorMissedRoundBitArray(ctx)
153-158: Add Error Handling for Store OperationsThe store deletion operation should include error handling to ensure data consistency.
Consider adding error logging and handling:
func (k Keeper) ClearValidatorMissedRoundBitArray(ctx sdk.Context, validator string) { store := ctx.KVStore(k.storeKey) iterator := sdk.KVStorePrefixIterator(store, types.SlashingMissedBitArrayValidatorPrefix(validator)) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { + if err := store.Delete(iterator.Key()); err != nil { + ctx.Logger().Error("failed to delete validator missed round", "validator", validator, "error", err) + } - store.Delete(iterator.Key()) } }
Line range hint
160-167: Add Documentation and MetricsThis critical cleanup method lacks detailed documentation about its usage and impact.
Add comprehensive documentation:
+// ClearAllValidatorMissedRoundBitArray clears all instances of ValidatorMissedBlockBitArray in the store. +// This is a critical operation that should be used with caution as it affects all validator data. +// It is typically called during: +// - Chain upgrades +// - State migrations +// - Emergency cleanup operations func (k Keeper) ClearAllValidatorMissedRoundBitArray(ctx sdk.Context) { + logger := ctx.Logger().With("module", "oracle", "operation", "clear_missed_rounds") + deletedCount := 0 store := ctx.KVStore(k.storeKey) iterator := sdk.KVStorePrefixIterator(store, types.MissedBitArrayPrefix) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { store.Delete(iterator.Key()) + deletedCount++ } + logger.Info("cleared validator missed rounds", "count", deletedCount) }x/oracle/keeper/feedermanagement/types.go (4)
12-26: Add interface documentation.Consider adding documentation for the
SubmitterandCacheReaderinterfaces to explain their roles and responsibilities in the system.+// Submitter defines the interface for updating various cache components in the oracle system type Submitter interface { SetValidatorUpdateForCache(sdk.Context, oracletypes.ValidatorUpdateBlock) SetParamsForCache(sdk.Context, oracletypes.RecentParams) SetMsgItemsForCache(sdk.Context, oracletypes.RecentMsg) } +// CacheReader defines the interface for retrieving cached validator and rule information type CacheReader interface { GetPowerForValidator(validator string) (*big.Int, bool) GetTotalPower() (totalPower *big.Int) GetValidators() []string IsRuleV1(feederID int64) bool IsDeterministic(sourceID int64) (bool, error) GetThreshold() *threshold }
109-109: Fix typo in comment: "prividing" → "providing".-// TODO: V2: accumulatedValidPower only includes validators who prividing all sources required by rules(defined in oracle.Params) +// TODO: V2: accumulatedValidPower only includes validators who providing all sources required by rules(defined in oracle.Params)
139-167: Add examples to threshold documentation.The threshold calculation logic would be clearer with examples.
// threshold is defined as (thresholdA * thresholdB) / totalPower // when do compare with power, it should be (thresholdB * power) > (thresholdA * totalPower) to avoid decimal calculation +// Example: +// If thresholdA = 2, thresholdB = 3, totalPower = 10 +// Then threshold = (2 * 3) / 10 = 0.6 +// For a power of 7, (3 * 7) > (2 * 10) is true, so the power exceeds the threshold
279-288: Consider using bitfields for boolean flags.Multiple boolean flags could be consolidated into a bitfield for better memory efficiency.
type FeederManager struct { fCheckTx *FeederManager k common.KeeperOracle sortedFeederIDs orderedSliceInt64 rounds map[int64]*round cs *caches - paramsUpdated bool - validatorsUpdated bool - forceSeal bool - resetSlashing bool + flags uint8 // Use bitfields instead of separate booleans } +const ( + flagParamsUpdated uint8 = 1 << iota + flagValidatorsUpdated + flagForceSeal + flagResetSlashing +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
app/app.go(1 hunks)proto/exocore/oracle/v1/genesis.proto(2 hunks)proto/exocore/oracle/v1/params.proto(1 hunks)tests/e2e/oracle/data.go(3 hunks)testutil/utils.go(1 hunks)x/oracle/keeper/cache/caches.go(0 hunks)x/oracle/keeper/cache/caches_test.go(0 hunks)x/oracle/keeper/cache/info_test.go(0 hunks)x/oracle/keeper/feedermanagement/aggregator.go(1 hunks)x/oracle/keeper/feedermanagement/feedermanager.go(1 hunks)x/oracle/keeper/feedermanagement/types.go(1 hunks)x/oracle/keeper/slashing.go(3 hunks)x/oracle/types/genesis_test.go(0 hunks)x/oracle/types/params.go(10 hunks)
💤 Files with no reviewable changes (4)
- x/oracle/types/genesis_test.go
- x/oracle/keeper/cache/info_test.go
- x/oracle/keeper/cache/caches.go
- x/oracle/keeper/cache/caches_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- proto/exocore/oracle/v1/genesis.proto
- testutil/utils.go
- app/app.go
- tests/e2e/oracle/data.go
- x/oracle/types/params.go
🧰 Additional context used
📓 Learnings (2)
x/oracle/keeper/feedermanagement/types.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/types.go:0-0
Timestamp: 2025-01-14T03:14:39.425Z
Learning: The `finalDetID` field in the `recordsDS` struct in `x/oracle/keeper/feedermanagement/types.go` should be kept despite the TODO comment suggesting its removal.
x/oracle/keeper/feedermanagement/aggregator.go (4)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:248-254
Timestamp: 2025-01-14T03:14:27.939Z
Learning: In the oracle module's price validation, when a process follows an "all-or-nothing" pattern where all items must be valid for success, the order of map iteration does not need to be deterministic as it doesn't affect the final outcome.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:216-218
Timestamp: 2025-01-14T02:34:18.361Z
Learning: In the oracle module's feedermanagement package, map iteration is made deterministic by extracting keys into a slice, sorting them with `slices.Sort`, and then iterating over the sorted keys. This pattern is demonstrated in the `GetFinalPrice` method of `recordsValidators`.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#275
File: x/oracle/keeper/feedermanagement/aggregator.go:458-486
Timestamp: 2025-01-14T02:28:24.408Z
Learning: In the oracle module's feedermanagement package, input validation in recordsValidators ensures no duplicate detID values can reach the AddPrice method of recordsDS, maintaining deterministic behavior.
🪛 GitHub Check: break-check
proto/exocore/oracle/v1/params.proto
[failure] 75-75:
Field "5" with name "slash_fraction_malicious" on message "SlashingParams" changed option "json_name" from "slashFractionMiss" to "slashFractionMalicious".
[failure] 75-75:
Field "5" on message "SlashingParams" changed name from "slash_fraction_miss" to "slash_fraction_malicious".
🪛 GitHub Check: CodeQL
x/oracle/keeper/feedermanagement/feedermanager.go
[warning] 57-57: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 57-57: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
[warning] 121-125: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 174-183: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 243-260: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 309-447: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 378-382: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 385-389: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 400-403: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 453-457: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 603-605: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 623-625: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 717-723: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (11)
x/oracle/keeper/feedermanagement/aggregator.go (2)
109-110: Address the TODO comment about validator power accumulationThe comment indicates that the power accumulation logic needs to be updated in V2 to consider all valid validators that provide required sources.
Would you like me to help implement this enhancement or create a GitHub issue to track this task?
228-229: Well implemented deterministic iterationGood practice: The keys are sorted before iteration to ensure deterministic results when calculating final prices.
proto/exocore/oracle/v1/params.proto (1)
Line range hint
75-79: Breaking Change: Verify Migration StrategyThe removal of
slash_fraction_missand reuse of its index forslash_fraction_maliciousconstitutes a breaking change in the protocol buffer schema. This could impact:
- Existing state migrations
- Client applications parsing these messages
- Chain upgrades
Let's verify the impact and migration handling:
🧰 Tools
🪛 GitHub Check: break-check
[failure] 75-75:
Field "5" with name "slash_fraction_malicious" on message "SlashingParams" changed option "json_name" from "slashFractionMiss" to "slashFractionMalicious".
[failure] 75-75:
Field "5" on message "SlashingParams" changed name from "slash_fraction_miss" to "slash_fraction_malicious".x/oracle/keeper/feedermanagement/types.go (1)
41-48: Add validation for cache initialization.The
cachesstruct contains pointer fields but lacks validation to ensure they're properly initialized before use.Consider adding a constructor function:
+// NewCaches creates a new caches instance with initialized components +func NewCaches(k Submitter) *caches { + return &caches{ + k: k, + msg: &cacheMsgs{}, + validators: &cacheValidator{validators: make(map[string]*big.Int)}, + params: &cacheParams{}, + } +}x/oracle/keeper/feedermanagement/feedermanager.go (7)
243-260: Ensure deterministic iteration over rounds incommitRoundsThe
commitRoundsmethod iterates overf.sortedFeederIDs, which is sorted. Ensure that all iterations over maps or slices are deterministic to prevent consensus issues.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 243-260: Iteration over map
Iteration over map may be a possible source of non-determinism
725-725: Enhance test coverage forinitCachesmethodThe
initCachesmethod is critical for initializing the FeederManager's state. Ensure that comprehensive unit tests cover various scenarios, including initialization with different parameters and validator sets.Do you want me to assist in creating unit tests for the
initCachesmethod?
174-179:⚠️ Potential issueEnsure deterministic iteration over rounds in
prepareRoundsThe iteration over
f.roundsin theprepareRoundsmethod may lead to non-deterministic behavior. Sorting the rounds before iteration ensures consistent execution across nodes.Apply this refactor:
for _, r := range f.rounds { + // Collect feeder IDs + } + feederIDs := make([]int64, 0, len(f.rounds)) + for feederID := range f.rounds { + feederIDs = append(feederIDs, feederID) + } + sort.Slice(feederIDs, func(i, j int) bool { + return feederIDs[i] < feederIDs[j] + }) + for _, feederID := range feederIDs { + r := f.rounds[feederID] if open := r.PrepareForNextBlock(ctx.BlockHeight()); open { feederIDs = append(feederIDs, r.feederID) } }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 174-183: Iteration over map
Iteration over map may be a possible source of non-determinism
378-382:⚠️ Potential issueEnsure deterministic iteration over validators when updating missed rounds
The iteration over the validators map can cause non-deterministic behavior. Sort the validators before iterating.
Apply this refactor:
validators := f.cs.GetValidators() + sort.Strings(validators) for _, validator := range validators { // existing logic }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 378-382: Iteration over map
Iteration over map may be a possible source of non-determinism
121-125:⚠️ Potential issueEnsure deterministic iteration over validators
In the
setupNoncesmethod, iterating over the mapf.roundswithout ordering can lead to non-deterministic behavior across nodes. This can cause consensus failures in the blockchain. To ensure deterministic execution, extract the keys, sort them, and iterate over the sorted keys.Apply this refactor:
validators := f.cs.GetValidators() + sort.Strings(validators) for _, feederID := range feederIDs { r := f.rounds[feederID] // existing logic }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 121-125: Iteration over map
Iteration over map may be a possible source of non-determinism
623-625: Avoid non-deterministic behavior when accessingf.roundsIn the
ProcessQuotemethod, accessingf.roundswithint64(msgItem.FeederID)assumes deterministic behavior. Ensure that the map access is consistent across all nodes.Run the following script to confirm that all
FeederIDvalues are consistently mapped:🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 623-625: Iteration over map
Iteration over map may be a possible source of non-determinism
309-447:⚠️ Potential issueEnsure deterministic iteration over validators in
handleQuotingMisBehaviorIn the
handleQuotingMisBehaviorfunction, iterating over maps without ordering can lead to non-deterministic behavior. Sort the validators and rounds before iteration to ensure consistency.Apply this refactor for validators:
validators := f.cs.GetValidators() + sort.Strings(validators) for _, validator := range validators { // existing logic }And for rounds:
- for _, r := range f.rounds { + roundKeys := make([]int64, 0, len(f.rounds)) + for key := range f.rounds { + roundKeys = append(roundKeys, key) + } + sort.Slice(roundKeys, func(i, j int) bool { + return roundKeys[i] < roundKeys[j] + }) + for _, key := range roundKeys { + r := f.rounds[key] if r.IsQuotingWindowEnd(height) { // existing logic } }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 309-447: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 378-382: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 385-389: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 400-403: Iteration over map
Iteration over map may be a possible source of non-determinism
return nil for empty list when do median
This reverts commit 7fc9bef.
This reverts commit 131e85e.
typo rename exceedPowerLimit to exceedsThreshold interface guard for aggMedian refactor cachevalidators.add, cache totalpower to avoid calculation refactor PerformanceReview remove nstversion, use uint64bytes instead
return false when input is nil
leonz789
force-pushed
the
develop-oracle-refactor2.0
branch
from
7f84e07 to
b49bfaf
Compare
Description
The current architecture for handling transactions relies on aggregator and cache, which leads to scattered responsibilities, complex state management, and redundant code. To address these issues, this PR introduces a new structure, FeederManager, to streamline the management of caches, token feeders, and KV store state updates.
Refactor oracle module
Changes in This PR
FeederManager Implementation:
Per-TokenFeeder Round Management:
Caches for Oracle Parameters and Validator Set:
Message Processing:
Round State Updates:
Aggregator Price Handling:
Price Aggregation Logic:
The aggregation process is performed in two steps:
Notable changes
Closes #XXX
Summary by CodeRabbit
New Features
Improvements