-
Notifications
You must be signed in to change notification settings - Fork 26
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
New composite router for boosted pool operations #572
Conversation
test/v3/addLiquidityBoosted/addLiquidityBoosted.integration.test.ts
Outdated
Show resolved
Hide resolved
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.
Just a few loose ends to tidy and a changeset to add.
test/v3/addLiquidityBoosted/addLiquidityBoosted.integration.test.ts
Outdated
Show resolved
Hide resolved
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.
Can you please add an add liquidity proportional test covering a scenario that was not previously supported?
I believe that will expose an issue with existing calculateProportionalAmounts helper not being able to handle these new possibilities for amountsIn combinations.
…posite-liquidity-router
@brunoguerios new tests for query add with single wrap and remove with single unwrap are ready for review. Also, thanks again for the assist! ❤️ |
src/entities/removeLiquidityBoosted/doRemoveLiquidityProportionalQuery.ts
Outdated
Show resolved
Hide resolved
switch (input.kind) { | ||
case AddLiquidityKind.Unbalanced: { | ||
// Use poolState to add token index to amountsIn | ||
const tokensIn = input.amountsIn.map((amountIn) => { |
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 feels like a lot of this tokensIn
logic is repeated within unbalanced and proportional implementations. Do you think it's worth extracting/reusing?
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.
given we're in a bit of a tight schedule, we can leave this refactor to be done in a new (low-priority) PR
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.
yup I agree on both notes 👍
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 like this solution a lot better 😃 👏
Thanks for following up on the requested changes!
There is one detail on the priceImpact side of things that I'd like to double check, but other than that it LGTM! ✅
* } | ||
* } | ||
*/ | ||
export function buildPoolStateTokenMap( |
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.
Nice helper 👏
Closes #555
Summary
Update 1
tokensIn
andtokensOut
that are returned by composite routerUpdate 2
wrapUnderlying
andunwrapWrapped
so user only has to worry about which tokens they want to add / remove