Conversation
Co-authored-by: EndymionJkb <EndymionJkb@gmail.com>
…e trouble than it's worth, and associated helpers
jubeira
left a comment
jubeira
left a comment
There was a problem hiding this comment.
First pass done.
I have some doubts about pool balances now that we're zooming in though.
…he for performance; for consistency, use current live balances in _updateVirtualBalances, to prevent rate drift; computeCurrentPoolCenteredness should call _computeCurrentVirtualBalances, like all other view functions; also address stale interface comments
jubeira
left a comment
jubeira
left a comment
There was a problem hiding this comment.
LGTM!
Just a few minor comments and questions; otherwise it's good to go.
| vm.prank(admin); | ||
| ReClammPool(pool).setDailyPriceShiftExponent(_DEFAULT_DAILY_PRICE_SHIFT_EXPONENT); |
There was a problem hiding this comment.
why is this needed here? Time has already been skipped at this point
test/foundry/ReClammPool.t.sol
Outdated
| (uint256 storedA, ) = ReClammPool(pool).getLastVirtualBalances(); | ||
| uint256 diffFromLive = storedA > expectedFromLive ? storedA - expectedFromLive : expectedFromLive - storedA; | ||
| uint256 diffFromStale = storedA > expectedFromStale ? storedA - expectedFromStale : expectedFromStale - storedA; | ||
| assertLt(diffFromLive, diffFromStale, "Stored virtual balance A is closer to stale than to live"); |
There was a problem hiding this comment.
I think I don't get what we're trying to prove here. Could you please clarify?
There was a problem hiding this comment.
Added comments to explain
test/medusa/SwapReClamm.medusa.sol
Outdated
|
|
||
| function computeAddLiquidity(uint256 exactBptOut) public { | ||
| uint256 oldTotalSupply = ReClammPool(address(pool)).totalSupply(); | ||
| if (oldTotalSupply < 1e18) { |
There was a problem hiding this comment.
This came from trying to get the reverts down to 0 (from 100). It was an early attempt that didn't work (hypothesis that turned out to be wrong), and it turns out I don't need it.
jubeira
left a comment
jubeira
left a comment
There was a problem hiding this comment.
Just a few final suggestions (comments only); LGTM!
Looks much more consistent than before now, thanks for pushing this @EndymionJkb.
2 participants
Description
I seem to remember being the one to introduce and argue for the
onlyWithinTargetRange, modifier, but I think it's more trouble than it's worth. It was only used once, required a bunch of helpers, caused multiple redundant calls, and had a bug besides.In particular, several ReClamm view functions used the Vault’s last live balances (getPoolTokenInfo). For rate-bearing tokens, those balances become stale between interactions as rates accrue, causing isPoolWithinTargetRange and computeCurrentPoolCenteredness to report outdated results. Now everything uses current real and virtual balances.
We routed setCenterednessMargin through _updateVirtualBalances() to ensure centeredness checks are performed against freshly updated virtual balances/timestamp and live balances, reducing duplication and potential drift.
We removed the redundant isPoolWithinTargetRangeUsingCurrentVirtualBalances and updated interface docs/tests accordingly. Hopefully this isn't being used? If it is, we can keep it as a deprecated wrapper I guess.
One extraneous thing (could separate if too annoying)... I wanted to check the medusa tests, and made a fix there.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution