dogfood, oracle: remove default params #293
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the blockchain node's configuration, focusing on the genesis file and oracle parameters. The changes primarily involve restructuring how tokens, chains, sources, and rules are defined across multiple files. The modifications enhance the flexibility of parameter initialization by moving from static assignments to more dynamic append-based configurations, particularly in the oracle and dogfood modules. The changes aim to provide a more granular and extensible approach to defining blockchain node parameters. Changes
Sequence DiagramsequenceDiagram
participant Genesis as Genesis Configuration
participant Oracle as Oracle Module
participant Dogfood as Dogfood Module
Genesis->>Oracle: Configure Chains
Genesis->>Oracle: Add Tokens
Genesis->>Oracle: Define Sources
Genesis->>Oracle: Create Rules
Genesis->>Oracle: Set Token Feeders
Oracle-->>Dogfood: Share Configuration
Dogfood-->>Genesis: Validate Parameters
Possibly related PRs
Suggested Labels
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: 1
🧹 Nitpick comments (3)
testutil/network/genesis_data.go (1)
70-77: Clarify usage of empty offchain endpoint.
You’re appending a newSourcenamed"Chainlink"with an offchain map containing an empty string. Confirm this aligns with your intended usage or add comments explaining why it’s empty.testutil/keeper/oracle.go (2)
80-87: Explain empty offchain map for Chainlink.
An empty offchain endpoint might create potential runtime issues if you rely on it for external data. Consider specifying a placeholder URL or providing a comment clarifying future usage.
92-98: Revisit token feeder block assumptions.
StartBaseBlock: 1andInterval: 10can influence how quickly the feeder updates. Validate these parameters against your desired testing or production flow to avoid missed or repetitive updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
local_node.sh(2 hunks)testutil/keeper/oracle.go(1 hunks)testutil/network/genesis_data.go(1 hunks)testutil/utils.go(2 hunks)x/dogfood/types/genesis_test.go(1 hunks)x/dogfood/types/params.go(2 hunks)x/oracle/types/params.go(0 hunks)
💤 Files with no reviewable changes (1)
- x/oracle/types/params.go
🧰 Additional context used
🪛 GitHub Actions: Tests
x/dogfood/types/genesis_test.go
[error] 658-658: Test failure: Expected error containing 'non-positive last total power' but got 'params: asset IDs: invalid parameter value: []: the genesis data supplied is invalid'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (12)
testutil/network/genesis_data.go (3)
60-69: Ensure consistency and validate the placeholder contract address.
Appending a chain named"Ethereum"and a token named"ETH"withContractAddressset to"0x"might be a placeholder. Confirm that chain ID1and"0x"are intentional for testing to avoid accidental mismatches in production.
78-81: Confirm rule configuration.
The appended rule referencesSourceIDs: []uint64{0}. Verify that0indeed correlates to the"Chainlink"source appended above and no off-by-one indexing errors exist.
82-89: Check feeder’s indexing and interval.
A new token feeder is being appended withTokenID: 1andStartBaseBlock: 10. Ensure these indices and intervals align with other tokens’ IDs and block heights, especially since indexing starts at 1 here.testutil/keeper/oracle.go (2)
71-79: Validate chain and token setup.
You’re appending"Ethereum"toChainsand then"ETH"toTokenswithContractAddress: "0x". Confirm that a placeholderContractAddressis valid in your tests and won’t interfere with real-world addresses.
88-91: Confirm source references for rules.
SourceIDs: []uint64{0}must align with the newly added"Chainlink"source. Ensure correct indexing and no off-by-one errors if more sources are appended later.x/dogfood/types/params.go (3)
32-34: Check for empty default asset IDs.
DefaultAssetIDs = []string{}may cause validation failures if not overridden, as empty asset IDs are deemed invalid inValidateAssetIDs(). Ensure you replace it with actual values elsewhere.
35-37: Zero minimum self-delegation is acceptable.
DefaultMinSelfDelegation = sdkmath.ZeroInt()fits within the non-negative requirement. No further action needed unless you require a strictly positive minimum for production.
66-66: DefaultParams now uses empty asset IDs.
PassingDefaultAssetIDsas an empty slice will fail unless those IDs are injected later. Verify that your runtime configuration sets valid asset IDs before usage.x/dogfood/types/genesis_test.go (1)
41-41:⚠️ Potential issueInvestigate pipeline failure related to new asset ID.
This newly assigned
params.AssetIDs = []string{"0x0b34c4d876cd569129cf56bafabb3f9e97a4ff42_0x9ce1"}may lead to a mismatch with other code or tests, as indicated by the pipeline failure referencing “invalid parameter value: []”. Verify whether this new asset ID aligns with the rest of the codebase and ensure the genesis and test logic consistently handle non-empty lists.testutil/utils.go (1)
206-233: Validate consistency of newly appended Oracle parameters.These lines add a new chain, token (ETH), source (Chainlink), rule, and token feeder. Verify that:
- The token decimals (18) match other references (e.g., local_node.sh sets 8 in one place).
- No references rely on default parameters that were simultaneously removed.
local_node.sh (2)
182-208: 🛠️ Refactor suggestionEnforce consistent token decimal usage in the Oracle configuration.
In local_node.sh, the newly configured “ETH” token is set to decimal=8, which conflicts with the code using decimal=18 in some places. Decide on the correct decimal precision and ensure it’s uniformly applied, especially if the pipeline or test relies on a specific decimal value.
168-168:⚠️ Potential issueCheck for mismatch in dogfood asset ID.
Here,
"0xdac17f958d2ee523a2206206994597c13d831ec7_0x65"differs from the test’s"0x0b34c4d876cd...". This discrepancy can trigger validation errors. Confirm which asset ID should appear across the genesis config and tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/oracle/keeper/keeper_suite_test.go (1)
110-110: Deduplicate parameter initialization logic
You're repeating almost the same parameter initialization here and inReset(). Consider moving the common setup steps into a shared utility method to adhere to the DRY principle, improving maintainability.local_node.sh (1)
183-184: Consider adding a more descriptive chain description.The chain description is set to "-", which provides no meaningful information about the Ethereum chain.
- jq '.app_state["oracle"]["params"]["chains"][1]["desc"]="-"' "$GENESIS" >"$TMP_GENESIS" && mv "$TMP_GENESIS" "$GENESIS" + jq '.app_state["oracle"]["params"]["chains"][1]["desc"]="Ethereum Mainnet"' "$GENESIS" >"$TMP_GENESIS" && mv "$TMP_GENESIS" "$GENESIS"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
local_node.sh(2 hunks)x/dogfood/types/genesis_test.go(2 hunks)x/oracle/keeper/keeper_suite_test.go(2 hunks)x/oracle/keeper/msg_server_create_price_test.go(1 hunks)x/oracle/keeper/msg_server_update_params_test.go(4 hunks)x/oracle/keeper/params_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/dogfood/types/genesis_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (12)
x/oracle/keeper/keeper_suite_test.go (1)
76-76: Ensure valid indexing ofTokenFeeders
Switching toDefaultParamsForTest()provides more consistent predefined data for these tests. However, please verify that theTokenFeedersslice always has an index[1]populated, as here you're directly referencing it without any prior checks.x/oracle/keeper/msg_server_create_price_test.go (1)
71-71: Consistency in test parameters
UsingDefaultParamsForTest()aligns with the updated approach seen in other test files. This ensures all tests run on a uniform and carefully controlled parameter set. Nice job.x/oracle/keeper/params_test.go (2)
179-179: Centralized usage ofDefaultParamsForTest()
AdoptingDefaultParamsForTest()helps maintain a singular, clearly defined source of default testing parameters. This approach reduces duplication and potential inconsistencies.
285-285: Reinitialization concerns
Reassigningp = DefaultParamsForTest()resets any previous mutations top. If your test logic relies on appended tokens or feeders from earlier operations, ensure this reset is intentional.x/oracle/keeper/msg_server_update_params_test.go (4)
5-5: Newtimeimport
Bringing in thetimepackage is necessary for handling jail durations inSlashingParams. This addition looks appropriate.
18-85: Introduction ofDefaultParamsForTest()
Defining a dedicated function for test parameters is an excellent move. It centralizes test configurations, ensuring a consistent, reliable baseline for all test scenarios.
98-98: Uniform setup across tests
UsingDefaultParamsForTest()fordefaultParamsaligns with the new pattern. This uniform approach simplifies maintenance and prevents divergence in test data.
186-186: Index safety
token := DefaultParamsForTest().Tokens[1]requires thatDefaultParamsForTest()consistently returns at least two token entries. Confirm that adding or removing tokens in the future won't lead to an out-of-range error.local_node.sh (4)
168-168: LGTM: Asset ID configuration for dogfood module.The asset ID configuration aligns with the PR objective of removing default parameters, requiring manual addition of the asset ID.
186-191: Verify ETH token configuration.The token configuration for ETH uses:
- A contract address of "0x" which seems incorrect for ETH
- An asset_id that appears to be for a different token (USDT's contract address)
Run this script to verify the ETH token configuration:
193-197: Enhance Chainlink source configuration.The Chainlink source configuration has an empty offchain entry and no onchain configuration, which might limit its functionality.
Run this script to verify the Chainlink source configuration:
199-207: Review token feeder configuration.The token feeder configuration appears incomplete:
- Uses hardcoded IDs (token_id="1", rule_id="1")
- Has a static interval of 10 blocks
- Uses end_block="0" which might need clarification
Run this script to verify the token feeder configuration:
✅ Verification successful
The token feeder configuration is valid and follows the protocol design
The configuration follows the protocol's requirements:
- IDs starting from 1 is by design (0 is reserved)
- Interval of 10 blocks is valid as long as MaxNonce ≤ 5 (protocol requires interval > 2*MaxNonce)
- EndBlock=0 is a valid setting that indicates continuous operation with no end time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify token feeder configuration # Test: Check for token feeder configurations and their usage # Expect: Find consistent token feeder configurations # Search for token feeder configurations rg -A 10 "token_feeders.*token_id" --type yaml --type json # Search for interval configurations rg -A 5 "interval.*10" --type yaml --type jsonLength of output: 5467
Script:
#!/bin/bash # Search for token feeder documentation and configurations rg -l "token.*feeder" --type md rg -A 5 "end_block.*0" --type yaml --type json # Look for other token feeder configurations fd -e json -e yaml -e toml | xargs rg -l "token_feeders" # Search for rule_id patterns rg "rule_id.*[0-9]+" --type json --type yamlLength of output: 222
Script:
#!/bin/bash # Search for token feeder related code and tests rg -t go -t rust -t js "TokenFeeder" -B 2 -A 2 rg -t go -t rust -t js "token.*feeder" -i -B 2 -A 2 # Search for test files containing token feeder configurations fd -e test.js -e test.go -e test.rs | xargs rg -l "token.*feeder" -i # Look for struct or type definitions ast-grep --pattern 'struct TokenFeeder { $$$ }' ast-grep --pattern 'type TokenFeeder struct { $$$ }'Length of output: 67101
MaxMustermann2
requested review from
cloud8little,
trestinlsd,
TimmyExogenous and
leonz789
The default asset_id was added for convenience. Now that our testing infrastructure is fleshed out, it can be removed.
Similarly, the default parameters used in x/oracle have been removed and must now be manually added.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor