Fix missing emission Jacobian in VRGDA decay integral#166
Fix missing emission Jacobian in VRGDA decay integral#166
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded an Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Slither reportStatic Analysis Report**THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [locked-ether](#locked-ether) (1 results) (Medium) - [reentrancy-no-eth](#reentrancy-no-eth) (3 results) (Medium) - [unused-return](#unused-return) (9 results) (Medium) - [incorrect-modifier](#incorrect-modifier) (1 results) (Low) - [calls-loop](#calls-loop) (2 results) (Low) - [reentrancy-benign](#reentrancy-benign) (3 results) (Low) - [reentrancy-events](#reentrancy-events) (4 results) (Low) - [dead-code](#dead-code) (44 results) (Informational) - [solc-version](#solc-version) (3 results) (Informational) - [missing-inheritance](#missing-inheritance) (2 results) (Informational) - [naming-convention](#naming-convention) (13 results) (Informational) - [unimplemented-functions](#unimplemented-functions) (4 results) (Informational) - [unindexed-event-address](#unindexed-event-address) (1 results) (Informational) ## locked-ether Impact: Medium Confidence: High - [ ] ID-0 Contract locking ether found: Contract [MutablePauseTarget](https://github.com/equilibria-xyz/root/blob/1ca90e482d2f6341ab6f0256c6ced56cb451fca9/src/mutability/Mutable.sol#L138-L143) has payable functions: - [MutablePauseTarget.fallback()](https://github.com/equilibria-xyz/root/blob/1ca90e482d2f6341ab6f0256c6ced56cb451fca9/src/mutability/Mutable.sol#L139-L141) But does not have a function to withdraw the etherreentrancy-no-ethImpact: Medium
root/src/mutability/Mutator.sol Lines 34 to 46 in 1ca90e4
root/src/mutability/Mutable.sol Lines 123 to 127 in 1ca90e4
root/src/mutability/Mutable.sol Lines 97 to 113 in 1ca90e4 unused-returnImpact: Medium
root/src/token/types/Token6.sol Lines 43 to 45 in 1ca90e4
root/src/distribution/Airdrop.sol Lines 46 to 51 in 1ca90e4
root/src/token/types/Token.sol Lines 51 to 53 in 1ca90e4
root/src/token/types/Token18.sol Lines 53 to 55 in 1ca90e4
root/src/token/types/Token6.sol Lines 54 to 56 in 1ca90e4
root/src/distribution/Airdrop.sol Lines 28 to 34 in 1ca90e4
root/src/token/types/Token.sol Lines 40 to 42 in 1ca90e4
root/src/mutability/Mutator.sol Lines 34 to 46 in 1ca90e4
root/src/token/types/Token18.sol Lines 43 to 45 in 1ca90e4 incorrect-modifierImpact: Low
root/src/attribute/Attribute.sol Lines 28 to 32 in 1ca90e4 calls-loopImpact: Low
root/src/mutability/Mutator.sol Lines 62 to 65 in 1ca90e4
root/src/mutability/Mutator.sol Lines 57 to 60 in 1ca90e4 reentrancy-benignImpact: Low
root/src/mutability/Mutator.sol Lines 57 to 60 in 1ca90e4
root/src/mutability/Mutator.sol Lines 62 to 65 in 1ca90e4
root/src/mutability/Mutator.sol Lines 34 to 46 in 1ca90e4 reentrancy-eventsImpact: Low
root/src/mutability/Mutable.sol Lines 116 to 120 in 1ca90e4
root/src/mutability/Mutator.sol Lines 62 to 65 in 1ca90e4
root/src/mutability/Mutable.sol Lines 123 to 127 in 1ca90e4
root/src/mutability/Mutator.sol Lines 57 to 60 in 1ca90e4 dead-codeImpact: Informational
root/src/number/types/UFixed18.sol Lines 288 to 290 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 295 to 297 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 295 to 297 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 287 to 289 in 1ca90e4
root/src/mutability/Implementation.sol Lines 66 to 68 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 288 to 290 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 311 to 313 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 327 to 329 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 311 to 314 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 335 to 337 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 304 to 306 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 343 to 346 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 313 to 316 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 280 to 282 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 329 to 331 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 280 to 282 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 303 to 305 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 361 to 363 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 279 to 281 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 369 to 371 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 330 to 332 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 272 to 274 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 352 to 355 in 1ca90e4
root/src/number/types/Fixed6.sol Lines 319 to 321 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 296 to 298 in 1ca90e4
root/src/mutability/Implementation.sol Lines 71 to 73 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 312 to 314 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 320 to 323 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 345 to 348 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 336 to 339 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 362 to 364 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 320 to 322 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 264 to 266 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 303 to 305 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 304 to 307 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 322 to 324 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 271 to 273 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 263 to 265 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 296 to 298 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 354 to 356 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 337 to 339 in 1ca90e4
root/src/number/types/UFixed6.sol Lines 287 to 289 in 1ca90e4
root/src/number/types/UFixed18.sol Lines 256 to 258 in 1ca90e4
root/src/number/types/Fixed18.sol Lines 328 to 330 in 1ca90e4 solc-versionImpact: Informational
root/src/attribute/Attribute.sol Line 2 in 1ca90e4
root/src/number/types/Fixed18.sol Line 2 in 1ca90e4
root/src/vrgda/VRGDADecayMath.sol Line 2 in 1ca90e4 missing-inheritanceImpact: Informational
root/src/utils/OwnableStub.sol Lines 9 to 15 in 1ca90e4
naming-conventionImpact: Informational
root/src/attribute/Pausable.sol Lines 34 to 36 in 1ca90e4
root/src/mutability/Mutable.sol Lines 37 to 41 in 1ca90e4
root/src/mutability/Mutable.sol Line 34 in 1ca90e4
root/src/attribute/Ownable.sol Line 21 in 1ca90e4
root/src/attribute/Ownable.sol Lines 42 to 44 in 1ca90e4
root/src/mutability/Implementation.sol Line 21 in 1ca90e4
root/src/attribute/Ownable.sol Lines 24 to 28 in 1ca90e4
root/src/mutability/Implementation.sol Lines 24 to 28 in 1ca90e4
root/src/attribute/Pausable.sol Line 23 in 1ca90e4
root/src/attribute/Pausable.sol Lines 26 to 30 in 1ca90e4
root/src/mutability/Implementation.sol Line 76 in 1ca90e4
root/src/attribute/Attribute.sol Lines 20 to 24 in 1ca90e4
root/src/attribute/Attribute.sol Line 17 in 1ca90e4 unimplemented-functionsImpact: Informational
root/src/attribute/Withdrawable.sol Lines 11 to 18 in 1ca90e4
root/src/mutability/Implementation.sol Lines 13 to 77 in 1ca90e4
root/src/attribute/Delegatable.sol Lines 12 to 20 in 1ca90e4
root/src/attribute/Executable.sol Lines 12 to 21 in 1ca90e4 unindexed-event-addressImpact: Informational
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/vrgda/VRGDAMath.t.sol (1)
186-194: Fuzzemissioninstead of pinning it to200.The changed behavior is now parameterized by
emission, but Lines 192 and 194 still only exercise a single value. Varyingemissionover a nonzero range would cover the Jacobian scaling directly and make it much easier to keep a regression for the current CI counterexample.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6a1e32-8dc4-408b-a9a7-928a8911341b
📒 Files selected for processing (4)
src/vrgda/VRGDADecayMath.solsrc/vrgda/types/LinearExponentialVRGDA.soltest/vrgda/LinearExponentialVRGDA.t.soltest/vrgda/VRGDAMath.t.sol
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/vrgda/LinearExponentialVRGDA.t.sol (2)
33-37: Tighten the Jacobian regression tolerances.Line 33 and Line 37 allow 10% relative error, which is quite loose for a targeted correctness guard. Consider reducing to ~1% (or tighter if stable) so scaling regressions are caught earlier.
♻️ Proposed refactor
- assertApproxEqRel(UFixed18.unwrap(cost), 0.1e18, 0.1e18, "neutral time price should approximate price * amount"); + assertApproxEqRel(UFixed18.unwrap(cost), 0.1e18, 0.01e18, "neutral time price should approximate price * amount"); ... - assertApproxEqRel(UFixed18.unwrap(recovered), UFixed18.unwrap(smallAmount), 0.1e18, "neutral time toAmount(toCost(x)) should approximate x"); + assertApproxEqRel(UFixed18.unwrap(recovered), UFixed18.unwrap(smallAmount), 0.01e18, "neutral time toAmount(toCost(x)) should approximate x");
24-27: Make neutral-time setup relative, not absolute.Line 26 and line 27 hardcode both wall-clock time and issued amount. This makes the test fragile: it implicitly assumes
block.timestampat setUp time equals exactly 1 day (86400 seconds), which is not guaranteed by Foundry orRootTest. Use relative timing instead.♻️ Proposed refactor
- vm.warp(86400 + 86400); // time() = 2.0 days, timestamp = 1.0 day, elapsed = 1.0 day - issued = UFixed18Lib.from(200); // exactly on schedule + vm.warp(block.timestamp + 1 days); + issued = vrgda.emission; // exactly on schedule
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fe3e737-4499-4108-aa8c-bd34113c445f
📒 Files selected for processing (1)
test/vrgda/LinearExponentialVRGDA.t.sol
…f dividing by emission
Unit Test Coverage ReportCoverage after merging britz-fix-vrgda-emission into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1 participant
Summary
exponentialDecayintegrates the VRGDA price curve over auction-time but was missing the Jacobian factor (emission) from the change of variables out of token-space (dn = emission * dt). This caused thepriceparameter to behave as price-per-unit-of-auction-time instead of price-per-token.emissionparameter toexponentialDecay()andexponentialDecayI()inVRGDADecayMath.sol, applying* emissionin the forward formula and/ emissionin the inverse.LinearExponentialVRGDA.toCost()andtoAmount()to threademissionthrough.emission = 200. Both fuzz tests pass.Why
The canonical VRGDA defines
targetPriceas the per-token price. When extending to a continuous integral and changing variables from token-amount to auction-time, a Jacobian factor ofemissionis required. The existing tests didn't catch it because they only verified self-consistency (roundtrip, monotonicity), not absolute pricing at neutral time.Note
Medium Risk
Changes core VRGDA pricing/inversion math and function signatures, which will materially change auction prices and could impact any integrators relying on previous (incorrect) behavior.
Overview
Fixes a pricing bug in
VRGDADecayMathby adding anemissionparameter toexponentialDecay/exponentialDecayIand applying the missing emission Jacobian sopriceis interpreted as per-token rather than per unit of auction-time.Threads
emissionthroughLinearExponentialVRGDA.toCost/toAmount, updates all affected test vectors, and adds a new neutral-time test to assert marginal cost ≈price * amountand thattoAmount(toCost(x))round-trips near schedule.Reviewed by Cursor Bugbot for commit f537683. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests