Alt fee oracle #809
Alt fee oracle #809
Conversation
WalkthroughAdds a new token-price-oracle service (CLI, config, flags, metrics, Docker, Makefiles), Bitget price feed and fallback mechanism, L2 client and TxManager, periodic on-chain price updater with batching and threshold logic, related tests, dependency bumps across modules, and a small node beacon test and oracle refactor. Changes
Sequence Diagram(s)sequenceDiagram
participant App as CLI/App
participant Updater as PriceUpdater
participant Registry as L2TokenRegistry
participant PriceFeed as PriceFeed(s)
participant TxMgr as TxManager
participant Chain as L2 Chain
App->>Updater: Start(ctx)
activate Updater
loop every interval
Updater->>Registry: GetSupportedIDList()
Registry-->>Updater: tokenIDs
Updater->>PriceFeed: GetBatchTokenPrices(tokenIDs)
PriceFeed-->>Updater: prices + ETH price
Updater->>Updater: calculatePriceRatio(tokenID)
Updater->>Registry: GetTokenInfo(tokenID)
Registry-->>Updater: tokenInfo
Updater->>Updater: shouldUpdatePrice(old,new,threshold)
alt needs update
Updater->>TxMgr: SendTransaction(UpdatePrices)
TxMgr->>Chain: EstimateGas & SendTx
Chain-->>TxMgr: txHash
TxMgr->>TxMgr: waitForReceipt(txHash)
TxMgr-->>Updater: receipt
Updater->>Updater: updateBalanceMetrics()
else skip
Note over Updater: No transaction sent
end
end
App->>Updater: Stop()
deactivate Updater
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oracle/oracle/batch.go (1)
186-190: Log statement uses the wrong error variableOn
ParseCommitBatchfailure you correctly returnparseErr, but the log statement still printserr, which at this point may be stale ornil, degrading debuggability.Consider updating the log to use
parseErr:- rollupCommitBatch, parseErr := o.rollup.ParseCommitBatch(lg) - if parseErr != nil { - log.Error("get l2 BlockNumber", "err", err) - return parseErr - } + rollupCommitBatch, parseErr := o.rollup.ParseCommitBatch(lg) + if parseErr != nil { + log.Error("get l2 BlockNumber", "err", parseErr) + return parseErr + }
🧹 Nitpick comments (6)
token-price-oracle/Makefile (1)
1-1: Consider addingallas the default PHONY target.It is conventional in Makefiles to provide an
alltarget as the default (typically depending onbuild). This ensures that runningmakewithout arguments builds the service.-.PHONY: build test lint clean run docker-build +.PHONY: all build test lint clean run docker-build + +all: buildnode/derivation/beacon_test.go (5)
19-20: Uset.Skip()instead of silent return.When skipping a test due to missing configuration, use
t.Skip()to make the skip explicit in test output rather than silently returning.Apply this diff:
if url == "" { - return + t.Skip("Set BLOB_URL environment variable to run this integration test") }
26-26: Remove unused initial txHash declaration.The
txHashvariable is declared here but immediately overwritten on line 36 inside the loop. This initial value serves no purpose.Apply this diff:
var ( start uint64 = 1590159 end uint64 = 1590159 ) - txHash := common.HexToHash("0xf742e20b788e02baab643413ec61e075bb03f3f69c5b12d69f6f90bd7312aa28")
43-43: Replacefmt.Printlnwitht.Logfor test output.Using
fmt.Printlnin tests is not idiomatic. Uset.Logort.Logffor consistent test logging that integrates with the testing framework.Apply this diff:
indexedBlobHashes := dataAndHashesFromTxs(block.Transactions(), tx) - fmt.Println(indexedBlobHashes) + t.Logf("Indexed blob hashes: %v", indexedBlobHashes)
22-58: Consider refactoring test with deterministic data or meaningful assertions.The test still contains several characteristics of debug/exploration code rather than a production test:
- Hard-coded block numbers (1590159) and transaction hash
- Hard-coded contract address in helper function
- No assertions beyond
require.NoError- the test just logs output- No validation that the retrieved blobs are correct or expected
Consider either:
- Converting to a proper integration test with meaningful assertions about the retrieved data
- Adding fixtures or mocked data for deterministic testing
- Documenting the purpose and expected behavior of the test
60-73: Consider parameterizing the rollup contract address.The
RollupContractAddressis hard-coded at0x511d92b63ae7471fd5239bded29b76a446698a00. For better reusability and maintainability, consider passing the contract address as a parameter or loading it from configuration.Example refactor:
func testTchRollupLog(l1Client *ethclient.Client, ctx context.Context, contractAddr common.Address, from, to uint64) ([]eth.Log, error) { query := ethereum.FilterQuery{ FromBlock: big.NewInt(0).SetUint64(from), ToBlock: big.NewInt(0).SetUint64(to), Addresses: []common.Address{ contractAddr, }, Topics: [][]common.Hash{ {RollupEventTopicHash}, }, } return l1Client.FilterLogs(ctx, query) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node/derivation/beacon_test.go(1 hunks)oracle/oracle/batch.go(1 hunks)token-price-oracle/Makefile(1 hunks)token-price-oracle/updater/token_price.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
node/derivation/beacon_test.go (3)
node/derivation/base_client.go (1)
NewBasicHTTPClient(29-37)node/derivation/beacon.go (2)
NewL1BeaconClient(36-38)L1BlockRef(96-101)node/derivation/derivation.go (1)
RollupEventTopicHash(36-36)
token-price-oracle/updater/token_price.go (4)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)token-price-oracle/client/price_feed.go (2)
PriceFeed(20-26)TokenPrice(12-17)token-price-oracle/updater/tx_manager.go (1)
TxManager(14-17)token-price-oracle/metrics/metrics.go (2)
UpdateErrors(13-19)AccountBalance(22-27)
🪛 checkmake (0.2.2)
token-price-oracle/Makefile
[warning] 31-31: Target body for "help" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (rust)
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: check
- GitHub Check: test
🔇 Additional comments (9)
oracle/oracle/batch.go (1)
191-196: Refactored error construction is correct and idiomaticUsing
fmt.Errorf("batch is incontinuity, expect %v, have %v", ...)is the idiomatic way to construct this error and preserves the existing behavior while simplifying the code.token-price-oracle/updater/token_price.go (7)
18-34: LGTM! Concurrency contract is now clear.The struct definition properly declares all concurrency controls:
muprotectstokenIDsandlastPrices, andstopOnceensuresstopChanis closed safely. This addresses the earlier review concern about unclear concurrency discipline.
62-108: LGTM! Proper logging and resilient startup.The earlier debug print statement has been replaced with structured logging (line 64), and the startup logic appropriately handles initial update failures as non-fatal. The lifecycle coordination using context, stopChan, and ticker is correct.
110-118: LGTM! Safe shutdown mechanism.The use of
stopOnce.DoensuresstopChanis closed exactly once, preventing panics from concurrent calls toStop(). This addresses the earlier concurrency concern.
251-335: LGTM! Robust price ratio calculation with precision handling.The method includes comprehensive validation (nil checks, active token, decimals, zero prices) and correctly implements the documented formula. The precision check on lines 315-323 addresses the earlier review concern by logging warnings when
Float.Int()conversion loses precision.
400-409: LGTM! Defensive copy prevents aliasing.The method now creates a defensive copy of the input slice (line 407), preventing external mutations from affecting the updater's internal state. This addresses the earlier review concern about shared mutable slices.
411-420: LGTM! Returns defensive copy for safety.The method returns a copy of the internal slice (lines 417-418), preventing callers from modifying the updater's state. This addresses the earlier review concern about exposing internal slices.
471-507: LGTM! Proper locking and non-fatal error handling.The method now acquires the lock before all modifications to
u.tokenIDs(line 474) and uses non-fatal error logging (line 479) instead of crashing the process. These changes address the earlier review concerns about race conditions and harsh failure modes.node/derivation/beacon_test.go (1)
42-42: No action required—function is properly defined and accessible.The function
dataAndHashesFromTxsexists atnode/derivation/beacon.go:224and is defined in the same package as the test file. It is properly accessible and there are no issues with this code.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
token-price-oracle/updater/factory.go (1)
121-127: Binance feed still accepted in config but unimplemented at runtime.The past review comment remains valid:
config.ValidPriceFeedTypes()includesPriceFeedTypeBinance, so users can configure--price-feed-priority binancesuccessfully, but the feed creation will always fail with "binance price feed not yet implemented" at runtime. This leads to a confusing user experience.Consider removing
PriceFeedTypeBinancefromconfig.ValidPriceFeedTypes()until the implementation is ready, so configuration validation matches actual capabilities.token-price-oracle/go.mod (2)
3-3: Go version pinned to 1.24.0 while latest stable is 1.25.4.The past review comment remains valid: Go 1.25.4 is the latest stable version (released 2025-10-31). Evaluate whether pinning to 1.24.0 is intentional or if the project should be updated to the current stable version for security and stability improvements.
10-90: HIGH severity vulnerabilities in dependencies remain unaddressed.The past review comment remains valid: static analysis flags multiple HIGH-severity vulnerabilities in transitive dependencies:
- ethereum/go-ethereum 1.10.26: vulnerable to GO-2023-2046 (fixed in v1.12.1) and GO-2024-2819 (fixed in v1.13.15) - unbounded memory consumption and DoS
- consensys/gnark-crypto 0.16.0: vulnerable to GO-2025-4087 (fixed in v0.18.1) - unchecked memory allocation
- golang-jwt/jwt/v4 4.5.0: vulnerable to GO-2024-3250 (fixed in v4.5.1) and GO-2025-3553 (fixed in v4.5.2) - improper error handling and excessive memory allocation
Required actions:
- Evaluate risk for this service's deployment context
- Plan upgrade path to patched versions: go-ethereum ≥1.13.15, gnark-crypto ≥0.18.1, jwt/v4 ≥4.5.2
- Verify the morph-l2 go-ethereum fork includes necessary security patches
- Document threat model and implement mitigations if upgrades are deferred
token-price-oracle/config/config.go (2)
23-29: Binance feed type still exposed in ValidPriceFeedTypes but unimplemented.The past review comment remains valid:
PriceFeedTypeBinanceis included inValidPriceFeedTypes(), allowing users to configure"binance"in--price-feed-priority, but the actual feed implementation always returns "binance price feed not yet implemented" (seeupdater/factory.go). This creates a confusing UX where valid configuration fails at runtime.Consider removing
PriceFeedTypeBinancefromValidPriceFeedTypes()until the feed is implemented, or add explicit validation at config load time.
69-70: LogTerminal field declared but never populated.The past review comment remains valid:
Configdefines theLogTerminalfield butLoadConfig(lines 77-206) never sets it. Either wire it from a CLI flag (e.g.,ctx.Bool(flags.LogTerminalFlag.Name)) or remove the field if terminal logging is not supported.node/derivation/beacon_test.go (1)
35-37: Critical: Remove stale error check (duplicate issue).Line 36 calls
require.NoError(t, err)buterrhasn't been reassigned since line 32. This checks a stale error value from the previous operation. Line 35 only assigns totxHash, noterr.Apply this diff:
for _, lg := range logs { txHash := lg.TxHash - require.NoError(t, err) block, err := l1Client.BlockByNumber(context.Background(), big.NewInt(int64(lg.BlockNumber)))
🧹 Nitpick comments (4)
token-price-oracle/Makefile (1)
31-38: Address checkmake linting preference on help target body length.The static analysis tool
checkmakeflags thehelptarget for exceeding a body length of 5 lines (it has 7). While the current content is valuable, you can either:
- Suppress the warning by adding a
# nolintcomment if the project ignores this rule, or- Split the help into a more concise version.
If your project's Makefile standard enforces this rule, consider condensing the help text or restructuring it.
node/derivation/beacon_test.go (3)
19-21: Uset.Skip()for clarity in integration tests.The test silently returns when the environment variable is not set. Using
t.Skip()would make the test's behavior more explicit and show up properly in test reports.Apply this diff:
url := os.Getenv("BLOB_URL") if url == "" { - return + t.Skip("Set BLOB_URL environment variable to run this integration test") }
23-24: Add comments to explain hard-coded block numbers.The hard-coded block range
1590159lacks context. Adding a comment would clarify why this specific block is used for testing.Apply this diff:
var ( + // Block 1590159 is used for testing blob retrieval (contains blob transactions) start uint64 = 1590159 end uint64 = 1590159 )
60-60: Consider extracting hard-coded contract address to a constant.The hard-coded rollup contract address
0x511d92b63ae7471fd5239bded29b76a446698a00should be extracted to a test constant or passed as a parameter for better maintainability.Apply this diff at the package level:
+const ( + testRollupContractAddress = "0x511d92b63ae7471fd5239bded29b76a446698a00" +) + func testTchRollupLog(l1Client *ethclient.Client, ctx context.Context, from, to uint64) ([]eth.Log, error) { - RollupContractAddress := common.HexToAddress("0x511d92b63ae7471fd5239bded29b76a446698a00") + RollupContractAddress := common.HexToAddress(testRollupContractAddress)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
token-price-oracle/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
node/derivation/beacon_test.go(1 hunks)token-price-oracle/Makefile(1 hunks)token-price-oracle/config/config.go(1 hunks)token-price-oracle/flags/flags.go(1 hunks)token-price-oracle/go.mod(1 hunks)token-price-oracle/local.sh(1 hunks)token-price-oracle/updater/factory.go(1 hunks)token-price-oracle/updater/token_price.go(1 hunks)token-price-oracle/updater/tx_manager.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- token-price-oracle/flags/flags.go
🧰 Additional context used
🧬 Code graph analysis (5)
token-price-oracle/updater/factory.go (7)
token-price-oracle/config/config.go (4)
Config(48-75)PriceFeedType(16-16)PriceFeedTypeBitget(19-19)PriceFeedTypeBinance(20-20)token-price-oracle/client/l2_client.go (1)
L2Client(15-19)token-price-oracle/updater/tx_manager.go (2)
TxManager(17-20)NewTxManager(23-27)token-price-oracle/updater/token_price.go (2)
PriceUpdater(19-29)NewPriceUpdater(32-51)bindings/bindings/l2tokenregistry.go (1)
NewL2TokenRegistry(138-144)token-price-oracle/client/price_feed.go (2)
PriceFeed(20-26)NewFallbackPriceFeed(38-44)token-price-oracle/client/bitget_sdk.go (1)
NewBitgetSDKPriceFeed(52-62)
node/derivation/beacon_test.go (3)
node/derivation/base_client.go (1)
NewBasicHTTPClient(29-37)node/derivation/beacon.go (2)
NewL1BeaconClient(36-38)L1BlockRef(96-101)node/derivation/derivation.go (1)
RollupEventTopicHash(36-36)
token-price-oracle/updater/tx_manager.go (1)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)
token-price-oracle/config/config.go (2)
bindings/predeploys/addresses.go (1)
L2TokenRegistryAddr(60-60)token-price-oracle/flags/flags.go (17)
L2EthRPCFlag(17-22)PrivateKeyFlag(24-29)MetricsServerEnableFlag(124-128)MetricsHostnameFlag(130-135)MetricsPortFlag(137-142)LogLevelFlag(90-95)LogFilenameFlag(97-101)LogFileMaxSizeFlag(103-108)LogFileMaxAgeFlag(110-115)LogCompressFlag(117-121)PriceUpdateIntervalFlag(40-45)PriceThresholdFlag(47-52)PriceFeedPriorityFlag(54-59)TokenMappingBitgetFlag(61-66)TokenMappingBinanceFlag(68-73)BitgetAPIBaseURLFlag(75-80)BinanceAPIBaseURLFlag(82-87)
token-price-oracle/updater/token_price.go (4)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)token-price-oracle/client/price_feed.go (2)
PriceFeed(20-26)TokenPrice(12-17)token-price-oracle/updater/tx_manager.go (1)
TxManager(17-20)token-price-oracle/metrics/metrics.go (2)
UpdateErrors(13-19)AccountBalance(22-27)
🪛 checkmake (0.2.2)
token-price-oracle/Makefile
[warning] 31-31: Target body for "help" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
🪛 OSV Scanner (2.2.4)
token-price-oracle/go.mod
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: Unchecked memory allocation during vector deserialization in github.com/consensys/gnark-crypto
(GO-2025-4087)
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: gnark-crypto allows unchecked memory allocation during vector deserialization
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Unbounded memory consumption in github.com/ethereum/go-ethereum
(GO-2023-2046)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Denial of Service in github.com/ethereum/go-ethereum
(GO-2024-2819)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to DoS via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Go-Ethereum vulnerable to denial of service via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to denial of service via crafted GraphQL query
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
token-price-oracle/Makefile (2)
15-15: Ensure go mod download success and build repeatability.The
go mod downloadstep on line 14 is good for offline builds, but verify that it succeeds in the CI pipeline (a prior review flagged issues withgo.workmodule paths). Also confirm that the build environment hasgolangci-lintinstalled before running thelinttarget, and that CGO build dependencies (-ldl) are available in the build environment.
7-10: No action required—LDFLAGS configuration is verified correct.All three variables (
GitVersion,GitCommit,GitDate) are properly declared at package level incmd/main.go(lines 24–26). The ldflags configuration in the Makefile correctly targets these variables, and they will be injected at link time as intended.token-price-oracle/local.sh (1)
1-14: LGTM!The local development script properly wires the new
--bitget-api-base-urlflag and uses appropriate defaults for local testing. The hardcoded private key is a standard test account from Hardhat, which is acceptable for local development.token-price-oracle/updater/factory.go (1)
14-67: Well-structured factory with proper error handling.The
CreatePriceUpdaterfunction correctly validates the registry address, binds the contract, creates feeds with fallback support, and aggregates token mappings by priority. Error handling and logging are comprehensive.token-price-oracle/updater/tx_manager.go (2)
29-72: LGTM! Gas estimation properly implemented.The
SendTransactionmethod correctly implements gas estimation with a 1.5x buffer, ensuring transactions have sufficient gas while avoiding excessive waste. The mutex ensures serialization to prevent nonce conflicts. Logging is comprehensive for debugging.
74-116: Robust receipt waiting with proper timeout handling.The
waitForReceipthelper implements proper polling with timeout, context cancellation support, and graceful handling of transient errors. The implementation is production-ready.token-price-oracle/config/config.go (1)
77-206: Comprehensive config loading with proper validation.The
LoadConfigfunction implements thorough validation for all configuration parameters, including interval ranges, threshold bounds, price feed priority parsing (with duplicate detection), token mapping validation, and API URL checks. Error messages are descriptive and actionable.token-price-oracle/updater/token_price.go (5)
18-29: Proper concurrency primitives in place.The
PriceUpdaterstruct correctly usessync.Onceto ensure safe shutdown viastopChan, addressing the past review concern about double-close panics.
83-91: Thread-safe Stop implementation.The
Stopmethod correctly usesstopOnce.Doto ensure the channel is closed exactly once, making it safe to call multiple times. This addresses the past review concern.
229-313: Comprehensive price ratio calculation with validation.The
calculatePriceRatiofunction implements proper input validation, fetches token info from the contract, validates token state (active status, decimal bounds), performs the price calculation with correct order of operations to minimize precision loss, and checks conversion accuracy. The precision check at lines 293-301 properly logs when Float-to-Int conversion loses precision, addressing the past review concern.
341-376: Robust threshold checking with safety bounds.The
shouldUpdatePricefunction validates inputs, handles edge cases (zero lastPrice, nil values), caps unreasonable thresholds at 100%, and performs percentage calculation using integer arithmetic to avoid floating-point precision issues.
94-227: Well-orchestrated update workflow with proper error handling.The
updatemethod implements a comprehensive workflow: fetches token IDs from contract, retrieves prices from the feed, calculates ratios, filters by threshold, and submits batch updates via the transaction manager. Error handling is graceful with appropriate logging at each stage. The use ofdeferfor balance metrics ensures they're updated regardless of outcome.node/derivation/beacon_test.go (1)
26-26: Original review comment is incorrect—the logger field is never used.Verification shows that
BasicHTTPClient.Get()method (the only method of this type) never references thelogfield. Passingnilis safe because the field is never dereferenced. No fix is required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
bindings/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumoracle/go.sumis excluded by!**/*.sumtoken-price-oracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
MakefileEks.mk(1 hunks)bindings/go.mod(1 hunks)contracts/go.mod(1 hunks)node/derivation/beacon_test.go(1 hunks)node/go.mod(2 hunks)ops/l2-genesis/go.mod(2 hunks)ops/tools/go.mod(1 hunks)oracle/go.mod(2 hunks)token-price-oracle/go.mod(1 hunks)tx-submitter/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- node/go.mod
- node/derivation/beacon_test.go
- tx-submitter/go.mod
- ops/l2-genesis/go.mod
- MakefileEks.mk
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
token-price-oracle/go.mod
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: Unchecked memory allocation during vector deserialization in github.com/consensys/gnark-crypto
(GO-2025-4087)
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: gnark-crypto allows unchecked memory allocation during vector deserialization
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Unbounded memory consumption in github.com/ethereum/go-ethereum
(GO-2023-2046)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Denial of Service in github.com/ethereum/go-ethereum
(GO-2024-2819)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to DoS via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Go-Ethereum vulnerable to denial of service via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to denial of service via crafted GraphQL query
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
oracle/go.mod (1)
10-10: Dependency updates align with repository-wide refresh.The go-ethereum pseudo-version, urfave/cli, and go-md2man updates are consistent with coordinated dependency maintenance across related modules. These are routine maintenance updates.
Verify that the morph-l2 go-ethereum fork v1.10.14-0.20251120124625-16a606312846 includes security patches addressing GO-2023-2046 and GO-2024-2819 (DoS vulnerabilities fixed in upstream v1.12.1 and v1.13.15 respectively). If not backported, document the threat model and any mitigations in place.
Also applies to: 14-14, 27-27
bindings/go.mod (1)
7-7: Dependency update is routine.The go-ethereum update aligns with other modules in the PR.
contracts/go.mod (1)
9-9: Dependency update is consistent across modules.ops/tools/go.mod (1)
8-8: Dependency update is consistent across modules.token-price-oracle/go.mod (2)
3-3: Clarify Go version 1.24.0 pinning.Go 1.24.0 is specified, but current stable is 1.25.4 (released 2025-10-31). Determine whether pinning to 1.24.0 is intentional (e.g., compatibility requirement) or should be updated to the latest stable version for security and stability benefits.
40-40: I need to verify the vulnerability claims in the review comment. Let me search for information on these Go security advisories.Let me search for the specific gnark-crypto vulnerability ID to complete the verification:
Address HIGH-severity vulnerabilities in transitive dependencies; note GO-2025-4087 could not be verified.
Go-ethereum has improper error handling in ParseWithClaims (GO-2024-3250), with a fix in v4.5.1, and excessive memory allocation during header parsing (GO-2025-3553) affecting jwt/v4 before v4.5.2. The current v4.5.0 is vulnerable to both. go-ethereum can be made to consume very large amounts of memory from specially crafted p2p messages, causing denial of service (GO-2024-2819), and this affects v1.10.26 which was released in 2021. The fix is in geth v1.12.1 or later.
GO-2025-4087 for gnark-crypto could not be verified in the Go Vulnerability Database; web searches found unchecked memory allocation vulnerabilities in gnark-crypto but did not return this specific ID.
Per past review feedback, required actions:
- Document whether these vulnerabilities pose acceptable risk for this service's deployment context
- Confirm the correct vulnerability ID for gnark-crypto and plan upgrade path
- Plan upgrade path: go-ethereum ≥1.13.15, jwt/v4 ≥4.5.2
- Verify whether the morph-l2 go-ethereum fork includes backports of these security patches
- If upgrades are deferred, implement mitigations (input validation, rate-limiting on price feeds, DoS detection)
Also applies to: 26-26, 34-34
token-price-oracle/go.mod
Outdated
| ) | ||
|
|
||
| require ( | ||
| github.com/morph-l2/go-ethereum v1.10.14-0.20251121022222-5ecdbc91b324 |
There was a problem hiding this comment.
🧩 Analysis chain
This file uses go-ethereum v1.10.14-0.20251121022222-5ecdbc91b324 (Nov 21 timestamp) while oracle, bindings, contracts, and ops/tools all use v1.10.14-0.20251120124625-16a606312846 (Nov 20 timestamp). This version mismatch will likely cause go mod tidy conflicts and inconsistent behavior across the workspace.
Align this to the same pseudo-version used by other modules:
- github.com/morph-l2/go-ethereum v1.10.14-0.20251121022222-5ecdbc91b324
+ github.com/morph-l2/go-ethereum v1.10.14-0.20251120124625-16a606312846🏁 Script executed:
#!/bin/bash
# Find all go.mod files in the repository
echo "=== Finding all go.mod files ==="
find . -name "go.mod" -type f | sort
echo ""
echo "=== go-ethereum versions in all go.mod files ==="
# Search for go-ethereum versions in all go.mod files
find . -name "go.mod" -type f -exec grep -H "github.com/morph-l2/go-ethereum" {} \;
echo ""
echo "=== Go version pins in all go.mod files ==="
# Check Go version declarations
find . -name "go.mod" -type f -exec sh -c 'echo "--- {} ---"; head -5 "$1"' _ {} \;Length of output: 2241
All 7 other modules (oracle, bindings, contracts, node, ops/tools, ops/l2-genesis, tx-submitter) use go-ethereum v1.10.14-0.20251120124625-16a606312846 (Nov 20 timestamp), but token-price-oracle uses v1.10.14-0.20251121022222-5ecdbc91b324 (Nov 21 timestamp). This version mismatch will cause go mod tidy conflicts and inconsistent behavior across the workspace.
Align this to match the version used by all other modules:
- github.com/morph-l2/go-ethereum v1.10.14-0.20251121022222-5ecdbc91b324
+ github.com/morph-l2/go-ethereum v1.10.14-0.20251120124625-16a606312846📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| github.com/morph-l2/go-ethereum v1.10.14-0.20251121022222-5ecdbc91b324 | |
| github.com/morph-l2/go-ethereum v1.10.14-0.20251120124625-16a606312846 |
🤖 Prompt for AI Agents
In token-price-oracle/go.mod around line 11, the github.com/morph-l2/go-ethereum
pseudo-version differs from the rest of the repo (uses
v1.10.14-0.20251121022222-5ecdbc91b324); update it to the canonical
pseudo-version used by the other modules v1.10.14-0.20251120124625-16a606312846
so all modules reference the same go-ethereum pseudo-version, then run `go mod
tidy` to ensure go.sum and module graph are consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
MakefileEks.mk (2)
108-116: Add missing secret-manager-wrapper provisioning stepsThe build target is missing the
aws s3 cpandtar -xvzfcommands to fetch and extractsecret-manager-wrapper.tar.gz, but the start target (line 115) invokes/data/secret-manager-wrapper. This issue was previously identified.Add these lines before line 112:
cp token-price-oracle/token-price-oracle dist/ + aws s3 cp s3://morph-0582-morph-technical-department-testnet-data/testnet/hoodi/morph-setup/secret-manager-wrapper.tar.gz ./ + tar -xvzf secret-manager-wrapper.tar.gz
118-126: Add missing secret-manager-wrapper provisioning stepsThe build target is missing the
aws s3 cpandtar -xvzfcommands to fetch and extractsecret-manager-wrapper.tar.gz, but the start target (line 125) invokes/data/secret-manager-wrapper. This issue was previously identified.Add these lines before line 122:
cp token-price-oracle/token-price-oracle dist/ + aws s3 cp s3://morph-0582-morph-technical-department-mainnet-data/morph-setup/secret-manager-wrapper.tar.gz ./ + tar -xvzf secret-manager-wrapper.tar.gz
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MakefileEks.mk(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
token-price-oracle/go.mod (2)
3-3: Update Go version to latest stable.Go 1.24.0 is pinned, but the latest stable version is go1.25.4 (released 2025-10-31). Unless there is a specific reason to pin to 1.24.0, update the go directive to 1.25.4 and run
go mod tidyto ensure compatibility.
11-11: Align go-ethereum pseudo-version with other repo modules.The go-ethereum pseudo-version on line 11 (v1.10.14-0.20251125061742-69718a9dcab9, Nov 25 timestamp) diverges from the canonical version used by all other modules (v1.10.14-0.20251120124625-16a606312846, Nov 20 timestamp). This mismatch will cause
go mod tidyconflicts and inconsistent behavior across the workspace.Update to match:
- github.com/morph-l2/go-ethereum v1.10.14-0.20251125061742-69718a9dcab9 + github.com/morph-l2/go-ethereum v1.10.14-0.20251120124625-16a606312846Then run
go mod tidyto ensure the module graph is consistent.
🧹 Nitpick comments (1)
token-price-oracle/Makefile (1)
34-43: Help target body length exceeds linter threshold.The help target body contains 9 lines but checkmake enforces a maximum of 5 lines (maxbodylength). While the help output is well-formatted and useful, consider refactoring into a separate shell script or adding a linter exception if enforcing the strict body length limit.
Optional: If retaining this structure is preferred, you can suppress the linting warning with a checkmake directive (
#checkmake: disable=maxbodylength).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bindings/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumoracle/go.sumis excluded by!**/*.sumtoken-price-oracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
node/go.mod(2 hunks)ops/l2-genesis/go.mod(2 hunks)oracle/go.mod(2 hunks)token-price-oracle/Makefile(1 hunks)token-price-oracle/go.mod(1 hunks)tx-submitter/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ops/l2-genesis/go.mod
- tx-submitter/go.mod
- node/go.mod
🧰 Additional context used
🪛 checkmake (0.2.2)
token-price-oracle/Makefile
[warning] 34-34: Target body for "help" exceeds allowed length of 5 (9).
(maxbodylength)
🪛 OSV Scanner (2.2.4)
token-price-oracle/go.mod
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: Unchecked memory allocation during vector deserialization in github.com/consensys/gnark-crypto
(GO-2025-4087)
[HIGH] 1-1: github.com/consensys/gnark-crypto 0.16.0: gnark-crypto allows unchecked memory allocation during vector deserialization
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Unbounded memory consumption in github.com/ethereum/go-ethereum
(GO-2023-2046)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Denial of Service in github.com/ethereum/go-ethereum
(GO-2024-2819)
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to DoS via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: Go-Ethereum vulnerable to denial of service via malicious p2p message
[HIGH] 1-1: github.com/ethereum/go-ethereum 1.10.26: go-ethereum vulnerable to denial of service via crafted GraphQL query
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 1-1: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
oracle/go.mod (1)
14-14: Dependency versions updated appropriately.The urfave/cli and go-md2man/v2 version bumps align with parallel updates across other modules in the codebase, ensuring consistent dependency management.
Also applies to: 27-27
token-price-oracle/go.mod (1)
10-88: I'll help you verify this review comment. Let me gather information about the dependencies and vulnerabilities mentioned.Based on my verification, I found that the CVE information in the review comment is substantially accurate, but there are important clarifications and corrections needed regarding version numbers and transitive dependency management:
Verify direct access to code and go mod why output to confirm vulnerability path and dependency structure.
The vulnerabilities mentioned are confirmed:
- ethereum/go-ethereum v1.10.26 vulnerabilities are fixed in v1.12.1 and v1.13.15
- golang-jwt/jwt/v4 v4.5.0 has improper error handling in ParseWithClaims, documented in GHSA-29wx-vh33-7x7r and fixed in v4.5.1
- v4.5.2 addresses GHSA-mh63-6h87-95cp (the excessive memory allocation vulnerability)
- GHSA-fj2x-735w-74vq affects github.com/consensys/gnark-crypto for unchecked memory allocation during vector deserialization
However, critical clarifications are needed:
ethereum/go-ethereum dependency: The review says this is transitive via morph-l2/go-ethereum v1.10.14-..., meaning you cannot directly upgrade ethereum/go-ethereum. The actual vulnerability path and whether the morph-l2 fork has backported patches requires verification.
gnark-crypto fix version: The review recommends v0.18.1, but the latest confirmed release is v0.18.0. Verify if v0.18.1 exists or if v0.18.0 includes the needed fix for GHSA-fj2x-735w-74vq.
Transitive dependency control: Go modules may allow constraining transitive dependencies, but this depends on whether the direct dependencies (morph-l2/go-ethereum, consensys/gnark-crypto imports) expose their sub-dependencies in go.mod.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
token-price-oracle/config/config.go (1)
68-75:LogTerminalremains unused inConfigand is never populated.
ConfigdefinesLogTerminal(Line 70), butLoadConfigdoesn’t assign it from any flag or env, so it will always befalseunless manually set elsewhere. This mirrors the concern from the earlier review.Either:
- Wire it from the appropriate CLI flag, e.g.
LogTerminal: ctx.Bool(flags.LogTerminalFlag.Name),if such a flag exists, or- Remove
LogTerminal(and any related flag/docs) if terminal logging isn’t actually supported.Also applies to: 77-92
🧹 Nitpick comments (2)
token-price-oracle/README.md (2)
1-41: README appears to still describe the gas-price-oracle service rather than the new token-price-oracle/token price updater.The doc consistently references “Gas Price Oracle”,
GAS_ORACLE_*env vars, base-fee/scalar updaters, and agas-price-oracle/directory tree, while the code in this module exposes token price–specific config (price feed priority, Bitget/Binance mappings and API base URLs, token registry, etc.). This can easily mislead operators wiring the new service.Consider updating:
- Service name, binary name, and env prefix to match the actual token-price-oracle binary and flags.
- Configuration sections to document
--price-feed-priority,--token-mapping-*, and Bitget/Binance API base URL flags fromconfig.Config/flags.- The architecture tree path (
gas-price-oracle/) to reflect the correct module layout.Also applies to: 71-97, 118-132
118-132: Add a language identifier to the architecture fenced code block.The block starting at line 118 uses plain ``` with no language, which triggers MD040 and loses syntax highlighting. Since it’s directory structure, something like:
```text ...would satisfy markdownlint and keep intent clear. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9daceaca44ddddba5443146fbd50ef7cd15f8dd6 and d1289bb1b87061fb9d85a1aa1e9c23097485f7b8. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `token-price-oracle/README.md` (1 hunks) * `token-price-oracle/config/config.go` (1 hunks) * `token-price-oracle/updater/factory.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>token-price-oracle/updater/factory.go (6)</summary><blockquote> <details> <summary>token-price-oracle/config/config.go (4)</summary> * `Config` (48-75) * `PriceFeedType` (16-16) * `PriceFeedTypeBitget` (19-19) * `PriceFeedTypeBinance` (20-20) </details> <details> <summary>token-price-oracle/client/l2_client.go (1)</summary> * `L2Client` (15-19) </details> <details> <summary>token-price-oracle/updater/tx_manager.go (2)</summary> * `TxManager` (17-20) * `NewTxManager` (23-27) </details> <details> <summary>bindings/bindings/l2tokenregistry.go (1)</summary> * `NewL2TokenRegistry` (138-144) </details> <details> <summary>token-price-oracle/client/price_feed.go (2)</summary> * `PriceFeed` (20-26) * `NewFallbackPriceFeed` (38-44) </details> <details> <summary>token-price-oracle/client/bitget_sdk.go (1)</summary> * `NewBitgetSDKPriceFeed` (52-62) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>token-price-oracle/README.md</summary> 118-118: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)</summary> * GitHub Check: Analyze (rust) * GitHub Check: Analyze (go) * GitHub Check: check * GitHub Check: test * GitHub Check: test * GitHub Check: test * GitHub Check: check * GitHub Check: test </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>token-price-oracle/updater/factory.go (1)</summary><blockquote> `13-104`: **Price updater factory and fallback feed construction look solid.** The factory cleanly binds the token registry, builds a prioritized fallback price feed, and aggregates token mappings respecting priority (first mapping wins). Error paths for missing feeds and registry binding are explicit, and misconfigured feeds are logged and skipped rather than failing the whole updater, which is a reasonable trade-off. No changes requested here. </blockquote></details> <details> <summary>token-price-oracle/config/config.go (1)</summary><blockquote> `15-29`: **Config now correctly restricts valid feed types to implemented ones.** Limiting `ValidPriceFeedTypes` to `bitget` and leaving `binance` commented with a TODO keeps CLI validation aligned with runtime support while preserving the enum for future use. This resolves the earlier mismatch between accepted types and implemented feeds. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
token-price-oracle/config/config.go (1)
68-91: Remove unusedLogTerminalfield.The
LogTerminalfield (line 70) is declared inConfigbut never populated inLoadConfig(lines 77–91), and there is no correspondingLogTerminalFlaginflags.go. This appears to be a leftover from an earlier design.Apply this diff to remove the dead field:
// Logging LogLevel string - LogTerminal bool LogFilename string LogFileMaxSize int LogFileMaxAge int LogCompress bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
token-price-oracle/config/config.go(1 hunks)token-price-oracle/env.example(1 hunks)token-price-oracle/flags/flags.go(1 hunks)token-price-oracle/local.sh(1 hunks)token-price-oracle/updater/token_price.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- token-price-oracle/local.sh
🧰 Additional context used
🧬 Code graph analysis (2)
token-price-oracle/updater/token_price.go (4)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)token-price-oracle/client/price_feed.go (2)
PriceFeed(20-26)TokenPrice(12-17)token-price-oracle/updater/tx_manager.go (1)
TxManager(17-20)token-price-oracle/metrics/metrics.go (2)
UpdateErrors(13-19)AccountBalance(22-27)
token-price-oracle/config/config.go (2)
bindings/predeploys/addresses.go (1)
L2TokenRegistryAddr(60-60)token-price-oracle/flags/flags.go (9)
L2EthRPCFlag(17-22)PrivateKeyFlag(24-29)PriceUpdateIntervalFlag(40-45)PriceThresholdFlag(47-52)PriceFeedPriorityFlag(54-59)TokenMappingBitgetFlag(61-66)TokenMappingBinanceFlag(68-73)BitgetAPIBaseURLFlag(75-80)BinanceAPIBaseURLFlag(82-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
token-price-oracle/env.example (1)
1-40: LGTM!The environment configuration example is well-documented and consistent with the CLI flag definitions in
flags.go. The basis points unit is now clearly documented (line 15), and all variable names match the expectedTOKEN_PRICE_ORACLE_prefix.token-price-oracle/config/config.go (1)
15-45: LGTM!The price feed type validation correctly excludes the unimplemented Binance feed (line 27). Users cannot configure
"binance"until the feed is implemented, preventing runtime failures.token-price-oracle/updater/token_price.go (6)
18-51: LGTM!The struct design with
sync.Oncefor safe shutdown (line 28) and the constructor are well-implemented. ThetokenMappingis effectively immutable after construction, so no mutex is needed for it.
53-91: LGTM!The lifecycle methods are properly implemented with concurrent-safe shutdown via
sync.Once(lines 86-89). The goroutine correctly handles both context cancellation and explicit stop signals.
93-227: LGTM!The update workflow is comprehensive and handles errors gracefully:
- Empty token lists are handled without crashing (lines 108-111)
- Individual price calculation failures are logged and skipped (lines 124-128)
- Receipt status is properly validated (lines 206-212)
- Local snapshots prevent data races (line 189 uses local
tokenIDs)
229-313: LGTM!The price ratio calculation is thorough and handles edge cases well:
- Comprehensive input validation (lines 233-274)
- Proper use of
big.Floatfor precision (lines 276-290)- Precision loss is detected and logged (lines 293-301)
- Detailed logging aids debugging (lines 303-311)
315-387: LGTM!Both helper methods are well-implemented:
updateBalanceMetricsproperly queries and updates the balance metricshouldUpdatePricecorrectly calculates basis points change (lines 366-377) and defensively caps the threshold (lines 357-364)
389-395: LGTM!Clean wrapper around the registry contract's
GetSupportedIDListcall.token-price-oracle/flags/flags.go (1)
1-172: LGTM!The flag definitions are well-structured and address all previous concerns:
- Correct
TOKEN_PRICE_ORACLE_prefix (line 9)- Required flags without conflicting defaults (lines 17-29)
- Comprehensive optional flags with sensible defaults (lines 33-143)
- Clear usage documentation throughout
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
token-price-oracle/config/config.go (1)
92-96:LogTerminalfield is still not populated.The
Config.LogTerminalfield declared at line 75 is never assigned inLoadConfig. This issue was flagged in the previous review but remains unresolved. All other logging fields are populated exceptLogTerminal.If terminal logging is supported, add the assignment:
LogLevel: ctx.String(flags.LogLevelFlag.Name), +LogTerminal: ctx.Bool(flags.LogTerminalFlag.Name), LogFilename: ctx.String(flags.LogFilenameFlag.Name),If terminal logging is not supported, remove the
LogTerminalfield from theConfigstruct at line 75 to avoid maintaining unused fields.
🧹 Nitpick comments (3)
token-price-oracle/updater/token_price.go (3)
19-52: ClarifyPriceUpdaterAPI aroundtokenMappingand mutation semantics.
tokenMappingis accepted and stored but not used in this file. If other package files don’t use it yet, consider dropping the field/parameter to avoid confusion. If it is meant for future/other‑file use, consider copying the map inNewPriceUpdater(e.g.,make+for+range) or documenting that callers must not mutate it after construction, to avoid surprising shared‑state changes in a running updater.
94-228: Tightenupdatelog messages to match actual behavior and edge cases.A couple of log strings are slightly misleading relative to current control flow:
- Line 105:
"Failed to fetch tokenIDs from contract, price updater will not start"is no longer accurate now thatupdateis called on every ticker cycle. It’s really “this cycle won’t run”, not “updater will not start”.- Line 183:
"No prices need updating (all changes below threshold)"will also be hit whennewPriceRatiosis empty because earlier steps failed/skipped tokens (e.g., price feed errors orcalculatePriceRatiofailures). That message implies only “below threshold”, which may hide upstream issues.Suggestion: rephrase these logs to something like “skipping price update cycle” / “no token prices qualified for update; see earlier warnings for skipped tokens” so operators can distinguish configuration/contract issues from benign “no change” scenarios.
342-389: Document threshold semantics, especially for 0 and “>100%” values.The basis‑points computation and capping to
MaxPriceThresholdBPSlook correct. Behavior for a couple of edge values is worth calling out in docs/config comments:
priceThreshold == 0effectively means “update on any change” (with non‑zerolastPrice).priceThreshold > MaxPriceThresholdBPSgets clamped to 100% with a warning.Explicitly documenting these in config or CLI help will make it easier for operators to choose sensible thresholds and understand why updates are/aren’t happening.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
token-price-oracle/config/config.go(1 hunks)token-price-oracle/updater/token_price.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
token-price-oracle/config/config.go (3)
bindings/predeploys/addresses.go (1)
L2TokenRegistryAddr(60-60)node/flags/flags.go (1)
LogLevel(244-248)token-price-oracle/flags/flags.go (9)
L2EthRPCFlag(17-22)PrivateKeyFlag(24-29)PriceUpdateIntervalFlag(40-45)PriceThresholdFlag(47-52)PriceFeedPriorityFlag(54-59)TokenMappingBitgetFlag(61-66)TokenMappingBinanceFlag(68-73)BitgetAPIBaseURLFlag(75-80)BinanceAPIBaseURLFlag(82-87)
token-price-oracle/updater/token_price.go (5)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)token-price-oracle/client/price_feed.go (2)
PriceFeed(20-26)TokenPrice(12-17)token-price-oracle/updater/tx_manager.go (1)
TxManager(17-20)token-price-oracle/metrics/metrics.go (2)
UpdateErrors(13-19)AccountBalance(22-27)token-price-oracle/config/config.go (1)
MaxPriceThresholdBPS(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
token-price-oracle/config/config.go (2)
28-34: Good fix - Binance feed properly excluded from validation.The previous review concern about exposing
binanceas a valid feed type when it's not implemented has been correctly addressed by commenting it out fromValidPriceFeedTypes(). Now users cannot configurebinancein the priority list until it's implemented.
108-145: Comprehensive validation logic for price configuration.The validation logic properly handles:
- Price threshold range checking with clear error messages (lines 108-111)
- Price feed priority list parsing with duplicate detection (lines 119-141)
- Empty input rejection (lines 115-117, 143-145)
- Type validation with helpful error messages listing valid types (lines 128-134)
token-price-oracle/updater/token_price.go (4)
84-92:Stopis now idempotent and concurrency‑safe.Using
sync.Oncearoundclose(u.stopChan)cleanly avoids double‑close panics and makesStop()safe to call from multiple goroutines. This is a solid improvement over the usual bareclosepattern.
230-314: Price‑ratio calculation and validation look consistent with the documented formula.Input validation (nil checks, active flag, decimals ≤ 18, non‑zero/positive USD prices) plus the
tokenScale * tokenPriceUSD * 10^(18‑decimals) / ethPriceUSDordering matches the comment and avoids obvious precision pitfalls. Logging and the accuracy check on the float→int conversion give good observability into any rounding behavior.
316-340: Balance‑metrics update is straightforward and appropriately isolated.Deferring
updateBalanceMetricsfromupdatekeeps balance reporting decoupled from price‑update success, and the ETH balance conversion to a gauge is clear. Current behavior (leaving the previous gauge value on error and just logging) is reasonable for a first iteration.
391-397: Contract token‑ID fetch wrapper looks good.Using
bind.CallOpts{Context: ctx}and delegating toGetSupportedIDListkeeps this helper minimal and testable; the higher‑level error handling and logging inupdatecomplete the picture.
| // Start starts the price updater | ||
| func (u *PriceUpdater) Start(ctx context.Context) error { | ||
| go func() { | ||
| log.Info("Price updater starting", "interval", u.interval) | ||
| ticker := time.NewTicker(u.interval) | ||
| defer ticker.Stop() | ||
|
|
||
| if err := u.update(ctx); err != nil { | ||
| log.Error("Initial price update failed") | ||
| } | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| log.Info("Price updater stopped by context") | ||
| return | ||
| case <-u.stopChan: | ||
| log.Info("Price updater stopped") | ||
| return | ||
| case <-ticker.C: | ||
| if err := u.update(ctx); err != nil { | ||
| log.Error("Failed to update prices") | ||
| metrics.UpdateErrors.WithLabelValues("price").Inc() | ||
| } | ||
| } | ||
| } | ||
| }() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Include error context (and possibly metrics) when update fails in Start.
Both for the initial call and the ticker loop you log only a static message and drop the returned error. That makes it hard to diagnose issues where update fails before it logs anything (e.g., upstream/network problems).
Consider at least adding the error as a structured field:
- if err := u.update(ctx); err != nil {
- log.Error("Initial price update failed")
- }
+ if err := u.update(ctx); err != nil {
+ log.Error("Initial price update failed", "error", err)
+ }
...
- if err := u.update(ctx); err != nil {
- log.Error("Failed to update prices")
- metrics.UpdateErrors.WithLabelValues("price").Inc()
- }
+ if err := u.update(ctx); err != nil {
+ log.Error("Failed to update prices", "error", err)
+ metrics.UpdateErrors.WithLabelValues("price").Inc()
+ }Optionally, you could also increment UpdateErrors on the initial failure for consistent metrics semantics.
🤖 Prompt for AI Agents
In token-price-oracle/updater/token_price.go around lines 54 to 82, the Start
function currently logs only static messages when u.update(ctx) fails and drops
the returned error; change it to include the returned error as a structured/log
field (e.g., log.Error("...", "err", err)) for both the initial update and the
ticker loop, and increment metrics.UpdateErrors.WithLabelValues("price").Inc()
on the initial failure as well so metrics and logs consistently reflect
failures.
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.