Closed smol-ninja closed 3 weeks ago
Thanks for the PR.
I want to mention that this solution seems to work for _withdraw
(I couldnāt find anything that breaks it), but pause
and adjustRatePerSecond
still suffer from the delay issue.
You can run this:
Ran on this commit eb096bc5edc85c39317719deffdb46ef397e1e88 ```solidity function testDelayUsdc_OngoingDebt() public { uint128 rps = 0.000000011574e18; // 0.001e6 USDC per day, less than smallest value of USDC 0.00000001e6 uint128 depositAmount = 0.001e6; uint128 factor = uint128(10 ** (18 - 6)); uint40 constantInterval = uint40(factor / rps); // 10^12 / (1.1574 * 10^10) assertEq(constantInterval, 86, "constant interval"); uint256 streamId = flow.createAndDeposit(users.sender, users.recipient, ud21x18(rps), usdc, true, depositAmount); uint40 initialSnapshotTime = MAY_1_2024; assertEq(flow.getSnapshotTime(streamId), initialSnapshotTime, "snapshot time"); // rps * 1 days = 0.000999e6 due to how the rational numbers work in math :)) // so we need to warp one more second in the future to get the deposit amount vm.warp(initialSnapshotTime + 1 days + 1 seconds); assertEq(flow.ongoingDebtOf(streamId), depositAmount, "ongoing debt vs deposit amount"); // now, since everything has work as expected, let's go back in time to pause and then immediately restart // the first discrete release is at constantInterval + 1 second // after that, it is periodic to constantInterval // warp to a timestamp that ongoing debt is greater than zero vm.warp(initialSnapshotTime + constantInterval + 1); assertEq(flow.ongoingDebtOf(streamId), 1, "ongoing debt vs first discrete release"); // to test the constant interval is correct uint40 delay = constantInterval - 1; vm.warp(initialSnapshotTime + (constantInterval + 1) + delay); assertEq(flow.ongoingDebtOf(streamId), 1, "ongoing debt vs delay"); // same as before // we will have delay of (constantInterval - 1) flow.pause(streamId); flow.restart(streamId, ud21x18(rps)); // assert that the snapshot debt has been updated with the ongoing debt assertEq(flow.getSnapshotDebt(streamId), 1, "snapshot debt"); // now, let's go again at the time we've tested ongoingDebt == depositAmount vm.warp(initialSnapshotTime + 1 days + 1 seconds); // theoretically, it needs to be depositAmount - snapshotDebt, but it is not // as we have discrete intervals, the full initial deposited amount gets released now after the delay assertFalse( flow.ongoingDebtOf(streamId) == depositAmount - flow.getSnapshotDebt(streamId), "ongoing debt vs deposit amount - snapshot debt first warp" ); vm.warp(initialSnapshotTime + 1 days + 1 seconds + delay + 1 seconds); assertEq( flow.ongoingDebtOf(streamId), depositAmount - flow.getSnapshotDebt(streamId), "ongoing debt vs deposit amount - snapshot debt second warp" ); } ```
Therefore, my alternative is to reintroduce correctedTime, but calculate it only if certain criteria are met:
factor = 10 ^ (18 - tokenDecimals)
rps < factor
rps > factor && rps % factor != 0
ongoingDebt
from correctedTime
Not yet very sure about 3. , but re. 4:
newElapsedTime = correctedTime - snapshotTime;
uint128 recalculatedOngoingDebt = (newElapsedTime * ratePerSecond) / factor;
Thanks for the feedback @andreivladbrg. The following code in withdraw
is analogous to correctedTime
.
_streams[streamId].snapshotTime += uint40(
((amount - _streams[streamId].snapshotDebt) * (10 ** (18 - _streams[streamId].tokenDecimals)))
/ _streams[streamId].ratePerSecond.unwrap()
);
Are you talking about using storing correctedTime
in pause
and adjustPerSecond
instead of block.timestamp
? If yes, I have tried it already. It has some issues as described below:
adjustPerSecond
, when you store corrected time and then change the rps, since the snapshot time would be the corrected time, between corrected time and block.timestamp
, it would use the new rps to calculated streamed amount. This can result into over-streaming to the recipient.pause
, the objective with not using corrected time is to allow user to withdraw the entire amount.Thus, I intentionally did not use corrected time in both of them.
The following code in withdraw is analogous to correctedTime
Yes
Are you talking about using storing
correctedTime
inpause
andadjustPerSecond
instead ofblock.timestamp
? If yes, I have tried it already. It has some issues as described below:
In ongoingDebt
I mean.
- In case of
adjustPerSecond
, when you store corrected time and then change the rps, since the snapshot time would be the corrected time, between corrected time andblock.timestamp
, it would use the new rps to calculated streamed amount. This can result into over-streaming to the recipient.- In case of
pause
, the objective with not using corrected time is to allow user to withdraw the entire amount
Hmm, I don't understand these arguments, as correctedTime
is bounded between [t0,t1]
where each value would cause ongoingDebt in to a constant value. And we need to find t0
.
Having said that, how would this lead to over-streaming? Even more, what if the block.timestamp
when adjustRatePerSecond
is called is actually the correctedTime
?
One other alternative would be:
uint40 constantInterval = uint40((1e18 * factor) / rps);
// so ongoing debt would be how many intervals can fit in elapsed time
uint128 ongoingDebt = (1e18 * elapsedTime) / constantInterval;
what it does, it finds the "period" of time when the ongoingDebt remains constant, and then calculates the ongoing debt, by finding how many intervals can fit within elapsed time
but this version, since it has one more division involved might be more problematic
Hmm, I don't understand these arguments, as correctedTime is bounded between [t0,t1] where each value would cause ongoingDebt in to a constant value. And we need to find t0.
In adjustRatePerSecond
, this is what can happen:
t0
(new rps - old rps) * (tx time - t0)
. Its a small number we can adjust for it, but then we might have to modify some of the failing invariants because of this.
Damn, yeah got it now
since the idea behind the adjusment of withdrawAmount
is to find the largest multiple of rps
that is lower than or equal to the parameter itself, it would be a good idea to add an assert statement:
if (prevAmount != withdrawAmount) {
assert(10 ** (18 - _streams[streamId].tokenDecimals) * (prevAmount - withdrawAmount) < ratePerSecond.unwrap());
}
i find the current version of withdraw
verbose, it has too many comments which might make the reader "distracted" IMO - thus i would reduce the comments
also, we could consider dividing it in multiple functions (regardless of the gas implications)
wydt? @sablier-labs/solidity
we could consider dividing it in multiple functions (regardless of the gas implications)
Since these logics are only used in withdraw
function, how would splitting it into multiple function improve readability?
it has too many comments which might make the reader "distracted" IMO - thus i would reduce the comments
On the contrary, the comments helps readers while reading the code instead of distracting them. Without the comments, the readers might not even understand why does it have some lines.
Since these logics are only used in
withdraw
function, how would splitting it into multiple function improve readability?
One idea could be:
```solidity /// FlowBase function _updateProtocolState( uint128 totalAmount, IERC20 token ) internal returns (uint128 netAmount, uint128 feeAmount) { // Load the variables in memory. UD60x18 protocolFee = protocolFee[token]; if (protocolFee > ZERO) { // Calculate the fee amount based on the fee percentage. feeAmount = ud(totalAmount).mul(protocolFee).intoUint128(); // Calculate the net amount after subtracting the fee from the total amount. netAmount = totalAmount - feeAmount; // Safe to use unchecked because addition cannot overflow. unchecked { // Effect: update the protocol revenue. protocolRevenue[token] += feeAmount; } } else { netAmount = totalAmount; } unchecked { // Effect: update the aggregate amount. aggregateBalance[token] -= netAmount; } } /// Flow function _withdraw(uint256 streamId, address to, uint128 withdrawAmount) internal { // --snip-- (withdrawAmount, feeAmount) = _updateProtocolState(withdrawAmount, token); } ```
i think solutions are
the comments helps readers while reading the code instead of distracting them
i consider self-explanatory code + succinct comments to be the best way to write code. anything more detailed can be addressed in the docs IMO
As discussed on Slack, we will keep the verbose comments in the withdraw function because the logic is sophisticated and non-intuitive and requires a proper explanation.
In addition:
amount
in the refund functionwithdraw
and refund
The test failed because I moved withdrawnAmount == 0
check post if..else
which we need to update in the invariant handler.
which we need to update in the invariant handler.
Also in BTT trees right?
I am not sure if there is a need to round down the refund amount. Even if refund amount modifies the balance such that withdrawable is not a multiple of rps (when balance is withdrawable), the else
logic of _withdraw
is independent of balance value. Ergo, functions like deposit
, refund
etc does not affect it. cc @andreivladbrg.
Yeah, I think you're right @smol-ninja. If the stream is paused or voided, the withdrawable amount that is not a multiple of rps
would be withdrawable in full without being rounded down.
@andreivladbrg can we please merge this PR now since we have all agreed on the solution? And https://github.com/sablier-labs/flow/pull/255 also helps in verifying that the approach used in this PR is better.
Closing this PR. Thanks everyone the long productive discussion.
I have created a new branch: https://github.com/sablier-labs/flow/tree/feat/support-extremely-low-rps for future reference.
Please see natspec comments for the explanation.
Tasks:
amount
in withdrawwithdrawAmount
is not less thanrps
withdrawAmount