Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix closing fee #475

Merged
merged 2 commits into from
Apr 6, 2023
Merged

fix closing fee #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99c2f29
psize to micro scale
taiki1frsh Apr 4, 2023
ab689f8
fix closing fee retreving
taiki1frsh Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions x/derivatives/keeper/perpetual_futures.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,12 @@ func (k Keeper) OpenPerpetualFuturesPosition(ctx sdk.Context, positionId string,
func (k Keeper) ClosePerpetualFuturesPosition(ctx sdk.Context, position types.PerpetualFuturesPosition) error {
params := k.GetParams(ctx)
commissionRate := params.PerpetualFutures.CommissionRate
// FIXME: Size_ cannot be used like this.
// It causes the inconsistency between the position and the token amount.
feeAmountDec := position.PositionInstance.Size_.Mul(commissionRate)
tradeAmount := position.PositionInstance.Size_.Sub(feeAmountDec)

// At closing the position, the trading fee is deducted.
// fee = positionSize * commissionRate
positionSizeInMicroDec := sdk.NewDecFromInt(*position.PositionInstance.SizeInMicro)
feeAmountDec := positionSizeInMicroDec.Mul(commissionRate)
tradeAmount := positionSizeInMicroDec.Sub(feeAmountDec)
feeAmount := feeAmountDec.RoundInt()

baseUsdPrice, err := k.GetCurrentPrice(ctx, position.Market.BaseDenom)
Expand All @@ -117,19 +119,10 @@ func (k Keeper) ClosePerpetualFuturesPosition(ctx sdk.Context, position types.Pe
return err
}

// TODO: this is wrong. refer to Issue#407
// maybe this todo is related to the above fixme content.
if !feeAmount.IsZero() {
err = k.bankKeeper.SendCoinsFromAccountToModule(ctx, position.Address.AccAddress(), types.ModuleName, sdk.Coins{sdk.NewCoin(position.Market.BaseDenom, feeAmount)})
if err != nil {
return err
}
}

quoteTicker := k.GetPoolQuoteTicker(ctx)
baseMetricsRate := types.NewMetricsRateType(quoteTicker, position.Market.BaseDenom, baseUsdPrice)
quoteMetricsRate := types.NewMetricsRateType(quoteTicker, position.Market.QuoteDenom, quoteUsdPrice)
returningAmount, lossToLP := position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate)
returningAmount, lossToLP := position.CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate, feeAmount)

// Tell the loss to the LP happened by a trade
// This has to be restricted by the protocol behavior in the future
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The code seems to be related to closing a perpetual futures positions.
  2. 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.
  3. A TODO comment in line 24 suggests that something is wrong with the code, which may be related to the FIXME comment above.
  4. 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.
  5. Line 37 is updated to include feeAmount as a parameter, which will be deducted when closing a position.
  6. Line 42 indicates that the loss to LP should be restricted by the protocol behavior in the future.
  7. Line 45 updates the returning coin to be positive, and sends it to the account address in line 47.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch adds a trading fee deduction and modifies the returning amount calculation in ClosePerpetualFuturesPosition function within the Keeper.

Improvements:

Bug Risk:

  • None identified.

Overall, the changes seem to improve the code's functionality.

Expand Down
10 changes: 9 additions & 1 deletion x/derivatives/types/positions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (m PerpetualFuturesPosition) RequiredMarginInMetrics(baseMetricsRate, quote
}

// CalcReturningAmountAtClose calculates the amount of the principal and the profit/loss at the close of the position.
func (m PerpetualFuturesPosition) CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate MetricsRateType) (returningAmount math.Int, lossToLP math.Int) {
func (m PerpetualFuturesPosition) CalcReturningAmountAtClose(baseMetricsRate, quoteMetricsRate MetricsRateType, tradingFee sdk.Int) (returningAmount math.Int, lossToLP math.Int) {
principal := m.RemainingMargin.Amount
// pnlAmountInMetrics represents the profit/loss amount in the metrics asset of the market.
// In the most cases, it means it's in "usd".
Expand All @@ -260,6 +260,14 @@ func (m PerpetualFuturesPosition) CalcReturningAmountAtClose(baseMetricsRate, qu
lossToLP = sdk.ZeroInt()
}

// Subtract the trading fee.
if returningAmount.LT(tradingFee) {
// Return 0 returning amount and the trading fee subtracted by the returning amount as LossToLP
return sdk.ZeroInt(), tradingFee.Sub(returningAmount)
}

returningAmount = returningAmount.Sub(tradingFee)

return returningAmount, lossToLP
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the review:

  1. The patch adds a tradingFee parameter to the CalcReturningAmountAtClose method.
  2. 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.
  3. The code looks correct and there is no bug risk.
  4. 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
26 changes: 18 additions & 8 deletions x/derivatives/types/positions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,7 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
position types.PerpetualFuturesPosition
closedBaseRate sdk.Dec
closeQuoteRate sdk.Dec
tradingFee sdk.Int
expReturn math.Int
expLoss math.Int
}{
Expand All @@ -712,7 +713,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.00002"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(11000000),
tradingFee: sdk.NewInt(1000000),
expReturn: sdk.NewInt(10000000),
},
{
name: "Profit Short position in quote denom margin",
Expand All @@ -732,7 +734,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.00001"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(11000000),
tradingFee: sdk.NewInt(1000000),
expReturn: sdk.NewInt(10000000),
},
{
name: "Loss Long position in quote denom margin",
Expand All @@ -752,6 +755,7 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.000009"),
closeQuoteRate: uusdcRate,
tradingFee: sdk.ZeroInt(),
expReturn: sdk.NewInt(0),
},
{
Expand All @@ -772,7 +776,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.0000105"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(1000000),
tradingFee: sdk.NewInt(500000),
expReturn: sdk.NewInt(500000),
},
{
name: "Profit Long position in base denom margin",
Expand All @@ -792,7 +797,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.00002"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(1500000),
tradingFee: sdk.NewInt(500000),
expReturn: sdk.NewInt(1000000),
},
{
name: "Profit Short position in base denom margin",
Expand All @@ -812,7 +818,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.00001"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(2000000),
tradingFee: sdk.NewInt(1000),
expReturn: sdk.NewInt(1999000),
},
{
name: "Loss Long position in base denom margin",
Expand All @@ -832,7 +839,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.000009"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(888889),
tradingFee: sdk.NewInt(100),
expReturn: sdk.NewInt(888789),
},
{
name: "Loss Short position in base denom margin",
Expand All @@ -852,7 +860,8 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.000011"),
closeQuoteRate: uusdcRate,
expReturn: sdk.NewInt(909091),
tradingFee: sdk.NewInt(1000),
expReturn: sdk.NewInt(908091),
},
{
name: "Loss Exceeds Margin: Long position in base denom margin",
Expand All @@ -872,6 +881,7 @@ func TestCalcReturningAmountAtClose(t *testing.T) {
},
closedBaseRate: sdk.MustNewDecFromStr("0.000009"),
closeQuoteRate: uusdcRate,
tradingFee: sdk.ZeroInt(),
expReturn: sdk.NewInt(0),
expLoss: sdk.NewInt(-111111),
},
Expand All @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down