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.

Sign up for GitHub

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

improve(create): Prevent rounding errors that unintentionally break the GCR restraint on-chain #177

Closed
wants to merge 1 commit into from
Closed

improve(create): Prevent rounding errors that unintentionally break the GCR restraint on-chain #177

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions features/manage-position/Create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eng pls double check my math. If you CEIL the backing collateral, then the GCR check should always succeed right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how many decimal points will this be CEILing to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If we don't care about preserving precision then we can completely avoid the GCR errors that manifest itself as "gas required exceeds allowance"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense! SGTM

Copy link
Contributor

@daywiss daywiss Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example of what these could should be? I don't understand how this can even work without big numbers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};

Expand Down Expand Up @@ -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());
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, if you FLOOR the tokens to create, the GCR will always be below your desired ratio

const setTokensToMax = (
Expand Down