-
Notifications
You must be signed in to change notification settings - Fork 212
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
Constant product #3771
Constant product #3771
Conversation
24ed357
to
02b65e3
Compare
02b65e3
to
ff01c5a
Compare
51d0f43
to
57e8f50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a partial review. I'm still working on understanding the overall design, so these files need more review time:
packages/zoe/src/contracts/constantProduct/README.md
packages/zoe/src/contracts/constantProduct/swap.js
packages/zoe/test/unitTests/contracts/constantProduct/test-compareBondingCurves.js
packages/zoe/test/unitTests/contracts/constantProduct/test-compareNewSwapPrice.js
packages/zoe/test/unitTests/contracts/constantProduct/test-swapScenarios.js
packages/zoe/package.json
Outdated
@@ -59,6 +59,7 @@ | |||
"@agoric/install-ses": "^0.5.25", | |||
"ava": "^3.12.1", | |||
"c8": "^7.7.2", | |||
"jsverify": "^0.8.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't getting great results from jsverify, but it was fun to play with. I think we should move this to Agoric Labs instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
the pool and offer to deposit one of the two assets. They will receive an amount | ||
of the complementary asset that will maintain the invariant that the product of | ||
the pool values doesn't change. (Except that rounding is done in favor of the | ||
pool.) The liquidity providers are rewarded by charging a fee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make it clearer that we're talking about a swap, and not providing liquidity by changing the subject of the sentence?
pool.) The liquidity providers are rewarded by charging a fee. | |
pool.) A fee is charged on the swap to reward the liquidity providers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
||
* When the user names an input (or output) price, they shouldn't pay more | ||
(or receive less) than they said. | ||
* The pool fee is charged against the computed side of the price. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what the "computed side" is. Does this mean, the output if the user specified input, and the input if the user specified output? If so, can we say something like the side not specified by the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what it means.
"The side not specified by the user" doesn't seem like a phrase I can use more than once as the subject of a sentence. I'll try defining it explicitly once. If that doesn't make the explanation clear enough, I'd appreciate a suggestion.
| | In (X) | Out (Y) | PoolFee | Protocol Fee | ΔX | ΔY | pool Fee * | | ||
|---------|-----|-----|--------|-----|-----|-----|-----| | ||
| **RUN in** | RUN | BLD | BLD | RUN | sGive - PrFee | sGets | sGet - ΔY | ||
| **RUN out** | BLD | RUN | RUN | BLD | sGive | sGets + PrFee | ΔX - sGive | ||
| **BLD in** | BLD | RUN | BLD | RUN | sGive | sGest + PrFee | ΔY - sGet | ||
| **BLD out** | RUN | BLD | RUN | BLD | sGive - PrFee | sGets | sGive - ΔX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the idea of this table.
However, I think some of the values are wrong, or I don't understand how the table is supposed to work:
- The protocol fee is always charged in RUN, but in this chart, under the Protocol Fee column, there are both "BLD" and "RUN".
- If the pool fee is always charged in whatever denomination isn't specified, then I would expect the first two rows under "Pool Fee" to be "BLD" and the last two rows to be "RUN".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the last column, "pool Fee *", why is "RUN in" sGet - ΔY, but "BLD in" is ΔY - sGet? Shouldn't these both be the same?
Also, what is sGet? Is that like want
, but what the user actually gets? There are some typos so this is sometimes sGets and sGest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protocol fee is always charged in RUN, but in this chart, under the Protocol Fee column, there are both "BLD" and "RUN".
fixed.
If the pool fee is always charged in whatever denomination isn't specified, then I would expect the first two rows under "Pool Fee" to be "BLD" and the last two rows to be "RUN".
you're right. fixed.
In the last column, "pool Fee *", why is "RUN in" sGet - ΔY, but "BLD in" is ΔY - sGet? Shouldn't these both be the same?
As we discussed, this got broken up into multiple tables. The current version has rho + deltaX for the first two rows, and rho + deltaX for the last two.
Also, what is sGet? Is that like want, but what the user actually gets? There are some typos so this is sometimes sGets and sGest.
fixed the typo. Yes, this is what the swapper gets.
specifies that they want to buy BLD and are willing to spend up to 300 RUN, the | ||
fees will be 1 RUN and 1 BLD because the amounts are low for expository | ||
purposes. Since the user specified the input price, we calculate the output | ||
using the constant product formula for ΔY. The protocol fee is always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Wanting to buy BLD and being willing to spend up to 300 RUN" is ambiguous as to whether it's a swap in or swap out, since the user is always willing to spend up to the amount they gave. But, in the next sentence, it's said that the user specified the "input price" meaning a swap in. Can we make this clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What pool fee and protocol fee configs are being used in this example? It seems like the fee being 1 RUN and 1 BLD assumes a configured fee which isn't explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ambiguous as to whether it's a swap in or swap out,
Thanks. I'll fix that.
I'm gonna multiply everything by 1000 so we can get clearer numbers for the fees.
return result; | ||
} | ||
|
||
export const swap = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this function too confusing. Can we add more comments or simplify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more comments. I think your suggestions have reduced some of the noise.
// output of the calculation. When the specified value is in RUN, the protocol | ||
// fee will be deducted from amountGiven before adding to the pool. When BLD | ||
// was specified, we add the protocol fee to amountWanted. When the specified | ||
// value is in RUN, the protocol fee will be deducted from amountGiven before | ||
// adding to the pool or added to amountWanted to calculate amoutOut. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has duplicate explanations of how the protocol fee is charged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
actions of arbitrageurs. At any time, a trader can come to the pool and offer to | ||
deposit one of the two assets. They will receive an amount | ||
of the complementary asset that will maintain the invariant that the product of | ||
the balances doesn't change. (Except that rounding is done in favor of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the balances doesn't change. (Except that rounding is done in favor of the | |
the balances doesn't decrease. (Rounding is done in favor of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
This algorithm uses the x*y=k formula directly, without fees. Briefly, there are | ||
two kinds of assets, whose values are kept roughly in balance through the | ||
actions of arbitrageurs. At any time, a trader can come to the pool and offer to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the way this is written doesn't make it clear a trade is intended. It sounds like adding liquidity at first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to " At any time a trader can trade with the pool by offering to deposit ..."
so it charges $20. This matters whenever the values of the smallest unit of the | ||
currencies are significantly different, which is common in DeFi. (We refer to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand this sentence. Do you mean, when the two kinds of tokens have very different valuations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're saying the same thing. Dollars and Pounds are at the same order of magnitude, but if we were representing them in cents and nano-Pounds, then the smallest units would be very different.
What's a clearer way to say this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and δY. The fees are based on δX, and δY. ρ is the poolFee | ||
(e.g. .003). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found "poolFee (e.g. .003)" somewhat confusing, given that much discussion of fees elsewhere is not ratios but amounts, as in "The Protocol fee is always in RUN." above.
A change here is perhaps not critical; I'm happy to see poolFeeRatio
in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The poolFee is sometimes charged on RUN and sometimes on BLD, so we don't construct the ratio until we know which side is which.
amountGiven, | ||
poolAllocation, | ||
amountWanted, | ||
protocolFeeRatio, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numerator and denominator of a Ratio
must have the same brand, right? When I use "show type definition" here I get to contractSupport/types.js:125
, which doesn't state any such constraint.
oh... ratio.js
clarifies that the numerator and denominator may have different brands. Glad I checked.
|
||
In these tables BLD represents any collateral. The user can specify how much | ||
they want or how much they're willing to pay. We'll call the value they | ||
specified **sGive** or **sGet** and bold it. This table shows which brands the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked why sGet and not sWant and we talked about it a little...
FWIW, I see the code uses amountGiven
and amountWanted
.
packages/zoe/test/unitTests/contracts/constantProduct/test-calcDeltaY.js
Show resolved
Hide resolved
yarn.lock
Outdated
@@ -7993,6 +7993,16 @@ jss@10.6.0, jss@^10.5.1: | |||
is-in-browser "^1.1.3" | |||
tiny-warning "^1.0.2" | |||
|
|||
jsverify@^0.8.4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any package.json
changes; how did we get a change to yarn.lock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added jsverify to do algorithmic testing. It was too expensive for CI, so we've dropped it. Thanks for catching this change I missed.
update to current master type declarations renaming Allow swapOut() to have an empty amountIn representing "no restriction" Drop enforcement in swapIn/swapOut that the quotes are in the forward direction doublePool will need to call them in the reverse direction Improved documentation in the README
Drop jsverify improve README.md add more type declarations extract common code in swapIn & swapOut drop reporting on price improvements reduce duplicated code in swap.js more tests
Don't use q() to publish prices improve internal docs and types in core.js
rename calcSwapInPrices to pricesForStatedInput and calcSwapOutPrices to pricesForStatedOutput.
d3dd05c
to
cf3f73a
Compare
Constant Produce calculations
Takes pool sizes, and requested amount in or amount out, and computes poolFee, protocolFee, amount the user pays and gets, and the changes in the pool balances.
The next steps is to integrate with an autoswap contract.
refs #3340