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

Store price in fill_order_operation, for #449 #455

Merged
merged 7 commits into from
Nov 11, 2017

Conversation

abitmore
Copy link
Member

@abitmore abitmore commented Nov 2, 2017

Trying to fix #449. Needs testing.

@abitmore
Copy link
Member Author

abitmore commented Nov 9, 2017

I'm going to refactor get_ticker in order to improve performance a bit. After it's done, will start a new discussion about further improvements.

@svk31
Copy link
Contributor

svk31 commented Nov 9, 2017

Trying to test this today, but I recently upgraded Ubuntu to 17 and now I'm having a hard time compiling Bitshares again.

@abitmore
Copy link
Member Author

abitmore commented Nov 9, 2017

@svk31: @oxarbitrage once said he has an API node for you to test this.

@abitmore
Copy link
Member Author

abitmore commented Nov 9, 2017

Found a bug about swapping base and quote. Fixing.

Also,
* avoids a pagination issue exists in the original implementation
* fixed a price flipping bug in get_trade_history API
* add boundary check when looking for existing key
* catch and log potential exception
@oxarbitrage
Copy link
Member

sent node url with pull to sivek including last commit added today. he will be testing it tomorrow.

@svk31
Copy link
Contributor

svk31 commented Nov 10, 2017

I've tested the GUI using oxarbitrage's node and it's certainly a big improvement. If the performance of your solution is acceptable then it would be great to include this in the next release.

Here's an example in the CNY:USD market, at 1m zoom and 1d candles

Before:
image

After:
image

@abitmore
Copy link
Member Author

The peak price on the latter image is 7.57 CNY/USD recorded at 2017-11-02 08:12:42 UTC.

On block #21437324, bts-forever placed an order: wants 5,098 USD for 38,592 CNY with price 7.57, after most of it got filled as a taker, a dust of 0.0013 CNY was left on the market, then the dust got filled on block 21437325 as a maker, so the price got recorded as intended.

Before the fix, price of that order will be shown as 0.0013 CNY / 0.0001 USD = 13 CNY/USD.

Performance would be improved after #454 is done.

@oxarbitrage oxarbitrage self-requested a review November 11, 2017 00:38
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

with the improves that @svk31 reported in the charting and the new api calls this round up very good job, for the record here are the new calls working:

get_ticker now returning time(maybe add this also to get_24_volume?):

root@alfredo:~# curl --data '{"jsonrpc": "2.0", "params": ["database", "get_ticker", ["1.3.0", "1.3.112"]], "method": "call", "id": 10}' --silent http://localhost:8090/rpc | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "result": {
    "time": "2017-11-11T00:18:00",
    "base": "1.3.0",
    "quote": "1.3.112",
    "latest": "11.61649333333333267",
    "lowest_ask": "0.00000000000000000",
    "highest_bid": "10.04063558017851499",
    "percent_change": "0.00000000000000000",
    "base_volume": "0.00000000000000000",
    "quote_volume": "0.00000000000000000"
  }
}
root@alfredo:~# 

get_trade_history returning seq number:

root@alfredo:~# curl --data '{"jsonrpc": "2.0", "params": ["database", "get_trade_history", ["1.3.0", "1.3.113", "2017-11-10T10:00:00", "2017-11-10T02:00:00", 2]
], "method": "call", "id": 10}' --silent http://localhost:8090/rpc | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "result": [
    {
      "sequence": 1575537,
      "date": "2017-11-10T09:57:36",
      "price": "1.84167061892710127",
      "amount": "1664.17390000000000327",
      "value": "3064.86016999999992549",
      "side1_account_id": "1.2.411864",
      "side2_account_id": "1.2.422840"
    },
    {
      "sequence": 1575535,
      "date": "2017-11-10T09:57:30",
      "price": "1.84167061892710127",
      "amount": "17.34659999999999869",
      "value": "31.94672999999999874",
      "side1_account_id": "1.2.411864",
      "side2_account_id": "1.2.167208"
    }
  ]
}
root@alfredo:~# 

new call get_trade_history_by_sequence:

root@alfredo:~# curl --data '{"jsonrpc": "2.0", "params": ["database", "get_trade_history_by_sequence", ["1.3.0", "1.3.113", 1575538, "2017-11-10T02:00:00", 2]],
 "method": "call", "id": 10}' --silent http://localhost:8090/rpc | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "result": [
    {
      "sequence": 1575537,
      "date": "2017-11-10T09:57:36",
      "price": "1.84167061892710127",
      "amount": "1664.17390000000000327",
      "value": "3064.86016999999992549",
      "side1_account_id": "1.2.411864",
      "side2_account_id": "1.2.422840"
    },
    {
      "sequence": 1575535,
      "date": "2017-11-10T09:57:30",
      "price": "1.84167061892710127",
      "amount": "17.34659999999999869",
      "value": "31.94672999999999874",
      "side1_account_id": "1.2.411864",
      "side2_account_id": "1.2.167208"
    }
  ]
}
root@alfredo:~# 

@abitmore
Copy link
Member Author

get_ticker now returning time(maybe add this also to get_24_volume?):

Good idea. Will do.

@abitmore
Copy link
Member Author

Apparently the amount and value data returned by get_trade_history and get_trade_history_by_sequence are usually inaccurate due to #144, also price is not graceful. Will fix them in another PR.

@abitmore abitmore merged commit 70c2c83 into bitshares:develop Nov 11, 2017
@abitmore
Copy link
Member Author

Merged so far. If there is anything please submit a new issue.

@abitmore abitmore added this to the Next Non-Consensus-Changing Release - 201712 milestone Nov 28, 2017
@landry314
Copy link

landry314 commented Feb 1, 2018

The new update in the GUI has a much WORSE dust-trade-wick issue than the last release. Is there any progress? This is a huge downfall in the BitShares GUI. I wish it could be fixed in the GUI but @svk31 says it can not.

@oxarbitrage
Copy link
Member

please check #563

@svk31
Copy link
Contributor

svk31 commented Feb 2, 2018

Nothing has changed in the GUI that would affect this. Most likely you've connected to an API node that does not contain the recent fixes. The Bitshares Infrastructure nodes use the recent witness_node versions and have much better data for the charts.

@abitmore abitmore mentioned this pull request Feb 2, 2018
@abitmore abitmore deleted the 449-fill-order-price branch June 28, 2021 21:59
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.

4 participants