add megavault withdrawal fee estimation #2242
Conversation
WalkthroughThe changes introduce new functionalities in the vault management system, including mathematical calculations for skew and spread, improved error handling, and enhanced withdrawal slippage assessments. New tests ensure the correctness of these calculations, while additional error definitions bolster validation logic. Changes
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
protocol/x/vault/keeper/orders.go (1)
Line range hint
159-338: Refactoring improves code quality, consider further decomposition.The refactoring efforts in
GetVaultClobOrdershave significantly improved the code's readability, maintainability, and efficiency. The changes, such as consolidating retrieval operations, improving error handling, and refactoring calculations into separate methods, align with best practices like DRY and SRP.However, the function still remains quite large and complex. Consider further decomposing it into smaller, more focused functions to enhance readability and maintainability. For example, the order construction logic could be extracted into a separate method.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- protocol/lib/vault/utils.go (1 hunks)
- protocol/lib/vault/utils_test.go (1 hunks)
- protocol/x/vault/keeper/orders.go (5 hunks)
- protocol/x/vault/keeper/vault.go (3 hunks)
- protocol/x/vault/keeper/withdraw.go (1 hunks)
- protocol/x/vault/keeper/withdraw_test.go (1 hunks)
- protocol/x/vault/types/errors.go (2 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Additional comments not posted (9)
protocol/lib/vault/utils.go (2)
14-38: LGTM!The
SkewAntiderivativePpmfunction correctly implements the given formula for calculating the skew antiderivative using big integers. The code is clear, readable, and handles large numbers appropriately. The function signature and naming are also suitable for the purpose.The code changes are approved.
43-51: LGTM!The
SpreadPpmfunction correctly implements the given formula for calculating the spread using thelib.Maxfunction and the appropriate fields from theQuotingParamsandMarketParamstructs. The function signature and naming are also suitable for the purpose.The code changes are approved.
protocol/x/vault/types/expected_keepers.go (1)
49-52: LGTM!The code changes are approved for the following reasons:
- The new
GetLiquidityTiermethod follows the standard Go conventions for defining an interface method.- The method expands the functionality of the
PerpetualsKeeperinterface to retrieve liquidity tier information, which aligns with the PR objective of implementing a withdrawal fee estimation formula.- The AI-generated summary provides additional context, indicating that the liquidity tier information may be essential for managing liquidity in the context of perpetual contracts.
protocol/x/vault/types/errors.go (1)
123-127: LGTM!The new error variable
ErrInvalidWithdrawalPortionis correctly defined with a unique error code and a descriptive message. It enhances the validation logic for withdrawal operations by clearly specifying the valid range for the withdrawal portion.The code changes are approved.
protocol/lib/vault/utils_test.go (2)
14-69: LGTM!The test function
TestSkewAntiderivativePpmis well-structured and covers a good range of test cases. The test cases include various scenarios with different skew factors and leverages, including edge cases. The expected values are calculated correctly based on the formula mentioned in the comments. The test function uses therequire.Equalassertion to compare the actual and expected values, which is a good practice.The code changes are approved.
71-118: LGTM!The test function
TestSpreadPpmis well-structured and covers the important scenarios. The test cases cover different scenarios based on the relationship betweenSpreadMinPpm,SpreadBufferPpm, andMinPriceChangePpm. The expected values are calculated correctly based on the logic mentioned in the test case names. The test function uses therequire.Equalassertion to compare the actual and expected values, which is a good practice.The code changes are approved.
protocol/x/vault/keeper/vault.go (2)
65-97: LGTM!The
GetVaultLeverageAndEquityfunction is well-implemented and follows good coding practices. It correctly calculates the leverage of a vault based on its open notional and equity, handling the case when the equity is non-positive. The function is well-documented and uses thelibpackage for base-to-quote conversion and PPM calculation, promoting code reusability and maintainability.
137-178: LGTM!The
GetVaultClobPerpAndMarketfunction is well-structured and follows good error handling practices. It retrieves the necessary data for a vault in a step-by-step manner, properly handling errors at each step and providing appropriate error messages and context. The function uses theerrorsmodpackage for consistent error management and is well-documented with clear input parameters and return values.protocol/x/vault/keeper/withdraw_test.go (1)
21-394: LGTM!The test function is well-structured and covers a good range of test cases. The test cases are well-documented and easy to understand. The test cases cover both success and error scenarios. The test cases use a table-driven approach, which is a good practice for writing tests. The test cases use a test app builder to set up the test environment, which is a good practice for writing tests. The test cases use the
requirepackage for assertions, which is a good practice for writing tests.
tqin7
force-pushed
the
tq/tra-592
branch
2 times, most recently
from
938f901 to
32e2eff
Compare
| expectedSlippagePpm *big.Int | ||
| expectedErr string | ||
| }{ | ||
| "Success: leverage 0, skew 2, spread 0.003, withdraw 10%": { |
There was a problem hiding this comment.
nit: Add test where leverage ppm gets rounded down to 0?
There was a problem hiding this comment.
wouldn't that be testing leverage calculation instead of withdrawal?
There was a problem hiding this comment.
Yes, it would be. Is there a test for the leverage calculation where the fact that it's in ppm leads to the some small leverage being rounded down to 0?
There was a problem hiding this comment.
added a test case where leverage * imf gets below 6 decimal places and thus gets rounded up to 1 ppm
There was a problem hiding this comment.
No test where leverage ppm is 0 despite position size being non-negative?
There was a problem hiding this comment.
changed leverage to rational number
protocol/lib/vault/utils.go
Outdated
| b.Mul(b, bigSkewFactorPpm) | ||
| b = lib.BigDivCeil(b, big.NewInt(3)) | ||
|
|
||
| // normalize a and b. |
There was a problem hiding this comment.
nit: Maybe a bit more of a comment on the normalization (ppm * ppm * ppm / 1,000,000,000,000) = (ppm)?
| // = 0.0765 | ||
| // slippage = min(0.0765, leverage * imf) | ||
| // = min(0.0765, 1.5 * 0.2) = 0.0765 | ||
| expectedSlippagePpm: big.NewInt(76_500), |
There was a problem hiding this comment.
Not sure if this is covered by the min withdrawal amount convo, but I get a different result (7.2%) for this situation:
from decimal import Decimal
def skew_antiderivative(skew_factor, leverage):
return skew_factor**2 / 3 * leverage**3 + skew_factor * leverage**2
leverage = Decimal('1.5')
skew = Decimal('2')
withdrawer_share = Decimal('0.000001')
half_spread_bps = Decimal('30')
posterior_leverage = (leverage / (1 - withdrawer_share))
integral_a = skew_antiderivative(skew, posterior_leverage)
integral_b = skew_antiderivative(skew, leverage)
integral = integral_a - integral_b
average_skew = integral / (posterior_leverage - leverage)
fee_bps = half_spread_bps * (average_skew + 1) * leverage
{
"posterior_leverage": posterior_leverage,
"integral_a": integral_a,
"integral_b": integral_b,
"integral": integral,
"average_skew": average_skew,
"fee_bps": fee_bps
}{'posterior_leverage': Decimal('1.500001500001500001500001500'),
'integral_a': Decimal('9.000022500040500063000089999'),
'integral_b': Decimal('8.999999999999999999999999999'),
'integral': Decimal('0.000022500040500063000090000'),
'average_skew': Decimal('15.00001200001500001799995500'), # <- this is 15 not 16
'fee_bps': Decimal('720.0005400006750008099979750')}There was a problem hiding this comment.
i this case, i think it's because posterior_leverage gets rounded up from 1.5000015 to 1.500002 as we operate with 6 decimal places, resulting in fee being 0.765 instead of 0.750
protocol/x/vault/keeper/withdraw.go
Outdated
| func (k Keeper) GetVaultWithdrawalSlippagePpm( | ||
| ctx sdk.Context, | ||
| vaultId types.VaultId, | ||
| sharesToWithdraw *big.Int, | ||
| ) (*big.Int, error) { |
There was a problem hiding this comment.
non-blocking, I wonder if it would be better to have this be a state-less function instead that takes in quoting params + leverage + total shares + shares to withdraw, rather than fetching from state.
Benefits:
- tests run a lot faster
- [vague] can run multiple times w/o fetching state multiple times, perhaps if we change input params
Cons: - more indirection?
- more code?
There was a problem hiding this comment.
good thought. will keep this in mind when finishing up withdrawal and see if changing this to stateless makes sense
protocol/x/vault/keeper/withdraw.go
Outdated
| // 3. average_skew = integral / (posterior_leverage - leverage) | ||
| estimatedSlippagePpm.Mul(estimatedSlippagePpm, bigOneMillion) | ||
| estimatedSlippagePpm = lib.BigDivCeil( | ||
| estimatedSlippagePpm, | ||
| posteriorLeveragePpm.Sub(posteriorLeveragePpm, leveragePpm), | ||
| ) |
There was a problem hiding this comment.
Since posterior_leverage and leverage are often close, as withdrawals get smaller, this denominator loses precision.
Would it be better to treat both leverages as ratios?
Define
oopen notional (unsigned)evault equity, such that initial leveragel0 = o / emthe integer number of shares being withdrawnnthe integer number of total shares
Then define posterior leverage l1 = l0 / (1 - m/n) = (o/e) / (1 - m/n)
And define change in leverage dl (which is the denominator here):
dl = l1 - l0 = om / e(n - m)
Now average skew s is still s = integral / dl, but since we have dl as a ratio, you can invert it and take:
s = integral / dl = integral * e(n - m) / om
I think this avoids the loss of precision discussed here
protocol/x/vault/keeper/withdraw.go
Outdated
| estimatedSlippagePpm.Add(estimatedSlippagePpm, bigOneMillion) | ||
| estimatedSlippagePpm.Mul(estimatedSlippagePpm, leveragePpm) | ||
| estimatedSlippagePpm = lib.BigIntMulPpm( | ||
| estimatedSlippagePpm, | ||
| vault.SpreadPpm("ingParams, &marketParam), | ||
| ) | ||
| estimatedSlippagePpm = lib.BigDivCeil(estimatedSlippagePpm, bigOneMillion) |
There was a problem hiding this comment.
One additional simplification to postpone division:
slippage = spread * (1 + s) * l0
= spread * x
where
x = (1 + s) * l0
= [1 + (integral * e(n - m) / om)] * (o / e)
= [integral * (n - m) / m] + (o / e)
| // = spread * (1 + integral * (n - m) / (ln - l(n - m))) * l | ||
| // = spread * (1 + integral * (n - m) / lm) * l | ||
| // = spread * (l + integral * (n - m) / m) | ||
| func (k Keeper) GetVaultWithdrawalSlippage( |
There was a problem hiding this comment.
nit: Might as well convert back to a big.Int to return ppm slippage here rather than outside of this function?
There was a problem hiding this comment.
i was thinking that returning a rational number can allow parent functions to yield more accurate results (e.g. equity * (1 - slippage) can be more accurate if slippage isn't yet rounded to a big.Int yet)
Changelist
implement withdrawal fee estimation formula
Test Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation