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

[ES plugin] Wrong value of additional_data.fill_data.fill_order #1295

Closed
Zapata opened this Issue Aug 29, 2018 · 8 comments

Comments

Projects
5 participants
@Zapata

Zapata commented Aug 29, 2018

Bug Description

Value of additional_data.fill_data.fill_order is equal on both ways of a filled order and do not take asset precision into account:

USD -> BTS:

      {
        "_source": {
          "operation_history": {
            "op": """[4,{"fee":{"amount":75,"asset_id":"1.3.121"},"order_id":"1.7.183476663","account_id":"1.2.1034686","pays":{"amount":7714442,"asset_id":"1.3.0"},"receives":{"amount":75708,"asset_id":"1.3.121"},"fill_price":{"base":{"amount":10000000,"asset_id":"1.3.0"},"quote":{"amount":98138,"asset_id":"1.3.121"}},"is_maker":true}]"""
          },
          "operation_type": 4,
          "block_data": {
            "block_num": 29969255,
            "block_time": "2018-08-27T04:25:27",
            "trx_id": "a642253ffd3c2601164086df9004ae06bc85c960"
          },
          "additional_data": {
            "fill_data": {
              "order_id": "1.7.183476663",
              "account_id": "1.2.1034686",
              "pays_asset_id": "1.3.0",
              "pays_amount": 7714442,
              "receives_asset_id": "1.3.121",
              "receives_amount": 75708,
              "fill_price": "101.89732825205322797",
              "is_maker": true
            }
          }
        }
      },

BTS -> USD:

        "_source": {
          "operation_history": {
            "op": """[4,{"fee":{"amount":0,"asset_id":"1.3.0"},"order_id":"1.7.183701401","account_id":"1.2.162629","pays":{"amount":35721,"asset_id":"1.3.121"},"receives":{"amount":3624847,"asset_id":"1.3.0"},"fill_price":{"base":{"amount":3624847,"asset_id":"1.3.0"},"quote":{"amount":35721,"asset_id":"1.3.121"}},"is_maker":false}]"""
          },
          "operation_type": 4,
          "block_data": {
            "block_num": 29972527,
            "block_time": "2018-08-27T07:09:30",
            "trx_id": "d623a127e6fe5ed9f205944dc312c134d288cfa3"
          },
          "additional_data": {
            "fill_data": {
              "order_id": "1.7.183701401",
              "account_id": "1.2.162629",
              "pays_asset_id": "1.3.121",
              "pays_amount": 35721,
              "receives_asset_id": "1.3.0",
              "receives_amount": 3624847,
              "fill_price": "101.47663839198230562",
              "is_maker": false
            }
          }
        }
      },

The fill_price is same in both ways of the deals, wheras pay/recieve assets are inverted.

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • [X ] Other (please add below)

Elastic Search database content.
It is not trivial to compute an average price from filed orders using an Elastic Search query.
Stats displayed in Kibana or Open Explorer might be wrong.

Steps To Reproduce

  • Setup an Elastic Search service
  • Run witness node using Elastic Search plugin and elasticsearch-visitor=true
  • Check filled orders:
GET /bitshares-*/data/_search?pretty=true
{
  "size": 10,  
  "query": {
    "constant_score": {
      "filter": {
        "bool": {
          "must": [
            { "term": { "operation_type": "4" } },
            { "term": { "additional_data.fill_data.pays_asset_id": "1.3.121" } },
            { "term": { "additional_data.fill_data.receives_asset_id": "1.3.0" } },
            { "range": { 
                "block_data.block_time": { 
                  "gte": "2018-08-27T00:00:00", 
                  "lte": "2018-08-28T00:00:00"
                } 
              } 
            }
          ]
        }
      }
    }
  }
}

Expected Behavior

additional_data.fill_data.fill_order should be (recieves_asset_amount/recieves_asset_precision) / (pays_asset_amount / pays_asset_precision).

CORE TEAM TASK LIST

  • Evaluate / Prioritize Bug Report
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@clockworkgr

This comment has been minimized.

Member

clockworkgr commented Aug 29, 2018

Ah..another one for the base/quote design :P

If you look at the original op, you'll see that it too keeps price at a preferred base/quote order:

fill_price":{"base":{"amount":10000000,"asset_id":"1.3.0"},"quote":{"amount":98138,"asset_id":"1.3.121"}}

and

"fill_price":{"base":{"amount":3624847,"asset_id":"1.3.0"},"quote":{"amount":35721,"asset_id":"1.3.121"}}

So the plugin is simply copying the core representation...

Dont ask me why though

@ryanRfox

This comment has been minimized.

Member

ryanRfox commented Sep 7, 2018

@oxarbitrage I see you added this to the release milestone, but did not self assign. Are you actively working this one? I've added it to the Project Board as well to match. If this is in error, please move it Unassigned - Bug on the Project Backlog. thanks

@oxarbitrage oxarbitrage self-assigned this Sep 7, 2018

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 7, 2018

assigned to myself, thanks.

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 14, 2018

@oxarbitrage we need to decide whether we will fix this in 201810 release.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 30, 2018

I was thinking on pros and cons about adding this feature and decided to add it by the following:

pros:

  • will allow better kibana visualizations.
  • will allow to get better results with 1 single ES query.
  • adding additional data in new fields will not break current applications.

In regards to current applications, ES plugin is currently being used by more third parties than i was expecting including openledger, payger, cyptobridge and others so need to be careful when making changes.

cons:

  • it will make the replay slower.
  • additional data will be probably deprecated in the future in favour of indexing all the fields.

In regards to deprecation i have an elasticsearch plugin version from Dascoin that allows to index all the fields from all the operations. I am porting and testing this now, this will eventually deprecate the additional data we have now but at first plugin will support both options to remain compatible.

@abitmore abitmore moved this from To do to In progress in Feature release (201810) Oct 3, 2018

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 3, 2018

closed by #1351

@oxarbitrage oxarbitrage closed this Oct 3, 2018

Feature release (201810) automation moved this from In progress to Done Oct 3, 2018

@Zapata

This comment has been minimized.

Zapata commented Oct 3, 2018

@oxarbitrage Could you put an example of the new output for reference?

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 4, 2018

Yes, sorry @Zapata , i closed it too fast. We can reopen if you think is needed.

The code in the pull request will add some additional stuff to the additional data :)

For fee data we have the fee amount calculated with precision and the asset name on what the fee was paid:

1295_1

For transfer data we added the transfer amount in units and the asset name:

1295_2

For fill orders we added pays_asset_name, pays_amount_units, receives_asset_name, receives_amount_units and fill_price_units:

1295_3

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