Rustyrabbit's comment on precision issue

[smol-ninja]

From Rustyrabit:

So I see 3 separate issues:

1. user wants a rate that cannot be achieved by RPS accuracy. Solution store the rate as amount and interval. This requires a lot of change though and probably not worth it.
2. snapshotDebt is stored in decimals of the token which leads to a error/delay at each withdrawal. Solution: decrease the error/delay by storing it in 18 decimals regardless of the token’s decimals
3. deciding the precision of the RPS based on the decimals of the token. Effectively you’re saying that for tokens with 18 decimals it is acceptable that they differ more from what the user wants (the incline that is) than for tokens with low decimals. This is because you’re assuming 1 wei of a high decimal token is worth less than 1 wei of a low decimal token. This assumption may not always be true.

As an example these are the values of 1 wei of each of these tokens vs their decimals:

 $0,000_001                    = value of 1 wei of USDC ($1)        1e-6         6 decimals
 $0,000_6                      = value of 1 wei of WBTC ($60k)        6e-4         8 decimals
 $0,000_000_000_000_002_000    = value of 1 wei of WETH ($2k)        2e-15        18 decimals

As you can see it doesn’t hold up when you compare WBT against USDC. WBTC has more decimals but is worth more per wei.

FYI we also looked at the fact that the descaling is based on the decimals of the token as determined at the creation of the stream. If a token would change it’s decimals after creation of the stream it could cause problems. We didn’t find a way to exploit this though an any non-malicious token that alters their decimals is just a rebasing token and as such not supported.

Submission 1 Submission 2

---

My comments:

Note that they are assuming that $0.01 loss per ~20-30 withdrawals is not something we should dismiss, whereas we considered it to be extremely small variation and thus not realistic. Should we defend using this argument? cc @PaulRBerg @andreivladbrg

Two solutions being proposed:

1. Store amount and the interval in which it is expected to be streamed. The check if the time passed is a multiple of “intervals” and if so,withdraw the full amount leaving no precision loss to the users.
2. Store snapshotDebt as 18 decimal format to minimize the difference between the actual and expected.

I like the first solution as it can mitigate the precision loss entirely. Given that (cc @razgraf) users will be entering his interest in the form of “tokens streamed per interval” instead of exact rps , it makes sense to store it as it is to align with the users expectation. Thought it will be more effort and can affect our timeline for public audit and launch. We can also start a discussion out of it for for Flow’s future release.

Second solution, though, seems affordable given the timeline. But, $0.01 is too small, and I think that may still exist even after storing sd in 18 decimal format, it might end up being offering no solution. Given the fact that BTC is $60k now which can be $600k in a month and then it can end up being $0.01 loss to users even with sd as 18 decimals.

Thoughts?

tagging @sablier-labs/engineers as the decision may affect users' UX.

[razgraf]

Would solution 1 also remove the condition that deposit amount is a multiple of RPS? Or does it just affect withdrawals?

Regarding solution 2, are there any other advantages to storing snapshot in 18 decimals? Meaning consistency with other internal representations etc. aside from this "temporary" fix for high-worth tokens (as you said, after BTC gets a 10x, the problems comes back).

[smol-ninja]

Would solution 1 also remove the condition that deposit amount is a multiple of RPS? Or does it just affect withdrawals?

I didn't get what you meant by "deposit amount as a multiple of RPS"? deposit amount can be anything and doesn't require knowledge of rps.

are there any other advantages to storing snapshot in 18 decimals

• Yes for consistency: Since only rps is stored as 18 decimals, and given that snapshotDebt is indirectly a function of rps (sd += elapsedTime * rps) and is only used for internal accounting, these two can be seen as consistent with each other.
• Currently, when adjustRps, refund (i.e. non-withdraw functions) are called, even thought there are no withdraws to the recipient, we still divide ongoing debt by scaling factor before updating it to snapshot debt. The precision loss introduced by this will be mitigated if we have snapshot debt as 18 decimal format.

PS: I later read @andreivladbrg's answer below which is very well explained.

[PaulRBerg]

Thanks for the summary @smol-ninja.

user wants a rate that cannot be achieved by RPS accuracy. Solution store the rate as amount and interval. This requires a lot of change though and probably not worth it.

I've started a discussion about this a long time ago: #152

Notes:

• This only makes sense for intervals up to ~a day.
• For monthly, this is a non-solution because the assumption is that the user wants to stream the tokens. It's just that it's easier to do the mental accounting when thinking in terms of monthly payment rates.
• A user who wishes actual monthly payment rates doesn't need Flow; they just need to ERC-20 approve an address that debits tokens from the user's wallet.

If a token would change it’s decimals after creation of the stream it could cause problems. We didn’t find a way to exploit this though an any non-malicious token that alters their decimals is just a rebasing token and as such not supported.

Yes.

Note that they are assuming that $0.01 loss per ~20-30 withdrawals is not something we should dismiss, whereas we considered it to be extremely small variation and thus not realistic. Should we defend using this argument?

Yes. We will simply add a warning about this in the docs and in the tooltips of the UI.

This is Flow v1.0.0 — it doesn't have to be perfect. It just has to be launched ASAP so that we ship a new product this year.

[smol-ninja]

I've started a discussion about this a long time ago

All hail to Sir PRB 🙇🙇🙇.

This is Flow v1.0.0 — it doesn't have to be perfect. It just has to be launched ASAP so that we ship a new product this year.

Fully agree.

[andreivladbrg]

Thank you for opening this.

---

re solution 1, pasting my reply to Rusty on discord:

was trying to think of a POC for implementing the amount + interval, but I’m not sure how this would fix the precision issue.

Also, how would this be achievable in an “open-ended” environment (with no specific start or end time)? A solution I have in mind is:

elt = (now - start);
intervals_passed = elt / interval_time;
td = interval_amount * intervals_passed; 

Assuming interval > 1 second, and if the block.timestamp is start + (interval - 1), wouldn't the delay be greater compared to using rps?

At some point, we would still need to perform a division (which is the apple of discord here). If this operation is done outside the contract, it would be better since Solidity is sensitive to this kind of mathematical operation.

(a small point, but, how would the equivalent of adjustRps function would be?)

---

to @smol-ninja

The check if the time passed is a multiple of “intervals” and if so,withdraw the full amount leaving no precision loss to the users.

On the contrary, as mentioned above, I believe this would introduce a greater delay in certain scenarios

Second solution, though, seems affordable given the timeline. But, $0.01 is too small, and I think that may still exist even after storing sd in 18 decimal format, it might end up being offering no solution. Given the fact that BTC is $60k now which can be $600k in a month and then it can end up being $0.01 loss to users even with sd as 18 decimals.

Yes, very well put Shub, I agree with you.

---

to @razgraf

Would solution 1 also remove the condition that deposit amount is a multiple of RPS? Or does it just affect withdrawals?

That condition was only for the withdraw function, but we ended up rejecting that change before the audit. It’s part of a separate branch of the main, as Shub mentioned during the last all-hands. I hope it doesn’t impact app development—I still believe that, through our UI, it would be best practice to pass only amounts that divide RPS in the withdraw function. Tho, in the contract, it wouldn't revert if we don’t do so.

Meaning consistency with other internal representations etc. aside from this "temporary" fix for high-worth tokens (as you said, after BTC gets a 10x, the problems comes back).

In terms of internal representations, this change would complicate things. In the current version (aside from the stored rps variable), there is only one place where we represent an amount variable using 18 decimals, which is here:

flow/src/SablierFlow.sol

Line 474 in fbf6ff5

uint128 scaledOngoingDebt = elapsedTime * ratePerSecond;

If we implement this change, we would need to maintain two versions of the total debt: one in token decimals to check the external amount parameter in withdraw/refund, and another to update the snapshot debt for the internal amount. I won’t list all the places where we would need to account for these two versions, but there are multiple locations throughout the code.

[smol-ninja]

Great feedback from everyone.

1. option (1): We all seem to pass on it and may consider in future when we implement multiple cadence
2. option (2): Shall we go ahead and implement it i.e. storing snapshotDebt in 18 decimal format? or pass on it as well? Pros is better precision, cons is increase in complexity of scaling and descaling as mentioned by @andreivladbrg above.

PS: I am in favour of implementing snapshotDebt as 18 decimal format because it offers slightly better version of the current implementation in terms of precisions and the complexity is only extra lines of code as scaling and descaling are pretty straightforward.

[PaulRBerg]

Let's go with option (2) on the basis that the extra complexity involves only a few extra lines of code. If the overhead is greater than that, let's halt the implementation.

[andreivladbrg]

let's go with the implementation

complexity is only extra lines of code as scaling and descaling are pretty straightforward

depends on wether we use UD60x18 or not, either way, it shouldn't be a huge change

[smol-ninja]

IMO lets not use UD60x18 for snapshotDebt. We do not use UD60x18 for ongoingDebt. If we decide to use UD60x18 for snapshotDebt then we would also have to use it for ongoingDebt. Then also in totalDebt and then when comparing with balance which we have at many places, we would be casting it to uint256. This adds extra complexity which can be avoided with a simple datatype.

Lets make a decision now. What do you think?

[andreivladbrg]

I agree with not using UD60x18.

@PaulRBerg tagging you in case you have any objections

[PaulRBerg]

No point in using UD60x18 for snapshotDebt because it's not a user-provided parameter.

[smol-ninja]

Closing this as we all are in agreement.

• Store snapshot debt as a 18 decimal value #302