Repository navigation
fix: round protocol calculations in favor of reserves - #62
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe contract now uses explicit rounding for price, reserve, issuance, redemption, fusion, transmutation, and fee calculations. Tiny operations can complete with zero-rounded outputs when minimum-output checks allow them. Tests and README documentation cover the updated rules. ChangesRounding and value accounting
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~50 minutes Change: Bug fix Suggested labels: Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: updating protocol calculations so rounding favors reserves. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the rounded tide
Tiny coins pass through the gate
Prices turn with careful sides
Fees leave dust to guard the state
Tests hop over bounds and strain
The reserve holds its measured grain
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
| if (protonSupplyTokens == 0) return WAD; | ||
| if (normalizedReserve == 0) return 0; | ||
| if (normalizedReserve == 0 || basePrice == 0) return 0; | ||
|
|
There was a problem hiding this comment.
It is not clear why this function became so complicated. The corresponding _neutronPriceInBase function is not as complicated.
There was a problem hiding this comment.
Neutron pricing takes the lower of two prices. Proton pricing uses what remains in the reserve after covering Neutrons, so I added rounding and overflow handling for that calculation. Some of the code is repeated; I will simplify it while keeping those protections.
|
|
||
| liabilities = Math.min(normalizedReserve, liabilities); | ||
| uint256 equity = normalizedReserve - liabilities; | ||
| // An unrepresentable liability leaves no equity. |
There was a problem hiding this comment.
There must be a bug somewhere. Gluon's mathematical equations are explicitly designed to ensure that the equity, and hence the proton price never becomes zero.
| if (high >= denominator) return 0; | ||
|
|
||
| uint256 liabilities = Math.mulDiv(neutronSupplyTokens, pegFactor, denominator); | ||
| if (liabilities >= normalizedReserve) return 0; |
There was a problem hiding this comment.
There must be a bug here too.
|
|
||
| return Math.mulDiv(equity, WAD, protonSupplyTokens, rounding); | ||
| return Math.mulDiv(normalizedReserve - liabilities, WAD, protonSupplyTokens, rounding); | ||
| } |
There was a problem hiding this comment.
My suggestion to solve these bugs cleanly:
- create internal functions that calculate the liabilities and the equity, like the Djed contract does.
And a general advice:
- whenever your code has
ifstatements with specialreturnstatements for corner cases, the probability of bugs is high.
Addressed Issues:
No related issue — addresses the review requests to favour the reserve when rounding and to use shared Gluon Z liabilities and equity calculations.
Screenshots/Recordings:
Not applicable. This PR changes contract arithmetic, tests and documentation.
Additional Notes:
Previously, rounding fusion burns down could allow a small reserve withdrawal without burning enough tokens. Rounded intermediate prices could also overstate transmutation output.
This PR applies rounding according to the direction of value transfer:
Gluon Z pricing
Following the review feedback, both tranche prices now use shared internal
_liabilitiesand_equityfunctions.The calculation follows the Ergo Gluon Z implementation, using its fixed
qStar = 0.99.For target Neutron liabilities
Tand reserveR:min(T, R * T / (T + 0.01 * R))R - liabilitiesThe 99% value is a normalization threshold, not a fixed cap on liabilities. It is separate from the deployer-selected critical reserve ratio.
This replaces the pricing branches that assigned the whole reserve to Neutron liabilities and returned zero Proton equity. Intermediate overflow is handled without treating an unrepresentable target liability as zero equity.
Liabilities round opposite to equity. The continuous equation preserves positive equity for positive reserve and finite target liabilities; integer rounding can still produce a zero price for an unrepresentably small value.
Existing behaviour
The remaining user-operation
AmountTooSmallchecks are removed, as discussed in review. Small inputs may produce zero output. Transmutations retainExcessiveSlippageprotection when output is below the caller's requested minimum.Initialization still requires valid, positive seed supplies. Oracle fallback, intentional acceptance of older prices, zero-price no-ops, peg-adjustment steps, beta settings and dev access controls are preserved.
Already-merged PR #63 is integrated into this branch. Its deployer-selected reserve-ratio interface is retained.
Validation
git diff --checkforge fmt --checkforge lint --use 0.8.30 src test --deny warningsPricing tests compare allocation and price bounds against an independent rational reference. They cover the normalization threshold, liabilities above reserve, extreme arithmetic, empty supply, zero oracle prices, reserve-token decimals from 0 to 18, and transmutation value conservation with and without dev fees.
Factory runtime is 24,143 bytes, leaving 433 bytes below the deployment limit under the tested settings. Reactor runtime is 14,165 bytes.
Slither has not been rerun on this revision. This PR does not deploy contracts or change network configuration. The updated pricing has not yet been exercised through a new Sepolia deployment.
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:
Checklist
Summary by CodeRabbit
Bug Fixes
Documentation