Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that "getWithdrawableAmount" works with non-18-decimal tokens #11

Closed
PaulRBerg opened this issue May 31, 2022 · 15 comments
Closed
Assignees

Comments

@PaulRBerg
Copy link
Member

The issue with the current implementation is that it expects the token amounts to be compatible with the PRBMath UD60x18 number format (which uses 18 decimals):

https://github.com/sablierhq/v2-core/blob/1527035a7dfdafecab971ad7d41889c5868022c6/src/SablierV2Linear.sol#L89

This won't work with non-18-decimal tokens, such as USDC, which uses 6 decimals.

@PaulRBerg PaulRBerg added the bug label May 31, 2022
@andreivladbrg
Copy link
Member

I wrote a test for this. It works fine, see: bf4e8d8 . You can clone & test.

@PaulRBerg
Copy link
Member Author

PaulRBerg commented Jun 7, 2022

@andreivladbrg there may be values for which the current approach still works, but there are certainly many other values for which it breaks.

The solution is to normalize the token amounts to have 18 decimals too. My worry though is that the implementation isn't as easy at it seems - we might have to cache the token decimals so we don't make a query to the ERC-20 token contract on every call to getWithdrawableAmount.

@PaulRBerg PaulRBerg self-assigned this Jun 9, 2022
@PaulRBerg
Copy link
Member Author

PaulRBerg commented Jun 14, 2022

I think I was wrong when I said that there may be "other values for which it breaks".

The reason the math still works out is that the scaling factors cancel out. Let's take USDC as an example. If we multiply depositAmount by 10^12, and then we divide the resulting product by 10^12, we reach the same result as if we had not multiplied and divided by 10^12 in the first place.

That said, there might still be some precision gains if we normalize the amounts to 18 decimals. We have to investigate this - I'll mark this issue as stale until we have clarity.

@PaulRBerg PaulRBerg changed the title Generalize "getWithdrawableAmount" to work with non-18-decimal tokens Check that "getWithdrawableAmount" works with non-18-decimal tokens Jun 21, 2022
@PaulRBerg
Copy link
Member Author

As per the recent work I did in #78, it looks like getWithdrawableAmount works just fine when the token has 6 decimals.

I will keep this issue open until we knock out #22. Fuzzing the number of decimals of the ERC-20 tokens used for streaming would reveal whether the current approach scales.

@razgraf

This comment was marked as off-topic.

@PaulRBerg

This comment was marked as off-topic.

@razgraf

This comment was marked as off-topic.

@PaulRBerg

This comment was marked as off-topic.

@razgraf

This comment was marked as off-topic.

@PaulRBerg

This comment was marked as off-topic.

@razgraf

This comment was marked as off-topic.

@PaulRBerg

This comment was marked as off-topic.

@andreivladbrg

This comment was marked as off-topic.

@PaulRBerg

This comment was marked as off-topic.

@PaulRBerg
Copy link
Member Author

I have closed this since in #220 I have added fuzz tests for the grossDepositAmount param.

I realized that we don't actually need to test tokens with different decimals. We are never actually pulling the token decimals in our code base - what we need to test is simply different deposit amounts, which we are doing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants