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.

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

[SPEC ONLY] Clarify unregistering data sources #1207

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

barnabee
Copy link
Member

Clarfiy that once a single use data source is used on a market it is "unregistered" immediately and we don't wait for the market to be removed entirely from core.

edd
edd previously approved these changes Aug 28, 2022
Copy link
Member

@edd edd left a comment

Choose a reason for hiding this comment

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

👍 Good ACs

AnExsomnis
AnExsomnis previously approved these changes Aug 28, 2022
Copy link
Contributor

@davidsiska-vega davidsiska-vega left a comment

Choose a reason for hiding this comment

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

It seems to me that 0016-PFUT-015 contradicts 0016-PFUT-008.
It seems tome that 0016-PFUT-015 and 0016-PFUT-016 seem to be duplicates of each other.

@barnabee barnabee changed the title Clarify unreigtsering data sources Clarify unregistering data sources Aug 29, 2022
@barnabee barnabee self-assigned this Aug 29, 2022
@barnabee barnabee dismissed stale reviews from AnExsomnis and edd via 6d245e1 August 30, 2022 09:32
@gordsport gordsport added this to the 🤠 Oregon Trail milestone Sep 14, 2022
waitForMarketStatus(TRADING_TERMINATED)

// Store settlement data (replaces data from previous event so always use latest event up to the point we settle)
cash_settled_future.settlement_data = event.data
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we always want to do that. Imagine data that are of the form [timestamp, price]. Now imagine I receive first [2022-09-01-23:59, price1] and later I receive something with earlier timestamp [2022-09-01-12:00, price2]. In this case I doubt we'd want to replace the settlement data with the newer event.

Copy link
Member Author

@barnabee barnabee Sep 14, 2022

Choose a reason for hiding this comment

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

That was the thinking behind the original spec attempts — normally settlement, valuation, etc. happen with the first price printed after or exactly for the appropriate timestamp. I've never seen a system where if you want to expire something and you have two prices in a feed that are after the fixing time you'd use the later one. If you want more recent data, usually you would specify that (with a filter or by having a data source that doesn't send data that is too old at all). But here we are…

There have been three iterations of the spec for this:

1st attempt: take the first data that matches and is received after you are in a state where you are ready to settle the market.
2nd attempt: take the first data that matches and is received after the enactment of the market whether or not it is ready to settle yet
3rd attempt (now): take the most recent data received after the enactment of the market (if trading is terminated before any data is received, this will also be the first data received)

Note that my preference and the original design was for (1). Though it was vulnerable to potentially dropping the real preferred data due to a late trading termination trigger, and perhaps even never settling at all:

  • dropping the "correct" data but settling with later data I did see as a problem, but one could be avoided by using a no-later-than timestamp filter (so that it'd never settle in that case); but
  • dropping the correct data and never settling I saw as a feature, not a bug, because if trading termination was delayed until after the price was known then it made sense to me that you might want to agree a settlement price via a governance update rather than settle to a price that some people had already seen with potentially no warning.

We then went via iteration (2) to always use the first data, which, like (1) avoids the problem you mention in your comment but with trade-off of less likely to need governance but more likely to suffer whatever issues out of order termination vs settlement might cause. I am not sure why this change was made but it may simply have been to try to align with what we thought had been built and because it seeemed "good enough".

Now we have (3), which I'm pretty sure boils down to discovering it doesn't work like (2) either and agreeing to change the spec to match implementation. I can't remember if anyone argued it was actually preferable to (1) or (2) but I think there might have been some discussion in that direction…

Anyhow:

  1. I doubt it's a problem for trading enabled, though (3) is definitely my least favourite of the iterations.
  2. We could probably change it fairly simply though.
  3. We could also add a product param to the cash settled future at some point to specify the behaviour (but see next point - that is probably not be the best way to deal with it)
  4. With the time triggered data source modifier/combinastor (which was not yet built), it would be possible to trivially emulate iteration (2) given (1), and with additional data source features (or WASM or similar), you could also create (3) entirely within the data source definition. This is almost certainly preferable to building all this logic into each product, and also speaks to why iteration (1), which is the simplest (either you consumer the data source now and settle or you throw it away and store no state), is also probably the better design.

So I don't know if we want to change anything now. But I do agree what we have isn't particularly good, doesn't compose well with other features, adds complexity, etc. (though it clearly is workable).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to go back to (1). I wouldn't require it for "enable trading" but we may want to do it anyway... (because writing one set of tests now and rewriting is a pain.

My reasoning: it's better to miss a settlement data transaction (this can be fixed be governance) than a situation where someone can try to submit an "old coinbase price" on a market without a filter just before trading terminated.

@@ -74,3 +107,7 @@ cash_settled_future.settlement_data(event) {
1. Lifecycle events are processed atomically as soon as they are triggered, i.e. the above condition always holds even for two or more transactions arriving at effectively the same time - only the transaction that is sequenced first triggers final settlement (<a name="0016-PFUT-010" href="#0016-PFUT-010">0016-PFUT-010</a>)
1. Once a market is finally settled, the mark price is equal to the settlement data and this is exposed on event bus and market data APIs (<a name="0016-PFUT-011" href="#0016-PFUT-011">0016-PFUT-011</a>)
1. Assure [settment-at-expiry.feature](https:/vegaprotocol/vega/blob/develop/core/integration/features/verified/0002-STTL-settlement_at_expiry.feature) executes correctly (<a name="0016-PFUT-012" href="#0016-PFUT-012">0016-PFUT-012</a>)
1. After trading termination has been triggered the trading terminated data source is no longer active (assuming it is not used anywhere else) and data from that source is no longer processed. (<a name="0016-PFUT-013" href="#0016-PFUT-013">0016-PFUT-013</a>)
1. After settlement data has been recieved and the market has settled, the settlement data source is no longer active (assuming it is not used anywhere else) and data from that source is no longer processed. (<a name="0016-PFUT-014" href="#0016-PFUT-014">0016-PFUT-014</a>)
1. After settlement data has been recieved and the market has not settled yet because trading termination has not been triggered, the settlement data source is remains active (assuming it is not used anywhere else) and data from that source is still processed (because each updated data event recieved is stored and eventually used for settlement up until trading terminated it triggered). (<a name="0016-PFUT-015" href="#0016-PFUT-015">0016-PFUT-015</a>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. After settlement data has been recieved and the market has not settled yet because trading termination has not been triggered, the settlement data source is remains active (assuming it is not used anywhere else) and data from that source is still processed (because each updated data event recieved is stored and eventually used for settlement up until trading terminated it triggered). (<a name="0016-PFUT-015" href="#0016-PFUT-015">0016-PFUT-015</a>)
1. After settlement data has been recieved and the market has not settled yet because trading termination has not been triggered, the settlement data source is remains active (regardless of whether it's used elsewhere) and data from that source is still processed (because each updated data event received is stored and eventually used for settlement up until trading terminated it triggered). (<a name="0016-PFUT-015" href="#0016-PFUT-015">0016-PFUT-015</a>)

Copy link
Member Author

Choose a reason for hiding this comment

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

For this test I think it needs to no be used elsewhere rather than left up to the implementer whether it is or not. Or perhaps there should even be 2 ACs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe two ACs.

Copy link
Contributor

@davidsiska-vega davidsiska-vega left a comment

Choose a reason for hiding this comment

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

Left a comment and a suggestion.

@davidsiska-vega davidsiska-vega changed the title Clarify unregistering data sources [SPEC ONLY] Clarify unregistering data sources Apr 17, 2024
@cdummett cdummett removed this from the 🏛️ Colosseo II milestone Jul 10, 2024
@cdummett cdummett added this to the 🥶 Icebox milestone Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants