search

sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
8 stars 0 forks source link

test: bound rps to realistic ranges #263

Closed smol-ninja closed 3 weeks ago

smol-ninja commented 3 weeks ago

Continuing from this argument, I suggest we use the realistic ranges for RPS so that our fuzz and invariant tests can test the contract for tight realistic ranges rather than a wide ranges for RPS.

I also think that it's a good idea to set the rps bounds in the Utils file for fuzz and invariant tests to a very reasonable range. Currently, the lower bound is 0.0000000001e18, which is $0.000259 per month in terms of USDC. We have agreed upon the assumption that we don't expect Flow to be used to stream such a low amount, so why waste runs on such a low stream? I'd recommend it to set something like [$100 per month, $10000 per month] so atleast we are confident that it wont bother us in that range.

PS: the test fails that means our tests have some issues.

andreivladbrg commented 3 weeks ago

Although I understand your rationale, I don’t agree with this change, as keeping the lower range for rps might help us find other problems we are not yet aware of.

smol-ninja commented 3 weeks ago

On the contrary, changing the rps to realistic range is leading us to new problems that we could not find otherwise. See the failing tests in the CI of this PR.

The problem is that, 100,000 runs are insufficient given the wide range of RPS. If we want to keep lower range for RPS, then based on the above argument, we should have 1M runs. Wider the range, higher the runs.

The only good argument in keeping low ranged rps is to check that no value of rps should lead to over streaming. Other than that, I dont see any value in using ranges of rps that can only stream $0.1 a year in terms of USDC. Thus I propose the following:

  1. Include a test in WithdrawMultiple contract that assert that actual withdrawn is always less than expected for a very wide range rps.
  2. Change rps to realistic range for all other tests
smol-ninja commented 3 weeks ago

The only good argument in keeping low ranged rps is to check that no value of rps should lead to over streaming. Other than that, I dont see any value in using ranges of rps that can only stream $0.1 a year in terms of USDC. Thus I propose the following:

  1. Include a test in WithdrawMultiple contract that assert that actual withdrawn is always less than expected for a very wide range rps.
  2. Change rps to realistic range for all other tests
smol-ninja commented 3 weeks ago

Tagging @PaulRBerg for your view on this.

PaulRBerg commented 3 weeks ago

Great idea @smol-ninja, I'm in favor as long as we're still testing the wide range in at least one or a few places.

andreivladbrg commented 3 weeks ago
  • Include a test in WithdrawMultiple contract that assert that actual withdrawn is always less than expected for a very wide range rps.
  • Change rps to realistic range for all other tests

the idea of WithdrawMultiple contract test is to see if there is a long delay for a realistic range for rps

so, i would do it the other way around - withdrawMultiple test, small range, other fuzz/invariant tests, wide range

smol-ninja commented 3 weeks ago

Thank you both of you for the feedback.

To @andreivladbrg,

other fuzz/invariant tests, wide range

How do you then explain the failed CI in this PR? I think with the wide ranges, seems like we have been asserting 0 values (0 due to rounding off).

andreivladbrg commented 3 weeks ago

How do you then explain the failed CI in this PR? I think with the wide ranges, seems like we have been asserting 0 values (0 due to rounding off).

i don't know the reason, i need to debug

smol-ninja commented 3 weeks ago

i don't know the reason, i need to debug

I'll debug and let you know.

smol-ninja commented 3 weeks ago

Meanwhile, @andreivladbrg can you please respond to my comment? You haven't said why you refuse to agree with them.

You mentioned that "keeping the lower range for rps might help us find other problems", to which I replied "The only good argument in keeping low ranged rps is to check that no value of rps should lead to over streaming". What other problems you think you can find with keeping lower range of rps?

The reason tests were failing is the following:

So, as a fix, I adjusted bounds for deposit amount based on 2 years depletion period. Also, changed uint40(solvencyPeriod) to solvencyPeriod.toUint40().

This is another good argument why realistic ranges are useful. Previously, in wide ranges, even though foundry could choose ultra low RPS and extremely high deposit, it could not because the range is so large that even millions of runs that we had run through CI could not detect it.

Your concern that low RPS might be useful to find issues is valid. But I am saying the only issue we need to care about is over streaming and for that, I agree to include wide range RPS in such tests but for all other tests, I haven't seen an argument to why we should not choose realistic ranges.

A perfect solution would be to run all the tests for various ranges of RPS similar to web2 software testing practices. However, I am not sure if foundry accept inputs from a separate file. That will also take a long time to run. In the long term, we should try to achieve that though.

smol-ninja commented 3 weeks ago

Given how tight the timeline is, I'd appreciate if we can take decision on this before the weekend. @PaulRBerg is in favour so @andreivladbrg I am just waiting for your comment / agreement / disagreement.

PaulRBerg commented 3 weeks ago

Let's merge and do this afterward: https://github.com/sablier-labs/flow/issues/272

smol-ninja commented 3 weeks ago

I believe it’s becoming one of those things that cause decision fatigue, so feel free to merge it

Thank you. I believe its the right approach :)). And as I mentioned in my comment:

A perfect solution would be to run all the tests for various ranges of RPS similar to web2 software testing practices. However, I am not sure if foundry accept inputs from a separate file. That will also take a long time to run. In the long term, we should try to achieve that though.

We can run the same tests suite for wide range of RPS in a much better way through the use of ffi (yet to figure out if its possible). We can pass different small ranges for RPS through CI and let the test run (CI-Deep or a new workflow) on all those various ranges. This will help us achieve the same as before but in a very efficient manner.