feed ids #10
feed ids #10
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds five Crypto USD price-feed constants to LibPyth, updates getPriceFeedId to recognize crypto symbols (checked before equity), adjusts a test signature in the gas snapshot, and extends tests to assert the new constants and price/no-older-than behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Tests
participant Lib as LibPyth
participant Const as PRICE_FEED_ID_*
rect rgb(245,250,255)
Note over Test,Lib: Resolve feed ID for an IntOrAString symbol
Test->>Lib: getPriceFeedId(feedSymbolIntOrAString)
alt symbol matches Crypto.* / USD
Lib->>Const: match crypto branch -> return PRICE_FEED_ID_CRYPTO_*
Const-->>Lib: PRICE_FEED_ID_CRYPTO_*
Lib-->>Test: bytes32 crypto feed id
else other (equity/previous cases)
Lib->>Const: evaluate equity branches -> return PRICE_FEED_ID_*
Const-->>Lib: PRICE_FEED_ID_*
Lib-->>Test: bytes32 feed id
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/pyth/LibPyth.sol (1)
127-161: Consider addressing the O(1) lookup table TODO.The
getPriceFeedIdfunction now contains 15+ sequential if-else branches. With the addition of 5 crypto feeds, this linear search pattern is becoming less maintainable and incurs increasing gas costs as the feed list grows.The TODO comment at line 124 suggests replacing this with an O(1) lookup table. Given the growing number of feeds, this would be a good time to implement a mapping-based solution. This would provide:
- O(1) lookup time (constant gas cost)
- Better maintainability
- Easier to add new feeds in the future
Would you like me to open an issue to track implementing the O(1) lookup table?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/lib/pyth/LibPyth.sol(2 hunks)test/src/lib/pyth/LibPyth.constants.t.sol(1 hunks)test/src/lib/pyth/LibPyth.getPriceFeedId.t.sol(2 hunks)
🔇 Additional comments (4)
src/lib/pyth/LibPyth.sol (1)
127-136: LGTM: Crypto feeds correctly integrated.The crypto feed branches are correctly implemented and logically placed before equity feeds. Each branch properly maps the IntOrAString symbol to its corresponding price feed ID.
test/src/lib/pyth/LibPyth.constants.t.sol (1)
16-35: LGTM: Comprehensive test coverage for crypto constants.The test assertions correctly validate that each crypto price feed symbol constant matches its expected IntOrAString encoding. The pattern is consistent with the existing equity tests and provides complete coverage of all 5 new crypto feeds.
test/src/lib/pyth/LibPyth.getPriceFeedId.t.sol (2)
16-32: LGTM: Crypto feed mappings correctly tested.The test assertions properly validate that
getPriceFeedIdreturns the correct price feed ID for each crypto symbol. The tests cover all 5 new crypto feeds and follow the established pattern from the equity feed tests.
81-85: LGTM: Fuzz test correctly excludes new crypto feeds.The
vm.assumeconditions properly exclude all 5 new crypto feed symbols from the unknown mappings fuzz test. This ensures the test only validates that truly unsupported symbols revert withUnsupportedFeedSymbol.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit