SOR - Update StableSurge hook support #1582
Conversation
brunoguerios
requested review from
johngrantuk,
mkflow27,
gmbronco and
franzns
🦋 Changeset detectedLatest commit: 7d26630 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -38,7 +38,7 @@ export const syncHookData = async ( | |||
| // Get hooks data | |||
| const data = await fetchHookData(viemClient, hookData.address, hookType, pool.address); | |||
|
|
|||
| const name = `${hookType.charAt(0).toUpperCase()}${hookType.slice(1)}`; | |||
| const name = `${hookType.charAt(0).toUpperCase()}${hookType.slice(1, -4)}`; | |||
There was a problem hiding this comment.
Concrete example:
- Hook name on API -> StableSurgeHook
- Hook name as balancer maths expect -> StableSurge
As mentioned on slack - I'd like feedback on this change.
Is it ok to update hook names to remove Hook suffix on DB?
Or should I move that workaround to inside SOR logic?
| @@ -107,6 +107,7 @@ export function getHookState(pool: any): HookState | undefined { | |||
| amp: parseUnits(pool.typeData.amp, 3), | |||
| // 18 decimal precision. | |||
| surgeThresholdPercentage: parseEther(pool.hook.dynamicData.surgeThresholdPercentage), | |||
| maxSurgeFeePercentage: parseEther(pool.hook.dynamicData.maxSurgeFeePercentage), | |||
There was a problem hiding this comment.
I see this was added at a later stage and was not initially part of the contracts. Was added in maths here: balancer/balancer-maths@eeff3ef.
| @@ -32,6 +32,13 @@ export async function sorGetPathsWithPools( | |||
| const basePools: BasePool[] = []; | |||
|
|
|||
| for (const prismaPool of prismaPools) { | |||
| // typeguard | |||
| if (prismaPool.protocolVersion === 3) { | |||
| if (!isLiquidityManagement(prismaPool.liquidityManagement)) { | |||
There was a problem hiding this comment.
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.
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
modules/sor/sor-debug.test.ts
Outdated
| useProtocolVersion, | ||
| // poolIds: ['0x10a04efba5b880e169920fd4348527c64fb29d4d'], // boosted | ||
| poolIds: ['0x9b677c72a1160e1e03fe542bfd2b0f373fa94a8c'], // boosted |
There was a problem hiding this comment.
Using a pool with StableSurge.
There was a problem hiding this comment.
I usually add this when I want to make an isolated test.
Removing this will open the SOR to build paths with all Sepolia pools and since some were not properly balanced, results were a bit off.
…e-surge # Conflicts: # modules/actions/pool/v3/sync-hook-data.ts
|
I pushed an update that handles the recently added |
| @@ -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.
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 GqlHookType by franz is a great addition.
| 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.
I thought about hard coding the hookType back then as well. What would be the advantage of hardcoding it here compared to forwarding the hookData.name?
There was a problem hiding this comment.
Just FYI, hook.name is being deprecated in the favor of hookType
There was a problem hiding this comment.
do we need this new hookType that is returned here for the Balancer maths repo?
There was a problem hiding this comment.
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.
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.
Since we always need to add code to support new hook types on the SOR, adding another attribute is trivial.
Close #1478