Merged
Conversation
Test price conversion, confidence subtraction, non-positive price revert, positive and negative exponents, and constructor immutables. Uses vm.mockCall for the Pyth contract. Closes #254 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a new Forge test contract that mocks IPyth responses to validate PythOracle price conversion, confidence subtraction, exponent scaling, revert cases for non-positive conservative prices, and constructor/getter values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/src/concrete/oracle/PythOracle.t.sol`:
- Around line 82-83: The doc comment for the test function testConstruction is
inaccurate: it claims the test verifies event emission and immutables but the
implementation only checks immutables. Either update the comment to only state
that immutables are validated, or extend testConstruction by adding
expectEmit/emit expectations and assertions for the constructor event (use the
contract/event name used in deployment) so the test actually asserts event
emission as described.
- Around line 85-87: The test assertions call non-existent getters
I_PRICE_FEED_ID, I_STALE_AFTER, and I_PYTH_CONTRACT; update the assertions in
PythOracle.t.sol to use the actual auto-generated immutable getters
iPriceFeedId, iStaleAfter, and iPythContract (refer to the PythOracle contract's
public immutables iPriceFeedId, iStaleAfter, iPythContract) so the assertEq
lines compare the correct fields.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4479e6cc-dda0-498d-8496-271e95b92881
📒 Files selected for processing (1)
test/src/concrete/oracle/PythOracle.t.sol
Comment on lines
+82
to
+83
| /// Construction emits event and sets immutables. | ||
| function testConstruction() external { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Doc comment overstates test scope.
Line 82 says construction test validates event emission, but the test only checks immutables. Either add expectEmit assertions or adjust the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/concrete/oracle/PythOracle.t.sol` around lines 82 - 83, The doc
comment for the test function testConstruction is inaccurate: it claims the test
verifies event emission and immutables but the implementation only checks
immutables. Either update the comment to only state that immutables are
validated, or extend testConstruction by adding expectEmit/emit expectations and
assertions for the constructor event (use the contract/event name used in
deployment) so the test actually asserts event emission as described.
Update immutable getter references to camelCase names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/src/concrete/oracle/PythOracle.t.sol (1)
82-83:⚠️ Potential issue | 🟡 MinorDoc comment is still overstating test scope.
Line 82 says event emission is validated, but
testConstructiononly asserts immutables. Update the comment (or addexpectEmit+ event assertions).✏️ Minimal fix
- /// Construction emits event and sets immutables. + /// Construction sets immutables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/oracle/PythOracle.t.sol` around lines 82 - 83, The doc comment for testConstruction incorrectly claims it validates event emission; either update the comment to only state that immutables are asserted or extend testConstruction to call expectEmit and assert the expected event (use expectEmit before the action and then assert the specific PythOracle construction event via its parameters). Locate function testConstruction and either change its comment text to remove the event-emission claim or add expectEmit + assertions for the construction event to match the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/src/concrete/oracle/PythOracle.t.sol`:
- Around line 82-83: The doc comment for testConstruction incorrectly claims it
validates event emission; either update the comment to only state that
immutables are asserted or extend testConstruction to call expectEmit and assert
the expected event (use expectEmit before the action and then assert the
specific PythOracle construction event via its parameters). Locate function
testConstruction and either change its comment text to remove the event-emission
claim or add expectEmit + assertions for the construction event to match the
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b0bddd8-cf99-401a-aaca-6b36ac068ed1
📒 Files selected for processing (1)
test/src/concrete/oracle/PythOracle.t.sol
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PythOraclecovering price conversion, confidence subtraction, non-positive price revert, positive/negative exponents, and constructor immutablesvm.mockCallfor the Pyth contract — no mock contract neededCloses #254
🤖 Generated with Claude Code
Summary by CodeRabbit