Price Impact Helper #323
Price Impact Helper #323
Conversation
# Conflicts: # pkg/interfaces/contracts/vault/IVaultErrors.sol # pkg/interfaces/contracts/vault/IVaultExtension.sol # pkg/solidity-utils/contracts/helpers/RevertCodec.sol # pkg/solidity-utils/test/foundry/RevertCodec.t.sol # pkg/vault/.forge-snapshots/vaultSwapSingleTokenExactInWithProtocolFee.snap # pkg/vault/contracts/VaultExtension.sol # pkg/vault/contracts/test/RouterMock.sol # pkg/vault/test/Queries.test.ts
There was a problem hiding this comment.
Thanks for pushing this @elshan-eth !
First pass done. Only a few minor questions and comments; otherwise LGTM!
|
|
||
| import { RevertCodec } from "@balancer-labs/v3-solidity-utils/contracts/helpers/RevertCodec.sol"; | ||
|
|
||
| contract CallAndRevert { |
There was a problem hiding this comment.
maybe we can make a library out of this? There's no state to save or to access, so a library would seem better suited. Agree about making it a standalone module.
There was a problem hiding this comment.
Ah this is to reuse the hook right? Then I guess it's fine as it is.
| // get deltas between exactAmountsIn and proportionalAmountsOut | ||
| int256[] memory deltas = new int256[](exactAmountsIn.length); | ||
| for (uint256 i = 0; i < exactAmountsIn.length; i++) { | ||
| deltas[i] = int(proportionalAmountsOut[i]) - int(exactAmountsIn[i]); |
There was a problem hiding this comment.
Nit: let's use full types.
| deltas[i] = int(proportionalAmountsOut[i]) - int(exactAmountsIn[i]); | |
| deltas[i] = int256(proportionalAmountsOut[i]) - int256(exactAmountsIn[i]); |
|
|
||
| // calculate price impact ABA with remaining delta and its respective exactAmountIn | ||
| // remaining delta is always negative, so by multiplying by -1 we get a positive number | ||
| uint256 delta = uint(-deltas[remainingDeltaIndex]); |
There was a problem hiding this comment.
I'd still just use safe cast here. Everything is static so we don't care about gas, and we have plenty of bytecode to spare.
| uint256 tokenIndex, | ||
| int256[] memory deltas, | ||
| address sender | ||
| ) internal returns (int256 deltaBPT) { |
There was a problem hiding this comment.
I think I don't get the back and forth here. Why is this a signed value if we always change the result to be positive?
| // calculate expectedPriceImpact for comparison | ||
| uint256 expectedPriceImpact = effectivePrice.divDown(spotPrice) - 1e18; | ||
|
|
||
| // assert within acceptable bounds of +-1% |
There was a problem hiding this comment.
hm isn't 1% quite large for this? Does it break with a lower tolerance?
Description
This PR aims to create a helper contract to calculate price impact for add liquidity unbalanced operations.
Price impact is currently being done off-chain, within the SDK, through the ABA method (described here)
For most operations (e.g. swaps, remove liquidity single token, etc..) this is ok to be done off-chain, because they contain a respective "reverse" operation and ABA method requires only 2 on-chain queries (A -> B and B -> A) to calculate price impact.
Add Liquidity Unbalanced is the only operation that requires an adapted version of ABA because Remove Liquidity Unbalanced is not available in v3.
More details on the adapted ABA logic can be found here
A few items still open for discussion:
standalone-utilsas a new package by copy/paste from another existing package. It might have some dependencies (or other things) that are not needed. Could use a help on identifying what is ok to remove or not.Depends on #383.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution