Skip to content

Draft vision of market events model#1636

Open
OpenLedgerApp wants to merge 1 commit intobitshares:developfrom
AccelBooks:issue-1277
Open

Draft vision of market events model#1636
OpenLedgerApp wants to merge 1 commit intobitshares:developfrom
AccelBooks:issue-1277

Conversation

@OpenLedgerApp
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

  1. Please let's discuss the vision first, then start coding.
  2. This PR doesn't solve #1277. instead, it creates a new API that may or may not be better than what we have so far.
  3. You can't just add fields to existing operations. That's a consensus change. The only reason why this doesn't break is that the field isn't serialized. Not serializing the field means it gets lost during transmission.

@OpenLedgerApp
Copy link
Copy Markdown
Contributor Author

1. Please let's discuss the vision first, then start coding.

There is an invitation to discussion: #1277 (comment). Unfortunately there are no comments.

 2. This PR doesn't solve #1277. instead, it creates a new API that may or may not be better than what we have so far.

This API is based on operations events and it is more logically and obviously. All market events could be catched in the right order.
Current API, that is in Bitshares, is based on operations events and object events. And this approach is a main cause #1277 (no events + wrong order)

 3. You can't just add fields to existing operations. That's a consensus change. The only reason why this doesn't break is that the field isn't serialized. Not serializing the field means it gets lost during transmission.

This field is used only for market events and it is not necessary to serialize it.

@ghost
Copy link
Copy Markdown

ghost commented Mar 6, 2019

@OpenLedgerApp Could you explain what parse_market_operations is supposed to do?

@OpenLedgerApp
Copy link
Copy Markdown
Contributor Author

@OpenLedgerApp Could you explain what parse_market_operations is supposed to do?

It gathers market operations, fills events queue and notifies market listeners.
The name of this functions should be more intuitive.

@pmconrad
Copy link
Copy Markdown
Contributor

pmconrad commented Mar 7, 2019

I see no API additions being proposed in the issue. A draft implementation without a verbal description of what it does and how it solves the problem at hand is hardly a base for discussion.

Adding a new API that fulfills client expectations in a better way may be a good move, still it doesn't solve the problem for the existing API and requires all clients to switch to the new API.

Files in the protocol subdirectories define the core protocol. They are not to be abused for implementation details, like ordering stuff the way you want. Serialization is required for dumping the database to disk and loading it upon restart, btw..

@ghost
Copy link
Copy Markdown

ghost commented Mar 7, 2019

@OpenLedgerApp Isn't this the current behavior already? Can you explain what this function does differently from the current implementation?

@OpenLedgerApp
Copy link
Copy Markdown
Contributor Author

Currently, notifications of market events come from two different sources. It is formed by filtering and mixing events from block_database (events: virtual fill_order_operation) and object_database (events: create / update / delete call_order_object / limit_order_object / force_settlement_object).

As for the non-deterministic behavior described here. As you know, order_object is created in object_database only if order matching did not occur in this block and you need to write the makers order to the database to wait for the next order takers. Two identical operations (limit_order_create_operation) can lead to different results of the API, if these operations are in the same block, or in different. In the current API version, it is considered normal if you see fill_order, fill_order, and only then create_order, or create_order is missing. You can also see the following sequence: fill_order, create_order, fill_order. These are all variants of normal behavior, events occur exactly in this sequence.

From the discussion here we found out the essence of the problem.
Expected behavior: IMHO the correct order of events should be: order creation(alice), order creation(bob), fill_order(alice), fill_order(bob), no matter if alice's order or bob's has been fully filled or both.

In this PR, we presented our vision of this API, which should eliminate misunderstanding on the part of client-side developers. For convenience, we completely saved the interface, left a similar function name subscribe_to_market_events, but (!) the event source is now one - block_database (events: limit_order_create_operation, virtual fill_order_operation, limit_order_cancel_operation). Using this api, you can see only user-initiated operations: create / cancel order and also virtual fill order order for matched orders. This eliminates confusion in understanding the principles of the two different in nature databases: block and object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants