vegaprotocol / vega

A Go implementation of the Vega Protocol, a protocol for creating and trading derivatives on a fully decentralised network.
https://vega.xyz
GNU Affero General Public License v3.0
38 stars 22 forks source link

insurance pool not redistributed on SETTLE #8529

Closed Sohill-Patel closed 1 year ago

Sohill-Patel commented 1 year ago

_When the market enters transitions from "trading terminated state" to "settled" state (see market lifecyle), the insurance pool account has its balanceredistributed to the on-chain treasury for the settlement asset of the market and other insurance pools using the same asset.

Will setup 4 markets with same asset/oracle all at different states
Create insurance balance for  market1 e.g 88
Terminate/Settle market1
Check insurance pool balance for market1 is 0
Check insuruance pool balance for market2, 3 and 4 is 22, 22, 22
Check treasury balance is 22

Covers AC: 0015-INSR-002_

The test is to ensure when a market is SETTLED, the insurance pool is redistributed to the other markets with same asset. This is an intermitted failure proven by the two jenkins runs below.

Testing locally we have disproved a timing issue with the test by setting break points after forcing trading TERMINATED and again after trading SETTLED events are sent. Then multiple queries to get account balance does not show it updated with 0 for the account type Insurance.

Jenkins run as failure https://jenkins.ops.vega.xyz/job/common/job/system-tests-wrapper/75827/testReport/junit/tests.orders/closeouts_test/test_insurance_pool_behaviour_on_market_settlement/

Jenkins run as success https://jenkins.ops.vega.xyz/blue/organizations/jenkins/common%2Fsystem-tests-wrapper/detail/system-tests-wrapper/74861/pipeline/

gordsport commented 1 year ago

Initial triage suggests this is the culprit: keepInsurance := mktState == types.MarketStateSettled && !m.succeeded it should be: keepInsurance := mktState == types.MarketStateSettled && m.succeeded

EVODelavega commented 1 year ago

We keep the insurance pool during the succession time window, or in code terms: if the market is settled and not succeeded. The condition is correct, and this behaviour is covered in integration tests

ze97286 commented 1 year ago

then the expectation/AC is wrong as you can see above. also, where is it released?

EVODelavega commented 1 year ago

@ze97286 So the spec at play here is this one: https://github.com/vegaprotocol/specs/blob/cosmicelevator/protocol/0081-SUCM-successor_markets.md

Specifically this paragraph:

settled state but with time since settlement less than or equal market.liquidity.successorLaunchWindowLength or c) cancelled (closed by governance) but with the closing time less than or equal market.liquidity.successorLaunchWindowLength. The point of setting up a market to be successor of an existing market is to a) allow LPs continue claim their virtual stake / equity-like-share (ELS) by committing liquidity to the successor market during the pending period if they wish to, and b) allow the successor market to inherit the insurance pool of the parent market. When the successor market leaves the opening auction (moves from pending to active) the amount equal to insurancePoolFraction x parent market insurance pool balance is transferred to the successor market insurance pool. Once the parent market moves from "trading terminated" to "settled" state, the entire remaining insurance pool of the successor market is transferred to the successor market insurance pool.

This was discussed somewhat, and IIRC the concesus was that this means that some state (including the insurance pool) for any market (whether it has a successor in the wing or not) cannot be released once a market is settled unless the market in question has been succeeded, or the successor time window has expired.

The insurance pool balance will be released in the execution engine (towards the end of the OnTick func). The marketCPState data is checked for settled markets, and if the TTL has passed, the insurance pool balance is released there.

Sohill-Patel commented 1 year ago

Given the spec above, the ST needs to be updated to account for the changes.