refactor: isPaused, snapshot time, create function

[smol-ninja]

Changelog

• Closes https://github.com/sablier-labs/flow/issues/276
• Closes https://github.com/sablier-labs/flow/issues/279
• Adds a new invariant between rps and pause status of the stream

[smol-ninja]

Updated this PR to close https://github.com/sablier-labs/flow/issues/279.

[andreivladbrg]

should we add concrete tests for rps = 0 in create and adjustRatePerSecond since:

• in create it will just into PAUSED_SOLVENT status (which wasn't possible before)
• in adjustRatePerSecondit will change the status from STREAMING to PAUSED

[smol-ninja]

I don't think they are necessary. We have status concrete tests which already cover all those scenarios. So it would be just a duplicate of that imo.

   ├── given voided
   │  └── it should return VOIDED
   ├── given paused and no uncovered debt
   │  └── it should return PAUSED_SOLVENT
   ├── given paused and uncovered debt
   │  └── it should return PAUSED_INSOLVENT
   ├── given streaming and no uncovered debt
   │  └── it should return STREAMING_SOLVENT
   └── given streaming and uncovered debt
      └── it should return STREAMING_INSOLVENT

[andreivladbrg]

I don't think they are necessary. We have status concrete tests which already cover all those scenarios. So it would be just a duplicate of that imo.

well "necessary" is not even to write tests at all :))

they are new possible scenarios so i'd say it is useful to add them.

[smol-ninja]

Haha. Can you share the tree branch you have in your mind? May be I am missing something.

[smol-ninja]

well "necessary" is not even to write tests at all

Writing tests are necessary imo. What I said was not complete so let me reshare my views.

For create, the following trees cover all the branches for create function.

├── when sender address zero
│  └── it should revert
└── when sender not address zero
   ├── when token not implement decimals
   │  └── it should revert
   └── when token implements decimals
      ├── when token decimals exceeds 18
      │  └── it should revert
      └── when token decimals not exceed 18
         ├── when recipient address zero
         │  └── it should revert
         └── when recipient not address zero
            ├── it should create the stream
            ├── it should bump the next stream id
            ├── it should mint the NFT
            └── it should emit 1 {MetadataUpdate}, 1 {CreateFlowStream} and 1 {Transfer} events

We do not compare stream status post create test. And, the status of a stream are checked through statusOf tree which covers all the possible scenarios. What you are proposing with rps = 0 in create is same as given paused and no uncovered debt branch of statusOf tree. That's why I said that the tests look complete if we consider both create and statusOf branches.

[andreivladbrg]

Can you share the tree branch you have in your mind? May be I am missing something

// --snip--
├── when rate per second zero
│  └── it should create a paused stream
└── when rate per second zero
   ├── it should create a streaming stream
   ├── it should bump the next stream id
   ├── it should mint the NFT
   └── it should emit 1 {MetadataUpdate}, 1 {CreateFlowStream} and 1 {Transfer} events

    function test_WhenRatePerSecondZero()
        external
        whenNoDelegateCall
        whenSenderNotAddressZero
        whenTokenImplementsDecimals
        whenTokenDecimalsNotExceed18
        whenRecipientNotAddressZero
    {
        uint256 streamId = createWithRps(0);
        assertEq(statusOf(streamId) == `PAUSED_SOLVENT);
    }

[smol-ninja]

Looks good. This is similar to what I added to fuzz. Makes sense to me to go with your proposed changes.

If you have time, feel free to push a commit and then merge this PR without my approval.

[andreivladbrg]

great, thanks, will do