Add proportional helper to calculate BPT from a reference amount for boosted pools #486
Conversation
test/v3/addLiquidityBoosted/addLiquidityPartialBoosted.integration.test.ts
Outdated
Show resolved
Hide resolved
…ount-boosted # Conflicts: # src/entities/utils/getPoolStateWithBalancesV3.ts
There was a problem hiding this comment.
This is an attempt to fix an unrelated test that was intermittently failing.
There was a problem hiding this comment.
Nice creating the test client just once in the before all hook is good improvment 🏆
There was a problem hiding this comment.
Thanks for trying to address this! Whats your thoughts on the reason it was intermittently failing?
There was a problem hiding this comment.
My assumption was that the failure came from parallel threads not being handled properly. Since tests were reusing the same fork, they could be affecting each other's results.
The idea of forcing a snapshot and keeping each test on its own fork sounded like something that would prevent this from happening.
But then again, I'm not completely sure this is the case and that's why I mentioned it was an "attempt" 😅
We can dig deeper in case the issue starts happening again 🤷♂️
| ({ rpcUrl } = await startFork( | ||
| ANVIL_NETWORKS.SEPOLIA, | ||
| undefined, | ||
| 7010800n, |
There was a problem hiding this comment.
Is the block number fixed here to protect against external changes to pool liquidity that could cause test to fail?
Looks like addLiquidityBoosted.integration.test.tsdoes not specify a block number when starting the fork.
Is there some reason that is okay there but not here?
Or is it better practice to always fix the blockNumber the fork starts? Or is that not worth the trade off of having to update block numbers each time a new version of v3 is deployed?
Would it make sense to centralize the block all v3 tests run at using a constants file?
There was a problem hiding this comment.
Also interested about this. Seems to run ok without fixing block number (which I would expect to always be the case) and I think ideally we avoid explicitly setting it if possible?
There was a problem hiding this comment.
Yeah, I had to fix the blockNumber due to how tests were setup (see line 220).
On this addLiquidityPartialBoosted we're testing the queryOutput separated from the actual transaction, so we can't compare to token balance delta as we do on other files.
For add liquidity unbalanced that's ok, because we can compare queryOutputs with amountsIn provided as input, but for add liquidity proportional, we don't have that.
So, if we want to check queryOutput against all amountsIn, we have to provide a known/hardcoded value, which requires blockNumber to be fixed.
With that being said, I usually prefer to avoid fixing blockNumbers as well.
We could refactor addLiquidityPartialBoosted to follow the same pattern as addLiquidityBoosted and this would be solved.
What do you think?
There was a problem hiding this comment.
Soooo clean and ez to read
Closes #455