feat(token-price-oracle): add multi-source price feeds - #1002
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Chainlink, Pyth Hermes, Binance, and OKX price feeds with configuration, validation, fallback integration, and documentation. Devnet genesis now pre-registers three test tokens. Derivation confirmation behavior now uses explicit confirmation settings without an implicit Layer1 override. ChangesMulti-source Token Price Feeds
Devnet TokenRegistry Initialization
Derivation Confirmation Behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PriceUpdater
participant ChainlinkPriceFeed
participant AggregatorV3
participant PythHermesPriceFeed
participant HermesAPI
participant CEXPriceFeed
participant ExchangeAPI
PriceUpdater->>ChainlinkPriceFeed: Request batch token prices
ChainlinkPriceFeed->>AggregatorV3: Read round data and decimals
AggregatorV3-->>ChainlinkPriceFeed: Return feed values
PriceUpdater->>PythHermesPriceFeed: Request batch token prices
PythHermesPriceFeed->>HermesAPI: Request parsed price IDs
HermesAPI-->>PythHermesPriceFeed: Return price and confidence data
PriceUpdater->>CEXPriceFeed: Request exchange prices
CEXPriceFeed->>ExchangeAPI: Fetch ticker prices
ExchangeAPI-->>CEXPriceFeed: Return ticker data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
token-price-oracle/client/chainlink_feed_test.go (1)
63-69: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSingle-case test with potential precision loss in
TestChainlinkAnswerToFloat.The test verifies only one conversion scenario (123456789000 → 1234.56789) and relies on
Float64()conversion, which truncates to ~17 significant digits. The implementation usesbig.Float.SetPrec(256)for high-precision arithmetic, but the test doesn't validate precision preservation or edge cases:
decimals = 0(no scaling)decimals = 18(extreme precision, common for ERC-20 tokens)- Large answer values where Float64 conversion may lose precision
- Answer = 1 with various decimal positions
Adding parameterized test cases for different decimal values would increase confidence in correct price scaling.
📋 Suggested parameterized test cases
func TestChainlinkAnswerToFloat(t *testing.T) { tests := []struct { name string answer *big.Int decimals uint8 expected float64 }{ { name: "standard 8 decimals", answer: big.NewInt(123456789000), decimals: 8, expected: 1234.56789, }, { name: "no decimals", answer: big.NewInt(2000), decimals: 0, expected: 2000, }, { name: "18 decimals (ERC-20 standard)", answer: big.NewInt(1_000_000_000_000_000_000), // 1 token with 18 decimals decimals: 18, expected: 1.0, }, { name: "single unit", answer: big.NewInt(1), decimals: 8, expected: 0.00000001, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { price := chainlinkAnswerToFloat(tt.answer, tt.decimals) got, _ := price.Float64() if got != tt.expected { t.Fatalf("chainlinkAnswerToFloat(%s, %d) = %v, want %v", tt.answer.String(), tt.decimals, got, tt.expected) } }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@token-price-oracle/client/chainlink_feed_test.go` around lines 63 - 69, The TestChainlinkAnswerToFloat function only tests a single case and does not validate the chainlinkAnswerToFloat implementation across edge cases and different decimal positions. Refactor TestChainlinkAnswerToFloat into a parameterized test by creating a test cases table with fields for test name, answer value as *big.Int, decimals as uint8, and expected float64 result. Iterate through the test cases using t.Run() and verify the chainlinkAnswerToFloat function correctly handles scenarios including: decimals=0 (no scaling), decimals=18 (ERC-20 standard), large answer values, and answer=1 with various decimal positions. This ensures the high-precision arithmetic in the implementation is properly validated across different scaling scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@token-price-oracle/client/chainlink_feed_test.go`:
- Around line 9-61: The TestValidateChainlinkRound test is missing coverage for
critical validation paths in validateChainlinkRound: future timestamp
validation, nil parameter checks, and staleness boundary conditions. Add four
new test cases to the tests table: one for future updatedAt values beyond the
maxStaleness window, separate cases for nil values in each of the four input
parameters (answer, updatedAt, roundID, answeredInRound), and one for the exact
staleness boundary condition where updatedAt is exactly maxStaleness in the
past, with appropriate wantErr values for each.
In `@token-price-oracle/client/chainlink_feed.go`:
- Around line 243-245: The future timestamp validation in the condition check at
line 243 is too permissive because it allows timestamps up to now plus
maxStaleness in the future. Change the condition to reject any updatedAt
timestamp that is after the current time (now), rather than allowing it to be up
to maxStaleness into the future. Replace now.Add(maxStaleness) with just now in
the After() comparison to ensure future-dated timestamps are properly rejected.
In `@token-price-oracle/updater/factory.go`:
- Around line 118-121: The log.Info statement at line 120 logs the raw
ChainlinkRPC URL directly, which exposes potentially sensitive API keys or
authentication credentials. Create a helper function called redactRPCForLog that
parses the RPC URL and returns only the scheme and host portion (e.g.,
"https://example.com"), stripping out any credentials or path-based API keys.
Then update the log.Info call to use the redactRPCForLog function on
cfg.ChainlinkRPC before logging it in the "rpc" field.
---
Nitpick comments:
In `@token-price-oracle/client/chainlink_feed_test.go`:
- Around line 63-69: The TestChainlinkAnswerToFloat function only tests a single
case and does not validate the chainlinkAnswerToFloat implementation across edge
cases and different decimal positions. Refactor TestChainlinkAnswerToFloat into
a parameterized test by creating a test cases table with fields for test name,
answer value as *big.Int, decimals as uint8, and expected float64 result.
Iterate through the test cases using t.Run() and verify the
chainlinkAnswerToFloat function correctly handles scenarios including:
decimals=0 (no scaling), decimals=18 (ERC-20 standard), large answer values, and
answer=1 with various decimal positions. This ensures the high-precision
arithmetic in the implementation is properly validated across different scaling
scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 238ff072-6b3c-4840-af5f-4bf64cbaa274
📒 Files selected for processing (10)
token-price-oracle/README.mdtoken-price-oracle/client/chainlink_feed.gotoken-price-oracle/client/chainlink_feed_test.gotoken-price-oracle/client/price_feed.gotoken-price-oracle/config/config.gotoken-price-oracle/docker-compose.ymltoken-price-oracle/env.exampletoken-price-oracle/flags/flags.gotoken-price-oracle/local.shtoken-price-oracle/updater/factory.go
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
token-price-oracle/client/pyth_feed.go (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused write-lock:
muis only ever RLocked.
tokenPriceIDsis populated once in the constructor and never mutated afterward, yetmuis read-locked inGetTokenPrice/GetBatchTokenPrices. There is noLock()call anywhere in the file, so thesync.RWMutexis vestigial and could mislead future maintainers into assuming mutation support exists.Also applies to: 69-72, 109-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@token-price-oracle/client/pyth_feed.go` around lines 21 - 31, The sync.RWMutex field mu is never write-locked and protects immutable state only. Remove mu from PythHermesPriceFeed, eliminate the corresponding RLock/RUnlock calls in GetTokenPrice and GetBatchTokenPrices, and retain the existing read behavior without synchronization.token-price-oracle/client/cex_feed.go (1)
96-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
GetBatchTokenPricesissues one sequential HTTP request per token instead of a batch call.Both Binance (
GET /api/v3/ticker/pricewith nosymbolreturns all tickers) and OKX (GET /api/v5/market/tickerwithinstType) support fetching multiple prices in a single call. The current implementation does N sequential round-trips per update cycle, which scales poorly and increases exposure to per-exchange rate limits as the number of mapped tokens grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@token-price-oracle/client/cex_feed.go` around lines 96 - 115, Update CEXPriceFeed.GetBatchTokenPrices to fetch all requested token prices through a single exchange batch request instead of calling GetTokenPrice once per token. Use the Binance and OKX batch ticker APIs, map returned symbols to the requested tokenIDs, preserve the existing ETH-price update and per-token skip behavior, and return the assembled prices map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@token-price-oracle/client/cex_feed.go`:
- Around line 62-94: Update CEXPriceFeed.GetTokenPrice to lazily initialize
ethPrice when it has not yet been set, fetching the ETH price through the
existing CEX price-fetching path instead of returning the “ETH price not
initialized” error. Reuse the same initialization and synchronization behavior
as GetBatchTokenPrices, while preserving the existing token lookup, mapped-price
fetch, and return flow.
In `@token-price-oracle/local.sh`:
- Line 31: Update the commented Pyth API-key argument in the example command to
expand the documented TOKEN_PRICE_ORACLE_PYTH_API_KEY variable instead of
PYTH_API_KEY.
In `@token-price-oracle/README.md`:
- Around line 74-76: Update the README documentation for
TOKEN_PRICE_ORACLE_PRICE_FEED_PRIORITY to consistently mark it as optional and
include its default value bitget, matching the default assigned in flags.go;
apply the same correction to the other occurrence and leave the CLI behavior
unchanged.
---
Nitpick comments:
In `@token-price-oracle/client/cex_feed.go`:
- Around line 96-115: Update CEXPriceFeed.GetBatchTokenPrices to fetch all
requested token prices through a single exchange batch request instead of
calling GetTokenPrice once per token. Use the Binance and OKX batch ticker APIs,
map returned symbols to the requested tokenIDs, preserve the existing ETH-price
update and per-token skip behavior, and return the assembled prices map.
In `@token-price-oracle/client/pyth_feed.go`:
- Around line 21-31: The sync.RWMutex field mu is never write-locked and
protects immutable state only. Remove mu from PythHermesPriceFeed, eliminate the
corresponding RLock/RUnlock calls in GetTokenPrice and GetBatchTokenPrices, and
retain the existing read behavior without synchronization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f1accfd-350f-4a5f-a35c-52ff0f91f2ce
📒 Files selected for processing (11)
token-price-oracle/README.mdtoken-price-oracle/client/cex_feed.gotoken-price-oracle/client/cex_feed_test.gotoken-price-oracle/client/pyth_feed.gotoken-price-oracle/client/pyth_feed_test.gotoken-price-oracle/config/config.gotoken-price-oracle/docker-compose.ymltoken-price-oracle/env.exampletoken-price-oracle/flags/flags.gotoken-price-oracle/local.shtoken-price-oracle/updater/factory.go
- Add devnet_tokens.go with BTC/ETH/BGB test token definitions - Automatically register tokens in genesis for devnet environment - Enable token-price-oracle to work out-of-the-box without manual setup - Tokens: BTC (ID=1), ETH (ID=2), BGB (ID=3) - Supports multi-source price feeds: Chainlink, Pyth, Bitget, OKX This ensures complete end-to-end flow from genesis generation to price oracle operation in devnet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Complete setup guide for devnet token pre-registration - Configuration examples for token-price-oracle - Troubleshooting and extension recommendations - Final test summary covering all PRs (1021, 1023, 1002) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary of completed tasks: - PR1021 testing (beacon fallback) - PR1023 testing (layer1-verify metrics) - PR1002 testing with devnet TokenRegistry implementation - Complete documentation and troubleshooting guides Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DEVNET_TOKENREGISTRY_SETUP.md`:
- Around line 137-147: Update the token-price-oracle docker run command in
DEVNET_TOKENREGISTRY_SETUP.md to assign the explicit container name
token-price-oracle, matching the existing docker logs command. Keep the
remaining run options unchanged.
- Line 54: Replace the concrete TOKEN_PRICE_ORACLE_PRIVATE_KEY value in the
setup documentation with a clearly marked placeholder, and explicitly state that
the key must never be used outside an isolated local devnet. Keep the
surrounding setup instructions intact while making the devnet-only restriction
unmistakable.
In `@ops/l2-genesis/morph-chain-ops/genesis/devnet_tokens.go`:
- Around line 122-126: The devnet activation flow and its documentation must
agree on whether tokens are immediately updateable. In
ops/l2-genesis/morph-chain-ops/genesis/devnet_tokens.go lines 122-126,
deliberately retain or revise the inactive token initialization and test the
intended behavior; in DEVNET_TOKENREGISTRY_SETUP.md lines 5 and 127-141, remove
the unconditional out-of-the-box update claim or state prerequisites, and place
activation/allowlist setup before oracle startup; in FINAL_TEST_SUMMARY.md lines
131-151 and 274-285, mark transaction logs and complete-loop/merge-readiness
claims as conditional until activation and allowlisting are verified.
- Around line 101-130: Update setTokenInfo to encode token.BalanceSlot as
BalanceSlot plus one when non-zero, while preserving zero as zero, matching
L2TokenRegistry’s getTokenInfo and getTokenInfoByAddress contract. Add a
regression test covering getTokenInfo, reverse lookup, getAllTokenIDs,
getSupportedTokenList, and zero/non-zero balance-slot storage encoding.
In `@TODAY_WORK_SUMMARY.md`:
- Around line 187-201: Reconcile all validation reports to use the pending
devnet RPC checks as the authoritative status: in TODAY_WORK_SUMMARY.md lines
187-201 retain pending states, lines 94-113 label unverified logs as expected
output, lines 263-277 avoid claiming the full loop is complete, and lines
301-309 update the conclusion and merge recommendation; uncheck unrerun
validations in DEVNET_TOKENREGISTRY_SETUP.md lines 159-166 and align the
coverage matrix with actual evidence in FINAL_TEST_SUMMARY.md lines 171-185.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6845cc95-9f3e-478e-9365-074215486c8f
📒 Files selected for processing (5)
DEVNET_TOKENREGISTRY_SETUP.mdFINAL_TEST_SUMMARY.mdTODAY_WORK_SUMMARY.mdops/l2-genesis/morph-chain-ops/genesis/devnet_tokens.goops/l2-genesis/morph-chain-ops/genesis/layer_two.go
Harden feed validation and secret handling, align devnet storage with the contract, and replace unverified Chinese reports with concise English setup guidance. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
DEVNET_TOKENREGISTRY_SETUP.md (1)
76-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake partial provider coverage explicit.
For the configured
[1,2,3]batch, Chainlink and Pyth mappings omit token ID 3, while only Bitget and OKX cover all three tokens. Since incomplete provider responses are rejected before fallback acceptance, Chainlink and Pyth cannot serve this batch independently. Document that CEX feeds are required for token 3, or provide complete mappings/remove partial providers from the example priority.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DEVNET_TOKENREGISTRY_SETUP.md` around lines 76 - 92, Update the provider configuration example around TOKEN_PRICE_ORACLE_PRICE_FEED_PRIORITY and the Chainlink/Pyth mappings to explicitly account for token ID 3 being unavailable from those providers. Document that Bitget and OKX are required for token 3, or alternatively provide complete Chainlink and Pyth mappings or remove the partial providers from the priority list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DEVNET_TOKENREGISTRY_SETUP.md`:
- Around line 112-117: Update the setup instructions before the
token-price-oracle docker run command to explicitly create devnet.env from the
documented environment values, and state the working directory or use an
unambiguous path so Docker can locate it. Keep the existing docker run
configuration unchanged aside from the env-file path if needed.
---
Nitpick comments:
In `@DEVNET_TOKENREGISTRY_SETUP.md`:
- Around line 76-92: Update the provider configuration example around
TOKEN_PRICE_ORACLE_PRICE_FEED_PRIORITY and the Chainlink/Pyth mappings to
explicitly account for token ID 3 being unavailable from those providers.
Document that Bitget and OKX are required for token 3, or alternatively provide
complete Chainlink and Pyth mappings or remove the partial providers from the
priority list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7733fb92-3fd0-481e-a1d4-6ceda3959894
📒 Files selected for processing (11)
DEVNET_TOKENREGISTRY_SETUP.mdops/l2-genesis/morph-chain-ops/genesis/devnet_tokens.goops/l2-genesis/morph-chain-ops/genesis/devnet_tokens_test.gotoken-price-oracle/README.mdtoken-price-oracle/client/cex_feed.gotoken-price-oracle/client/cex_feed_test.gotoken-price-oracle/client/chainlink_feed.gotoken-price-oracle/client/chainlink_feed_test.gotoken-price-oracle/local.shtoken-price-oracle/updater/factory.gotoken-price-oracle/updater/factory_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- token-price-oracle/local.sh
- token-price-oracle/client/chainlink_feed_test.go
- token-price-oracle/updater/factory.go
- ops/l2-genesis/morph-chain-ops/genesis/devnet_tokens.go
- token-price-oracle/README.md
- token-price-oracle/client/cex_feed.go
| ```bash | ||
| docker run -d \ | ||
| --name token-price-oracle \ | ||
| --network docker_default \ | ||
| --env-file devnet.env \ | ||
| morph/token-price-oracle:latest |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Create and locate devnet.env before the Docker command.
The guide defines shell exports but never writes them to devnet.env; running this container command as documented therefore fails when Docker cannot find the env file. Add an explicit env-file creation step and an unambiguous working directory/path.
Suggested documentation change
+cd token-price-oracle
+# Save the configuration above as devnet.env before running:
docker run -d \
--name token-price-oracle \
--network docker_default \
- --env-file devnet.env \
+ --env-file ./devnet.env \
morph/token-price-oracle:latest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DEVNET_TOKENREGISTRY_SETUP.md` around lines 112 - 117, Update the setup
instructions before the token-price-oracle docker run command to explicitly
create devnet.env from the documented environment values, and state the working
directory or use an unambiguous path so Docker can locate it. Keep the existing
docker run configuration unchanged aside from the env-file path if needed.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Deriving from L1 finalized made a validator's own chain trail L1 by roughly two epochs, which delayed batch-divergence alerts by the same amount and collapsed the L2 safe and finalized tags onto a single value. It also broke startup on an L1 with no finalized block yet, since the first-run startHeight default reads at the configured depth. Validators now share the fixed-depth default and its reorg detector with every other node type; deployments that want a consensus-backed read set --derivation.confirmations explicitly. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
L2TokenRegistryunchanged; all external data sources are consumed by the off-chain updater before writing existingpriceRatiovalues.Test plan
cd token-price-oracle && go test ./...cd token-price-oracle && go vet ./...Closes #977
Summary by CodeRabbit
New Features
Bug Fixes
Documentation