-
Notifications
You must be signed in to change notification settings - Fork 13
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
SOR - Update StableSurge hook support #1582
Changes from all commits
b31755a
38dcff5
c95010b
0ca70e1
730cd63
545bfdd
80c27c2
7d26630
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'backend': patch | ||
--- | ||
|
||
SOR - Update stable surge hook support |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ import { HookState } from '@balancer-labs/balancer-maths'; | |
import { LiquidityManagement } from '../../../types'; | ||
|
||
import { parseEther, parseUnits } from 'viem'; | ||
import { PrismaPoolAndHookWithDynamic } from '../../../../../prisma/prisma-types'; | ||
import { HookData } from '../../../../sources/transformers'; | ||
|
||
export function checkInputs( | ||
tokenIn: Token, | ||
|
@@ -77,41 +79,53 @@ export function getOutputAmount(paths: PathWithAmount[]): TokenAmount { | |
return amounts.reduce((a, b) => a.add(b)); | ||
} | ||
|
||
export function getHookState(pool: any): HookState | undefined { | ||
if (pool.hook === undefined || pool.hook === null) { | ||
export function getHookState(pool: PrismaPoolAndHookWithDynamic): HookState | undefined { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great change. I recall running into build issues when trying it. But it seems like back then I was not aware of the existing HookData type, which would have allowed me to assign the pool the PrismaPoolAndHookWithDynamic type (but still stick to using the name). The recently added |
||
if (!pool.hook) { | ||
return undefined; | ||
} | ||
|
||
if (pool.hook.name === 'ExitFee') { | ||
// api for this hook is an Object with removeLiquidityFeePercentage key & fee as string | ||
const dynamicData = pool.hook.dynamicData as { removeLiquidityFeePercentage: string }; | ||
|
||
return { | ||
tokens: pool.tokens.map((token: { address: string }) => token.address), | ||
// ExitFeeHook will always have dynamicData as part of the API response | ||
removeLiquidityHookFeePercentage: parseEther(dynamicData.removeLiquidityFeePercentage), | ||
hookType: pool.hook.name, | ||
}; | ||
} | ||
|
||
if (pool.hook.name === 'DirectionalFee') { | ||
// this hook does not require a hook state to be passed | ||
return { | ||
hookType: pool.hook.name, | ||
} as HookState; | ||
const hookData = pool.hook as HookData; | ||
|
||
switch (hookData.type) { | ||
case 'EXIT_FEE': { | ||
// api for this hook is an Object with removeLiquidityFeePercentage key & fee as string | ||
const dynamicData = hookData.dynamicData as { removeLiquidityFeePercentage: string }; | ||
|
||
return { | ||
tokens: pool.tokens.map((token: { address: string }) => token.address), | ||
// ExitFeeHook will always have dynamicData as part of the API response | ||
removeLiquidityHookFeePercentage: parseEther(dynamicData.removeLiquidityFeePercentage), | ||
hookType: 'ExitFee', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about hard coding the hookType back then as well. What would be the advantage of hardcoding it here compared to forwarding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just FYI, hook.name is being deprecated in the favor of hookType There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this new hookType that is returned here for the Balancer maths repo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's how Balancer Maths knows which hook is being used. This is the mapper I said I'd add to decouple hook type from API vs hook type from balancer maths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answering @mkflow27 - the advantage of rewriting the hook type here instead of forwarding is that we decouple how they are declared in the api vs in Balancer Maths. If we simply forwarded what was specified in the api, we add the overhead of having to sync and reach consensus on how each hook type is handled in both codebases. |
||
}; | ||
} | ||
case 'DIRECTIONAL_FEE': { | ||
// this hook does not require a hook state to be passed | ||
return { | ||
hookType: 'DirectionalFee', | ||
} as HookState; | ||
} | ||
case 'STABLE_SURGE': { | ||
const typeData = pool.typeData as { amp: string }; | ||
const dynamicData = hookData.dynamicData as { | ||
surgeThresholdPercentage: string; | ||
maxSurgeFeePercentage: string; | ||
}; | ||
return { | ||
// amp onchain precision is 1000. Api returns 200 means onchain value is 200000 | ||
amp: parseUnits(typeData.amp, 3), | ||
// 18 decimal precision. | ||
surgeThresholdPercentage: parseEther(dynamicData.surgeThresholdPercentage), | ||
maxSurgeFeePercentage: parseEther(dynamicData.maxSurgeFeePercentage), | ||
hookType: 'StableSurge', | ||
}; | ||
} | ||
default: | ||
if (hookData.type) { | ||
console.warn(`pool ${pool.id} with hook type ${hookData.type} not implemented`); | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
if (pool.hook.name === 'StableSurge') { | ||
return { | ||
// amp onchain precision is 1000. Api returns 200 means onchain value is 200000 | ||
amp: parseUnits(pool.typeData.amp, 3), | ||
// 18 decimal precision. | ||
surgeThresholdPercentage: parseEther(pool.hook.dynamicData.surgeThresholdPercentage), | ||
hookType: pool.hook.name, | ||
}; | ||
} | ||
|
||
throw new Error(`${pool.hook.name} hook not implemented`); | ||
} | ||
|
||
export function isLiquidityManagement(value: any): value is LiquidityManagement { | ||
|
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.
Moving the typeguard here seems to be more elegant as it just does not consider the pool for route building and simply continues.
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.
Yeah, the main problem was that it was throwing an error if LiquidityManagement wasn't properly set.
This means that the whole SOR will break with a single corrupted pool in the DB.
Moving here allows us to simply skip that pool and later investigate why it doesn't have that attribute properly set