test(invariant): include delay in each period

[andreivladbrg]

This PR is fixing the invariant_TotalStreamedEqTotalDebtPlusWithdrawn invariant by introducing the delay in the Period struct.

Refer to: https://gist.github.com/andreivladbrg/b5968b8c16f5924d082dbce607f55e97#first-example

[smol-ninja]

@andreivladbrg should we merge it into #240 so that we can test #240 with your changes?

[andreivladbrg]

@andreivladbrg should we merge it into #240 so that we can test #240 with your changes?

you need to make additional changes (remove flowStore.updateDelay flowStore.pushPeriod in withdraw handler) to see if your fix is actually correct

[smol-ninja]

you need to make additional changes (remove flowStore.updateDelay flowStore.pushPeriod in withdraw handler) to see if your fix is actually correct

OK but shouldn't we create this PR against that branch instead of main branch?

to see if your fix is actually correct

I am now more confident that fix is better than main. The test that differentiates the two can be found here: #255

[smol-ninja]

While reviewing this PR, a few thoughts came to mind:

• First, this invariant seems to be becoming an example where we are no longer comparing states but instead relying on an implementation. We've modified the logic of calculateTotalStreamedAmount to ensure it passes the test. We should be checking the difference between simple states (rps * (end - start) is the most intuitive value for the streaming amount) and ensuring it doesn’t exceed a certain limit.

• Additionally, with its latest implementation, this invariant would fail if we devise a more efficient solution that minimizes the delay. I believe an invariant shouldn't rely so heavily on the core logic; it should focus more on comparing states. Currently, how we're determining the streamed amount is too closely tied to the internal logic rather than the states.

• For example, if there is a discrepancy between a calculated value and an expected value, a good invariant would be (expected - actual) / expected < 0.1% rather than merely asserting them as equal. The former ensures that the error is below 0.1%, whereas the latter can pass even when the error is 10% because we will calculate the delay and add to it.

• Therefore, I do not think we should include this change and rather rely on the previous implementation. The role of invariant tests is to ensure that a certain kind of relation between the states hold true..

[andreivladbrg]

Thanks for the feedback.

• First, this invariant seems to be becoming an example where we are no longer comparing states but instead relying on an implementation.

Actually we are comparing "states" (totalDebtOf and implicitly snapshotTime)

We've modified the logic of calculateTotalStreamedAmount to ensure it passes the test. We should be checking the difference between simple states (rps * (end - start) is the most intuitive value for the streaming amount) and ensuring it doesn’t exceed a certain limit

we did not modified to "pass" the test, but we modified to test the fact the delay is a real thing in certain functions (e.g. adjustRatePerSecond & pause)

with its latest implementation, this invariant would fail if we devise a more efficient solution that minimizes the delay

if the solution in the withdraw function is correct, this invariant shouldn't fail

I believe an invariant shouldn't rely so heavily on the core logic; it should focus more on comparing states

what core logic is used in this test? it compares the states (totalDebt, and implicitly the snapshoTime - if it was updated with a delay or not) the concept of delay is tied up with st (i.e. state)

how we're determining the streamed amount is too closely tied to the internal logic

not sure what you mean, it was always this way

(expected - actual) / expected < 0.1% rather than merely asserting them as equal. The former ensures that the error is below 0.1%, whereas the latter can pass even when the error is 10% because we will calculate the delay and add to it.

that's exactly the problem IMO. we are asserting something that we don't why exactly is is below. the delay happens in the flow contract without actually writing logic per se for it the contract. we are testing (exactly) a behaviour that happens as a "consequence" of fixed point numbers and how rational math works

to make an analogy, why i say using a percentage like 0.1% is not that precise, is like testing in withdraw, that the balance of the stream has simply decreased, instead of testing exactly the new balance is: prevBal - withdrawAmount

Therefore, I do not think we should include this change and rather rely on the previous implementation

i disagree with this; therefore, i highly suggest keeping this implementation as it is more robust. besides the state test, it also tests a behavior that occurs unintentionally

[andreivladbrg]

@smol-ninja added the new assert in the invariant that ensures no over streaming:

assertGe(
    totalStreamedAmount,
    totalStreamedAmountWithDelay,
    "Invariant violation: total streamed amount >= total streamed amount with delay"
);

[smol-ninja]

LGTM. Thanks for making all those changes. What I was suggesting is something similar to #258 where instead of having a dynamic delay variable, we set fixed limits to ensure the differences never exceed those bounds. And if in the future, we see a test which fails because the difference exceed the bounds that would mean, we have to give it attention. With dynamic delay, that wont be possible because even if the delay is 1 week, lets say, the test would still pass.

However, given we merge that fuzz test, I am fine with your implementation. I only want to have some test that keep checking that delay is within limits.

New commits that requires your attentions:

• 12130a8
• 92cdc8d

[andreivladbrg]

New commits that requires your attentions

thanks, looks good, merging