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

Market engine improvements #641

Closed
wants to merge 50 commits into from

Conversation

Projects
3 participants

abitmore added some commits Feb 3, 2018

Market engine improvements.
For:
* #338 Margin call order fills at price of matching limit_order
* #343 Inconsistent sorting of call orders between matching against a limit order and a force settle order
* #453 Multiple limit order and call order matching issue
* #606 Undercollateralized short positions should be called regardless of asks

@abitmore abitmore added the hardfork label Feb 5, 2018

@abitmore abitmore requested a review from pmconrad Feb 5, 2018

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2018

Linking a forum post (in Chinese) here: https://bitsharestalk.org/index.php/topic,25926.0.html

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2018

This (the fix for #343) should fix #649 as well.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2018

Since we need to process all calls at hard fork time, it may take some time, so best to do it at a maintenance interval.

Then we need to activate the new logic only after processed all calls, which means we need to postpone the activation of new logic to after next maintenance interval if hard fork time is set between 2 maintenance intervals (best set like this to avoid unintended forking on maintenance due to block misses).

Then we need a variable in the chain state to indicate activation of new logic but not use the HARDFORK_* macros directly. Perhaps it's time to introduce a versioning mechanism.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2018

Looks like we can use these two variables for the hard fork time check:

get_dynamic_global_properties
{
  ...
  "next_maintenance_time": "2018-02-07T18:00:00",
  "last_budget_time": "2018-02-07T17:00:00",
  ...
}

@abitmore abitmore changed the title Work in progress on market engine issues Market engine improvements Feb 8, 2018

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2018

Update:

  • Hard fork will occur at the maintenance interval before or at scheduled time
  • Fixed black swan event checking and global settlement price calculation
  • Added unit tests

All dev jobs are done. @pmconrad and other devs please review. Thanks.

@abitmore abitmore added this to the 201803 - Consensus Changing Release milestone Feb 8, 2018

@abitmore abitmore moved this from In progress to Pending review/testing in 201806 Protocol Upgrade Release (Hard Fork) Feb 9, 2018

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

I think this is ready for review (BSIP 30-35). In the meanwhile I'll finish the missing test cases.
@pmconrad @xeroc @oxarbitrage @jmjatlanta @ryanRfox

}

auto least_collateral = call_itr->collateralization();
if( ~least_collateral >= highest )
{
wdump( (*call_itr) );
elog( "Black Swan detected: \n"

This comment has been minimized.

Copy link
@xeroc

xeroc Apr 6, 2018

Member

Can we add the symbol of the asset that black-swanned? i remember that this message doesn't tell

This comment has been minimized.

Copy link
@abitmore

abitmore Apr 6, 2018

Author Member

Good idea. Will do.

@xeroc

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

I very much like the clear distinction between filling limit, margin, and settle orders.
Good job - Will try do a review asap.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2018

Good job.
I have a bad feeling though, because this PR has become HUGE.

Would it be possible to cherry-pick this into several smaller PRs that can be reviewed independently (but serially, because there's a lot of overlapping code)?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

@pmconrad yes, I'll try. Please check #827 first, it's fix for #672 which is relatively independent.

It's related to BSIP30: Always Allow Increasing Collateral Ratio If Debt Not Increased.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

@pmconrad seems it's a bit hard to cherry-pick other commits due to conflicts introduced by #697, actually need to solve conflict when cherry-picking the first commit. Please leave a comment if have an idea.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

@pmconrad please check #829 for stuff related to BSIP31-34 (#338 #343 #453 #606 #625 #649).

I will create a new branch for BSIP35 containing remaining commits in this PR, then submit a new PR.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

Created a new PR #830 for stuff related to BSIP35. Closing this one.

@abitmore abitmore closed this Apr 7, 2018

201806 Protocol Upgrade Release (Hard Fork) automation moved this from In progress to Done Apr 7, 2018

@abitmore abitmore removed their assignment Apr 7, 2018

@abitmore abitmore deleted the 201803-market-engine branch Apr 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.