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

Draft: highlight where constant product differs from v1 and find out why #3942

Closed
wants to merge 4 commits into from

Conversation

katelynsills
Copy link
Contributor

refs: #3716 (original constant product code with tests)
refs: #3771 (constant product code that got merged)

Description

This draft PR is to set up tests so that we can do a direct comparison of the constant product code and Uniswap v1. We're assuming that if the protocolFee is 0 and the price is not being "improved" or "reduced", the results should be the same. I'm getting different results. The file to look at is packages/zoe/test/unitTests/contracts/constantProduct/test-v1.js. I highlighted a particular test with test.only, but the other tests are failing too.

@katelynsills katelynsills self-assigned this Oct 8, 2021
@katelynsills
Copy link
Contributor Author

@dckc and @Chris-Hibbert, here's the differences I was seeing

@katelynsills katelynsills marked this pull request as draft October 8, 2021 21:13
@Chris-Hibbert
Copy link
Contributor

We're assuming that if the protocolFee is 0 and the price is not being "improved" or "reduced", the results should be the same.

@katelynsills thinks the answers should be the same. I think they might be the same (in at least some cases, like getInputPrice vs. getOutputPrice), and it would be interesting to understand why they're different.

@katelynsills
Copy link
Contributor Author

@katelynsills thinks the answers should be the same. I think they might be the same (in at least some cases, like getInputPrice vs. getOutputPrice), and it would be interesting to understand why they're different.

Good point, I forgot we had talked about that.

@dckc
Copy link
Member

dckc commented Oct 8, 2021

FWIW, I reproduced the results in a spreadsheet

as well as with yarn test.

@dckc
Copy link
Member

dckc commented Oct 8, 2021

oh... I didn't undo the "improved" stuff in my spreadsheet.

@Chris-Hibbert
Copy link
Contributor

This code was all removed long ago.

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.

3 participants