What if we remove the 18 decimal format requirement? #150
Closed
smol-ninja
started this conversation in
Ideas
Replies: 1 comment 3 replies
-
Before giving an answer, I want to mention that the reason for introducing the 18-decimal format for internal amounts is detailed in the README. |
Beta Was this translation helpful? Give feedback.
3 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Context
Andrei published this excellent bug report today, noting that one of the invariants fails if the decimal of ERC20 tokens is less than 18. This is due to the precision error in division. So, this raised a question for me:
"What if we remove the 18 decimal format requirement?"
Now
Let's say a user wants to deposit 100 USDC.
deposit(streamId, amount)
function assumes the input amount to have an 18 decimal format. Since USDC has 6 decimals, the user would have to passERC20.decimal()
. But it gets stored asThen
If we remove the 18 decimal requirement and treat 1 token as 1 token, then there is no need to convert deposit amounts into 18 decimal format. Lets take the same example again.
The user wants to deposit 100 USDC.
deposit(streamId, amount)
function takesratePerSecond = 1000
, that means 1000 tokens are streamed every second.Tests and Risk
I tested removing all normalizations, and storing the token amounts as they are without converting them to 18 decimal format. All values were treated as absolute, ignoring decimals.
The result is that all tests passed, including the previously failing invariant test for decimal values less than 6. This approach doesn't raise precision errors due to rounding and treats all amounts in their absolute form without us making any conversions between different decimals and 18.
All streams belonging to one asset are mutually exclusive from the streams belonging to another asset. So it does not have any cross-over effects.
Let me know if I am missing something. But this approach looks better to me than having an 18-decimal format.
So, I would love to know your thoughts on this.
PS: This does not require a big change in the codebase. I have already done it locally for testing and it works.
cc @sablier-labs/engineers.
Beta Was this translation helpful? Give feedback.
All reactions