Update to Deploy 11 #502
Update to Deploy 11 #502
Conversation
| // TODO: double check if comparing query outputs against balanceDeltas isn't redundant with query tests | ||
| // if yes, we should be able to remove query tests (and avoid relying on a fixed blockNumber) | ||
| describe.skip('query method should return correct updated', () => { |
There was a problem hiding this comment.
Highlighting this as it might be the one that sparks a bit of discussion.
Main question is:
do we need to test queryOutputs against hardcoded amounts?
I have a feeling that testing queryOutput against balanceDeltas from the actual transaction should be enough.
If we manage to remove these, we won't need to rely on a fixed blockNumber for these tests.
There was a problem hiding this comment.
I agree with Bruno's feeling. Doesn't seem like the hardcoded amounts here add meaningful value
There was a problem hiding this comment.
Keeping in mind that after we launch there will be no new deployments and fixed block number will be permanent so it's probably not going to be that important soon. The main benefit I see is it potentially catches if changes break anything unexpectedly (e.g. results should always be the same). Its maybe not that important, especially if we're definitely checking that the query result is at least > 0 in assertSwapExactIn/Out but I'm not 100% sure that is the case and it would need to be confirmed first.
test/v3/addLiquidityBoosted/addLiquidityPartialBoosted.integration.test.ts
Outdated
Show resolved
Hide resolved
| // FIXME: zeroOutDeltas is swapping a huge amount (~200 WETH) and hitting MaxInRatio | ||
| describe.skip('Nested pool', () => { |
There was a problem hiding this comment.
This one is indeed not working, but I'd suggest we fix this in a separate PR, so we unblock the deploy update. Let me know what you think and I'll create a ticker over here
There was a problem hiding this comment.
I agree with kicking this down the road
There was a problem hiding this comment.
Tracking in this issue: #503 ideally address it next.
There was a problem hiding this comment.
This v3 pool creation script is still kinda messy and would benefit from Permit2helper method, but seems like decent pinch option for deploying v3 pools and could start discussion about examples that are set up to send transactions to live network
No worries if you prefer to dump it for this PR, just thought I'd offer the option for consideration
There was a problem hiding this comment.
Can leave it in for now but should be tidied as part of wider example changes.
Closes #499