Give market objects a canonical ID #286

Closed ashleyvega closed 4 years ago

In GitLab by @ashleyvega on May 15, 2019, 10:04

Current situation

Markets have a plain text ID, for example BTC/DEC19. We'd like to improve on this and create a market ID that has properties like immutability, comparability and succinct format.

Proposed solution

Give markets a canonical ID, i.e.

func CanonicalMarketID(market *Market) string {
  // << take the hash of immutable fields >>
  // encode with Crockford's base32 - or maybe base64
  // return
}

The canonical ID would ideally be deterministic, but this is up for discussion.

Possible future requirements?

via @ashleyvega: include the Party ID of the party who created the market, to avoid market duplicates / clones / forgeries.

In GitLab by @cdm on May 15, 2019, 10:13

changed the description

In GitLab by @ashleyvega on May 15, 2019, 11:19

changed the description

In GitLab by @ashleyvega on May 15, 2019, 11:19

changed the description

In GitLab by @cdm on May 15, 2019, 11:22

changed the description

In GitLab by @cdm on May 15, 2019, 11:22

changed the description

In GitLab by @cdm on May 15, 2019, 11:26

@barnabee and @tamlyn10 importantly we'd like some thoughts on this, and the following:

Will it be possible to have two BTC/DEC19 markets with almost exactly the same params? I've assumed that when a market is created it is immutable, but then we discussed briefly in SFO that people can vote to change a market to perpetual or edit it's status. If someone votes to change a market at runtime it's checksum or similar will need to change?
Will we want/need to check/de-dupe markets via their ID field? Speed/simplicity vs a DB query on all fields.
Which fields in a market will be immutable? Any?
Ashley's comment feedback in future requirements

Cheers in advance, we'd like to do this work within the next 24 hours if possible :100:

In GitLab by @tamlyn10 on May 15, 2019, 20:14

@cdm @barnabee

@barnabee I'm interested that you think the expiry should be changeable. I don't like this, as a general rule, as it changes the fundamental product, rendering the historical data inconsistent. @cdm yes you can have a market with almost exactly the same params. In fact, I've assumed that just one parameter being different is sufficient to define a whole new market. It's not ideal, as we don't want to spread liquidity across lots of markets but for now we rely on token holders voting to help limit this.
Sorry, this is out of my wheelhouse - @barnabee ?
I vote that expiry and strike (where applicable such as with options) are immutable. @davidsiska-vega any thoughts on any others?
I don't understand how this prevents duplicates, clones or forgeries?

In GitLab by @davidsiska-vega on May 15, 2019, 20:39

@tamlyn10 First of all, immutable fields: anything that defines the payoff of the derivative being traded should be immutable. So to take an example of an option it should be:

the settlement currency e.g. USD
the reference asset (in our case most likely another currency e.g. BTC
option type e.g. European put
strike
maturity

When we move to more general smart product language the whole "contract" described by the language (i.e. all the code describing the contract) should be immutable. If it's not then I have a gap in understanding that would need an explanation. So in my mind expiry is immutable.

In my mind if a group of 'people' want to have BTC/perpetual instead of BTC/DEC19 then they should put market making stake and create that new market (I appreciate that this isn't really getting closer to answering @cdm's question).

Other than that I suppose everything is changeable either by trading core code (e.g. risk parameters) or by vote (e.g. risk model for margins). I can also see the reasoning for risk model and calibration method to be immutable.

To solve the canonical market ID problem? Wouldn't it make sense to oblige the market creator to propose basically a random string that's not being used (and hasn't yet been used)?

In GitLab by @cdm on May 15, 2019, 21:20

Maybe the correct question is which fields are NOT immutable?

If a market can be changed (and perhaps my example of expiry is incorrect) then we can’t use a hash of the marshaled bytes to check if two markets are the same.

In GitLab by @barnabee on May 15, 2019, 22:03

I don’t recall saying that expiry might be mutable by voting but it’s possible. Votes themselves will have an expiry though. In any case I expect that the ~~Tradable~~ Instrument for a market is always immutable, once created, but that market or risk parameters may be mutable by stakeholder voting.

In GitLab by @cdm on May 16, 2019, 09:10

market or risk parameters may be mutable - Thanks :thinking:

In GitLab by @cdm on May 16, 2019, 15:21

We need a solution to this as it's blocking @ashleyvega's REST API progress

In GitLab by @barnabee on May 16, 2019, 17:52

This should be workable. I think it is an acceptable assumption that there can only be one market for an ~~Tradable~~ Instrument, so the [potential] mutability of market and risk parameters ought not to be a problem (those fields would not be included in the ID).

Alternatively perhaps just find a deterministic way of generating the ID on each node, e.g. hash(BLOCK_HASH ++ TX_SEQ_NO)...

In GitLab by @barnabee on May 16, 2019, 17:56

Re: @ashleyvega’s suggestion to include the ID of the party who created the market the calculation of the market ID would allow for rather than prevent duplicates, as it would be valid to have two markets for the same Instrument if creator was included.

In GitLab by @barnabee on May 16, 2019, 17:57

Updated above: we should be able to assume there can be only one market per Instrument, not Tradable Instrument.

In GitLab by @cdm on May 16, 2019, 18:02

We were talking earlier and if there are several fields that are immutable then that will help with key grouping - potentially the start of the ID would be composed by those values and the rest seeded by sequence number or other hash value. Please can you help us identify these 😎

In GitLab by @ashleyvega on May 17, 2019, 11:36

marked this issue as related to #254

In GitLab by @edd on May 20, 2019, 10:27

marked this issue as related to product#80

In GitLab by @tamlyn10 on May 20, 2019, 14:27

@cdm I'm not sure who you were speaking to but I'm not sure about this approach as I'm not sure there are enough immutable parameters in common across all potential ever products? In any case, I think the following are if you can make something of them (see https://gitlab.com/vega-protocol/product/issues/80)

Every product will immutably define:

Cash/asset flows
Settlement asset/s (perhaps also the pricing / reference asset and margin asset - would you assume these are immutable @barnabee @davidsiska-vega ? )
Expiry (may be event driven or time driven or perpetual)

From the above-mentioned ticket:

Note: the specific parameters for a product are defined by the product and will vary between products, so the system needs to be flexible in this regard.

I probably think @barnabee 's suggestion above to find a deterministic way of generating the ID on each node makes sense to me.

In GitLab by @barnabee on May 20, 2019, 19:33

@cdm it would be nice to understand how you are thinking of solving this.

I think the content-addressable option alone (hash of immutable fields) probably has problems. It is difficult to say for sure the uniqueness requirement will always hold true - what if we want to allow block or RFQ and order book trading for a single instrument, or allow smart products to market certain parameters as mutable.

However, I am not sure about key grouping by immutable data as products don’t necessarily overlap much in their definitions of these fields (I think this is what @tamlyn10 is getting at above) and I don’t think what is common to all products is going to vary enough to be useful.

Another option to create a deterministic ID is to hash(proposer_pub_key + nonce) like Ethereum does - see https://ethereum.stackexchange.com/questions/760/how-is-the-address-of-an-ethereum-contract-computed - (we could use proposer = 0 before we have proposers). I’m not sure of the particular benefits of this over, for instance hashing other things like block ID and sequence number, unless it is potentially useful that a node can know the market ID thats going to be used before the block is finalised.

@tamlyn10:

The cash/asset flows themselves are not immutable, only the logic that defines how to calculate them (perhaps that is what you mean)
The settlement (and other) defined assets will be, but most times these will be shared across a lot of markets so probably not very useful for grouping
Expiry is not a parameter/field for all products, it’s an event or output from the product logic, so couldn’t be relied on as an always available immutable field. This would usually be a field where it is a simple date based expiry though this doesn’t have to be the case (i.e. some product where the expiry date comes from an oracle or with a date plus logic that can expire it earlier.)

In GitLab by @tamlyn10 on May 20, 2019, 19:39

@barnabee

thanks, yes the cash/asset flow logic is what i meant
agree not useful for grouping
yeah, as i mentioned, expiry may be event driven, not time driven. I didn't think this would be useful either, though all products on Vega will have some sense of lifecycle / expiry, even if that is instant (e.g. for a physical spot exchange).

All in all, I agree with you about not being sure about grouping by immutable values. I defer to your judgement on alternatives that don't rely on such things.

In GitLab by @ashleyvega on May 21, 2019, 09:33

Perhaps we can settle on the length of the MarketID (20 bytes?), then the function can be hash(all market ID fields + nonce) for now, and can always be changed later.

Then I can get on with #254.

In GitLab by @cdm on May 21, 2019, 09:59

< All > my recent focus for this has been on optimising the start of the key to be using immutable fields for key grouping and optimised look up on a node, however it seems like for the short term future we can focus on creating a deterministic ID which is an easier task as we can do something like hash(proto-binary-marshall + sequence-number)

Great conversation for the future of market IDs when they are proposed - thank you everyone :muscle_tone1:

So for nicenet we're just loading these from json config files at start up time, so we will manually insert a 20 byte or similar unique ID in the ID fields in the json config (with URI safe chars) and we can get on with the core work.

In GitLab by @ashleyvega on May 22, 2019, 16:23

created branch 286-give-market-objects-a-canonical-id to address this issue

In GitLab by @ashleyvega on May 22, 2019, 16:23

assigned to @ashleyvega

In GitLab by @ashleyvega on May 22, 2019, 18:29

mentioned in commit 7c7d190c11c35df7a60b92513895d8f7d5a295ac

In GitLab by @ashleyvega on May 22, 2019, 18:31

@cdm @jeremyletang @EVODelavega Here's the first version of MarketID: 7c7d190c. Comments, please.

In GitLab by @cdm on May 23, 2019, 08:47

Hi @ashleyvega I'm not sure if this is the correct approach for nicenet... I suggested we seed the json config file for each market manually for nicenet. This is easy to do and does not require throw away code.

So for nicenet we're just loading these from json config files at start up time, so we will manually insert a 20 byte or similar unique ID in the ID fields in the json config (with URI safe chars) and we can get on with the core work.

In GitLab by @ashleyvega on May 23, 2019, 09:28

@cdm Thanks for the comments. I was going by:

for the short term future we can focus on creating a deterministic ID which is an easier task as we can do something like hash(proto-binary-marshall + sequence-number)

In GitLab by @cdm on May 23, 2019, 09:30

Sorry I can see how this could be a bit confusing, I should have said short term future post nicenet so when we have some way to programmatically create markets via tx msg

In GitLab by @ashleyvega on May 23, 2019, 10:06

No worries. What's the plan for now though? This code works, and the fn contents can be updated whenever needed.

In GitLab by @cdm on May 23, 2019, 11:19

Let's keep your code for now, just reviewed with @jeremyletang also

In GitLab by @ashleyvega on May 24, 2019, 08:18

mentioned in commit 692931d2b0c15766b092813ab47e03d38814c2b2

In GitLab by @ashleyvega on May 24, 2019, 08:20

mentioned in merge request !225

In GitLab by @ashleyvega on May 24, 2019, 10:57

mentioned in commit 92ff41f4e6c1f0e93b300f5a939301add2fa71b7

In GitLab by @jeremyletang on May 24, 2019, 11:08

closed via merge request !225

vegaprotocol / vega