Implement BSIP 74: Margin Call Fee Ratio #2180

MichelSantos · 2020-05-25T23:51:38Z

An implementation of BSIP74 for #2129

Co-authored by John Jones

Co-authored by John Jones Co-authored by Michel Santos

MichelSantos · 2020-05-25T23:55:38Z

tests/tests/margin_call_fee_tests.cpp

+         fill_order_operation alice_fill_op = histories.front().op.get<fill_order_operation>();
+         BOOST_CHECK(alice_fill_op.fee == asset(0));
+         // Alice's fill order's fill price should equal the expected match price
+         BOOST_CHECK(~alice_fill_op.fill_price == expected_match_price);


@abitmore What do you think should be the fill_price recorded in fill_order_operation under these different maker and taker scenarios? I'm particularly curious about the two tests where target collateral ratio (TCR) are active.

I think it's fine to record the match_price. It's used to generate price charts.
Actually I didn't get what your concerns were.

christophersanborn · 2020-05-26T00:56:51Z

libraries/chain/db_market.cpp

+             limit_receives = usd_to_buy.multiply_and_round_up( match_price ); // round up, in favor of limit order
+             call_pays      = usd_to_buy.multiply_and_round_up( call_pays_price ); // BSIP74; excess is fee.
+                                              // Note: TODO: Due to different rounding, couldn't this potentialy be
+                                              // one satoshi more than the blackswan check above? Can this bite us?


@abitmore — Wanting to call attention to this question. It's not a new issue — old code uses multiply_and_round_up seemingly without problem — but since we do a black swan check above at line 1226 using multiply-and-round-down, is there a possibility that with the exact right price feed we could end up with call_pays being one satoshi more than the collateral in the call order? I expect this would eventually cause an exception to be thrown, perhaps preventing a feed_update_operation or other operation that can move the call order price from confirming?

Thanks for the question.

As you've noticed, the blackswan check above at line 1226 is only for pre-hf-1270 (or even earlier, pre-hf-338) scenarios.

After hf-1270, the effective check is the check_black_swan() call at line 1187:

bitshares-core/libraries/chain/db_market.cpp

Line 1187 in 11aa1be

