Fix APR math in nextRevenuePoolSettleApr - #2207
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe insurance APR calculation now divides projected revenue by an integer payout denominator. The optional ChangesInsurance APR calculation
Estimated code review effort: 2 (Simple) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/src/math/insurance.ts (1)
20-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for both APR fixes.
Cover the non-zero payout path and the omitted
amountpath. Assert that payout is divided by 10 and thatamount === undefinedusesvaultBalancewithout throwing. Also cover the zero-total-balance case.Also applies to: 37-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/math/insurance.ts` at line 20, Add regression tests for the APR logic near payoutRatioDenominator and the related amount-handling path: verify non-zero payouts are divided by 10, omitted amount defaults to vaultBalance without throwing, and zero total balance is handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdk/src/math/insurance.ts`:
- Line 20: Add regression tests for the APR logic near payoutRatioDenominator
and the related amount-handling path: verify non-zero payouts are divided by 10,
omitted amount defaults to vaultBalance without throwing, and zero total balance
is handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e57f5d33-37ed-435c-8586-3f1bc84a88a5
📒 Files selected for processing (1)
sdk/src/math/insurance.ts
…eApr The fractional settlesPerYear was passed to BN.muln, which only accepts a small integer and truncates otherwise, so the projected annual revenue was wrong for any revenue settle period that does not evenly divide a year. Multiply then divide with integers to keep the value exact.
nextRevenuePoolSettleAprinsdk/src/math/insurance.tshas two bugs:revenuePoolBN.muln(settlesPerYear).muln(payoutRatio)withpayoutRatio = 0.1.BN.mulndoes not multiply by a fraction; it multiplies each limb and masks, so a fractional argument yields garbage rather thanvalue * 0.1. For examplenew BN(1_000_000_000).muln(0.1)returns73156454, not100000000(about 27 percent off). SincepayoutRatiois the constant0.1, this is integerdivn(10).amountis declared optional (amount?: BN), butvaultBalance.add(amount)is called unconditionally, soadd(undefined)throws when a caller omits it. Defaulting a missing delta toZEROpreserves the intended behavior.The function is exported from the SDK (
export * from './math/insurance'), so both reach downstream consumers.Summary by CodeRabbit