improve(create): Prevent rounding errors that unintentionally break the GCR restraint on-chain #177
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/uma/emp-tools/csw7sudjc |
| @@ -96,7 +96,7 @@ const Create = () => { | |||
| // We want to round down the number for better UI display, but we don't actually want the collateral | |||
| // amount to round down since we want the minimum amount of collateral to pass the GCR constraint. So, | |||
| // we'll add a tiny amount of collateral to avoid accidentally rounding too low. | |||
| setCollateral((minBackingCollateral + 0.00005).toFixed(4)); | |||
| setCollateral(Math.ceil(minBackingCollateral).toString()); | |||
There was a problem hiding this comment.
@eng pls double check my math. If you CEIL the backing collateral, then the GCR check should always succeed right?
There was a problem hiding this comment.
how many decimal points will this be CEILing to?
There was a problem hiding this comment.
- If we don't care about preserving precision then we can completely avoid the GCR errors that manifest itself as "gas required exceeds allowance"
There was a problem hiding this comment.
I see, that makes sense! SGTM
There was a problem hiding this comment.
can you give an example of what these could should be? I don't understand how this can even work without big numbers.
There was a problem hiding this comment.
This seems okay to me... but I think ceiling to the nearest 1BTC or 1 WETH kinda defeats the point of these buttons. If we're ceiling to the nearest full unit of collateral, the max and min buttons seem like they add complexity to the dapp without adding a lot of value.
It seems like all of these issues are basically coming out of the fact that we're doing very high precision math with plain JS floating point numbers. Is it difficult to move all of these to BNs? Or is that super annoying?
There was a problem hiding this comment.
Oh shoot I didn't even think about the fact that 1 BTC and 1 WETH are very large numbers. This seems like a good fix for the tokens side of things, but not the collateral
| @@ -141,7 +141,7 @@ const Create = () => { | |||
| const maxTokensToCreate = _gcr > 0 ? collateral / _gcr - startingTokens : 0; | |||
| // Unlike the min collateral, we're ok if we round down the tokens slightly as round down | |||
| // can only increase the position's CR and maintain it above the GCR constraint. | |||
| setTokens((maxTokensToCreate - 0.0001).toFixed(4)); | |||
| setTokens(Math.floor(maxTokensToCreate).toString()); | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Similarly, if you FLOOR the tokens to create, the GCR will always be below your desired ratio
I agree, moving this to draft mode. I'm hoping to fix this without changing everything to BN's but that might be the fix. This will just be painful but better for us long term. Already mentioned this problem in #118 |
|
Still in progress or should we close? |
|
Should we close this? @nicholaspai |
There was a problem hiding this comment.
i dont think this really works, as matt points out, you dont want to round up if you are at 1.00001 btc. same with flooring to show the estimated max tokens. Id be a lot happier if we considering doing this as more generalized math utility which can be unit tested.
Conservatively CEIL and FLOOR the [min amount of backing collateral]/[max amount of tokens] when calling create. This trades off perfect capital efficiency for UX. We want to eliminate this oft-repeated user error where an obscure "gas required exceeeds allowance" error message occurs, which just indicates that the transaction is failing on-chain, because the front-end is incorrectly rounding numbers