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

OrderBook: Use ratio instead of rate #1546

Merged
merged 1 commit into from
Sep 12, 2023
Merged

OrderBook: Use ratio instead of rate #1546

merged 1 commit into from
Sep 12, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Sep 12, 2023

Description

Ideally, order-book should use an 18-decimal fixed point number too.

Changes and Descriptions

  • Change type in dev runtime.
  • Remove dependency from Rate type in tests/benchs.

@lemunozm lemunozm added Q1-easy Can be done by primarily duplicating and adapting code. I3-annoyance The code behaves as expected, but "expected" is an issue. P7-asap Issue should be addressed in the next days. labels Sep 12, 2023
@lemunozm lemunozm self-assigned this Sep 12, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Why ideally? Changes look fine though

@lemunozm
Copy link
Contributor Author

As I see it, the SellRate is used to denominate prices in units of other prices. So, they are specifying quantities or ratios, not 27-decimal rates (whose purpose is to be used for accruing interest per second). Actually, if some currency is 10^9 more valued than others, with 27-decimal, we can no longer use order-book for those.

Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! LGTM

@wischli
Copy link
Contributor

wischli commented Sep 12, 2023

Actually, if some currency is 10^9 more valued than others, with 27-decimal, we can no longer use order-book for those.

Thanks so much for making this explicit! Given that DAI has 18 decimals and USDT having only 6, this fix is a must-have.

@lemunozm lemunozm enabled auto-merge (squash) September 12, 2023 14:32
@lemunozm
Copy link
Contributor Author

lemunozm commented Sep 12, 2023

DAI has 18 decimals and USDT having only 6

To expand this, here we have two kinds of relations:

  • The amount of DAIs in 1 USDT (how many DAIs compound an USDT, that now we suppose 1:1).
  • The precission difference between both.

Right now, I do not know if SellRate contains the information of one or both of the above relations. If it also tracks the second relation about precision, then it's a must-have, as you've said.

@lemunozm lemunozm merged commit 7934317 into main Sep 12, 2023
10 checks passed
@lemunozm lemunozm deleted the order-book/use-ratio branch September 12, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I3-annoyance The code behaves as expected, but "expected" is an issue. P7-asap Issue should be addressed in the next days. Q1-easy Can be done by primarily duplicating and adapting code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants