feat: add preview functions for reactor operations#19
Open
arunabha003 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds four read-only preview functions to StableCoinReactor enabling off-chain quotation of fission, fusion, and transmute operation outputs and fees. Internal fee calculators use block-timestamp-based logic without state mutations. Test mocks and comprehensive test coverage validate preview accuracy against execution behavior. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 3
🧹 Nitpick comments (2)
test/StableCoinPreview.t.sol (1)
119-125: Assert the transmute preview intermediates too.
grossBaseandnetBaseare part of the new public API, but both tests discard them. A regression in those fields would still pass even though integrations may rely on them.reactor.protonPriceInBase()/reactor.neutronPriceInBase()are enough to derive and pin them here.Also applies to: 137-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/StableCoinPreview.t.sol` around lines 119 - 125, The test calls reactor.previewTransmuteProtonToNeutron and discards the returned grossBase and netBase, which can regress silently; update the test to assert those intermediates by computing expectedGrossBase and expectedNetBase using reactor.protonPriceInBase() / reactor.neutronPriceInBase() and the input amount (15e18), then compare them to the preview output (grossBase, netBase) before calling reactor.transmuteProtonToNeutron; apply the same assertions in the other similar test block (lines around 137-142) so both previewTransmuteProtonToNeutron and previewTransmuteNeutronToProton return pinned grossBase/netBase values that match derived expectations.src/StableCoin.sol (1)
436-480: Share the decay projection logic between preview and execution.
_decayLedger,_previewBetaPlusFeeWad, and_previewBetaMinusFeeWadeach implement the same decay step. Extracting a single helper that projectsdecayedVolumeBasefor a timestamp will make it much harder for preview math and execution math to drift apart later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/StableCoin.sol` around lines 436 - 480, The decay step is duplicated across _decayLedger, _previewBetaPlusFeeWad, and _previewBetaMinusFeeWad; extract a single helper (e.g. _projectDecayedVolumeBase(uint256 atTs) internal view returns (int256)) that takes a timestamp and returns the projected decayedVolumeBase using decayPerSecondWad, lastDecayTs and _rpow, preserving sign handling. Replace the repeated block in _previewBetaPlusFeeWad and _previewBetaMinusFeeWad to call this helper (pass block.timestamp) and likewise have _decayLedger call it when computing the new decayedVolumeBase so previews and execution share identical decay logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/StableCoin.sol`:
- Around line 261-275: The preview functions compute outputs assuming the caller
can burn tokens later, but they don't guard impossible cases where protonIn or
neutronIn exceed PROTON_TOKEN.totalSupply() or NEUTRON_TOKEN.totalSupply(); add
explicit checks in the preview paths (the functions that mirror the
state-changing flows which perform burns) to require protonIn <=
protonSupplyCached and neutronIn <= neutronSupplyCached as appropriate,
returning/reverting early with a clear error (e.g., "insufficient total supply
for burn") before price math (use the existing protonSupplyCached and
neutronSupplyCached variables and the same totalSupply() calls used elsewhere);
apply the same guard to the other preview variant mentioned (the block around
the alternate snippet at lines 286-300).
- Around line 240-250: previewFusion currently computes
neutronBurn/protonBurn/feeAmount/netBaseOut even when m > reserveBalance, but
actual fusion would revert when transferring more than reserve; update
previewFusion (the function named previewFusion that reads reserve(), computes
reserveBalance, neutronBurn, protonBurn, feeAmount, netBaseOut) to check and
revert when m > reserveBalance (e.g., require(m <= reserveBalance, "insufficient
reserve") or mirror the same condition used in fusion) before calculating and
returning the quote so previewFusion matches on-chain behavior.
In `@test/mocks/MockPyth.sol`:
- Around line 10-11: The mock currently stores a single Price in currentPrice
and ignores the requested feed id; modify the mock to be id-sensitive by
changing the storage to a mapping(bytes32 => Price) (or similar) keyed by the
feed id, update setPrice to accept an id parameter (e.g. bytes32 id) and assign
the Price into mapping[id], and update all getters (the functions that currently
read currentPrice) to lookup and return mapping[id] instead of the single
currentPrice; also update tests to call setPrice(PRICE_ID, ...) so the mock
returns the correct feed-specific value.
---
Nitpick comments:
In `@src/StableCoin.sol`:
- Around line 436-480: The decay step is duplicated across _decayLedger,
_previewBetaPlusFeeWad, and _previewBetaMinusFeeWad; extract a single helper
(e.g. _projectDecayedVolumeBase(uint256 atTs) internal view returns (int256))
that takes a timestamp and returns the projected decayedVolumeBase using
decayPerSecondWad, lastDecayTs and _rpow, preserving sign handling. Replace the
repeated block in _previewBetaPlusFeeWad and _previewBetaMinusFeeWad to call
this helper (pass block.timestamp) and likewise have _decayLedger call it when
computing the new decayedVolumeBase so previews and execution share identical
decay logic.
In `@test/StableCoinPreview.t.sol`:
- Around line 119-125: The test calls reactor.previewTransmuteProtonToNeutron
and discards the returned grossBase and netBase, which can regress silently;
update the test to assert those intermediates by computing expectedGrossBase and
expectedNetBase using reactor.protonPriceInBase() / reactor.neutronPriceInBase()
and the input amount (15e18), then compare them to the preview output
(grossBase, netBase) before calling reactor.transmuteProtonToNeutron; apply the
same assertions in the other similar test block (lines around 137-142) so both
previewTransmuteProtonToNeutron and previewTransmuteNeutronToProton return
pinned grossBase/netBase values that match derived expectations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd7c727b-acb1-4874-b202-7a5d34d13924
📒 Files selected for processing (4)
src/StableCoin.soltest/StableCoinPreview.t.soltest/mocks/MockERC20.soltest/mocks/MockPyth.sol
Comment on lines
+240
to
+250
| uint256 reserveBalance = reserve(); | ||
| require(reserveBalance > 0, "R=0"); | ||
|
|
||
| uint256 neutronSupplyTotal = NEUTRON_TOKEN.totalSupply(); | ||
| uint256 protonSupplyTotal = PROTON_TOKEN.totalSupply(); | ||
| require(neutronSupplyTotal > 0 && protonSupplyTotal > 0, "empty S"); | ||
|
|
||
| neutronBurn = Math.mulDiv(m, neutronSupplyTotal, reserveBalance); | ||
| protonBurn = Math.mulDiv(m, protonSupplyTotal, reserveBalance); | ||
| feeAmount = Math.mulDiv(m, FUSION_FEE, WAD); | ||
| netBaseOut = m - feeAmount; |
Contributor
There was a problem hiding this comment.
Reject previews for withdrawals larger than the reserve.
In src/StableCoin.sol Line 380-Line 381, fusion transfers net + fee == m base tokens out of the vault. If m > reserveBalance, execution must revert, but previewFusion still returns a quote.
🛡️ Proposed fix
uint256 reserveBalance = reserve();
require(reserveBalance > 0, "R=0");
+ require(m <= reserveBalance, "amount>reserve");
uint256 neutronSupplyTotal = NEUTRON_TOKEN.totalSupply();
uint256 protonSupplyTotal = PROTON_TOKEN.totalSupply();📝 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.
Suggested change
| uint256 reserveBalance = reserve(); | |
| require(reserveBalance > 0, "R=0"); | |
| uint256 neutronSupplyTotal = NEUTRON_TOKEN.totalSupply(); | |
| uint256 protonSupplyTotal = PROTON_TOKEN.totalSupply(); | |
| require(neutronSupplyTotal > 0 && protonSupplyTotal > 0, "empty S"); | |
| neutronBurn = Math.mulDiv(m, neutronSupplyTotal, reserveBalance); | |
| protonBurn = Math.mulDiv(m, protonSupplyTotal, reserveBalance); | |
| feeAmount = Math.mulDiv(m, FUSION_FEE, WAD); | |
| netBaseOut = m - feeAmount; | |
| uint256 reserveBalance = reserve(); | |
| require(reserveBalance > 0, "R=0"); | |
| require(m <= reserveBalance, "amount>reserve"); | |
| uint256 neutronSupplyTotal = NEUTRON_TOKEN.totalSupply(); | |
| uint256 protonSupplyTotal = PROTON_TOKEN.totalSupply(); | |
| require(neutronSupplyTotal > 0 && protonSupplyTotal > 0, "empty S"); | |
| neutronBurn = Math.mulDiv(m, neutronSupplyTotal, reserveBalance); | |
| protonBurn = Math.mulDiv(m, protonSupplyTotal, reserveBalance); | |
| feeAmount = Math.mulDiv(m, FUSION_FEE, WAD); | |
| netBaseOut = m - feeAmount; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/StableCoin.sol` around lines 240 - 250, previewFusion currently computes
neutronBurn/protonBurn/feeAmount/netBaseOut even when m > reserveBalance, but
actual fusion would revert when transferring more than reserve; update
previewFusion (the function named previewFusion that reads reserve(), computes
reserveBalance, neutronBurn, protonBurn, feeAmount, netBaseOut) to check and
revert when m > reserveBalance (e.g., require(m <= reserveBalance, "insufficient
reserve") or mirror the same condition used in fusion) before calculating and
returning the quote so previewFusion matches on-chain behavior.
Comment on lines
+261
to
+275
| uint256 reserveTokens = reserve(); | ||
| uint256 protonSupplyCached = PROTON_TOKEN.totalSupply(); | ||
| uint256 neutronSupplyCached = NEUTRON_TOKEN.totalSupply(); | ||
|
|
||
| Price memory price = PYTH_ORACLE.getPriceNoOlderThan(PRICE_ID, MAXIMUM_AGE); | ||
| uint256 basePrice = _pythPriceToWad(price); | ||
|
|
||
| uint256 protonPriceBase = _protonPriceInBase(reserveTokens, protonSupplyCached, neutronSupplyCached, basePrice); | ||
| uint256 neutronPriceBase = _neutronPriceInBase(reserveTokens, neutronSupplyCached, basePrice); | ||
| require(protonPriceBase > 0 && neutronPriceBase > 0, "bad price"); | ||
|
|
||
| grossBase = Math.mulDiv(protonIn, protonPriceBase, WAD); | ||
| feeWad = _previewBetaPlusFeeWad(reserveTokens); | ||
| netBase = Math.mulDiv(grossBase, (WAD - feeWad), WAD); | ||
| neutronOut = Math.mulDiv(netBase, WAD, neutronPriceBase); |
Contributor
There was a problem hiding this comment.
Guard impossible preview burns with total-supply checks.
The state-changing paths burn the input token on Line 515 and Line 571. If protonIn or neutronIn is larger than the corresponding total supply, no caller can satisfy that burn, but these previews still return outputs.
🛡️ Proposed fix
function previewTransmuteProtonToNeutron(uint256 protonIn)
external
view
returns (uint256 neutronOut, uint256 feeWad, uint256 grossBase, uint256 netBase)
{
require(protonIn > 0, "amount=0");
uint256 reserveTokens = reserve();
uint256 protonSupplyCached = PROTON_TOKEN.totalSupply();
uint256 neutronSupplyCached = NEUTRON_TOKEN.totalSupply();
+ require(protonIn <= protonSupplyCached, "amount>totalSupply"); function previewTransmuteNeutronToProton(uint256 neutronIn)
external
view
returns (uint256 protonOut, uint256 feeWad, uint256 grossBase, uint256 netBase)
{
require(neutronIn > 0, "amount=0");
uint256 reserveTokens = reserve();
uint256 protonSupplyCached = PROTON_TOKEN.totalSupply();
uint256 neutronSupplyCached = NEUTRON_TOKEN.totalSupply();
+ require(neutronIn <= neutronSupplyCached, "amount>totalSupply");Also applies to: 286-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/StableCoin.sol` around lines 261 - 275, The preview functions compute
outputs assuming the caller can burn tokens later, but they don't guard
impossible cases where protonIn or neutronIn exceed PROTON_TOKEN.totalSupply()
or NEUTRON_TOKEN.totalSupply(); add explicit checks in the preview paths (the
functions that mirror the state-changing flows which perform burns) to require
protonIn <= protonSupplyCached and neutronIn <= neutronSupplyCached as
appropriate, returning/reverting early with a clear error (e.g., "insufficient
total supply for burn") before price math (use the existing protonSupplyCached
and neutronSupplyCached variables and the same totalSupply() calls used
elsewhere); apply the same guard to the other preview variant mentioned (the
block around the alternate snippet at lines 286-300).
Comment on lines
+10
to
+11
| function setPrice(int64 price_, int32 expo_, uint256 publishTime_) external { | ||
| currentPrice = Price({price: price_, conf: 0, expo: expo_, publishTime: publishTime_}); |
Contributor
There was a problem hiding this comment.
Make the mock price feed ID-sensitive.
test/StableCoinPreview.t.sol Line 27-Line 35 wires a concrete PRICE_ID into StableCoinReactor, but every getter here ignores the requested id and returns the same price. These tests will still pass if the contract looks up the wrong feed.
🧪 Proposed fix
contract MockPyth is IPyth {
- Price internal currentPrice;
+ mapping(bytes32 => Price) internal prices;
uint256 internal updateFee;
- function setPrice(int64 price_, int32 expo_, uint256 publishTime_) external {
- currentPrice = Price({price: price_, conf: 0, expo: expo_, publishTime: publishTime_});
+ function setPrice(bytes32 id, int64 price_, int32 expo_, uint256 publishTime_) external {
+ prices[id] = Price({price: price_, conf: 0, expo: expo_, publishTime: publishTime_});
}
@@
- function getPrice(bytes32) external view returns (Price memory price) {
- return currentPrice;
+ function getPrice(bytes32 id) external view returns (Price memory price) {
+ return prices[id];
}
- function getEmaPrice(bytes32) external view returns (Price memory price) {
- return currentPrice;
+ function getEmaPrice(bytes32 id) external view returns (Price memory price) {
+ return prices[id];
}
- function getPriceUnsafe(bytes32) external view returns (Price memory price) {
- return currentPrice;
+ function getPriceUnsafe(bytes32 id) external view returns (Price memory price) {
+ return prices[id];
}
- function getPriceNoOlderThan(bytes32, uint256 age) external view returns (Price memory price) {
- require(currentPrice.publishTime + age >= block.timestamp, "stale");
- return currentPrice;
+ function getPriceNoOlderThan(bytes32 id, uint256 age) external view returns (Price memory price) {
+ price = prices[id];
+ require(price.publishTime + age >= block.timestamp, "stale");
+ return price;
}test/StableCoinPreview.t.sol Line 25 would then need to pass PRICE_ID into setPrice.
Also applies to: 22-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/mocks/MockPyth.sol` around lines 10 - 11, The mock currently stores a
single Price in currentPrice and ignores the requested feed id; modify the mock
to be id-sensitive by changing the storage to a mapping(bytes32 => Price) (or
similar) keyed by the feed id, update setPrice to accept an id parameter (e.g.
bytes32 id) and assign the Price into mapping[id], and update all getters (the
functions that currently read currentPrice) to lookup and return mapping[id]
instead of the single currentPrice; also update tests to call setPrice(PRICE_ID,
...) so the mock returns the correct feed-specific value.
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.
Addressed Issues:
closes #18
Summary
fission,fusion, and both transmute flowsValidation
forge test -vvAdditional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Tests