feat(token-price-oracle): add multi-source price feeds - #1002
Conversation
Co-authored-by: Cursor <[email protected]>
|
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:
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 <[email protected]>
curryxbo
changed the title
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) <[email protected]>
- 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) <[email protected]>
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) <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 <[email protected]>
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
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 <[email protected]>
… feeds BitgetSDKPriceFeed rejected a standalone GetTokenPrice call whenever the cached ETH leg had never been primed, while CEXPriceFeed fetched it on demand. One interface method with two preconditions meant a caller's failure mode depended on which feed happened to be configured. Bitget now self-initializes like the Binance/OKX feed, matching Chainlink and Pyth which fetch both legs on every call. The batch path is unchanged: it primes ETH before the loop so the self-init branch never fires, keeping a cycle at N+1 requests against rate-limited endpoints rather than 2N. Co-authored-by: Cursor <[email protected]>
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.
panos-xyz
left a comment
panos-xyz
left a comment
There was a problem hiding this comment.
I completed a correctness, integration, and spec review of the current head. CI is green, but the issues below include merge-blocking state-layout, fallback, and availability defects that are not covered by the current tests.
Additional spec-level gaps that are not anchored to changed lines:
- Issue #977 requires a heartbeat that forces an on-chain update despite a below-threshold deviation; source
maxStalenessdoes not implement that behavior. - Per-token/per-source metrics and the P0 risk/degradation policy are still missing.
- Genesis mutation and derivation-finality changes are outside #977's explicit off-chain-only scope and should be split into separately reviewed PRs.
- The existing unresolved
devnet.envdocumentation comment remains valid.
Please resolve the P1 findings before merge and keep #977 open (or narrow the closing semantics) until its P0 requirements are complete.
| } | ||
|
|
||
| // Pre-register test tokens in TokenRegistry for devnet | ||
| if err := SetDevnetTestTokens(db); err != nil { |
There was a problem hiding this comment.
P1: This writes devnet-only registry state into every generated network genesis. The generic genesis l2 command calls BuildL2DeveloperGenesis for any deploy config, including mainnet, Hoodi, QA, and testnet configs where fundDevAccounts is false. Because this call is unconditional, regenerating one of those genesis files reserves token IDs 1-3, installs test addresses, and changes the state root. Gate this behind an explicit devnet setting (at minimum config.FundDevAccounts) and add a regression test proving that a non-dev config leaves the registry empty.
There was a problem hiding this comment.
Fixed in 1bab5a9. SetDevnetTestTokens now sits inside the same config.FundDevAccounts gate as the dev accounts, so mainnet and testnet genesis are untouched.
| { | ||
| TokenID: 2, | ||
| TokenAddress: common.HexToAddress("0x5300000000000000000000000000000000000011"), // L2WETH predeploy address | ||
| BalanceSlot: common.BigToHash(big.NewInt(3)), // WETH standard balance slot |
There was a problem hiding this comment.
P1: The L2WETH balance slot is incorrect. bindings/bindings/wrappedether_more.go places _balances at slot 0; slot 3 is _name. After token ID 2 is activated, fee accounting reads keccak256(user, 3), so deposited WETH at keccak256(user, 0) appears absent and WETH-paid transactions fail. The current model also uses a zero hash to mean 'no balance slot', so it cannot represent an actual slot of zero. Add a separate NeedBalanceSlot/HasBalanceSlot field and encode actual slot 0 as stored value 1, or intentionally use the EVM-call path, then cover a WETH fee-payment flow in a genesis-level test.
There was a problem hiding this comment.
Fixed in 1bab5a9. L2WETH now registers slot 0.
You were right that the encoding needed a separate flag: slot 0 is a real balance slot, so a zero hash could not distinguish it from "no slot". DevnetTestToken gained a NeedBalanceSlot field mirroring the parameter L2TokenRegistry.registerToken already takes, and setTokenInfo now follows _toStoredBalanceSlot exactly.
The existing storage test had the same ambiguity baked into its assertion, so it was corrected as well, and a new test pins the WETH entry to slot 0 with the flag set.
| return []DevnetTestToken{ | ||
| { | ||
| TokenID: 1, | ||
| TokenAddress: common.HexToAddress("0x0000000000000000000000000000000000000001"), // Mock BTC address |
There was a problem hiding this comment.
P2: These are precompile addresses, not mock ERC-20 contracts. Address 0x01 executes ECRECOVER and 0x03 executes RIPEMD160. The setup guide later activates all three IDs, after which balanceOf/transfer calls for BTC and BGB return invalid or hash-derived data and cannot settle fee-token transactions. Deploy real mock ERC-20 bytecode at non-precompile addresses (or do not advertise/activate these IDs) and add one fee-payment integration test per advertised token.
There was a problem hiding this comment.
Fixed in 1bab5a9. The placeholder addresses moved to 0x1111...1111 and 0x3333...3333, clear of the 0x01-0x0a precompile range, and a test asserts the registered addresses stay above 0xff so this cannot regress. The setup guide was updated to match and now says the addresses are placeholders with no deployed contract.
| for tokenID, price := range prices { | ||
| for _, tokenID := range tokenIDs { | ||
| price, exists := prices[tokenID] | ||
| if !exists { |
There was a problem hiding this comment.
P1: Batch fallback is all-or-nothing and behaves differently when only one feed is configured. Every feed receives the full active-token set, and an incomplete map is discarded, so complementary mappings across providers can never succeed (for example, the documented Chainlink/Pyth mappings cannot serve active ID 3). Conversely, createFallbackPriceFeed returns a single feed directly, bypassing this completeness validation; CEX feeds can then return a partial or empty map with a nil error and the updater can record a successful cycle. Implement per-token fallback/merging (querying each provider only for unresolved supported IDs) and enforce the same completeness contract for both one-feed and multi-feed configurations.
There was a problem hiding this comment.
Splitting this into the two parts.
Single-feed bypass — fixed in 1bab5a9. createFallbackPriceFeed no longer returns feeds[0] directly, so one-feed and multi-feed configurations share the same completeness contract. FallbackPriceFeed already handles a single feed correctly, so this was just deleting the special case.
Worth recording that the impact was a bit worse than a recorded success: an empty batch reaches the "no prices need updating" branch, which also calls LastSuccessfulUpdateTimestamp.Set(now). A total feed outage therefore refreshed the very metric that would have detected it.
Per-token merging across providers — agreed, but out of scope here. Querying each provider only for unresolved IDs changes both the PriceFeed batch contract and the updater's notion of a successful cycle, and it needs its own tests around partial resolution. I would rather not grow this PR further; I will track it as a follow-up and link it here.
There was a problem hiding this comment.
Following up on the per-token part — you were right that this belongs in this PR, and my earlier "out of scope" call was wrong. Fixed in 7d18942.
Tracing it end to end, this was worse than a missing enhancement. The updater passes the full active set to the fallback feed, and ChainlinkPriceFeed/PythHermesPriceFeed returned an error on the first token absent from their mapping, so one unmapped active token failed those feeds for every token, every cycle. The oracle-first design silently degraded to CEX-only. The devnet configuration in this PR reproduces it: genesis registers tokens 1-3 while the documented Chainlink and Pyth mappings cover only 1 and 2, so once all three are active neither oracle feed is ever used.
The fix has two halves. Chainlink and Pyth now omit tokens they have no mapping for instead of failing the batch, which is the behaviour the CEX feeds already had, so the contract is now consistent across providers; genuine fetch failures still error. FallbackPriceFeed.GetBatchTokenPrices then resolves across feeds in priority order, passing each feed only the tokens still unresolved.
On the completeness contract, I went with partial resolution rather than all-or-nothing: a token no provider can serve no longer blocks updates for every other token. It is reported by its absence from the returned map, and the updater logs it and exports a new unresolved_tokens gauge plus an unresolved_token error counter, so a token going stale is visible rather than silent. A cycle that resolves nothing at all is still an error, which preserves the guarantee from the single-feed fix above.
Tests cover the merge (higher-priority feed serves what it can, the next feed is asked only for the remainder), an unservable token yielding a partial result, a failing feed not shrinking the next feed's request, and the empty-result case still erroring.
| // derivation cursor on hash mismatch so a deeper reorg is recoverable. | ||
| // Operators wanting strict no-reorg-possible reads can still set | ||
| // --derivation.confirmations=-3 (rpc.FinalizedBlockNumber). | ||
| // Applies to every verify mode. Layer1 validators derive their whole |
There was a problem hiding this comment.
P1: Making layer1 verification use the 10-block default can permanently stall a validator after a valid deep reorg. The reorg handler only rewinds the L1 cursor; reorg.go explicitly states that it does not roll back L2. If a >10-block reorg replaces a committed batch with different content, re-derivation reuses the old L2 blocks, root verification fails, and subsequent polls keep retrying the same failure. Preserve finalized-by-default behavior for layer1 mode until L2 rollback exists, or implement and integration-test changed-batch reorg recovery. This consensus-critical policy change should also be split from the oracle PR.
There was a problem hiding this comment.
Acknowledged, and raised again in your latest review. I am not going to argue the deep-reorg scenario inside this PR: it deserves its own discussion rather than being settled as a side effect of a price-feed change, so I will follow up on it separately.
| if price.PublishTime <= 0 { | ||
| return fmt.Errorf("publish_time must be positive") | ||
| } | ||
| if published.After(now.Add(maxStaleness)) { |
There was a problem hiding this comment.
P2: Future-dated prices are accepted for the full staleness window. With the one-hour default, a publish time almost one hour in the future passes this check and remains valid for almost two hours. This undermines the claimed freshness validation. Reject timestamps after now or after a small, explicit clock-skew allowance, matching the hardened Chainlink path, and add a future-timestamp regression test.
There was a problem hiding this comment.
Fixed in 1bab5a9. validatePythPrice now rejects any publish time after the local clock rather than allowing the full staleness window, which matches validateChainlinkRound. Added a table case for a future-dated publish time.
There was a problem hiding this comment.
Amended in e974620: the check now allows 30s of clock skew rather than rejecting anything ahead of the local clock.
Matching validateChainlinkRound exactly turned out to be wrong for this feed. updatedAt there is an L1 block timestamp and is always in the past by the time it is read, so zero tolerance is safe. Pyth's publish_time is the publisher's near-real-time wall clock at second granularity, so a host whose clock drifts behind the publisher would have rejected every Pyth price and taken the whole feed down. The 30s bound still fixes the reported defect, which was accepting anything inside the staleness window — nearly an hour at the default. Tests cover both sides of the bound.
| if priceStr == "" { | ||
| return nil, fmt.Errorf("no price data returned for symbol %s", symbol) | ||
| } | ||
| price, err := strconv.ParseFloat(priceStr, 64) |
There was a problem hiding this comment.
P2: strconv.ParseFloat accepts NaN and infinities. NaN passes the current <= 0 test and big.NewFloat(NaN) panics; positive infinity later causes conversion to an integer ratio to return nil and can panic the updater. The same issue exists in the fixed stablecoin parser. Reject math.IsNaN/math.IsInf and non-positive values before constructing big.Float, then test that malformed exchange data falls through to the next provider instead of crashing the process.
There was a problem hiding this comment.
Fixed in 1bab5a9. Both parsers now go through a shared newFinitePositiveFloat guard that rejects NaN, infinities and non-positive values before big.NewFloat.
Two things worth flagging beyond the original report. Bitget carries its own copies of both parsers rather than reusing the CEX ones, and its ticker parser had no positivity check at all, so it accepted zero and negatives as well; both now use the shared guard. Separately, ParseFloat("1e400") does return an out-of-range error, so overflow via decimal literal was already rejected.
The test asserts rejection at the parser level for NaN, ±Inf, zero and negatives. I did not add an end-to-end test that a malformed ticker falls through to the next provider; the parser returns an error and FallbackPriceFeed already advances on error, but say the word if you would like that covered explicitly.
| contract := bind.NewBoundContract(feedAddress, parsedChainlinkAggregatorABI, c.caller, nil, nil) | ||
|
|
||
| var roundData []interface{} | ||
| if err := contract.Call(&bind.CallOpts{Context: ctx}, &roundData, "latestRoundData"); err != nil { |
There was a problem hiding this comment.
P2: Chainlink calls have no bounded per-call timeout. ethclient.Dial uses an HTTP client without a request timeout, and these calls inherit the service's long-lived context. A stalled high-priority RPC can therefore block the updater indefinitely, preventing fallback from ever being attempted. Wrap each feed operation in a bounded child context (or inject a timed HTTP/RPC client) and add a hanging-RPC fallback test.
There was a problem hiding this comment.
Fixed in 1bab5a9. Each eth_call now runs under its own 10s deadline, covering both latestRoundData and decimals.
I put the deadline on the call context rather than on the dialed HTTP client so it applies regardless of transport, since NewChainlinkPriceFeedWithCaller can be handed any bind.ContractCaller.
| export TOKEN_PRICE_ORACLE_TOKEN_MAPPING_BITGET="1:BTCUSDT,2:ETHUSDT" | ||
|
|
||
| # Optional: prefer oracle feeds first, fallback to CEX sources | ||
| export TOKEN_PRICE_ORACLE_PRICE_FEED_PRIORITY="chainlink,pyth,bitget,binance,okx" |
There was a problem hiding this comment.
P2: This quick-start configuration cannot start as written. The priority enables Binance and OKX, but the example provides neither provider's required token mapping. LoadConfig rejects the configuration before startup. Limit the priority list to the feeds configured in this snippet, or include complete Binance and OKX mappings/base configuration.
There was a problem hiding this comment.
Fixed in 1bab5a9. The quick start now includes the Binance and OKX mappings, which brings it in line with the example further down the same README that already had them.
|
|
||
| ```bash | ||
| export TOKEN_PRICE_ORACLE_L2_ETH_RPC=http://localhost:8545 | ||
| export TOKEN_PRICE_ORACLE_L2_TOKEN_REGISTRY_ADDRESS=0x5300000000000000000000000000000000000021 |
There was a problem hiding this comment.
P2: The documented registry-address and token-ID controls are ignored. There are no corresponding flags/config fields: the service always binds the predeploy address and always reads every supported token ID from the contract. This is especially misleading because it hides the full-batch coverage failure. Either implement both options end-to-end with tests, or remove these exports and document the actual fixed behavior.
There was a problem hiding this comment.
Fixed in 1bab5a9. Both exports are gone from the devnet guide.
They were not only in the new doc: env.example and docker-compose.yml carried the same two dead variables from before this PR, so those were cleaned up in the same commit rather than leaving two more copies of the same misdirection. env.example now states that the registry address is fixed at its predeploy and that token IDs always come from the contract.
…d docs Require a Pyth API key whenever the pyth feed is enabled, since Hermes rejects unauthenticated requests from 2026-08-18. Bound the Pyth exponent magnitude so a malformed expo cannot ask big.Int for 10^2147483648, and reject publish times ahead of the local clock instead of accepting the whole staleness window. Reject NaN and infinities before constructing a big.Float: they pass a bare `<= 0` test, and NaN panics while an Inf survives until Float.Int returns nil. Bitget had the same parser twice, one of which had no positivity check at all. Give Chainlink calls their own deadline. The service context has none and the RPC transport imposes none, so a hung endpoint would stall the updater forever and the fallback feeds would never be reached. Wrap a single configured feed in FallbackPriceFeed rather than returning it directly. That bypassed the completeness check, letting a partial or empty CEX batch through as success, which the updater then recorded as a successful cycle and used to advance the last-successful-update timestamp. Gate the devnet token pre-registration on fundDevAccounts so it no longer writes into mainnet and testnet genesis, correct the L2WETH balance slot from 3 (_name) to 0 (_balances) via an explicit NeedBalanceSlot flag, since a zero slot is a real slot and cannot double as "no slot", and move the placeholder token addresses out of the precompile range. Fix the quick-start config, which enabled Binance and OKX without their mappings, and drop the documented registry-address and token-ID controls, which have no corresponding flags. Co-authored-by: Cursor <[email protected]>
… all-or-nothing FallbackPriceFeed discarded a feed's entire response when it was missing any one requested token, and Chainlink and Pyth failed a whole batch on the first token absent from their mapping. Together that meant a provider covering part of the active set could never contribute: with any active token missing from the oracle mappings, Chainlink and Pyth failed every cycle and every token silently came from a CEX. The devnet configuration this repo documents hits exactly that, since genesis registers three tokens while the oracle mappings cover two. Feeds now omit tokens they have no mapping for rather than failing the batch, which is what the CEX feeds already did, and the fallback layer resolves across feeds in priority order, passing each one only the tokens still unresolved. A token that no feed can price no longer blocks the rest of the cycle; it is reported by its absence from the returned map, and the updater surfaces it through a warning and the new unresolved_tokens gauge. A cycle that resolves nothing at all is still an error, so a total feed outage cannot be recorded as a successful update. Co-authored-by: Cursor <[email protected]>
Rejecting any publish time ahead of the local clock was too strict for this feed. The Chainlink path reads an L1 block timestamp, which is always in the past, but publish_time is the publisher's near-real-time wall clock at second granularity, so host clock drift would have turned into a total Pyth outage. Allow 30s of skew while still refusing prices dated far into the future, which was the actual defect: the previous check accepted anything inside the staleness window, nearly an hour at the default. Co-authored-by: Cursor <[email protected]>
Code reviewFound 4 issues:
morph/token-price-oracle/local.sh Lines 9 to 13 in 7d18942
morph/token-price-oracle/client/pyth_feed.go Lines 42 to 50 in 7d18942
morph/DEVNET_TOKENREGISTRY_SETUP.md Lines 110 to 123 in 7d18942
morph/node/derivation/config.go Lines 103 to 117 in 7d18942 🤖 Generated with Claude Code |
The devnet tokens were registered with 10^(18-decimals), which double-applies the decimals adjustment the oracle already makes on its own. That collapses to a scale of 1 for the two 18-decimal tokens, and since priceRatio is truncated to a uint256, any token cheaper than ETH then stores a ratio of 0. BGB was dropped every cycle with only a "Skipping zero price" warning and calculateTokenAmount reverted for it, which defeats the one token in the set that exercises a CEX-specific feed. Scale cancels out of calculateTokenAmount, so its only role is preserving significant digits, and 10^decimals is the convention L2TokenRegistry's own tests use. It also makes the stored ratio equal 10^18 * tokenPrice/ethPrice for every token regardless of decimals. Also drop BGB from the documented OKX mapping, since OKX does not list it. Co-authored-by: Cursor <[email protected]>
The restart recipe reads as a full reset, but docker compose down leaves named volumes in place, so L1 resumes from its old chain and the redeployed L1 contracts land on different addresses each run. Co-authored-by: Cursor <[email protected]>
The guide listed shell exports and then passed --env-file devnet.env, a file it never created, so the documented container command could not start. Co-authored-by: Cursor <[email protected]>
The script predates the pre-registered devnet tokens and still mapped ID 1 to BGB, ID 2 to BTC and ID 3 to a fixed $1.0. Genesis registers ID 1 as BTC, ID 2 as ETH and ID 3 as BGB, so running the documented devnet flow wrote a wrong price to all three. Co-authored-by: Cursor <[email protected]>
…re the cutover The comment stated the 2026-08-18 date and then rejected an empty key, which reads as a contradiction while unauthenticated requests still work. Say why: an updater configured today outlives the cutover, so failing at startup beats every request failing later with nothing changed on our side. Co-authored-by: Cursor <[email protected]>
|
Thanks — going through these in order. 1. 2. Pyth API key before the cutover — keeping it required, but the comment was the problem. You are right that unauthenticated requests still work until 2026-08-18, and the comment stated the date and then rejected an empty key without bridging the two. Rewritten in 5d60d5a to give the actual reason: an updater configured today keeps running past the cutover, at which point every Hermes request starts failing with nothing having changed on our side. Refusing to start is the earlier and louder failure than a mid-flight outage, and the cost of the strict check is one free API key. This is also the same direction as your earlier comment that the key cannot stay optional. Happy to gate it on the date instead if you would rather not block the next two weeks. I also fixed a stale 3. 4. Layer1 confirmations — acknowledged, and I am not going to argue it here. The deep-reorg path you describe is a fair concern and deserves better than a note buried in this PR, so I would rather track it separately than bundle the discussion into a price-feed change. I will follow up on it outside this review. Separately, this branch has now been through a full devnet integration run, which turned up one defect in the devnet additions and confirmed the rest. Details in the following comment. |
Devnet integration runRan the branch end to end on a local devnet (fresh genesis, L1 + L2 + oracle) rather than relying on unit tests alone. It found one defect in the devnet additions and confirmed the rest. Defect found: the pre-registered tokens used the wrong scale — fixed in 9c5e25b.
Scale cancels out of Verified on-chain after the fix:
Review fixes confirmed against a running chain:
|
… root Every other document in the repo lives beside the component it describes, with README.md the only file at the root. The guide is about running the oracle against a devnet, so it belongs with the oracle; genesis pre-registration is step zero rather than the subject. Co-authored-by: Cursor <[email protected]>
2 participants
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