Update integration tests to handle hook data #156
Conversation
| rate_rounded_up = _compute_rate_round_up(token_rates[index_out]) | ||
| amount_given_scaled_18 = _to_scaled_18_apply_rate_round_up( | ||
| amount_given_raw, | ||
| scaling_factors[index_out], | ||
| token_rates[index_out], | ||
| rate_rounded_up, |
There was a problem hiding this comment.
New tests highlighted some math issues that I decided to fix within this PR - this is one example.
| ), | ||
| pool_state=cast(PoolState, map_pool_state(pool_with_ints)), | ||
| pool_state=cast(PoolState, pool_state), | ||
| hook_state=hook_state, |
There was a problem hiding this comment.
Hook state was not being taken into account - this is now fixed on all implementations (python, rust and TS)
testData/src/hooks/config.ts
Outdated
| export const HOOK_CONFIG: Record<number, Record<string, HookType>> = { | ||
| // Sepolia | ||
| 11155111: { | ||
| '0xbb1761af481364a6bd7fdbdb8cfa23abd85f0263': 'FEE_TAKING', | ||
| '0xea672a54f0aa38fc5f0a1a481467bebfe3c71046': 'EXIT_FEE', | ||
| '0x1adc55adb4caae71abb4c33f606493f4114d2091': 'STABLE_SURGE', | ||
| '0xc0cbcdd6b823a4f22aa6bbdde44c17e754266aef': 'STABLE_SURGE', | ||
| '0x30ce53fa38a1399f0ca158b5c38362c80e423ba9': 'STABLE_SURGE', | ||
| '0x18b10fe9ec4815c31c4ab04fa6f91dce0695132f': 'MEV_TAX', | ||
| '0xec9578e79d412537095501584284b092d2f6b9f7': 'MEV_TAX', | ||
| }, | ||
| // Base | ||
| 8453: { | ||
| '0xb2007b8b7e0260042517f635cfd8e6dd2dd7f007': 'STABLE_SURGE', | ||
| '0xdb8d758bcb971e482b2c45f7f8a7740283a1bd3a': 'STABLE_SURGE', | ||
| '0x7a2535f5fb47b8e44c02ef5d9990588313fe8f05': 'MEV_TAX', | ||
| }, | ||
| // Mainnet | ||
| 1: { | ||
| '0xb18fa0cb5de8cecb8899aae6e38b1b7ed77885da': 'STABLE_SURGE', | ||
| '0xbdbadc891bb95dee80ebc491699228ef0f7d6ff1': 'STABLE_SURGE', | ||
| '0x1bca39b01f451b0a05d7030e6e6981a73b716b1c': 'MEV_TAX', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Addresses like these are usually stored within the SDK, but not sure if we should move it there. I'm keeping it here for simplicity for now.
Currently these addresses are duplicated here and in the backend repo.
If we decide that at some point it's interesting to have a single source of truth for them, we can move them around.
There was a problem hiding this comment.
I think its fine to keep here for now.
| "balancesLiveScaled18": [ | ||
| "311845355307990821859", | ||
| "409096377821670037730" |
There was a problem hiding this comment.
There is one integration test failing on all implementations where the root cause is that getCurrentLiveBalances applies a round down to calculate live amounts, while add liquidity flow applies a round up.
This leads to a off-by-1 error that is not related to maths, but rather a diff between the getter and the implementation for add liquidity.
We could fetch raw balances and attempt to calculate live balances, but that isn't straightforward and it would have a rather significant impact in complexity, required inputs (e.g. yieldFee), etc..
Sharing it here so we can discuss on how to move forward.
I'd say that for most usage, this off-by-1 error in the comparison between on-chain and off-chain is negligible, but it's open for discussion.
There was a problem hiding this comment.
Juani said this is not an issue for his use case, so I'd say we should accept the off-by-1 error and leave as is. I'll relax the test assertion
There was a problem hiding this comment.
Cool. Can you make sure to comment with that info in the test please.
There was a problem hiding this comment.
Seems like this is only used as part of testing? If that's the case would probably be better placed in the test/utils? (Same for other languages too I think).
There was a problem hiding this comment.
Is rust missing the hook mapping?
There was a problem hiding this comment.
I'll extract to a helper file. It's currently implemented within read_test_data.
testData/src/hooks/config.ts
Outdated
| import type { Address } from 'viem'; | ||
| import type { HookType } from './types'; | ||
|
|
||
| export const HOOK_CONFIG: Record<number, Record<string, HookType>> = { |
There was a problem hiding this comment.
What about changing it to Record<Address, HookType> to avoid mistakes?
testData/src/hooks/types.ts
Outdated
| @@ -0,0 +1,19 @@ | |||
| import type { Address } from 'viem'; | |||
|
|
|||
| export type HookType = 'FEE_TAKING' | 'EXIT_FEE' | 'STABLE_SURGE' | 'MEV_TAX' | 'UNKNOWN'; | |||
There was a problem hiding this comment.
Can we move this to hooks/config.ts? Thinking about the process of adding new hooks it would make it easier to remember what to add where.
Closes #134