Add assert statement in `Helpers.calculateAmountsFromFee`

[andreivladbrg]

We will merge this PR https://github.com/sablier-labs/flow/pull/222 without adding assert in Helpers.calculateAmountsFromFee.

// Calculate the fee amount based on the fee percentage.
feeAmount = ud(totalAmount).mul(fee).intoUint128();

// Assert that the total amount is strictly greater than the fee amount.
assert(totalAmount > feeAmount);

// Calculate the net amount after subtracting the fee from the total amount.
netAmount = totalAmount - feeAmount;

The reason is because the invariants would fail due to denormalization (we can have time > snapshotTime and a totalDebt = 0), as there is the possibility of passing an withdraw amount of zero which would result in totalAmount == feeAmount.

We will include this assert in the withdraw amount.

[smol-ninja]

Could you please explain again why totalAmount can be equal to feeAmount? Since we calculate feeAmount as ud(totalAmount).mul(fee).intoUint128(), I am not able to understand how it can be equal until totalAmount is zero itself, which is entirely possible in _withdrawAt.

[andreivladbrg]

Could you please explain again why totalAmount can be equal to feeAmount

as mentioned in the OP, it can be equal when withdrawAmount == 0 (zero), 1% of zero is still zero, i.e. they are equal:

// in case of USDC, i.e. 6 decimals
uint128 rps = 0.0000001e18 // less than MVT

// assume elapsed time is 5 seconds
uint128 ongoingDebtNorm = rps * elt; // 0.0000005e18
uint128 ongoingDebt = denormalize(ongoingDebtNorm, 6); // this will be zero

// when snapshotDebt is zero, totalDebt will be only the amount calculated above
uint128 withdrawAmount = ongoingDebt; // 0
uint128 feeAmount = ud(withdrawAmount).mul(fee).intoUint128(); // 0

// since both are 0, this will fail
assert(withdrawAmount > feeAmount);

[smol-ninja]

Why not assert(withdrawAmount >= feeAmount);? since totalAmount can also be zero.

[andreivladbrg]

Why not assert(withdrawAmount >= feeAmount);

because in case there other values which cause them to equal, this assertion will not fail, but it should

we should not use "=" only for the case when withdrawAmount is zero, which is going to be changed

[smol-ninja]

because in case there other values which cause them to equal, this assertion will not fail, but it should

What do you mean by this? In your example above, withdrawAmount is zero so fee would also be zero.

we should not use "=" only for the case when withdrawAmount is zero, which is going to be changed

Do you mean "we should only use = when withdrawAmount is zero"?

[andreivladbrg]

What do you mean by this?

Using assert(totalAmount >= feeAmount); allows for the case where totalAmount equals feeAmount, which can be misleading. Since the only predictable scenario where they might be equal is when totalAmount is zero, it’s better to use assert(totalAmount > feeAmount); to not create false positive.

In your example above, withdrawAmount is zero so fee would also be zero.

Nope, I meant what I've said :))

[smol-ninja]

I see. So, you propose to revert the withdrawal if the total amount is zero? And in that case, why do we even need to have assert since total amount cannot be zero? The fee has a cap, so it will always be the case that the total amount > fee amount. I also think having an assertion check here where the fee is capped by MAX_FEE is not really necessary.

[andreivladbrg]

you propose to revert the withdrawal if the total amount is zero

yeah, since we will have an amount as param in withdraw, we will need to revert:

https://github.com/sablier-labs/flow/blob/9c660108875f8dce287c7e976840a2671e91fe72/src/SablierFlow.sol#L390-L393

The fee has a cap, so it will always be the case that the total amount > fee amount. I also think having an assertion check here where the fee is capped by MAX_FEE is not really necessary

well that's the whole purpose of using assert statement, it is an invariant which we need to ensure, as the docs says

[smol-ninja]

yeah, since we will have an amount as param in withdraw, we will need to revert:

Sounds good

that's the whole purpose of using assert statement, it is an invariant which we need to ensure

I understand that assert is useful to check for invariant conditions. However, its useful when we do not have control over the "inputs". In this case, we are using prb-math library to find feeAmount which is a percentage of totalAmount, and also the feeAmount is capped at MAX_FEE which is a "constant". So there is no way, assert can fail until the EVM has a bug and mutates the constant value and in that case, nothing would matter. Additionally, assert would increase the gas cost. You can argue that gas cost is minimum but some gas cost here, some gas cost there and we might be seeing an increase in 5k-10k in gas.

PS: supporting my argument with what you have said in https://github.com/sablier-labs/flow/pull/232: "I have used unchecked for the balance subtraction, as we know for sure is not going to be greater than it." So same arguement applies here.

[andreivladbrg]

Yeah, I agree now. I might have been a bit too cautious about this

Happy to remove the assert