Replace isPaused state with runtime check with rps

[smol-ninja]

A paused stream is characterized by two states that are analogous to each other: rps == 0 and isPaused == true. Thus we can remove isPaused state from the contract and instead return true if rps == 0.

Pros

1. 2500 gas saving in paused, void and restart
2. Single state to track whether the stream is paused or not. Currently we have to mandatory update both rps and isPaused

Cons

1. None

Note

As we're quite ready for audits with flow, we can discuss this for an upcoming version of the protocol, post-launch. However, this change is minor so, if we all agree, it can be included before the audit. I am in favour of including this before audit.

cc @sablier-labs/solidity

[andreivladbrg]

I am against this proposal for the same reason why we intially introduced isStream in lockup contracts, and we no longer check for sender != address(0) if a stream exists. The same logic applies to more recent wasCanceled and isDepleted.

Using a dedicated boolean is a best practice for checking entry points in functions and ensures clarity and security.

2500 gas saving in paused, void and restart

In this case, I prioritize a robust and secure pattern over gas optimizations. But if you want to optimize we can, instead of removing isPaused, we could remove updating rps to zero in pause /void (since we check isPaused = true in ongoingDebt function). Tho, I’m not sure if I agree with this approach either, because updating it to zero is more secure.

[PaulRBerg]

I also agree to remove isStream from the Lockup and use runtime check for sender address.

I don't. Different problem situations because of the logical possibilities of users wanting to set sender to address(0).

[smol-ninja]

Understood, though I'm not entirely convinced how the sender could set the address to 0 if that's no longer allowed. We can dive deeper into this through a separate Lockup discussion later.

@andreivladbrg @PaulRBerg if could you share your thoughts, if anything new, on whether we should proceed with this proposal? To clarify, my reasoning is that when a stream is paused, rps is 0 and isPaused is true. Conversely, when it's unpaused, rps is greater than 0 and isPaused is false. This means we're using two storage states to convey the same information.

[andreivladbrg]

I still believe that having dedicated booleans is a safer design choice (it also improves code clarity) and provides a more robust pattern in case we scale to a more complex version with additional rps changes. I consider the version from my comment here to be the best.

Regarding the "isometry" point, we have these possible scenarios (when the stream does not exist or they are not in sync):

• stream does not exist: rps = 0 && isPaused = false
• stream does exist: rps > 0 && isPaused = false
• stream does exist: rps = 0 && isPaused = true

my reasoning is that when a stream is paused, rps is 0 and isPaused is true. Conversely, when it's unpaused, rps is greater than 0 and isPaused is false. This means we're using two storage states to convey the same information.

You’re correct. I never disagreed with this; I was simply discussing about "best practices"

sender used to be zero address sablier-labs/lockup#1018. That is why isStream was introduced in previous versions of Lockup

I don’t think you have the full context, sorry. Back in 2022, we used to also check the sender before introducing the isStream boolean.

---

Indeed. You should have just pointed out the error, not escalated it.

I didn’t find any error in the logic, so I responded on the subject. I apologize if it seemed like I was escalating; that wasn’t my intent. I was simply acknowledging that Shub was right in his statement, and we both lacked consistency in our thinking

re isStream + sender and rps analogy, that's exactly the same.

for lockup:

1. we checked wether sender == address(0) in create
2. we checked wether a stream existed based on sender == address(0)

for flow:

1. we check wether rps == 0 in create
2. we will check wether a stream is paused based on rps == 0

What I meant to say was that the sender may have wanted to set the sender to address(0). He was indeed blocked in the create function, but the point is that there was a logical possibility for the sender to the zero address and the stream to EXIST

as you can see above, that's pretty much the same thing

In Lockup, we had that check only when the boolean flag DID NOT exist.

We also have this in the current version, although we now use the isStream bool

---

As far as I can tell, the majority is in favor of making this change, and I don’t have anything further to add. Let’s go ahead with the change.

[smol-ninja]

Thanks for sharing your views on this, Andrei. Appreciate it. Understood everything you said and agree with you. Just a small note:

provides a more robust pattern in case we scale to a more complex version with additional rps changes

As far as I understand, we follow an adaptive decision-making approach when it comes to writing contracts. So, if we move towards a complex version of Flow with additional rps changes (as necessitated by the future requirement), we can then decide/discuss to implement the isPaused storage as needed. I will also be in favor of that.

[PaulRBerg]

Thank you for the additional color @andreivladbrg. I responded to the main point you made in the Loom recording shared below.

I didn’t find any error in the logic, so I responded on the subject. I apologize if it seemed like I was escalating; that wasn’t my intent

Got it. No worries.

[PaulRBerg]

Agree with Shub's proposal, let's implement it.

[smol-ninja]

Issue

• Remove isPaused from storage slot and update snapshotTime in pause and void #276

[PaulRBerg]

I am still in favor of Shub' proposal. @andreivladbrg I responded to your long comment above through this Loom recording.

---

To @smol-ninja:

we follow an adaptive decision-making approach

Indeed. Also called YAGNI.

This means we're using two storage states to convey the same information.

Correct

I'm not entirely convinced how the sender could set the address to 0 if that's no longer allowed.

It's a matter of logical possibility. It is possible to have a Lockup stream with a zero address sender if the stream creator wishes so (as in, logically; I know that our if check blocks this in practice). But it is NOT possible to have a Flow stream with an rps == 0 that is NOT paused.

That we don't allow rps == 0 in the create function is a subjective preference. It's not an objective necessity.

This makes me think — why not lift this requirement? Users may want to set up the streams in advance, and fund them later.

[PaulRBerg]

I will open a separate discussion, yes. I meant adjustRatePerSecondAndDeposit.

Could you please confirm whether you have accepted the refutation I have provided in the Loom video, or not?

[andreivladbrg]

I meant adjustRatePerSecondAndDeposit.

hmm, in that case why make 2 separate actions instead of one? from my understanding you are suggesting:

1. create with rps = 0
2. wait time
3. adjustRpsAndDeposit with rps > 0

instead of:

1. wait time
2. createAndDeposit with rps > 0

we can further discuss this in the upcoming discussion

---

Could you please confirm whether you have accepted the refutation I have provided in the Loom video, or not?

your point was already confirmed by me in my comment (this is why i haven't said anything). perhaps you missed it?

nervertherless, i will say it again. $isPaused = true \implies rps = 0$ && $isPaused = false \implies rps > 0$ is a correct statement

[PaulRBerg]

In the Loom video, I refuted this part (unrelated to your screenshot):

Could you please confirm whether you have understood my criticism and if yes, accept it or criticize it?

[andreivladbrg]

Could you please confirm whether you have understood my criticism and if yes, accept it or criticize it?

your criticism is "stream does not exist: rps = 0 && isPaused = false is correct but it doesn't matter to isPaused" i accept it, you are correct, it does not matter

[PaulRBerg]

Thank you. Point settled!

I will open a separate discussion for the adjustRatePerSecondAndDeposit idea.