From 62d162a3bb2f4b9b800bd617ab6d8ee913d447a1 Mon Sep 17 00:00:00 2001 From: Emily Williams Date: Thu, 14 Nov 2024 13:20:38 -0500 Subject: [PATCH] fix(v4-sdk): swap function accepts pools with all hooks except swap hooks (#195) Co-authored-by: Siyu Jiang (See-You John) <91580504+jsy1218@users.noreply.github.com> --- sdks/v4-sdk/src/entities/pool.test.ts | 25 +++++++ sdks/v4-sdk/src/entities/pool.ts | 10 ++- sdks/v4-sdk/src/index.ts | 1 - sdks/v4-sdk/src/{ => utils}/hook.test.ts | 94 ++++++++++++++++++++---- sdks/v4-sdk/src/{ => utils}/hook.ts | 40 +++++++++- sdks/v4-sdk/src/utils/index.ts | 1 + 6 files changed, 149 insertions(+), 22 deletions(-) rename sdks/v4-sdk/src/{ => utils}/hook.test.ts (77%) rename sdks/v4-sdk/src/{ => utils}/hook.ts (69%) diff --git a/sdks/v4-sdk/src/entities/pool.test.ts b/sdks/v4-sdk/src/entities/pool.test.ts index 06d89b3f3..24fc13e23 100644 --- a/sdks/v4-sdk/src/entities/pool.test.ts +++ b/sdks/v4-sdk/src/entities/pool.test.ts @@ -11,6 +11,8 @@ import { ONE_ETHER, TICK_SPACING_TEN, } from '../internalConstants' +import { constructHookAddress } from '../utils/hook.test' +import { HookOptions } from '../utils/hook' describe('Pool', () => { const USDC = new Token(1, '0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48', 6, 'USDC', 'USD Coin') @@ -261,6 +263,7 @@ describe('Pool', () => { describe('swaps', () => { let pool: Pool + let poolWithSwapHook: Pool beforeEach(() => { pool = new Pool( @@ -285,9 +288,26 @@ describe('Pool', () => { }, ] ) + + poolWithSwapHook = new Pool( + USDC, + DAI, + FEE_AMOUNT_LOW, + TICK_SPACING_TEN, + constructHookAddress([HookOptions.BeforeSwap]), + encodeSqrtRatioX96(1, 1), + ONE_ETHER, + 0, + [] + ) }) describe('#getOutputAmount', () => { + it('throws if pool has beforeSwap hooks', async () => { + const inputAmount = CurrencyAmount.fromRawAmount(USDC, 100) + await expect(() => poolWithSwapHook.getOutputAmount(inputAmount)).rejects.toThrow('Unsupported hook') + }) + it('USDC -> DAI', async () => { const inputAmount = CurrencyAmount.fromRawAmount(USDC, 100) const [outputAmount] = await pool.getOutputAmount(inputAmount) @@ -304,6 +324,11 @@ describe('Pool', () => { }) describe('#getInputAmount', () => { + it('throws if pool has beforeSwap hooks', async () => { + const outputAmount = CurrencyAmount.fromRawAmount(DAI, 98) + await expect(() => poolWithSwapHook.getInputAmount(outputAmount)).rejects.toThrow('Unsupported hook') + }) + it('USDC -> DAI', async () => { const outputAmount = CurrencyAmount.fromRawAmount(DAI, 98) const [inputAmount] = await pool.getInputAmount(outputAmount) diff --git a/sdks/v4-sdk/src/entities/pool.ts b/sdks/v4-sdk/src/entities/pool.ts index b705f8c6e..c66454218 100644 --- a/sdks/v4-sdk/src/entities/pool.ts +++ b/sdks/v4-sdk/src/entities/pool.ts @@ -12,6 +12,7 @@ import { } from '@uniswap/v3-sdk' import { defaultAbiCoder, isAddress } from 'ethers/lib/utils' import { sortsBefore } from '../utils/sortsBefore' +import { Hook } from '../utils/hook' import { ADDRESS_ZERO, NEGATIVE_ONE, Q192 } from '../internalConstants' import JSBI from 'jsbi' @@ -299,7 +300,7 @@ export class Pool { amountSpecified: JSBI, sqrtPriceLimitX96?: JSBI ): Promise<{ amountCalculated: JSBI; sqrtRatioX96: JSBI; liquidity: JSBI; tickCurrent: number }> { - if (this.nonImpactfulHook()) { + if (!this.hookImpactsSwap()) { return v3Swap( JSBI.BigInt(this.fee), this.sqrtRatioX96, @@ -316,8 +317,9 @@ export class Pool { } } - private nonImpactfulHook(): boolean { - // TODO: reference chain specific hook addresses or patterns that do not impact swaps - return this.hooks === ADDRESS_ZERO + private hookImpactsSwap(): boolean { + // could use this function to clear certain hooks that may have swap Permissions, but we know they don't interfere + // in the swap outcome + return Hook.hasSwapPermissions(this.hooks) } } diff --git a/sdks/v4-sdk/src/index.ts b/sdks/v4-sdk/src/index.ts index bdbc8e6c5..a1f307e7d 100644 --- a/sdks/v4-sdk/src/index.ts +++ b/sdks/v4-sdk/src/index.ts @@ -1,4 +1,3 @@ export * from './entities' export * from './utils' export * from './PositionManager' -export * from './hook' diff --git a/sdks/v4-sdk/src/hook.test.ts b/sdks/v4-sdk/src/utils/hook.test.ts similarity index 77% rename from sdks/v4-sdk/src/hook.test.ts rename to sdks/v4-sdk/src/utils/hook.test.ts index 3e1bb4927..9bb9ea540 100644 --- a/sdks/v4-sdk/src/hook.test.ts +++ b/sdks/v4-sdk/src/utils/hook.test.ts @@ -1,6 +1,6 @@ import { Hook, HookOptions, hookFlagIndex } from './hook' -function constructAddress(hookOptions: HookOptions[]): string { +export function constructHookAddress(hookOptions: HookOptions[]): string { let hookFlags = 0 for (const hookOption of hookOptions) { hookFlags = hookFlags | (1 << hookFlagIndex[hookOption]) @@ -13,20 +13,20 @@ function constructAddress(hookOptions: HookOptions[]): string { describe('Hook', () => { const allHooksAddress = '0x0000000000000000000000000000000000003fff' const emptyHookAddress = '0x0000000000000000000000000000000000000000' - const hookBeforeInitialize = constructAddress([HookOptions.BeforeInitialize]) - const hookAfterInitialize = constructAddress([HookOptions.AfterInitialize]) - const hookBeforeAddLiquidity = constructAddress([HookOptions.BeforeAddLiquidity]) - const hookAfterAddLiquidity = constructAddress([HookOptions.AfterAddLiquidity]) - const hookBeforeRemoveLiquidity = constructAddress([HookOptions.BeforeRemoveLiquidity]) - const hookAfterRemoveLiquidity = constructAddress([HookOptions.AfterRemoveLiquidity]) - const hookBeforeSwap = constructAddress([HookOptions.BeforeSwap]) - const hookAfterSwap = constructAddress([HookOptions.AfterSwap]) - const hookBeforeDonate = constructAddress([HookOptions.BeforeDonate]) - const hookAfterDonate = constructAddress([HookOptions.AfterDonate]) - const hookBeforeSwapReturnsDelta = constructAddress([HookOptions.BeforeSwapReturnsDelta]) - const hookAfterSwapReturnsDelta = constructAddress([HookOptions.AfterSwapReturnsDelta]) - const hookAfterAddLiquidityReturnsDelta = constructAddress([HookOptions.AfterAddLiquidityReturnsDelta]) - const hookAfterRemoveLiquidityReturnsDelta = constructAddress([HookOptions.AfterRemoveLiquidityReturnsDelta]) + const hookBeforeInitialize = constructHookAddress([HookOptions.BeforeInitialize]) + const hookAfterInitialize = constructHookAddress([HookOptions.AfterInitialize]) + const hookBeforeAddLiquidity = constructHookAddress([HookOptions.BeforeAddLiquidity]) + const hookAfterAddLiquidity = constructHookAddress([HookOptions.AfterAddLiquidity]) + const hookBeforeRemoveLiquidity = constructHookAddress([HookOptions.BeforeRemoveLiquidity]) + const hookAfterRemoveLiquidity = constructHookAddress([HookOptions.AfterRemoveLiquidity]) + const hookBeforeSwap = constructHookAddress([HookOptions.BeforeSwap]) + const hookAfterSwap = constructHookAddress([HookOptions.AfterSwap]) + const hookBeforeDonate = constructHookAddress([HookOptions.BeforeDonate]) + const hookAfterDonate = constructHookAddress([HookOptions.AfterDonate]) + const hookBeforeSwapReturnsDelta = constructHookAddress([HookOptions.BeforeSwapReturnsDelta]) + const hookAfterSwapReturnsDelta = constructHookAddress([HookOptions.AfterSwapReturnsDelta]) + const hookAfterAddLiquidityReturnsDelta = constructHookAddress([HookOptions.AfterAddLiquidityReturnsDelta]) + const hookAfterRemoveLiquidityReturnsDelta = constructHookAddress([HookOptions.AfterRemoveLiquidityReturnsDelta]) describe('permissions', () => { it('throws for an invalid address', () => { @@ -236,4 +236,68 @@ describe('Hook', () => { ).toEqual(false) }) }) + + describe('hasInitializePermissions', () => { + it('returns the correct results for beforeSwap', () => { + expect(Hook.hasInitializePermissions(hookBeforeInitialize)).toEqual(true) + }) + + it('returns the correct results for afterInitialize', () => { + expect(Hook.hasInitializePermissions(hookAfterInitialize)).toEqual(true) + }) + + it('returns false for non-donate hooks', () => { + expect(Hook.hasInitializePermissions(hookAfterSwap)).toEqual(false) + }) + }) + + describe('hasLiquidityPermissions', () => { + it('returns the correct results for beforeAddLiquidity', () => { + expect(Hook.hasLiquidityPermissions(hookBeforeAddLiquidity)).toEqual(true) + }) + + it('returns the correct results for afterAddLiquidity', () => { + expect(Hook.hasLiquidityPermissions(hookAfterAddLiquidity)).toEqual(true) + }) + + it('returns the correct results for beforeRemoveLiquidity', () => { + expect(Hook.hasLiquidityPermissions(hookBeforeRemoveLiquidity)).toEqual(true) + }) + + it('returns the correct results for afterRemoveLiquidity', () => { + expect(Hook.hasLiquidityPermissions(hookAfterRemoveLiquidity)).toEqual(true) + }) + + it('returns false if only delta flag is flagged (an incorrect address)', () => { + expect(Hook.hasLiquidityPermissions(hookAfterRemoveLiquidityReturnsDelta)).toEqual(false) + }) + }) + + describe('hasSwapPermissions', () => { + it('returns the correct results for beforeSwap', () => { + expect(Hook.hasSwapPermissions(hookBeforeSwap)).toEqual(true) + }) + + it('returns the correct results for afterSwap', () => { + expect(Hook.hasSwapPermissions(hookAfterSwap)).toEqual(true) + }) + + it('returns false if only delta flag is flagged (an incorrect address)', () => { + expect(Hook.hasSwapPermissions(hookBeforeSwapReturnsDelta)).toEqual(false) + }) + }) + + describe('hasDonatePermissions', () => { + it('returns the correct results for beforeSwap', () => { + expect(Hook.hasDonatePermissions(hookBeforeDonate)).toEqual(true) + }) + + it('returns the correct results for afterDonate', () => { + expect(Hook.hasDonatePermissions(hookAfterDonate)).toEqual(true) + }) + + it('returns false for non-donate hooks', () => { + expect(Hook.hasDonatePermissions(hookAfterSwap)).toEqual(false) + }) + }) }) diff --git a/sdks/v4-sdk/src/hook.ts b/sdks/v4-sdk/src/utils/hook.ts similarity index 69% rename from sdks/v4-sdk/src/hook.ts rename to sdks/v4-sdk/src/utils/hook.ts index bfc2b21b2..67bf32745 100644 --- a/sdks/v4-sdk/src/hook.ts +++ b/sdks/v4-sdk/src/utils/hook.ts @@ -39,7 +39,7 @@ export const hookFlagIndex = { export class Hook { public static permissions(address: string): HookPermissions { - invariant(isAddress(address), 'invalid address') + this._checkAddress(address) return { beforeInitialize: this._hasPermission(address, HookOptions.BeforeInitialize), afterInitialize: this._hasPermission(address, HookOptions.AfterInitialize), @@ -59,11 +59,47 @@ export class Hook { } public static hasPermission(address: string, hookOption: HookOptions) { - invariant(isAddress(address), 'invalid address') + this._checkAddress(address) return this._hasPermission(address, hookOption) } + public static hasInitializePermissions(address: string) { + this._checkAddress(address) + return ( + this._hasPermission(address, HookOptions.BeforeInitialize) || + Hook._hasPermission(address, HookOptions.AfterInitialize) + ) + } + + public static hasLiquidityPermissions(address: string) { + this._checkAddress(address) + // this implicitly encapsulates liquidity delta permissions + return ( + this._hasPermission(address, HookOptions.BeforeAddLiquidity) || + Hook._hasPermission(address, HookOptions.AfterAddLiquidity) || + Hook._hasPermission(address, HookOptions.BeforeRemoveLiquidity) || + Hook._hasPermission(address, HookOptions.AfterRemoveLiquidity) + ) + } + + public static hasSwapPermissions(address: string) { + this._checkAddress(address) + // this implicitly encapsulates swap delta permissions + return this._hasPermission(address, HookOptions.BeforeSwap) || Hook._hasPermission(address, HookOptions.AfterSwap) + } + + public static hasDonatePermissions(address: string) { + this._checkAddress(address) + return ( + this._hasPermission(address, HookOptions.BeforeDonate) || Hook._hasPermission(address, HookOptions.AfterDonate) + ) + } + private static _hasPermission(address: string, hookOption: HookOptions) { return !!(parseInt(address, 16) & (1 << hookFlagIndex[hookOption])) } + + private static _checkAddress(address: string) { + invariant(isAddress(address), 'invalid address') + } } diff --git a/sdks/v4-sdk/src/utils/index.ts b/sdks/v4-sdk/src/utils/index.ts index 6b64ff8b8..df7431cd6 100644 --- a/sdks/v4-sdk/src/utils/index.ts +++ b/sdks/v4-sdk/src/utils/index.ts @@ -8,3 +8,4 @@ export * from './encodeRouteToPath' export * from './pathCurrency' export * from './priceTickConversions' export * from './sortsBefore' +export * from './hook'