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

Liquidity oracle #98

Merged
merged 19 commits into from
Feb 3, 2023
Merged

Liquidity oracle #98

merged 19 commits into from
Feb 3, 2023

Conversation

anihamde
Copy link
Contributor

No description provided.

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

I think you're on the right track here. I think this implementation is going to work in terms of the error of the result.

Couple thoughts on testing:

  1. If you break things down into smaller functions, you can test the assumptions on those functions exhaustively to ensure that they satisfy the properties. For example, you can make a function like Price::fraction(x, y) which returns x/y in expo=-9. That function is much easier to test at all of the end ranges and tricky cases. Then you can use this function in affine_combination and depend on its properties.
  2. It's probably easier to test affine_combination than the wrapper functions (because the argument signature is simpler). I think here you need to ask what assumptions does this function make about the input Prices and, if those assumptions are satisfied, what guarantees does it make about the result? Can you add tests that validate that those properties are satisfied? For example, I think the current logic is not going to work well if the prices are not representable in expo=-18.

I left a couple of other minor comments throughout.

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

i think you're getting very close here

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

awesome!

@anihamde anihamde merged commit d06d9cf into main Feb 3, 2023
@anihamde anihamde deleted the liquidity_oracle branch February 3, 2023 18:39
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

Successfully merging this pull request may close these issues.

2 participants