Replace assert with if condition in checkAndCalculateBrokerFee

[smol-ninja]

There is only one place where we still use assert check.

v2-core/src/libraries/Helpers.sol

Line 42 in be1dea4

assert(totalAmount > amounts.brokerFee);

Integration tests also don't cover this scenario where broker fee exceeds total amount. This seems to be safe because we already have a check above it

v2-core/src/libraries/Helpers.sol

Lines 33 to 35 in be1dea4

	if (brokerFee.gt(maxBrokerFee)) {
	revert Errors.SablierV2Lockup_BrokerFeeTooHigh(brokerFee, maxBrokerFee);
	}

However, since we are re-checking whether broker amount is less than total amount, we should also cover this in tests and replae it it with if.

  // Assert that the the broker fee amount is strictly less than the total amount.
  if (amounts.brokerFee >= totalAmount) {
      revert Errors.SablierV2Lockup_BrokerFeeExceedsTotalAmount(brokerFee, totalAmount);
  }

This will also increase branch coverage of Helpers.sol.

Would love to hear your thoughts on this @PaulRBerg.

[PaulRBerg]

Since the max fee is hard-coded to 10%, it is impossible to write a test that makes the amounts.brokerFee greater than totalAmount. That is, unless we deploy a mock contract with a different max fee (or change the value with StdStorage?), but there's no point in doing this because the max fee is hard-coded.

This assertion is there on a precautionary basis - if there's an unknown bug, the assertion will stop contract execution. But since we don't know what bug there is, we cannot write a test for it.

Given this rationale, I'd keep the assertion as is.

P.S. See the discussion I opened on Twitter about production assertions: What are your thoughts on leaving asserts in production contracts?

[smol-ninja]

Fair. I understand your intention here. Having said that, it still looks superfluous to re-validate it right after an arithmetic operation. The two validations inside checkAndCalculateBrokerFee (brokerFee.gt(maxBrokerFee) and assert(totalAmount > amounts.brokerFee)) only have a multiplication operation between them. So, this validates whether .mul, ud and intoUint256 are working as expected or not.

What if we move this assert in _createWithTimestamps function after the following line?

Helpers.checkCreateWithTimestamps(createAmounts.deposit, params.segments, MAX_SEGMENT_COUNT, params.startTime);

Then the assertion is made in the context of the create function and not the helper function.

PS: I am only brainstorming to understand if the invariant is checked at the right place. I have already agreed with having assertions here.

[PaulRBerg]

On a separate note, this discussion makes me realize that it would be helpful to write a test that validates that MAX_BROKER_FEE has a value of 10%. WDYT?

[smol-ninja]

Though an interesting take on validating the constant values, I think it would be unnecessary. 10% is just a value assigned to a constant variable MAX_BROKER_FEE (unless we assume that constant can be changed too). Why made you realize this?

[PaulRBerg]

We might accidentally edit the constant value, and it isn't something that is obviously noticeable. Even an auditor might assume that the change was intentional.

With a test, we would detect the accidental edit.

[smol-ninja]

Fair enough.

[smol-ninja]

Closing it since no more discussion is required.