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

fix: add liquidity example #373

Merged
merged 1 commit into from
Aug 9, 2024
Merged

fix: add liquidity example #373

merged 1 commit into from
Aug 9, 2024

Conversation

mkflow27
Copy link
Contributor

@mkflow27 mkflow27 commented Aug 5, 2024

This pr fixes the add-liquidity-example. The following changes were made:

  • replace the api endpoint. Some V3 pools are not part of the production api endpoint. At a further point in time we need to change it back to the production endpoint
  • Reduce add liquidity amount as otherwise the priceImpact calculation (swap queries) will revert with the pool used, due to revert MaxAmountInRatio
  • Add the chain parameter to the api query
  • Added chain name strings the api requires to constants. I initially tried accessing the Chain.name from https://github.com/wevm/viem/blob/main/src/types/chain.ts#L49. However, even with uppercasing them they do not perfectly match with what the api expects.

@johngrantuk
Copy link
Member

@brunoguerios - "Reduce add liquidity amount as otherwise the priceImpact calculation (swap queries) will revert with the pool used, due to revert MaxAmountInRatio" - is that a bug in priceImpact?

export class Pools {
readonly poolStateQuery = `
query GetPool($id: String!) {
poolGetPool(id:$id) {
query poolGetPool($id: String!, $chain: GqlChain!) {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realise the API had changed to require the chain field. Seems nicely handled 👍

Copy link
Member

@johngrantuk johngrantuk left a comment

Choose a reason for hiding this comment

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

LGTM and runs locally.

@mkflow27
Copy link
Contributor Author

mkflow27 commented Aug 5, 2024

I tried to simulate the query on tenderly and it reverted. https://dashboard.tenderly.co/mcquardt/project/simulator/a4c4577e-ab88-44c9-88e9-244a97e9450f?trace=0.1.0.2.2.0.7.3 numbers I used were from a debugging run

@brunoguerios
Copy link
Member

brunoguerios commented Aug 5, 2024

@brunoguerios - "Reduce add liquidity amount as otherwise the priceImpact calculation (swap queries) will revert with the pool used, due to revert MaxAmountInRatio" - is that a bug in priceImpact?

Price Impact calculation for Add Liquidity Unbalanced relies on swap queries under the hood and if they are too large compared to pool balances, they will revert due to MaxAmountInRatio.

@johngrantuk
Copy link
Member

@brunoguerios - any further comments before this gets merged?

Copy link
Member

@brunoguerios brunoguerios left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@mkflow27 mkflow27 merged commit ad33bb5 into main Aug 9, 2024
4 checks passed
@johngrantuk johngrantuk deleted the fix/add-liquidity-example branch August 14, 2024 08:41
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