feat: add setAssetOracle to Pool and add AggregatorRETH contract#1526
Open
MilGard91 wants to merge 3 commits intorelease-candidatefrom
Open
feat: add setAssetOracle to Pool and add AggregatorRETH contract#1526MilGard91 wants to merge 3 commits intorelease-candidatefrom
MilGard91 wants to merge 3 commits intorelease-candidatefrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds oracle configuration functionality to the Pool contract. Introduces Changes
Sequence DiagramsequenceDiagram
participant Gov as Governor
participant Pool as Pool Contract
participant Agg as AggregatorRETH
participant rETH as rETH Token
Gov->>Pool: Execute setAssetOracle(rETH, aggregator, ETH, 18)
activate Pool
Pool->>Pool: Validate assetAddress != 0
Pool->>Pool: Validate aggregator != 0
Pool->>Pool: Validate asset is registered
Pool->>Pool: Validate decimals (18 for ETH type)
Pool->>Pool: Update oracles[rETH] mapping
deactivate Pool
Gov->>Agg: Deploy AggregatorRETH(rETH)
activate Agg
Agg->>Agg: Store rETH reference
deactivate Agg
Agg->>rETH: latestAnswer() queries
activate rETH
rETH-->>Agg: getExchangeRate()
deactivate rETH
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 4
🧹 Nitpick comments (4)
contracts/modules/capital/AggregatorRETH.sol (2)
14-16: Consider adding zero-address validation in constructor.If
_rETHis passed as the zero address,latestAnswer()calls will revert with an unhelpful error. Adding explicit validation provides a clearer failure mode at deployment time.Proposed fix
constructor(address _rETH) { + require(_rETH != address(0), "rETH address cannot be zero"); rETH = IRETH(_rETH); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/modules/capital/AggregatorRETH.sol` around lines 14 - 16, Add an explicit zero-address check in the constructor before assigning rETH: validate that the incoming _rETH is not the zero address (require(_rETH != address(0), "...")) and revert with a clear message if it is, then set rETH = IRETH(_rETH); this ensures calls to IRETH.latestAnswer() fail fast with a clear deployment error rather than later unhelpful reverts.
18-20: Return type mismatch withAggregatorinterface.The
Aggregatorinterface expectslatestAnswer()to returnint, but this implementation returnsuint256. WhenPool._getAssetToEthRatecallsoracle.aggregator.latestAnswer()and casts the result, this works because Solidity ABI decoding treats them as the same size, but it's fragile and non-standard.Consider matching the interface signature for clarity and explicit safety.
Proposed fix
- function latestAnswer() public view returns (uint256) { - return rETH.getExchangeRate(); + function latestAnswer() public view returns (int256) { + return int256(rETH.getExchangeRate()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/modules/capital/AggregatorRETH.sol` around lines 18 - 20, The implementation of latestAnswer in AggregatorRETH.sol returns uint256 but the Aggregator interface expects an int type; change the signature of function latestAnswer() to return int256 and convert rETH.getExchangeRate() to int256 before returning (e.g. check that the uint256 value <= type(int256).max and then return int256(value)). This makes the return type match the Aggregator interface and avoids fragile ABI decoding when Pool._getAssetToEthRate calls oracle.aggregator.latestAnswer().test/unit/Pool/setAssetOracle.js (1)
110-126: Test description and hardcoded index.
- The test description says "add new aggregator" but semantically it's updating/changing an existing oracle configuration.
- Line 115 uses hardcoded index
3- consider usinggetAssetId(token)to make the test more robust to setup changes.Proposed improvements
- it('should add new aggregator for an asset', async function () { + it('should update aggregator for an existing asset', async function () { const fixture = await loadFixture(setAssetOracleSetup); const { pool, governor, token, tokenAggregatorTwo } = fixture; await pool.connect(governor).setAssetOracle(token, tokenAggregatorTwo, 0, 18); - const tokenAsset = await pool.getAsset(3); + const assetId = await pool.getAssetId(token); + const tokenAsset = await pool.getAsset(assetId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/Pool/setAssetOracle.js` around lines 110 - 126, The test's description and use of a hardcoded asset index are brittle: update the test title to reflect that it's updating an existing oracle (e.g., "should update aggregator for an asset"), replace the hardcoded getAsset(3) lookup with a dynamic lookup using getAssetId(token) to fetch the asset id, and then call pool.getAsset(assetId) and pool.oracles(token) to assert the updated aggregator, aggregatorType, and assetDecimals after invoking pool.connect(governor).setAssetOracle(token, tokenAggregatorTwo, 0, 18); ensure references to setAssetOracle, getAsset, getAssetId and oracles are used so the test no longer depends on a hardcoded index.contracts/modules/capital/Pool.sol (1)
483-504: Consider emitting an event when the oracle is updated.Other state-changing functions in the contract emit events (e.g.,
MCRUpdated,Payout). Adding an event for oracle changes would improve off-chain observability and audit trails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/modules/capital/Pool.sol` around lines 483 - 504, Add an event and emit it from setAssetOracle to record oracle changes: define an event (e.g., OracleUpdated(address asset, address aggregator, AggregatorType aggregatorType, uint8 assetDecimals, address updatedBy)) and, at the end of the setAssetOracle function, emit OracleUpdated with assetAddress, aggregator, aggregatorType, assetDecimals and msg.sender (or tx originating contract) so every oracle update to the oracles mapping is logged for off-chain observability; ensure the event declaration uses the same AggregatorType enum and matches the oracles storage fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/modules/capital/AggregatorRETH.sol`:
- Line 11: The decimals state variable is declared as uint but must match the
Aggregator interface signature (decimals() returns uint8); change the
declaration of decimals in AggregatorRETH.sol from uint public decimals = 18; to
uint8 public decimals = 18; and update any places that rely on decimals to
handle uint8 (or explicitly cast to uint) to avoid type-mismatch warnings when
the contract is used as an Aggregator per IPool.sol.
In `@contracts/modules/capital/Pool.sol`:
- Around line 491-497: The validation is checking assetDecimals instead of the
aggregator's own decimals; update the checks in the Pool asset-add logic to call
the aggregator's decimals (e.g., aggregator.decimals() or via the price
aggregator interface used elsewhere) and compare that value to 18 for
AggregatorType.ETH and 8 for AggregatorType.USD (the same approach used in
_addAsset). Locate the AggregatorType checks and replace assetDecimals with the
aggregator's decimals accessor (ensuring you use the same interface/type as
_addAsset) so the aggregator's latestAnswer interpretation is validated
correctly.
In `@test/fork/rEthOracle/rETH.js`:
- Around line 119-136: The test is reading state from the old Pool instance
(this.pool) before the upgrade, so move or repeat the pool re-acquisition to use
the upgraded Pool when reading post-upgrade values: after the governor upgrades
the Pool call ethers.getContractAt('Pool', pool.target) (or assign to a new
variable like upgradedPool) and then call upgradedPool.getAssets(),
upgradedPool.getMCR(), upgradedPool.getMCRRatio(), and
upgradedPool.oracles(this.rEth.target) (and use this.rEth.getExchangeRate() as
before) so the assertions comparing rEthOracle.aggregator to
this.rETHAggregator.target and other state reflect the new Pool, not the old
reference.
In `@test/unit/Pool/setAssetOracle.js`:
- Line 96: Update the test description in the failing 'it' block that currently
reads "reverts if asset doesnt exists" to use proper punctuation and grammar;
change it to "reverts if asset doesn't exist" in the 'it' test case in
setAssetOracle.js so the message is clear and grammatically correct.
---
Nitpick comments:
In `@contracts/modules/capital/AggregatorRETH.sol`:
- Around line 14-16: Add an explicit zero-address check in the constructor
before assigning rETH: validate that the incoming _rETH is not the zero address
(require(_rETH != address(0), "...")) and revert with a clear message if it is,
then set rETH = IRETH(_rETH); this ensures calls to IRETH.latestAnswer() fail
fast with a clear deployment error rather than later unhelpful reverts.
- Around line 18-20: The implementation of latestAnswer in AggregatorRETH.sol
returns uint256 but the Aggregator interface expects an int type; change the
signature of function latestAnswer() to return int256 and convert
rETH.getExchangeRate() to int256 before returning (e.g. check that the uint256
value <= type(int256).max and then return int256(value)). This makes the return
type match the Aggregator interface and avoids fragile ABI decoding when
Pool._getAssetToEthRate calls oracle.aggregator.latestAnswer().
In `@contracts/modules/capital/Pool.sol`:
- Around line 483-504: Add an event and emit it from setAssetOracle to record
oracle changes: define an event (e.g., OracleUpdated(address asset, address
aggregator, AggregatorType aggregatorType, uint8 assetDecimals, address
updatedBy)) and, at the end of the setAssetOracle function, emit OracleUpdated
with assetAddress, aggregator, aggregatorType, assetDecimals and msg.sender (or
tx originating contract) so every oracle update to the oracles mapping is logged
for off-chain observability; ensure the event declaration uses the same
AggregatorType enum and matches the oracles storage fields.
In `@test/unit/Pool/setAssetOracle.js`:
- Around line 110-126: The test's description and use of a hardcoded asset index
are brittle: update the test title to reflect that it's updating an existing
oracle (e.g., "should update aggregator for an asset"), replace the hardcoded
getAsset(3) lookup with a dynamic lookup using getAssetId(token) to fetch the
asset id, and then call pool.getAsset(assetId) and pool.oracles(token) to assert
the updated aggregator, aggregatorType, and assetDecimals after invoking
pool.connect(governor).setAssetOracle(token, tokenAggregatorTwo, 0, 18); ensure
references to setAssetOracle, getAsset, getAssetId and oracles are used so the
test no longer depends on a hardcoded index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbbbeace-8f40-42f7-99d2-73ee6b0c9049
📒 Files selected for processing (7)
contracts/interfaces/IPool.solcontracts/mocks/generic/PoolGeneric.solcontracts/modules/capital/AggregatorRETH.solcontracts/modules/capital/Pool.soltest/fork/rEthOracle/rETH.jstest/fork/rEthOracle/rETHAbi.jsontest/unit/Pool/setAssetOracle.js
4 tasks
MilGard91
force-pushed
the
chore/new-rETH-oracle
branch
from
4bcfe2b to
60a4ac0
Compare
MilGard91
force-pushed
the
chore/new-rETH-oracle
branch
from
60a4ac0 to
265b23f
Compare
duxxud
reviewed
Mar 25, 2026
shark0der
reviewed
Mar 25, 2026
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
3 participants
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.
Description
Testing
Explain how you tested your changes to ensure they work as expected.
Checklist
Summary by CodeRabbit
Release Notes