Skip to content

Commit

Permalink
Merge pull request #10573 from vegaprotocol/10568-pnl-fix
Browse files Browse the repository at this point in the history
fix: clone amount to prevent subtracting twice, causing underflow
  • Loading branch information
EVODelavega authored Feb 6, 2024
2 parents f79e55d + 89176ac commit c80b659
Show file tree
Hide file tree
Showing 9 changed files with 332 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@
- [10533](https://github.com/vegaprotocol/vega/issues/10533) - Fix `EstiamtePosition` returning an error when trying to access the party id field via GraphQL.
- [10546](https://github.com/vegaprotocol/vega/issues/10546) - `EstimateTransferFee` API errors when there is no discount.
- [10532](https://github.com/vegaprotocol/vega/issues/10532) - Fix for games statistics.
- [10568](https://github.com/vegaprotocol/vega/issues/10568) - Fix for `PnL` underflow.

## 0.73.0

Expand Down
4 changes: 2 additions & 2 deletions core/collateral/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1163,7 +1163,7 @@ func (e *Engine) FinalSettlement(ctx context.Context, marketID string, transfers
responses = append(responses, res)

// Update to see how much we still need
requestAmount = requestAmount.Sub(requestAmount, amountCollected)
requestAmount.Sub(requestAmount, amountCollected)
if transfer.Owner != types.NetworkParty {
// no error possible here, we're just reloading the accounts to ensure the correct balance
general, margin, bond, _ := e.getMTMPartyAccounts(transfer.Owner, marketID, asset)
Expand Down Expand Up @@ -1412,7 +1412,7 @@ func (e *Engine) mtmOrFundingSettlement(ctx context.Context, marketID string, tr
responses = append(responses, res)

// Update to see how much we still need
requestAmount = requestAmount.Sub(requestAmount, amountCollected)
requestAmount.Sub(requestAmount, amountCollected)

// here we check if we were able to collect all monies,
// if not send an event to notify the plugins
Expand Down
98 changes: 98 additions & 0 deletions core/collateral/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2198,6 +2198,104 @@ func TestGetPartyMarginEmpty(t *testing.T) {
assert.NoError(t, err)
}

func TestMTMLossSocializationUnderflow(t *testing.T) {
eng := getTestEngine(t)
defer eng.Finish()
lossParty1 := "lossparty1"
winParty1 := "winparty1"
winParty2 := "winparty2"
winParty3 := "winparty3"

// create parties
eng.broker.EXPECT().Send(gomock.Any()).Times(13)
_, _ = eng.CreatePartyGeneralAccount(context.Background(), lossParty1, testMarketAsset)
margin, err := eng.CreatePartyMarginAccount(context.Background(), lossParty1, testMarketID, testMarketAsset)
eng.IncrementBalance(context.Background(), margin, num.NewUint(2))
assert.Nil(t, err)
_, _ = eng.CreatePartyGeneralAccount(context.Background(), winParty1, testMarketAsset)
_, err = eng.CreatePartyMarginAccount(context.Background(), winParty1, testMarketID, testMarketAsset)
assert.Nil(t, err)
_, _ = eng.CreatePartyGeneralAccount(context.Background(), winParty2, testMarketAsset)
_, err = eng.CreatePartyMarginAccount(context.Background(), winParty2, testMarketID, testMarketAsset)
assert.Nil(t, err)
_, _ = eng.CreatePartyGeneralAccount(context.Background(), winParty3, testMarketAsset)
_, err = eng.CreatePartyMarginAccount(context.Background(), winParty3, testMarketID, testMarketAsset)
assert.Nil(t, err)

// 1 party loses 3, 3 parties win 1, losing party only has a balance of 2 available
pos := []*types.Transfer{
{
Owner: lossParty1,
Amount: &types.FinancialAmount{
Amount: num.NewUint(3),
Asset: testMarketAsset,
},
Type: types.TransferTypeMTMLoss,
},
{
Owner: winParty3,
Amount: &types.FinancialAmount{
Amount: num.NewUint(1),
Asset: testMarketAsset,
},
Type: types.TransferTypeMTMWin,
},
{
Owner: winParty1,
Amount: &types.FinancialAmount{
Amount: num.NewUint(1),
Asset: testMarketAsset,
},
Type: types.TransferTypeMTMWin,
},
{
Owner: winParty2,
Amount: &types.FinancialAmount{
Amount: num.NewUint(1),
Asset: testMarketAsset,
},
Type: types.TransferTypeMTMWin,
},
}

eng.broker.EXPECT().SendBatch(gomock.Any()).AnyTimes()
eng.broker.EXPECT().Send(gomock.Any()).AnyTimes().Do(func(evt events.Event) {
ae, ok := evt.(accEvt)
assert.True(t, ok)
acc := ae.Account()
if acc.Owner == winParty3 && acc.Type == types.AccountTypeMargin {
assert.Equal(t, 0, stringToInt(acc.Balance))
}
if acc.Owner == winParty1 && acc.Type == types.AccountTypeMargin {
assert.Equal(t, 0, stringToInt(acc.Balance))
}
if acc.Owner == winParty2 && acc.Type == types.AccountTypeMargin {
assert.Equal(t, 2, stringToInt(acc.Balance))
}
})
transfers := eng.getTestMTMTransfer(pos)
evts, raw, err := eng.MarkToMarket(context.Background(), testMarketID, transfers, "BTC", func(string) bool { return true })
assert.NoError(t, err)
assert.Equal(t, 4, len(raw))
assert.NotEmpty(t, evts)

assert.Equal(t, 1, len(raw[0].Entries))
assert.Equal(t, num.NewUint(2), raw[0].Entries[0].ToAccountBalance)
assert.Equal(t, num.NewUint(2), raw[0].Entries[0].ToAccountBalance)

assert.Equal(t, 1, len(raw[1].Entries))
assert.Equal(t, num.NewUint(0), raw[1].Entries[0].ToAccountBalance)
assert.Equal(t, num.NewUint(0), raw[1].Entries[0].ToAccountBalance)

assert.Equal(t, 1, len(raw[2].Entries))
assert.Equal(t, num.NewUint(0), raw[2].Entries[0].ToAccountBalance)
assert.Equal(t, num.NewUint(0), raw[2].Entries[0].ToAccountBalance)

assert.Equal(t, 1, len(raw[3].Entries))
assert.Equal(t, num.NewUint(2), raw[3].Entries[0].ToAccountBalance)
assert.Equal(t, num.NewUint(2), raw[3].Entries[0].ToAccountBalance)
}

func TestMTMLossSocialization(t *testing.T) {
eng := getTestEngine(t)
defer eng.Finish()
Expand Down
9 changes: 5 additions & 4 deletions core/collateral/simple_distributor.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ func (s *simpleDistributor) Run(ctx context.Context) []events.Event {
if v.request.Owner == types.NetworkParty {
v := v
netReq = &v
continue // network events are to be ignored
}
evt = events.NewLossSocializationEvent(ctx, v.request.Owner, s.marketID, loss, true, s.ts)
s.log.Warn("loss socialization missing funds to be distributed",
Expand All @@ -90,14 +89,16 @@ func (s *simpleDistributor) Run(ctx context.Context) []events.Event {
}
// last one get the remaining bits
s.requests[len(s.requests)-1].request.Amount.Amount.AddSum(mismatch)
// decAmt is negative
loss := mismatch.Sub(evt.Amount().U, mismatch)
// if the remainder > the loss amount, this rounding error was profitable
// so the loss socialisation event should be flagged as profit
// profit will be true if the shortfall < mismatch amount
loss, profit := mismatch.Delta(evt.Amount().U, mismatch)
evts[len(evts)-1] = events.NewLossSocializationEvent(
evt.Context(),
evt.PartyID(),
evt.MarketID(),
loss,
true,
!profit, // true if party still lost out, false if mismatch > shortfall
s.ts)
}
return evts
Expand Down
76 changes: 76 additions & 0 deletions core/integration/features/settlement/10568-pnl-underflow.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
Feature: Test loss socialization case 1

Background:
Given the markets:
| id | quote name | asset | risk model | margin calculator | auction duration | fees | price monitoring | data source config | linear slippage factor | quadratic slippage factor | sla params |
| ETH/DEC19 | BTC | BTC | default-simple-risk-model-2 | default-margin-calculator | 1 | default-none | default-none | default-eth-for-future | 1e6 | 1e6 | default-futures |
And the following network parameters are set:
| name | value |
| market.auction.minimumDuration | 1 |
| network.markPriceUpdateMaximumFrequency | 0s |

@LossSocEvts
Scenario: Case 1: trader1 has insufficient MTM & only trader2 socialises the losses (0002-STTL-009)
Description : Case 1 from https://docs.google.com/spreadsheets/d/1CIPH0aQmIKj6YeFW9ApP_l-jwB4OcsNQ/edit#gid=1555964910

# setup accounts
Given the parties deposit on asset's general account the following amount:
| party | asset | amount |
| sellSideProvider | BTC | 100000000 |
| buySideProvider | BTC | 100000000 |
| party1 | BTC | 5000 |
| party2 | BTC | 50000 |
| party3 | BTC | 50000 |
| aux1 | BTC | 100000000 |
| aux2 | BTC | 100000000 |
# setup order book
When the parties place the following orders:
| party | market id | side | volume | price | resulting trades | type | tif | reference |
| sellSideProvider | ETH/DEC19 | sell | 1000 | 120 | 0 | TYPE_LIMIT | TIF_GTC | sell-provider-1 |
| buySideProvider | ETH/DEC19 | buy | 1000 | 80 | 0 | TYPE_LIMIT | TIF_GTC | buy-provider-1 |
| aux1 | ETH/DEC19 | sell | 1 | 120 | 0 | TYPE_LIMIT | TIF_GTC | aux-s-1 |
| aux2 | ETH/DEC19 | buy | 1 | 80 | 0 | TYPE_LIMIT | TIF_GTC | aux-b-1 |
| aux1 | ETH/DEC19 | sell | 1 | 100 | 0 | TYPE_LIMIT | TIF_GTC | aux-s-2 |
| aux2 | ETH/DEC19 | buy | 1 | 100 | 0 | TYPE_LIMIT | TIF_GTC | aux-b-2 |
Then the opening auction period ends for market "ETH/DEC19"
And the mark price should be "100" for the market "ETH/DEC19"
And the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
# party 1 place an order + we check margins
When the parties place the following orders with ticks:
| party | market id | side | volume | price | resulting trades | type | tif | reference |
| party1 | ETH/DEC19 | sell | 100 | 100 | 0 | TYPE_LIMIT | TIF_GTC | ref-1 |
Then the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
# then party2 place an order, and we calculate the margins again
When the parties place the following orders with ticks:
| party | market id | side | volume | price | resulting trades | type | tif | reference |
| party2 | ETH/DEC19 | buy | 100 | 100 | 1 | TYPE_LIMIT | TIF_GTC | ref-1 |
Then the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
# then we change the volume in the book
Then the parties cancel the following orders:
| party | reference |
| sellSideProvider | sell-provider-1 |
| buySideProvider | buy-provider-1 |
Then the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
When the parties place the following orders with ticks:
| party | market id | side | volume | price | resulting trades | type | tif | reference |
| sellSideProvider | ETH/DEC19 | sell | 1000 | 200 | 0 | TYPE_LIMIT | TIF_GTC | sell-provider-2 |
| buySideProvider | ETH/DEC19 | buy | 1000 | 80 | 0 | TYPE_LIMIT | TIF_GTC | buy-provider-2 |
Then the parties cancel the following orders:
| party | reference |
| aux1 | aux-s-1 |
| aux2 | aux-b-1 |
Then the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
When the parties place the following orders with ticks:
| party | market id | side | volume | price | resulting trades | type | tif | reference |
| party2 | ETH/DEC19 | buy | 100 | 180 | 0 | TYPE_LIMIT | TIF_GTC | ref-1 |
| party3 | ETH/DEC19 | sell | 100 | 180 | 1 | TYPE_LIMIT | TIF_GTC | ref-2 |
Then the trading mode should be "TRADING_MODE_CONTINUOUS" for the market "ETH/DEC19"
Then the parties should have the following profit and loss:
| party | volume | unrealised pnl | realised pnl |
| party1 | 0 | 0 | -5000 |
| party2 | 200 | 8000 | -2970 |
| party3 | -100 | 0 | 0 |
| aux2 | 1 | 80 | -30 |
And the insurance pool balance should be "0" for the market "ETH/DEC19"
And debug loss socialisation events
And the cumulated balance for all accounts should be worth "400105000"
6 changes: 6 additions & 0 deletions core/integration/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,12 @@ func InitializeScenario(s *godog.ScenarioContext) {
s.Step(`^a total of "([0-9]+)" events should be emitted$`, func(eventCounter int) error {
return steps.TotalOfEventsShouldBeEmitted(execsetup.broker, eventCounter)
})
s.Step(`^the loss socialisation amounts are:$`, func(table *godog.Table) error {
return steps.TheLossSocialisationAmountsAre(execsetup.broker, table)
})
s.Step(`^debug loss socialisation events$`, func() error {
return steps.DebugLossSocialisationEvents(execsetup.broker, execsetup.log)
})

// Decimal places steps
s.Step(`^the following assets are registered:$`, func(table *godog.Table) error {
Expand Down
8 changes: 8 additions & 0 deletions core/integration/steps/table_wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,14 @@ func (r RowWrapper) MustU64(name string) uint64 {
return value
}

func (r RowWrapper) MustInt(name string) *num.Int {
val, ok := num.IntFromString(r.MustStr(name), 10)
if ok {
panicW(name, fmt.Errorf("failed to parse int"))
}
return val
}

func (r RowWrapper) MustUint(name string) *num.Uint {
value, err := Uint(r.mustColumn(name))
panicW(name, err)
Expand Down
121 changes: 121 additions & 0 deletions core/integration/steps/the_loss_socialisation_amount_is.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// Copyright (C) 2023 Gobalsky Labs Limited
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as
// published by the Free Software Foundation, either version 3 of the
// License, or (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

package steps

import (
"fmt"

"code.vegaprotocol.io/vega/core/events"
"code.vegaprotocol.io/vega/core/integration/stubs"
"code.vegaprotocol.io/vega/libs/num"
"code.vegaprotocol.io/vega/logging"

"github.com/cucumber/godog"
)

func TheLossSocialisationAmountsAre(broker *stubs.BrokerStub, table *godog.Table) error {
lsMkt := getLossSocPerMarket(broker)
for _, r := range parseLossSocTable(table) {
lsr := lossSocRow{r: r}
mevts, ok := lsMkt[lsr.Market()]
if !ok {
return fmt.Errorf("no loss socialisation events found for market %s", lsr.Market())
}
parties := map[string]struct{}{}
for _, e := range mevts {
if lsr.Amount().EQ(e.Amount()) {
parties[e.PartyID()] = struct{}{}
}
}
if c := lsr.Count(); c != -1 {
if len(parties) != c {
return fmt.Errorf("expected %d loss socialisation events for market %s and amount %s, instead found %d", c, lsr.Market(), lsr.Amount().String(), len(parties))
}
}
for _, p := range lsr.Party() {
if _, ok := parties[p]; !ok {
return fmt.Errorf("no loss socialisation found for party %s on market %s for amount %s", p, lsr.Market(), lsr.Amount().String())
}
}
}
return nil
}

func DebugLossSocialisationEvents(broker *stubs.BrokerStub, log *logging.Logger) error {
lsEvts := getLossSocPerMarket(broker)
for mkt, evts := range lsEvts {
log.Infof("\nLoss socialisation events for market %s:", mkt)
for _, e := range evts {
log.Infof(
"Party: %s - Amount: %s",
e.PartyID(),
e.Amount().String(),
)
}
log.Info("----------------------------------------------------------------------------")
}
return nil
}

func getLossSocPerMarket(broker *stubs.BrokerStub) map[string][]*events.LossSoc {
evts := broker.GetLossSoc()
ret := map[string][]*events.LossSoc{}
for _, e := range evts {
mkt := e.MarketID()
mevts, ok := ret[mkt]
if !ok {
mevts = []*events.LossSoc{}
}
ret[mkt] = append(mevts, e)
}
return ret
}

func parseLossSocTable(table *godog.Table) []RowWrapper {
return StrictParseTable(table, []string{
"market",
"amount",
}, []string{
"party",
"count",
})
}

type lossSocRow struct {
r RowWrapper
}

func (l lossSocRow) Market() string {
return l.r.MustStr("market")
}

func (l lossSocRow) Amount() *num.Int {
return l.r.MustInt("amount")
}

func (l lossSocRow) Party() []string {
if l.r.HasColumn("party") {
return l.r.MustStrSlice("party", ",")
}
return nil
}

func (l lossSocRow) Count() int {
if l.r.HasColumn("count") {
return int(l.r.MustI64("count"))
}
return -1
}
Loading

0 comments on commit c80b659

Please sign in to comment.