snapshotTime is dubious

[smol-ninja]

Context

One would assume that snapshotTime represents the time when snapshotDebt is updated, which is not quite true. For example,

• pause and void update the snapshotDebt but not the snapshotTime.

This decision was made to make _ongoingDebtOf gas efficient for paused streams. But the same can also be achieved by checking for rps value.

Problem

Even though we describe snapshotTime as "The Unix timestamp used for the ongoing debt calculation" without mentioning anything about snapshot debt, the name itself can be confusing in practice, for auditors, for code readers as well as for the users.

Recommendation

I propose to redefine the snapshotTime as the "Unix timestamp when snapshot debt is updated by adding up the ongoing debt" and then:

• Update snapshot time every time snapshot debt is updated by adding up the ongoing debt, that means include it in pause and void
• Remove isPaused check from ongoingDebt and add rps check.
• The gas efficient changes introduced in perf: don't update st if withdraw amount is less than sd #266 won't be affected.

Anybody can then empirically check that the following hold true regardless of the stream status:

$$\text{total debt} = \text{snapshot debt} + rps \times (\text{block.timestamp} - \text{snapshot time})$$

Effects on the audit timeline

Given that it's a minor change, it won't delay the audit timeline for us.

Thoughts, cc @sablier-labs/engineers.

[andreivladbrg]

• pause and void update the snapshotDebt but not the snapshotTime.
• restart updates the snapshotTime but not the snapshotDebt

also create function updates snapshotTime to block.timestamp

• Update snapshot time every time snapshot debt is updated by adding up the ongoing debt, that means include it in pause and void and remove it from restart

i am strongly against the idea of changing core logic just for the reason: "the name itself can be confusing in practice"

we could technically update st every time sd is updated, but not the other way around (due to create and restart). the reason we don't do this is for gas efficiency (e.g. one less state update in pause).

---

more context: previously, this time variable was called lastTimeUpdated, but it was renamed by @PaulRBerg in his terminology feedback PR. if you find snapshotTime too problematic, we can simply rename it back or come up with a better name.

[smol-ninja]

also create function updates snapshotTime to block.timestamp

True.

we could technically update st every time sd is updated, but not the other way around (due to create and restart)

To justify, I can say that even in restart and create, sd and st are updated together, its just that sd is 0 at those moments.

the reason we don't do this is for gas efficiency

I agree. For the records, it would be 2500 additional gas for pause and void. pause currently cost 9371 gas cost (+21,000) so I don't think it will make it "expensive", its also the cheapest function Flow has. At the same time, it does not affect any other functions.

just for the reason: "the name itself can be confusing in practice"

Its not just that reason but also the fact that the following empirical relation will hold true all the times, which is not the case right now: $\text{total debt} = \text{snapshot debt} + rps \times (\text{block.timestamp} - \text{snapshot time})$

if you find snapshotTime too problematic

I do not find it too problematic, but its ambiguous and worth discussing for. As you have mentioned that you like code to be self explanatory, similarly, I like the variables names to be self explanatory. snapshotTime is a very good name compared to lastTimeUpdated.

[smol-ninja]

Given how tight the timeline is, I'd appreciate if we can take decision on this before the weekend.

[PaulRBerg]

Shub and I hopped on a call to discuss this proposal, and I think it makes sense. Let's implement it.

i am strongly against the idea of changing core logic just for the reason: "the name itself can be confusing in practice"

Naming is extremely important. By your logic @andreivladbrg, why not rename snapshotTime to mozzarellaTime? Or galaxySplurgeMagicTime?

• There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors
• Smart contract auditors should include recommendations for improved function and variable names in their reports

[andreivladbrg]

Unfortunately, I don’t think you got my point.

I didn’t say that naming is not important. What I said was: let’s not modify contract logic just to make a variable name better suited to its doing. Instead, it’s better to find a more appropriate name. Changing core logic can cause other things to break, whereas renaming a variable doesn’t affect components inside the contract. However, it can impact how easily external readers understand the code. These two things are of different magnitudes. I hope I’ve made myself clear.

[PaulRBerg]

let’s not modify contract logic just to make a variable name better suited to its doing.

• You did not say this. You said the name itself can be confusing in practice. That is a wholly different assertion, which justifies my retort with mozzarellaTime.
• Even if this is what you said, we should absolutely strive to find better names for variables. There is no such thing as self-explanatory code.

Changing core logic can cause other things to break, whereas renaming a variable doesn’t affect components inside the contract

There's no limit to our fallibility. This is what tests are for.

We should not stop improving variable names when there's no backwards-compatibility issues.

I hope I’ve made myself clear.

No, unfortunately.

[andreivladbrg]

• You did not say this. You said the name itself can be confusing in practice. That is a wholly different assertion, which justifies my retort with mozzarellaTime

literally what i said in my comment:

There is no such thing as self-explanatory code.

i am sorry, i have not said/mention this in my comment. you are bringing things to this discussion which i may said in the past and i may no longer agree with.

[andreivladbrg]

Update snapshot time every time snapshot debt is updated by adding up the ongoing debt, that means include it in pause and void
Remove isPaused check from ongoingDebt and add rps check.

After re-reading this discussion, I agree with the first point.

However, I don’t agree that the reasoning should be "the name itself can be confusing in practice" but rather the "robust pattern" argument presented in #271

[smol-ninja]

Agree that the reason: "the name itself can be confusing in practice" is not convincing enough and not a good reason to recommend changes. Lesson learned.

[smol-ninja]

Are you agreeing that this is a better approach in favor of having a robust pattern? The reason that "the name itself can be confusing in practice" isn't good enough, as I have now realized. I should have tried to reason with good arguments instead of that.

[andreivladbrg]

looks like we’re on the same page now 🤝

[smol-ninja]

Issue

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

[PaulRBerg]

I'm glad that this was solved. Some comments:

• There have been misunderstandings on all sides. I'm sorry for misunderstanding you @andreivladbrg.
• The crux of the disagreement between Andrei and I:
  • I think that the name itself can be confusing in practice is a good enough reason to make naming changes
  • Andrei doesn't
• My misunderstanding was to think that @andreivladbrg said that, but it was Shub who said it, and Andrei only was against that statement.
• However, that there has been this misconception doesn't do away with the general disagreement. I am still in favor of Shub's original statement, even he ultimately conceded it to Andrei.

you are bringing things to this discussion which i may said in the past and i may no longer agree with.

I'm glad to hear that you no longer think that code is self-explanatory. However, let me clarify that I did intend to bring something you said elsewhere in this discussion. I said that because I have considered it to be related to the discussion herein.