added composite oracle #187
Conversation
WalkthroughThe changes introduce the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Outside diff range and nitpick comments (1)
packages/periphery/test/int/others/MarginlyCompositeOracle.spec.ts (1)
9-82: The tests are well-structured and cover multiple composite scenarios. However, ensure that the print statements are intended for debugging purposes only and consider removing them before deployment to maintain clean and professional code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (1 hunks)
- packages/periphery/contracts/test/MockPriceOracleV2.sol (1 hunks)
- packages/periphery/hardhat-configs/fork.config.ts (1 hunks)
- packages/periphery/test/MarginlyCompositionOracle.spec.ts (1 hunks)
- packages/periphery/test/UniswapV2Oracle.spec.ts (1 hunks)
- packages/periphery/test/int/others/MarginlyCompositeOracle.spec.ts (1 hunks)
- packages/periphery/test/shared/fixtures.ts (2 hunks)
Additional comments not posted (9)
packages/periphery/hardhat-configs/fork.config.ts (1)
11-12: The updated forking configuration appears correct. Ensure that the new RPC URL and block number are consistent with your intended test environment.packages/periphery/contracts/test/MockPriceOracleV2.sol (1)
21-27: The getter functions for prices are straightforward and follow Solidity best practices.packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (3)
26-65: The implementation ofsetPairis robust with comprehensive error checks and state updates. Consider adding comments to explain the logic, especially how token pairs are stored bidirectionally.
67-76: ThegetBalancePricefunction correctly handles uninitialized pairs and performs safe arithmetic operations to calculate the composite price. Good use of theMath.mulDivfor precision.
78-87: ThegetMargincallPricefunction is consistent withgetBalancePricein handling errors and calculating prices, ensuring uniformity across different types of price retrievals.packages/periphery/test/shared/fixtures.ts (4)
18-18: The import ofMockPriceOracleV2is correctly placed within the import block, maintaining alphabetical order.
19-19: The import ofMarginlyCompositeOracleis correctly placed within the import block, maintaining alphabetical order.
1137-1144: The type definitionMarginlyCompositeOracleDatais well-structured and provides a clear interface for handling oracle data. This should facilitate easier manipulation and testing of oracle-related data.
1132-1135: The functioncreateEmptyMarginlyCompositeOracleis concise and correctly uses the factory pattern to instantiateMarginlyCompositeOracle. Ensure that the contract deployment is covered by appropriate tests to verify its initialization state.Verification successful
The previous shell script did not return any results, which suggests that there might not be any direct test cases for
createEmptyMarginlyCompositeOracle. However, to ensure thorough verification, we should extend our search to include any references tocreateEmptyMarginlyCompositeOraclewithin the test files. This will help us identify if the function is indirectly tested.
The search results indicate that
createEmptyMarginlyCompositeOracleis indeed referenced within the test files, particularly inMarginlyCompositionOracle.spec.ts. This suggests that the function is being utilized in tests, which implies that the contract deployment is covered by these tests.
packages/periphery/test/MarginlyCompositionOracle.spec.ts: The functioncreateEmptyMarginlyCompositeOracleis imported and used within this test file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that tests cover the deployment of MarginlyCompositeOracle. # Test: Search for the test cases for `createEmptyMarginlyCompositeOracle`. Expect: At least one test case. rg --type typescript $'describe\(.*createEmptyMarginlyCompositeOracle.*\)'Length of output: 110
Script:
#!/bin/bash # Description: Verify that tests cover the deployment of MarginlyCompositeOracle. # Test: Search for the test cases for `createEmptyMarginlyCompositeOracle`. Expect: At least one test case. # Fixing the file type issue by specifying extensions for TypeScript files rg 'describe\(.*createEmptyMarginlyCompositeOracle.*\)' --glob '*.ts' --glob '*.tsx'Length of output: 86
Script:
#!/bin/bash # Description: Verify that tests cover the deployment of MarginlyCompositeOracle. # Test: Search for any references to `createEmptyMarginlyCompositeOracle` within the test files. rg 'createEmptyMarginlyCompositeOracle' --glob '*.ts' --glob '*.tsx'Length of output: 626
| describe('MarginlyCompositeOracle', () => { | ||
| it('should fail when zero address passed', async () => { | ||
| const oracle = await loadFixture(createEmptyMarginlyCompositeOracle); | ||
| const quote = '0x0000000000000000000000000000000000000001'; | ||
| const interm = '0x0000000000000000000000000000000000000002'; | ||
| const base = '0x0000000000000000000000000000000000000003'; | ||
| const oracle1 = '0x0000000000000000000000000000000000000004'; | ||
|
|
||
| await expect(oracle.setPair(ZERO_ADDRESS, interm, base, ZERO_ADDRESS, ZERO_ADDRESS)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'ZeroAddress' | ||
| ); | ||
|
|
||
| await expect(oracle.setPair(quote, ZERO_ADDRESS, base, ZERO_ADDRESS, ZERO_ADDRESS)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'ZeroAddress' | ||
| ); | ||
|
|
||
| await expect(oracle.setPair(quote, interm, ZERO_ADDRESS, ZERO_ADDRESS, ZERO_ADDRESS)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'ZeroAddress' | ||
| ); | ||
|
|
||
| await expect(oracle.setPair(quote, interm, base, ZERO_ADDRESS, oracle1)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'ZeroAddress' | ||
| ); | ||
|
|
||
| await expect(oracle.setPair(quote, interm, base, oracle1, ZERO_ADDRESS)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'ZeroAddress' | ||
| ); | ||
| }); | ||
|
|
||
| it('should return price weth/usdc with arb intermediate', async () => { | ||
| const { oracle, quoteToken: usdc, baseToken: weth } = await loadFixture(createMarginlyCompositeOracle); | ||
|
|
||
| const balancePrice = await oracle.getBalancePrice(usdc.address, weth.address); | ||
| console.log(balancePrice); | ||
|
|
||
| const mcPrice = await oracle.getMargincallPrice(usdc.address, weth.address); | ||
| console.log(mcPrice); | ||
| }); | ||
|
|
||
| it('should return price usdc/weth with usdc intermediate', async () => { | ||
| const { oracle, quoteToken: usdc, baseToken: weth } = await loadFixture(createMarginlyCompositeOracle); | ||
|
|
||
| const balancePrice = await oracle.getBalancePrice(weth.address, usdc.address); | ||
| console.log(balancePrice); | ||
|
|
||
| const mcPrice = await oracle.getMargincallPrice(weth.address, usdc.address); | ||
| console.log(mcPrice); | ||
| }); | ||
|
|
||
| it('should fail when pair not initialized', async () => { | ||
| const { | ||
| oracle, | ||
| quoteToken: usdc, | ||
| baseToken: weth, | ||
| intermediateToken: arb, | ||
| } = await loadFixture(createMarginlyCompositeOracle); | ||
|
|
||
| await expect(oracle.getBalancePrice(arb.address, usdc.address)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'NotInitialized' | ||
| ); | ||
|
|
||
| await expect(oracle.getBalancePrice(usdc.address, arb.address)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'NotInitialized' | ||
| ); | ||
|
|
||
| await expect(oracle.getMargincallPrice(arb.address, usdc.address)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'NotInitialized' | ||
| ); | ||
|
|
||
| await expect(oracle.getMargincallPrice(usdc.address, arb.address)).to.be.revertedWithCustomError( | ||
| oracle, | ||
| 'NotInitialized' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test cases comprehensively cover scenarios including zero address checks and uninitialized pair handling. Consider adding tests for boundary conditions and unexpected inputs to further enhance the robustness of the oracle.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/periphery/test/UniswapV2Oracle.spec.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/periphery/test/UniswapV2Oracle.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (1 hunks)
Additional comments not posted (6)
packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (6)
1-1: Ensure SPDX license compatibility.The SPDX license identifier is set to
GPL-2.0-or-later. Make sure this is compatible with other licenses used in your project, particularly for dependencies and linked contracts.
4-7: Review import statements for correctness and necessity.Ensure that all imported contracts and libraries (
Ownable2Step,Math,IPriceOracle,CompositeOracle) are used within the file and are necessary for the functionality ofMarginlyCompositeOracle.
14-17: Error handling: Custom error definitions.The custom errors (
ZeroPrice,ZeroAddress,NotInitialized,PairAlreadyExists) are well-defined and enhance clarity and gas efficiency. Good practice in Solidity.
19-19: Constant usage:X96ONE.The constant
X96ONEis defined but ensure that its usage in multiplication and division operations is clear to maintainers, possibly by adding a comment explaining its purpose and significance.
36-40: Input validation usingrevertwithZeroAddress.Proper use of input validation to prevent zero address parameters. This is crucial for security in smart contracts to avoid unintended behavior.
70-79: FunctionsgetBalancePriceandgetMargincallPrice.Both functions perform similar operations with slight variations. They effectively use the
Math.mulDivfor price calculations, which is a safe way to handle overflow issues. Ensure that the division byX96ONEis always safe and consider edge cases wherefirstPriceorsecondPricecould be extremely large.Also applies to: 81-90
| function setPair( | ||
| address quoteToken, | ||
| address intermediateToken, | ||
| address baseToken, | ||
| IPriceOracle quoteIntermediateOracle, | ||
| IPriceOracle interMediateBaseOracle | ||
| ) external onlyOwner { | ||
| if (quoteToken == address(0)) revert ZeroAddress(); | ||
| if (intermediateToken == address(0)) revert ZeroAddress(); | ||
| if (baseToken == address(0)) revert ZeroAddress(); | ||
| if (address(quoteIntermediateOracle) == address(0)) revert ZeroAddress(); | ||
| if (address(interMediateBaseOracle) == address(0)) revert ZeroAddress(); | ||
|
|
||
| OracleParams memory params = getParams[quoteToken][baseToken]; | ||
| if (params.intermediateToken != address(0)) revert PairAlreadyExists(); | ||
|
|
||
| if (quoteIntermediateOracle.getBalancePrice(quoteToken, intermediateToken) == 0) revert ZeroPrice(); | ||
| if (quoteIntermediateOracle.getBalancePrice(intermediateToken, quoteToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (quoteIntermediateOracle.getMargincallPrice(quoteToken, intermediateToken) == 0) revert ZeroPrice(); | ||
| if (quoteIntermediateOracle.getMargincallPrice(intermediateToken, quoteToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (interMediateBaseOracle.getBalancePrice(intermediateToken, baseToken) == 0) revert ZeroPrice(); | ||
| if (interMediateBaseOracle.getBalancePrice(baseToken, intermediateToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (interMediateBaseOracle.getMargincallPrice(intermediateToken, baseToken) == 0) revert ZeroPrice(); | ||
| if (interMediateBaseOracle.getMargincallPrice(baseToken, intermediateToken) == 0) revert ZeroPrice(); | ||
|
|
||
| getParams[quoteToken][baseToken] = OracleParams({ | ||
| intermediateToken: intermediateToken, | ||
| quoteIntermediateOracle: quoteIntermediateOracle, | ||
| interMediateBaseOracle: interMediateBaseOracle | ||
| }); | ||
|
|
||
| getParams[baseToken][quoteToken] = OracleParams({ | ||
| intermediateToken: intermediateToken, | ||
| quoteIntermediateOracle: interMediateBaseOracle, | ||
| interMediateBaseOracle: quoteIntermediateOracle | ||
| }); | ||
| } |
There was a problem hiding this comment.
Complex function: setPair.
This function handles a lot of logic including input validation, price checks, and state updates. Consider breaking it down into smaller functions or adding helper functions to improve readability and maintainability.
| if (quoteIntermediateOracle.getBalancePrice(quoteToken, intermediateToken) == 0) revert ZeroPrice(); | ||
| if (quoteIntermediateOracle.getBalancePrice(intermediateToken, quoteToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (quoteIntermediateOracle.getMargincallPrice(quoteToken, intermediateToken) == 0) revert ZeroPrice(); | ||
| if (quoteIntermediateOracle.getMargincallPrice(intermediateToken, quoteToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (interMediateBaseOracle.getBalancePrice(intermediateToken, baseToken) == 0) revert ZeroPrice(); | ||
| if (interMediateBaseOracle.getBalancePrice(baseToken, intermediateToken) == 0) revert ZeroPrice(); | ||
|
|
||
| if (interMediateBaseOracle.getMargincallPrice(intermediateToken, baseToken) == 0) revert ZeroPrice(); | ||
| if (interMediateBaseOracle.getMargincallPrice(baseToken, intermediateToken) == 0) revert ZeroPrice(); |
There was a problem hiding this comment.
Price validation checks.
The repeated checks for zero prices are good for ensuring valid oracle responses. However, consider abstracting these checks into a separate function to reduce redundancy and improve code clarity.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/periphery/contracts/oracles/PendleMarketOracle.sol (1)
129-153: Minor naming nitpick for clarity.
The boolean parameterisMargincallPricecould be more descriptive, e.g.forLiquidation. This helps reflect its purpose more directly at call sites.packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts (1)
7-16: Consider extracting hardcoded values to constants file.Contract addresses and configuration values should be moved to a separate constants file for better maintainability and reusability across tests.
Consider creating a new file like
test/constants/addresses.ts:export const ETHEREUM_ADDRESSES = { PYTH_CONTRACT: '0x4305fb66699c3b2702d4d05cf36551390a4c69c6', WST_USR: '0x1202f5c7b4b9e47a1a484e8b270be34dbbc75055', // ... other addresses }; export const ORACLE_CONFIG = { PYTH_MAX_PRICE_AGE: 1800, PENDLE_MARKET_ORACLE_SECONDS_AGO: 1800, // ... other config values };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/periphery/contracts/oracles/PendleMarketOracle.sol(1 hunks)packages/periphery/hardhat-configs/arb-fork.config.ts(1 hunks)packages/periphery/hardhat-configs/eth-fork.config.ts(1 hunks)packages/periphery/package.json(1 hunks)packages/periphery/test/int/arb/others/AlgebraOracle.spec.ts(1 hunks)packages/periphery/test/int/arb/others/ChainlinkOracle.spec.ts(1 hunks)packages/periphery/test/int/arb/others/CurveOracle.spec.ts(1 hunks)packages/periphery/test/int/arb/others/MarginlyCompositeOracle.spec.ts(1 hunks)packages/periphery/test/int/arb/others/PythOracle.spec.ts(1 hunks)packages/periphery/test/int/arb/others/UniswapV2Oracle.spec.ts(1 hunks)packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/periphery/test/int/arb/others/CurveOracle.spec.ts
- packages/periphery/test/int/arb/others/UniswapV2Oracle.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts
[error] 5-5: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (9)
packages/periphery/test/int/arb/others/ChainlinkOracle.spec.ts (1)
4-5: LGTM! Import paths correctly updated.The import paths have been properly adjusted to reflect the new directory structure while maintaining the same functionality.
packages/periphery/test/int/arb/others/AlgebraOracle.spec.ts (1)
2-3: LGTM! Import paths correctly updated.The import paths have been properly adjusted to reflect the new directory structure while maintaining the correct relative paths to the imported modules.
Let's verify that the new import paths are correct and the imported modules exist:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that the imported modules exist at the specified paths # Check if the oracle contracts exist fd -t f "AlgebraTickOracle|AlgebraTickOracleDouble" typechain-types/contracts/oracles # Check if the common utilities exist fd -t f "common.ts" sharedLength of output: 329
Attention: Manual Verification Needed for Import Paths
The updated import paths in
packages/periphery/test/int/arb/others/AlgebraOracle.spec.tsappear to correctly reflect the intended restructuring. However, our initial file system search did not yield results because the search command used directory paths that do not exist in the repository structure. Please manually verify or run updated shell scripts to ensure that:
- The modules
AlgebraTickOracleandAlgebraTickOracleDoubleexist at the location corresponding to the relative path../../../../typechain-types/contracts/oracles- The utilities
getDecimalsDiffandprintPricesare present at the relative path../../../shared/commonFor further confidence, you can run a new script to search for the directories by name from the repository root:
Please verify that the output confirms the correct directory placement relative to
packages/periphery/test/int/arb/others/AlgebraOracle.spec.ts.
🏁 Script executed:
#!/bin/bash echo "Searching for directories named 'typechain-types' and 'shared':" fd -t d "typechain-types" fd -t d "shared"Length of output: 432
Below is an updated script to search for the relevant files across the repository. Please run it and verify that the files are located where the updated import paths in
packages/periphery/test/int/arb/others/AlgebraOracle.spec.tsexpect them to be:Once you confirm the search results match the expected locations relative to the file (
../../../../typechain-types/contracts/oraclesand../../../shared/common), please update the review accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for files containing 'AlgebraTickOracle' in the repository:" fd -t f "AlgebraTickOracle" echo "Searching for files containing 'AlgebraTickOracleDouble' in the repository:" fd -t f "AlgebraTickOracleDouble" echo "Searching for files containing 'getDecimalsDiff' in the repository:" fd -t f "getDecimalsDiff" echo "Searching for files containing 'printPrices' in the repository:" fd -t f "printPrices"Length of output: 1036
Action Required: Manual Verification Needed for Import Paths
The updated import paths in
packages/periphery/test/int/arb/others/AlgebraOracle.spec.tsreference modules that we couldn’t conclusively locate via our automated searches:
- The Oracle contracts are imported from a relative path ending in
typechain-types/contracts/oracles, but our search only revealed the Solidity sources underpackages/periphery/contracts/oracles. It’s likely that thetypechain-typesfolder is generated (and hence not present in the repository), so please confirm that the generated TypeChain files forAlgebraTickOracleandAlgebraTickOracleDoubleare indeed output to the expected location relative to this test file.- The utilities (
getDecimalsDiffandprintPrices) are imported from a path resolving topackages/periphery/test/shared/common(or a similar file), but our search for these strings didn’t return results. Note that theshareddirectory exists, so please verify manually that the required file (likelycommon.tsor similar) containing these utilities is present in that directory.To assist further, please run this additional script from the repository root to list files in the expected directories:
Based on the outcome of these checks, please manually verify that the import paths correctly resolve to the generated oracle types and the shared utility functions.
🏁 Script executed:
#!/bin/bash echo "Listing directories for generated types (if any):" find . -type d -iname "typechain-types" echo "Listing files in packages/periphery/test/shared (expected location for common file):" find packages/periphery/test/shared -type fLength of output: 657
LGTM! Import paths correctly updated.
- The relative import for the common utility now properly targets
packages/periphery/test/shared/common.ts.- The Oracle contracts’ generated TypeChain files are expected to reside in a
typechain-typesfolder outside of versioned source files; the absence in file searches is typical for generated outputs.Overall, the updates reflect the intended directory restructuring.
packages/periphery/contracts/oracles/PendleMarketOracle.sol (3)
26-35: Good use of descriptive custom errors.
No issues found with your approach here, as leveraging custom errors improves clarity and reduces bytecode size.
36-40: Constructor check looks correct.
Requiring a non-zero_pendleaddress is sufficient for initialization here, and storing it in an immutable variable is a good practice.
95-117: Verify new TWAP durations are properly initialized for liquidation.
When updatingsecondsAgoLiquidationto a new value, you may inadvertently set it to a range the oracle has not yet initialized. If intended, this is fine. Otherwise, repeating_assertOracleIsInitializedfor the updatedsecondsAgoLiquidationhelps prevent future retrieval errors.Would you like a script to locate and confirm all usage of updated TWAP durations, ensuring the oracle can handle both durations?
packages/periphery/hardhat-configs/eth-fork.config.ts (1)
11-12: Confirm correctness of forking block number.
Please ensure that block 21814800 meets your testing or debugging requirements. Using environment variables for the RPC URL and block number may offer more flexibility across different environments.packages/periphery/hardhat-configs/arb-fork.config.ts (1)
1-19: Check Arbitrum RPC and fork block number.
Everything looks fine for a dedicated Arbitrum forking config. However, verifying that block 221775851 is current enough for your testing or aligning it with a stable environment variable is recommended.packages/periphery/test/int/arb/others/PythOracle.spec.ts (1)
2-3: LGTM! Import paths updated correctly.The import paths have been properly updated to reflect the new directory structure.
packages/periphery/package.json (1)
15-16: LGTM! Well-organized test scripts.The integration tests have been properly split into network-specific commands, improving organization and clarity.
| /// @notice Create token pair oracle price params. Can be called only once per pair. | ||
| /// @param quoteToken address of IbToken or PtToken | ||
| /// @param baseToken address of PtToken or IbToken | ||
| /// @param pendleMarket Address of PendleMarket contract with PtToken and IbToken | ||
| /// @param secondsAgo Number of seconds in the past from which to calculate the time-weighted means | ||
| /// @param secondsAgoLiquidation Same as `secondsAgo`, but for liquidation case | ||
| function setPair( | ||
| address quoteToken, | ||
| address baseToken, | ||
| address pendleMarket, | ||
| uint16 secondsAgo, | ||
| uint16 secondsAgoLiquidation | ||
| ) external onlyOwner { | ||
| if (secondsAgo == 0 || secondsAgoLiquidation == 0) revert WrongValue(); | ||
| if (secondsAgo < secondsAgoLiquidation) revert WrongValue(); | ||
| if (quoteToken == address(0) || baseToken == address(0) || pendleMarket == address(0)) { | ||
| revert ZeroAddress(); | ||
| } | ||
|
|
||
| if (getParams[quoteToken][baseToken].pendleMarket != address(0)) revert PairAlreadyExist(); | ||
|
|
||
| _assertOracleIsInitialized(pendleMarket, secondsAgo); | ||
|
|
||
| (IStandardizedYield sy, IPPrincipalToken pt, ) = PendleMarketV3(pendleMarket).readTokens(); | ||
| address ibToken; | ||
| if (baseToken == address(pt)) { | ||
| ibToken = quoteToken; | ||
| } else if (quoteToken == address(pt)) { | ||
| ibToken = baseToken; | ||
| } else { | ||
| revert WrongPtAddress(); | ||
| } | ||
|
|
||
| if (!sy.isValidTokenIn(ibToken) || !sy.isValidTokenOut(ibToken)) revert WrongIbTokenAddress(); | ||
|
|
||
| uint8 ptDecimals = IERC20Metadata(baseToken).decimals(); | ||
| uint8 syDecimals = IERC20Metadata(address(sy)).decimals(); | ||
| uint8 ibDecimals = IERC20Metadata(ibToken).decimals(); | ||
|
|
||
| //We assume that sy ib ratio is 1:1 and decimals for both tokens are equals | ||
| if (syDecimals != ibDecimals) revert WrongIbSyDecimals(); | ||
|
|
||
| OracleParams memory oracleParams = OracleParams({ | ||
| pendleMarket: pendleMarket, | ||
| ibToken: ibToken, | ||
| secondsAgo: secondsAgo, | ||
| secondsAgoLiquidation: secondsAgoLiquidation, | ||
| ptSyDecimalsDelta: PRICE_DECIMALS + ptDecimals - syDecimals | ||
| }); | ||
|
|
||
| getParams[quoteToken][baseToken] = oracleParams; | ||
| getParams[baseToken][quoteToken] = oracleParams; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure oracle is initialized for both normal and liquidation scenarios.
Currently, _assertOracleIsInitialized is only called with secondsAgo. Consider doing the same check for secondsAgoLiquidation to confirm the oracle is adequately initialized for liquidation as well.
Apply this diff after line 62 to verify the oracle’s readiness for liquidation durations:
_assertOracleIsInitialized(pendleMarket, secondsAgo);
+ _assertOracleIsInitialized(pendleMarket, secondsAgoLiquidation);📝 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.
| /// @notice Create token pair oracle price params. Can be called only once per pair. | |
| /// @param quoteToken address of IbToken or PtToken | |
| /// @param baseToken address of PtToken or IbToken | |
| /// @param pendleMarket Address of PendleMarket contract with PtToken and IbToken | |
| /// @param secondsAgo Number of seconds in the past from which to calculate the time-weighted means | |
| /// @param secondsAgoLiquidation Same as `secondsAgo`, but for liquidation case | |
| function setPair( | |
| address quoteToken, | |
| address baseToken, | |
| address pendleMarket, | |
| uint16 secondsAgo, | |
| uint16 secondsAgoLiquidation | |
| ) external onlyOwner { | |
| if (secondsAgo == 0 || secondsAgoLiquidation == 0) revert WrongValue(); | |
| if (secondsAgo < secondsAgoLiquidation) revert WrongValue(); | |
| if (quoteToken == address(0) || baseToken == address(0) || pendleMarket == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| if (getParams[quoteToken][baseToken].pendleMarket != address(0)) revert PairAlreadyExist(); | |
| _assertOracleIsInitialized(pendleMarket, secondsAgo); | |
| (IStandardizedYield sy, IPPrincipalToken pt, ) = PendleMarketV3(pendleMarket).readTokens(); | |
| address ibToken; | |
| if (baseToken == address(pt)) { | |
| ibToken = quoteToken; | |
| } else if (quoteToken == address(pt)) { | |
| ibToken = baseToken; | |
| } else { | |
| revert WrongPtAddress(); | |
| } | |
| if (!sy.isValidTokenIn(ibToken) || !sy.isValidTokenOut(ibToken)) revert WrongIbTokenAddress(); | |
| uint8 ptDecimals = IERC20Metadata(baseToken).decimals(); | |
| uint8 syDecimals = IERC20Metadata(address(sy)).decimals(); | |
| uint8 ibDecimals = IERC20Metadata(ibToken).decimals(); | |
| //We assume that sy ib ratio is 1:1 and decimals for both tokens are equals | |
| if (syDecimals != ibDecimals) revert WrongIbSyDecimals(); | |
| OracleParams memory oracleParams = OracleParams({ | |
| pendleMarket: pendleMarket, | |
| ibToken: ibToken, | |
| secondsAgo: secondsAgo, | |
| secondsAgoLiquidation: secondsAgoLiquidation, | |
| ptSyDecimalsDelta: PRICE_DECIMALS + ptDecimals - syDecimals | |
| }); | |
| getParams[quoteToken][baseToken] = oracleParams; | |
| getParams[baseToken][quoteToken] = oracleParams; | |
| } | |
| /// @notice Create token pair oracle price params. Can be called only once per pair. | |
| /// @param quoteToken address of IbToken or PtToken | |
| /// @param baseToken address of PtToken or IbToken | |
| /// @param pendleMarket Address of PendleMarket contract with PtToken and IbToken | |
| /// @param secondsAgo Number of seconds in the past from which to calculate the time-weighted means | |
| /// @param secondsAgoLiquidation Same as `secondsAgo`, but for liquidation case | |
| function setPair( | |
| address quoteToken, | |
| address baseToken, | |
| address pendleMarket, | |
| uint16 secondsAgo, | |
| uint16 secondsAgoLiquidation | |
| ) external onlyOwner { | |
| if (secondsAgo == 0 || secondsAgoLiquidation == 0) revert WrongValue(); | |
| if (secondsAgo < secondsAgoLiquidation) revert WrongValue(); | |
| if (quoteToken == address(0) || baseToken == address(0) || pendleMarket == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| if (getParams[quoteToken][baseToken].pendleMarket != address(0)) revert PairAlreadyExist(); | |
| _assertOracleIsInitialized(pendleMarket, secondsAgo); | |
| _assertOracleIsInitialized(pendleMarket, secondsAgoLiquidation); | |
| (IStandardizedYield sy, IPPrincipalToken pt, ) = PendleMarketV3(pendleMarket).readTokens(); | |
| address ibToken; | |
| if (baseToken == address(pt)) { | |
| ibToken = quoteToken; | |
| } else if (quoteToken == address(pt)) { | |
| ibToken = baseToken; | |
| } else { | |
| revert WrongPtAddress(); | |
| } | |
| if (!sy.isValidTokenIn(ibToken) || !sy.isValidTokenOut(ibToken)) revert WrongIbTokenAddress(); | |
| uint8 ptDecimals = IERC20Metadata(baseToken).decimals(); | |
| uint8 syDecimals = IERC20Metadata(address(sy)).decimals(); | |
| uint8 ibDecimals = IERC20Metadata(ibToken).decimals(); | |
| //We assume that sy ib ratio is 1:1 and decimals for both tokens are equals | |
| if (syDecimals != ibDecimals) revert WrongIbSyDecimals(); | |
| OracleParams memory oracleParams = OracleParams({ | |
| pendleMarket: pendleMarket, | |
| ibToken: ibToken, | |
| secondsAgo: secondsAgo, | |
| secondsAgoLiquidation: secondsAgoLiquidation, | |
| ptSyDecimalsDelta: PRICE_DECIMALS + ptDecimals - syDecimals | |
| }); | |
| getParams[quoteToken][baseToken] = oracleParams; | |
| getParams[baseToken][quoteToken] = oracleParams; | |
| } |
| import { MarginlyCompositeOracle, PendleMarketOracle, PythOracle } from '../../../typechain-types/contracts/oracles'; | ||
| import { getDecimalsDiff, printPrices } from '../../shared/common'; | ||
|
|
||
| describe.only('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => { |
There was a problem hiding this comment.
Remove .only from the test suite.
The .only modifier prevents other tests from running. This is typically used during development and should be removed before merging.
-describe.only('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => {
+describe('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => {📝 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.
| describe.only('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => { | |
| describe('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => { | |
| // ... rest of your test implementation remains unchanged | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
| const wethArbMcPrice = await uniswapV3Oracle.getBalancePrice(arb, weth); | ||
|
|
There was a problem hiding this comment.
Fix incorrect price retrieval method.
Similar to the previous issue, the margin call price is being retrieved using getBalancePrice instead of getMargincallPrice.
- const wethArbMcPrice = await uniswapV3Oracle.getBalancePrice(arb, weth);
+ const wethArbMcPrice = await uniswapV3Oracle.getMargincallPrice(arb, weth);📝 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.
| const wethArbMcPrice = await uniswapV3Oracle.getBalancePrice(arb, weth); | |
| const wethArbMcPrice = await uniswapV3Oracle.getMargincallPrice(arb, weth); |
| const wbtcWethMcPrice = await camelotOracle.getBalancePrice(weth, wbtc); | ||
|
|
There was a problem hiding this comment.
Fix incorrect price retrieval method.
The margin call price is being retrieved using getBalancePrice instead of getMargincallPrice.
- const wbtcWethMcPrice = await camelotOracle.getBalancePrice(weth, wbtc);
+ const wbtcWethMcPrice = await camelotOracle.getMargincallPrice(weth, wbtc);📝 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.
| const wbtcWethMcPrice = await camelotOracle.getBalancePrice(weth, wbtc); | |
| const wbtcWethMcPrice = await camelotOracle.getMargincallPrice(weth, wbtc); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts (2)
71-71: 🛠️ Refactor suggestionRemove the
.onlyfocus on this suite.
Having.onlywill skip all other tests. This is typically used for debugging and should be removed before merging to ensure full test coverage.-describe.only('Composite oracle Spectra PT-wstUSR/USR, Pyth wstUSR/USR', () => { +describe('Composite oracle Spectra PT-wstUSR/USR, Pyth wstUSR/USR', () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
6-6: 🛠️ Refactor suggestionRemove the
.onlyfocus on this suite.
Having.onlywill skip all other tests. This is typically used for debugging and should be removed before merging to ensure full test coverage.-describe.only('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => { +describe('Composite oracle PT-wstUSR/USR with Pendle for PT-wstUSR/wstUSR, Pyth wstUSR/USR', () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
🧹 Nitpick comments (4)
packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (2)
9-13: Enforce consistent time settings for combined oracles.
The doc comment warns that both oracles must have the same time settings, yet there is no logic to enforce this requirement. Consider adding checks or clarifications to ensure both IPriceOracle instances align on timing parameters.
23-24: RenameinterMediateBaseOraclefor consistency.
Use a unified naming style to matchquoteIntermediateOracle. For example, renameinterMediateBaseOracletointermediateBaseOracleto maintain readability.- IPriceOracle interMediateBaseOracle; + IPriceOracle intermediateBaseOracle;packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts (2)
63-64: Rename variables referencing “wbtcArb.”
The variable nameswbtcArbBalancePriceandwbtcArbMcPricedo not reflect their underlying tokens (usrAddressandptWstUsr27Mar2025). Consider renaming them for clarity.- const wbtcArbBalancePrice = await compositeOracle.getBalancePrice(usrAddress, ptWstUsr27Mar2025); - const wbtcArbMcPrice = await compositeOracle.getMargincallPrice(usrAddress, ptWstUsr27Mar2025); + const ptWstUsrBalancePrice = await compositeOracle.getBalancePrice(usrAddress, ptWstUsr27Mar2025); + const ptWstUsrMcPrice = await compositeOracle.getMargincallPrice(usrAddress, ptWstUsr27Mar2025);
103-103: Test name mismatch with the actual token address.
The test description references "27Mar2025," but it usesptWstUsr26Jun2025in the code. Consider updating the description or the token address variable to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol(1 hunks)packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/periphery/test/int/eth/MarginlyCompositeOracle.spec.ts
[error] 6-6: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 71-71: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (2)
packages/periphery/contracts/oracles/MarginlyCompositeOracle.sol (2)
45-75: FunctionsetPairremains relatively complex.
Although this function is not excessively large, it performs several distinct steps: input validation, price checks, plus forward and reverse pair mapping. Splitting it into smaller helper functions can simplify logic and ease future maintenance.
77-85: Repetitive zero checks between_validateOracleand retrieval methods.
You are verifying zero prices in_validateOracle,getBalancePrice, andgetMargincallPrice, which could be somewhat repetitive. Consider centralizing or reusing a shared validation method if you want to reduce duplication.
Summary by CodeRabbit
New Features
PendleMarketOraclecontract for retrieving price information from the Pendle market.MarginlyCompositeOraclecontract to manage multiple price oracles and retrieve prices.Tests
MarginlyCompositeOracleand related oracles to handle various token pairs and price scenarios.