while( !check_for_blackswan( mia, enable_black_swan, &bitasset ) // TODO perhaps improve performance by passing in iterators

The code inside check_black_swan() reads:

bitshares-core/libraries/chain/db_update.cpp

Line 224 in ed6b6d8

highest = bitasset.current_feed.max_short_squeeze_price();

bitshares-core/libraries/chain/db_update.cpp

Lines 247 to 251 in ed6b6d8

auto least_collateral = call_ptr->collateralization();

if( ~least_collateral >= highest )

{

wdump( (*call_ptr) );

elog( "Black Swan detected on asset ${symbol} (${id}) at block ${b}: \n"

I think it guarantees that the rounding-up here won't make the call order pay more collateral than it has.

abitmore

Thanks. The code looks mostly good, except a few minor issues. I haven't reviewed test cases so far, will do later.

abitmore · 2020-05-26T12:41:29Z

libraries/chain/db_market.cpp

+            call_pays_price = call_match_price;
+         } else {
+            call_match_price = ~sell_abd->current_feed.
+               margin_call_order_price(sell_abd->options.extensions.value.margin_call_fee_ratio);


Potential performance improvement to do in the future: save the prices to the bitasset_data object when feeds or MCFR changed.

libraries/chain/db_market.cpp

abitmore · 2020-05-26T13:20:55Z

libraries/chain/db_market.cpp

+             limit_receives = usd_to_buy.multiply_and_round_up( match_price ); // round up, in favor of limit order
+             call_pays      = usd_to_buy.multiply_and_round_up( call_pays_price ); // BSIP74; excess is fee.
+                                              // Note: TODO: Due to different rounding, couldn't this potentialy be
+                                              // one satoshi more than the blackswan check above? Can this bite us?


Thanks for the question.

As you've noticed, the blackswan check above at line 1226 is only for pre-hf-1270 (or even earlier, pre-hf-338) scenarios.

After hf-1270, the effective check is the check_black_swan() call at line 1187:

bitshares-core/libraries/chain/db_market.cpp

Line 1187 in 11aa1be

while( !check_for_blackswan( mia, enable_black_swan, &bitasset ) // TODO perhaps improve performance by passing in iterators

The code inside check_black_swan() reads:

bitshares-core/libraries/chain/db_update.cpp

Line 224 in ed6b6d8

highest = bitasset.current_feed.max_short_squeeze_price();

bitshares-core/libraries/chain/db_update.cpp

Lines 247 to 251 in ed6b6d8

auto least_collateral = call_ptr->collateralization();

if( ~least_collateral >= highest )

{

wdump( (*call_ptr) );

elog( "Black Swan detected on asset ${symbol} (${id}) at block ${b}: \n"

I think it guarantees that the rounding-up here won't make the call order pay more collateral than it has.

libraries/chain/db_market.cpp

abitmore · 2020-05-26T14:08:21Z

libraries/chain/include/graphene/chain/database.hpp

         bool fill_call_order( const call_order_object& order, const asset& pays, const asset& receives,
-                               const price& fill_price, const bool is_maker );
+                               const price& fill_price, const bool is_maker, const asset& margin_fee = asset(0) );


The default value changes old data.

The old code stores asset(0,pays.asset_id) to the fee field of the virtual fill_order_operation, but the new code will store asset(0,asset_id_type(0)) which could have a different asset ID.

bitshares-core/libraries/chain/db_market.cpp

Lines 923 to 924 in ed6b6d8

push_applied_operation( fill_order_operation( order.id, order.borrower, pays, receives,

asset(0, pays.asset_id), fill_price, is_maker ) );

bitshares-core/libraries/chain/db_market.cpp

Lines 1003 to 1004 in 11aa1be

push_applied_operation( fill_order_operation( order.id, order.borrower, pays, receives,

margin_call_fee, fill_price, is_maker ) );

libraries/protocol/asset_ops.cpp

abitmore

The test cases are mostly fine.

IMO we need a test case for scenario:

asset owner reducing MCFR triggers new matching and filling of margin calls and limit orders.

tests/tests/margin_call_fee_tests.cpp

abitmore · 2020-05-26T15:17:59Z

tests/tests/margin_call_fee_tests.cpp

+         fill_order_operation alice_fill_op = histories.front().op.get<fill_order_operation>();
+         BOOST_CHECK(alice_fill_op.fee == asset(0));
+         // Alice's fill order's fill price should equal the expected match price
+         BOOST_CHECK(~alice_fill_op.fill_price == expected_match_price);


I think it's fine to record the match_price. It's used to generate price charts.
Actually I didn't get what your concerns were.

tests/tests/margin_call_fee_tests.cpp

abitmore

The new test case looks good. Thanks.

tests/tests/margin_call_fee_tests.cpp

abitmore

Thanks.
Code looks good. Waiting for CI results.

jmjatlanta and others added 6 commits May 25, 2020 09:55

BSIP74: Hardfork activation time

6b61025

BSIP74: Objects

363d003

Co-authored by John Jones

BSIP74: Operations

f7ea982

BSIP74: Operation evaluators

becea4d

Co-authored by John Jones

BSIP74: Database logic

d1b5624

Co-authored by John Jones Co-authored by Michel Santos

BSIP74: Tests

11aa1be

MichelSantos commented May 25, 2020

View reviewed changes

christophersanborn reviewed May 26, 2020

View reviewed changes

abitmore added the hardfork label May 26, 2020

abitmore added this to the 4.0.0 - Protocol Upgrade Release milestone May 26, 2020

abitmore reviewed May 26, 2020

View reviewed changes

abitmore mentioned this pull request May 26, 2020

Implement BSIP 74: Margin Call Fee Ratio #2130

Closed

3 tasks

MichelSantos force-pushed the pr-bsip74 branch 2 times, most recently from 516cd33 to d90e835 Compare May 27, 2020 04:14

abitmore reviewed May 27, 2020

View reviewed changes

tests/tests/margin_call_fee_tests.cpp Show resolved Hide resolved

tests/tests/margin_call_fee_tests.cpp Outdated Show resolved Hide resolved

tests/tests/margin_call_fee_tests.cpp Outdated Show resolved Hide resolved

MichelSantos force-pushed the pr-bsip74 branch from d90e835 to a19fd10 Compare May 27, 2020 14:04

abitmore reviewed May 27, 2020

View reviewed changes

tests/tests/margin_call_fee_tests.cpp Outdated Show resolved Hide resolved

BSIP74: Tests updated for code review

76e8818

MichelSantos force-pushed the pr-bsip74 branch from a19fd10 to 76e8818 Compare May 27, 2020 17:52

BSIP74: Corrections requested in code review.

620b1af

abitmore approved these changes May 27, 2020

View reviewed changes

abitmore merged commit 5323174 into hardfork May 28, 2020

MichelSantos deleted the pr-bsip74 branch May 28, 2020 00:25

MichelSantos mentioned this pull request May 28, 2020

Release Notes: BitShares Core 4.0.0 #2026

Closed

abitmore mentioned this pull request May 29, 2020

Implement BSIP 74: Margin Call Fee Ratio #2129

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BSIP 74: Margin Call Fee Ratio #2180

Implement BSIP 74: Margin Call Fee Ratio #2180

MichelSantos commented May 25, 2020

MichelSantos May 25, 2020 •

edited

Loading

abitmore May 26, 2020

christophersanborn May 26, 2020 •

edited

Loading

abitmore May 26, 2020

abitmore left a comment

abitmore May 26, 2020

abitmore May 26, 2020

abitmore May 26, 2020

abitmore left a comment

abitmore May 26, 2020

abitmore left a comment

abitmore left a comment

	auto least_collateral = call_ptr->collateralization();
	if( ~least_collateral >= highest )
	{
	wdump( (*call_ptr) );
	elog( "Black Swan detected on asset ${symbol} (${id}) at block ${b}: \n"

	push_applied_operation( fill_order_operation( order.id, order.borrower, pays, receives,
	asset(0, pays.asset_id), fill_price, is_maker ) );

	push_applied_operation( fill_order_operation( order.id, order.borrower, pays, receives,
	margin_call_fee, fill_price, is_maker ) );

Implement BSIP 74: Margin Call Fee Ratio #2180

Implement BSIP 74: Margin Call Fee Ratio #2180

Conversation

MichelSantos commented May 25, 2020

MichelSantos May 25, 2020 • edited Loading

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

christophersanborn May 26, 2020 • edited Loading

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

abitmore May 26, 2020

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

MichelSantos May 25, 2020 •

edited

Loading

christophersanborn May 26, 2020 •

edited

Loading