Feat boosted pool add and remove #450
Conversation
format
src/entities/addLiquidityBoosted/doAddLiquidityPropotionalQuery.ts
Outdated
Show resolved
Hide resolved
src/entities/addLiquidityBoosted/doAddLiquidityUnbalancedQuery.ts
Outdated
Show resolved
Hide resolved
| input: AddLiquidityInput, | ||
| poolState: PoolState | PoolStateWithUnderlyings, | ||
| ): Promise<AddLiquidityBaseQueryOutput> { | ||
| // Technically possible to pass singleToken adds here due to type |
There was a problem hiding this comment.
Can change type to be
AddLiquidityUnbalancedInput | AddLiquidityProportionalInput;
There was a problem hiding this comment.
With the refactor to support optional userData and userAddress, i added new types, so this point has also been addressed, if I understood it correctly.
| switch (input.kind) { | ||
| case AddLiquidityKind.Unbalanced: { | ||
| const bptAmountOut = await doAddLiquidityUnbalancedQuery( | ||
| input, |
There was a problem hiding this comment.
addLiquidity uses getAmounts to fill any missing token amounts because the contract needs values for all. E.g. if user tries to add with only USDC to a aUSDC/aDAI pool. Is this required here?
There was a problem hiding this comment.
I have added this and now fill the amounts in default values in case the user did not provide all the tokenAmounts he wants to join with.
| ): Promise<AddLiquidityBaseQueryOutput> { | ||
| // Technically possible to pass singleToken adds here due to type | ||
| // disallow it for this class | ||
| // TODO: Use input validator |
There was a problem hiding this comment.
Couple of validations I can think of:
- Protocol version
- Input amounts are underlying
| input.poolId, | ||
| input.amountsIn.map((amount) => amount.amount), | ||
| amounts.minimumBpt, | ||
| !!input.wethIsEth, |
There was a problem hiding this comment.
Hardcode to false until we can support?
| import { RemoveLiquidityProportionalInput } from 'src'; | ||
|
|
||
| export const doRemoveLiquidityProportionalQuery = async ( | ||
| { rpcUrl, chainId, bptIn }: RemoveLiquidityProportionalInput, |
There was a problem hiding this comment.
similar to adds, can update to accept address and userData
test/v3/addLiquidityNested/addLiquidityNestedV3.integration.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As discussed after your refactor ping me to let me know its good for review. I would recommend checking out the nested PR and maybe follow a similar approach which has two benefits imo: 1. Don't need to update all the existing test helpers which are convulted and hard to follow. 2. Makes the approval flow much easier to follow and confirm.
There was a problem hiding this comment.
I have refactored the tests to not use the helpers and instead do the intermediary steps as part of the tests. I did not change anything in the way how I handle approvals as I think the way they are done in the tests here are fine.
There was a problem hiding this comment.
I utilise a helper here (which I think is ok) as a precondition for remove liq testing. Similarly I think the way I do approvals is ok. Compared to your tests, you do approvals right in the actualy tests, whereas I spread it.
|
|
||
| // Check if userAddress and userData were provided, and assign default values if not | ||
| if (!('userAddress' in input) || input.userAddress === undefined) { | ||
| // TODO: I think using a random address might be better than using the 0 address. |
There was a problem hiding this comment.
@johngrantuk I think defaulting to the 0 address might not be the right choice but rather a random address. What do you think?
There was a problem hiding this comment.
I think zeroAddress is best but maybe you can explain why random is better?
There was a problem hiding this comment.
My reasoning was that it is more likely for a query to return different data than what is expected as it is with a random address. This is probably an exotic scenario, so overall unlikely. So for readability / potential debugging purposes address 0 makes it easiert to work with
| decimals | ||
| } | ||
| } | ||
| dynamicData { |
There was a problem hiding this comment.
I don't think we need this field?
There was a problem hiding this comment.
You are right. I think I can actually remove the changes in the api, since I am not using the api in my tests, as we decided to use mock data. I'll remove it.
| ...data.poolGetPool, | ||
| tokens: data.poolGetPool.poolTokens, | ||
| type: mapPoolType(data.poolGetPool.type), | ||
| //underlyings: data.poolGetPool.poolTokens.map((token) => {token}), |
src/entities/index.ts
Outdated
| @@ -24,3 +24,5 @@ export * from './types'; | |||
| export * from './utils'; | |||
| export * from './swap'; | |||
| export * from './swap/swaps/v2/auraBalSwaps'; | |||
| export * from './addLiquidityBoosted'; | |||
| export * from './removeLiquidityBoostedV3'; | |||
There was a problem hiding this comment.
Noticed one has V3 and one doesn't, lets use a consistent pattern, I'd say without V3?
There was a problem hiding this comment.
Done. Renamed the folders to not have v3 anymore. For the classnames I found it easier to leave them with V3. Can also change. What do you think?
…e-refactor Feat boosted pool add and remove refactor
Created a seperate issue #455 for tracking one outstanding todo.