search

sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
10 stars 2 forks source link

feat: protocol fee #222

Closed smol-ninja closed 2 months ago

smol-ninja commented 2 months ago

Changelog

PR dependency

smol-ninja commented 2 months ago

The PR is ready for your reviews. I didn't include protocol fee related changes in fork tests. Lmk if thats needed.

smol-ninja commented 2 months ago

Added a new commit. LMK if it all looks good now, could you please approve it then? I won't merge it before https://github.com/sablier-labs/flow/pull/221

andreivladbrg commented 2 months ago

there are two things in mind left:

  1. add assert in Helpers.calculateAmountsFromFee - but the invariant tests would fail, and i am not sure why
// 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;
  1. what if we add in the invariant handlers the collectProtocolRevenue and setProtocolFee only for the tokenWithProtocolFee
smol-ninja commented 2 months ago

add assert in Helpers.calculateAmountsFromFee - but the invariant tests would fail, and i am not sure why

As discussed on slack, since the withdraw amount can be zero, the assertion should be assert(totalAmount >= feeAmount);. Regardless, why do you want to add the assertion? totalAmount - feeAmount would revert if feeAmount exceeds totalAmount.

what if we add in the invariant handlers the collectProtocolRevenue and setProtocolFee only for the tokenWithProtocolFee

Not sure if its needed since they are admin only and does not affect other functionalities of the Flow contract especially collectProtocolRevenue. For setProtocolFee, we have a fuzz test in withdrawAt.t.sol called testFuzz_ProtocolFeeNotZero that tests withdraw for all values of protocol fee.

I think it would be redundant to add it to invariant. And if it does not add much value, it would increase the maintenance cost for the code.

smol-ninja commented 2 months ago

@andreivladbrg I've added a new commit https://github.com/sablier-labs/flow/pull/222/commits/d7dd3ff0dd72c112c727d51e666cba78d6f4789f, found a couple of errors while reviewing the PR. lmk if it looks good now. Also, looking forward to your response to this.

andreivladbrg commented 2 months ago

Not sure if its needed since they are admin only and does not affect other functionalities of the Flow contract especially collectProtocolRevenue. For setProtocolFee, we have a fuzz test in withdrawAt.t.sol called testFuzz_ProtocolFeeNotZero that tests withdraw for all values of protocol fee. I think it would be redundant to add it to invariant. And if it does not add much value, it would increase the maintenance cost for the code

fair enough, my idea was to add setProtocolFee, to have a fee switching between different values, may be it changes how things, but it was just an idea

@andreivladbrg I've added a new commit https://github.com/sablier-labs/flow/commit/d7dd3ff0dd72c112c727d51e666cba78d6f4789f, found a couple of errors while reviewing the PR. lmk if it looks good now. Also, looking forward to your response to https://github.com/sablier-labs/flow/pull/222#issuecomment-2332455606.

looks good