fix closing fee #475
fix closing fee #475
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| @@ -141,6 +134,7 @@ func (k Keeper) ClosePerpetualFuturesPosition(ctx sdk.Context, position types.Pe | |||
| } | |||
|
|
|||
| returningCoin := sdk.NewCoin(position.RemainingMargin.Denom, returningAmount) | |||
|
|
|||
| if returningCoin.IsPositive() { | |||
| if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, position.Address.AccAddress(), sdk.Coins{returningCoin}); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
the code review:
- The code seems to be related to closing a perpetual futures positions.
- There is a FIXME comment in line 5 indicating that the Size_ cannot be used like this, possibly causing inconsistency between the position and token amount.
- A TODO comment in line 24 suggests that something is wrong with the code, which may be related to the FIXME comment above.
- In line 35, a MetricsRateType is declared for both base and quote denom, which could be useful for calculating the returning amount and loss to LP.
- Line 37 is updated to include feeAmount as a parameter, which will be deducted when closing a position.
- Line 42 indicates that the loss to LP should be restricted by the protocol behavior in the future.
- Line 45 updates the returning coin to be positive, and sends it to the account address in line 47.
| } | ||
|
|
||
| returningAmount = returningAmount.Sub(tradingFee) | ||
|
|
||
| return returningAmount, lossToLP | ||
| } | ||
|
|
There was a problem hiding this comment.
the review:
- The patch adds a tradingFee parameter to the CalcReturningAmountAtClose method.
- It also adds code that subtracts the trading fee from the returning amount and stores the difference between the trading fee and the returning amount as LossToLP.
- The code looks correct and there is no bug risk.
- As an improvement suggestion, it might be useful to add comments to explain why the trading fee is being subtracted and why the difference is stored as LossToLP.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -141,6 +134,7 @@ func (k Keeper) ClosePerpetualFuturesPosition(ctx sdk.Context, position types.Pe | |||
| } | |||
|
|
|||
| returningCoin := sdk.NewCoin(position.RemainingMargin.Denom, returningAmount) | |||
|
|
|||
| if returningCoin.IsPositive() { | |||
| if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, position.Address.AccAddress(), sdk.Coins{returningCoin}); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
The code patch adds a trading fee deduction and modifies the returning amount calculation in ClosePerpetualFuturesPosition function within the Keeper.
Improvements:
- Consider rephrasing comments since they are not clear and may be outdated.
- Remove TODO after analyzing issue [derivatives] fix the way to retrieve fee when to close the position #407
- Consider updating the code to use sizeInMicro throughout the code instead of converting back and forth between Dec and Int.
Bug Risk:
- None identified.
Overall, the changes seem to improve the code's functionality.
| @@ -885,7 +895,7 @@ func TestCalcReturningAmountAtClose(t *testing.T) { | |||
| sizeInMicro := types.NormalToMicroInt(tc.position.PositionInstance.Size_) | |||
| tc.position.PositionInstance.SizeInMicro = &sizeInMicro | |||
|
|
|||
| returningAmount, lossToLP := tc.position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate) | |||
| returningAmount, lossToLP := tc.position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate, tc.tradingFee) | |||
| fmt.Println(returningAmount, lossToLP) | |||
| if !tc.expReturn.Equal(returningAmount) { | |||
| t.Error(tc, "expected %v, got %v", tc.expReturn, returningAmount) | |||
There was a problem hiding this comment.
with the code patch. It's adding a new variable (tradingFee) to the TestCalcReturningAmountAtClose function, and it is also being used in the CalcReturningAmountAtClose function. This new variable should be properly initialized before calling CalcReturningAmountAtClose, otherwise it might lead to unexpected behaviours. Also, it should be checked that the tradingFee value is valid (non-negative).
Apart from that, I would recommend adding some documentation to the TestCalcReturningAmountAtClose function, so it is easier for other developers to understand what the code is doing.
| } | ||
|
|
||
| returningAmount = returningAmount.Sub(tradingFee) | ||
|
|
||
| return returningAmount, lossToLP | ||
| } | ||
|
|
There was a problem hiding this comment.
The code changes seem to add functionality related to trading fees calculation. The CalcReturningAmountAtClose function now takes a tradingFee parameter and subtracts it from the returningAmount before returning.
One suggestion for improvement would be to validate the tradingFee parameter to make sure it is not negative. Additionally, it may be helpful to include comments or documentation to explain how the tradingFee parameter is calculated and what units it's in.
Overall, the code changes appear to be low risk, as they do not significantly alter the existing functionality.
| @@ -885,7 +895,7 @@ func TestCalcReturningAmountAtClose(t *testing.T) { | |||
| sizeInMicro := types.NormalToMicroInt(tc.position.PositionInstance.Size_) | |||
| tc.position.PositionInstance.SizeInMicro = &sizeInMicro | |||
|
|
|||
| returningAmount, lossToLP := tc.position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate) | |||
| returningAmount, lossToLP := tc.position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate, tc.tradingFee) | |||
| fmt.Println(returningAmount, lossToLP) | |||
| if !tc.expReturn.Equal(returningAmount) { | |||
| t.Error(tc, "expected %v, got %v", tc.expReturn, returningAmount) | |||
There was a problem hiding this comment.
The code patch appears to be a test case for the CalcReturningAmountAtClose() function. It sets the input parameters for different test scenarios, including trading fees, and then checks the expected return matches the actual return.
There are no obvious bug risks in this code review. However, some potential improvements could include providing more descriptive comments, renaming variables to make them clearer, or adding additional test cases to cover more edge cases.
close #407