Implement BSIP 74: Margin Call Fee Ratio #2130

jmjatlanta · 2020-04-04T21:45:24Z

Implements a margin call fee per BSIP 74 specifications

To Test:

hardfork param setting
assure fee is only charged when call order is margin called
assure no other tests affected

abitmore · 2020-04-10T14:51:31Z

Travis build failed.
The branch has conflicts to the base branch.

jmjatlanta · 2020-04-10T14:59:18Z

* Travis build failed.

* The branch has conflicts to the base branch.

Conflicts don't show on command line, so not sure what's going on there. I will research the Travis difficulties.

NOTE: I'd like to see BSIP 86 merged in first, as I'd like to use some of those changes in this. Also, there will be conflicts after the BSIP 86 merge as the same code was touched.

abitmore · 2020-04-10T15:07:28Z

I've merged bsip86 just now.

jmjatlanta · 2020-04-11T13:44:20Z

Note: The dockercloud failure looks to be a timeout. The build ran for 116 minutes, and then stopped with no real error message.

Scanning dependencies of target graphene_witness
[ 95%] Building CXX object libraries/plugins/witness/CMakeFiles/graphene_witness.dir/witness.cpp.o
Build canceled.

libraries/chain/db_market.cpp

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

tests/tests/bitasset_tests.cpp

abitmore · 2020-04-17T14:00:16Z

The conflicts need to be fixed.

abitmore

I stopped review so far due to the mismatch between the implementation and the specification.

libraries/chain/db_market.cpp

libraries/chain/asset_evaluator.cpp

libraries/chain/db_market.cpp

abitmore · 2020-04-17T14:35:38Z

libraries/chain/db_market.cpp

+         || *trade_asset.options.extensions.value.margin_call_fee_ratio == 0 )
+      return asset(0, trade_amount.asset_id);
+
+   auto value = detail::calculate_percent(trade_amount.amount, *trade_asset.options.extensions.value.margin_call_fee_ratio);


This seems different from the specification.

The problem I have is clarifying the specifications. I apologize for the lengthy post, but it is how my mind works. The BSIP says:

When a margin call trading happens, the buyer sells smartcoin with quantity X and get collaterals in quantity X*(MSSR-MCFR)/settlement price, the margin call order owner sells collaterals in quantity X*MSSR/settlement price and get smartcoin in quantity X, the delta in paid and received collaterals in quantity X*MCFR/settlement price will be paid to the owner of the smartcoin as margin call fee.

My interpretation (which I will attempt to keep updated within this comment, and probably move to my blog to help me in the future):

Doug the Debtor put up collateral of token C which created token D. Doug was charged an operation fee I think, although I am not sure but it is unimportant. We will assume Doug then sold or transferred D ( also unimportant in this context).

Larry the creator of a Limit order would like to sell the desired quantity of token D and receive the desired quantity of token C. Upon placing the order, Larry was charged a limit order create fee, which is not important in this context.

When the debt position was created, a call order was created for Doug the debtor. Within that call order is the current debt and collateral balances.

When the settlement price of the price feed reaches a certain level, Doug's position will get called. Such a margin call will generate revenue for the asset issuer, if the issuer has set the new margin_call_fee_ratio within that asset's options.

This fee does not affect Larry. He simply placed a limit order on the book of the exchange.

Doug will see the fee if he compares the amount of collateral he received from the transaction with the amount of debt that was paid off by the margin call.

Now the more technical details:

When will these orders be matched? When the settlement price from the price feed pushes the value of Doug's collateral below the allowed threshold for the debt asset (see db_market.cpp#database::check_call_orders).

What will Larry receive? Exactly what he wished in his limit order.

What will Doug receive? Relief from (some or all of) his debt, and some of his collateral back. He will not receive the full value of his collateral, based on the feed's settlement price. Some of the value of the transaction was paid to the issuer of the debt asset as a margin call fee.

Implementation details:

database::check_call_orders (db_market.cpp) knows when to trigger a margin call. When triggered, fill_call_order is called, and then fill_limit_order is called.

Within fill_call_order the fee is taken from the debtor's debt position and given to the asset issuer as a vesting balance. The remaining value of the debt is used to calculate the collateral that the debtor receives. That collateral is then placed in the debtor's available account balance.

In looking at the spec, I believe that I need to adjust the "trigger" of the margin call. The spec reads:

Margin call order price = settlement price/(MSSR-MCFR)

And that is part of check_call_orders

Update: No, this only affects the debtor, not when the margin call happens.

I may have written something wrong in previous comments. Let's forget them and start here.

With bsip74,

condition when a call order is possible to be matched does not change, it's still call_price >= feed_price;

condition where (at what price, aka match_price) the call order be matched does change,

previously, if a limit order buys at feed_price / mssr, it will match the call order,

now, if a limit order buys at feed_price / (mssr - mcfr), it will match the call order,

when matched, the limit order gets collateral (be filled) at feed_price / (mssr - mcfr), the call order pays (be filled) at feed_price / mssr, the difference goes to the debt asset owner;

the call order can be a maker or a taker, there is a small difference in match_price, iirc maker and taker are handled in different functions.

One important thing, the fee is some amount of the collateral asset, so it can not go to the accumulated_fees field of the debt asset, which means we likely can not reuse the code of pay_market_fees. It can not go to the accumulated_fees field of the collateral asset either, since the asset can be owned by someone else.

In addition, a complex scenario is the debt asset can change from one collateral to another, so we need to store the fees in a multi-asset container. Adding new fields into asset_object or another object means we need to add new operations to claim them, that's too much work to implement.

Another thing in mind: mssr-mcfr in the specification, and we don't allow something for nonthing, so it's implied that mssr>mcfr. However, mssr is updated by price feed producers and an internal timer, mcfr is updated by asset owner, so likely we can't validate when one of them is updated. So we need to cap the value of mcfr when use it in calculation.

@abitmore

I believe I am clear that this change modifies which limit orders qualify to be matched.

To be clear, let me walk through what I am seeing in db_market.cpp::check_call_orders. And just focusing on the price the limit order will transact at...

Prior to this change, the limit order was filled at the price dictated by the order itself:

price match_price = limit_order.sell_price;

Based on what I'm reading in your comment above, after the hardfork, the limit order will transact at a price based on feed price, MSSR and MCFR. That will result in the limit order receiving a price potentially very different than their order.

Or it could be that I am misreading your comment above, and you were only presenting the extreme example of when a limit order just happens to match right at the new calculated price for what qualifies a match, which would mean the order gets exactly what it asked for.

Or I am making assumptions in the code that are incorrect.

Would you clarify please?

Please keep in mind that match price is always price of the maker, see BSIP 32.

In check_call_orders, the call order is the taker and the limit order is the maker, so the match price will always be the price of the limit order.

In another function where the call order is the maker and the limit order is the taker, the match price will be settlement_price / (mssr-mcfr).

Thank you for the reference to BSIP 32. There are some inconsistencies in that BSIP that make it difficult to understand, but I believe the gist is as you say, when the limit order is the maker, the maker price is what is used.

So that leads me to believe that BSIP 74 applies only when the call order is the maker. Hence the margin call fee will not apply when the limit order is the maker. Is that correct?

IMO the margin call fee should always apply, although how much to pay is not clearly described in the BSIP when Peter approved the pull request.

In case when the call order is taker, I think it's fine to charge the amount filled_debt * mcfr / feed_price from the call order as fee.

libraries/chain/db_market.cpp

abitmore · 2020-04-17T22:44:50Z

libraries/chain/proposal_evaluator.cpp

@@ -52,6 +53,7 @@ struct proposal_operation_hardfork_visitor
   // hf_1774
   void operator()(const graphene::chain::asset_update_operation &v) const {
      detail::check_asset_options_hf_1774(block_time, v.new_options);
+      detail::check_asset_options_bsip74(block_time, v.new_options);


Missing code about asset_create_operation (a few lines above).

jmjatlanta · 2020-05-11T20:27:26Z

@abitmore I would appreciate a review of this. Please take special note of bitasset_tests/bsip74_insufficient_collateral_tests (around line 1956)

I am unsure if I have coded something incorrectly, or if we haven't talked about this particular case. Call order is taker, fee takes more collateral than what is left.

Meanwhile I will continue researching to attempt to see if it is my misunderstanding.

abitmore

Sorry, the code doesn't look good.

Due to the new MCFR parameter, new code should include logic changes about matching/filling price, but I didn't see code about it in this pull request.

Margin call fees should be always paid by the debt position owner, but not the limit order owner. The fee should be stored in fee field of the virtual fill_order_operation which contains the call order.

The limit order still needs to pay market fee when being matched with a call order.

I didn't review test cases.

libraries/chain/asset_object.cpp

abitmore · 2020-05-11T22:35:34Z

libraries/chain/db_market.cpp

+      a *= (ratio-GRAPHENE_COLLATERAL_RATIO_DENOM);
+      a /= GRAPHENE_COLLATERAL_RATIO_DENOM;
+      return static_cast<int64_t>(a);
+   }


The function name is misleading due to the ratio-GRAPHENE_COLLATERAL_RATIO_DENOM calculation in the implementation. By the way ratio-GRAPHENE_COLLATERAL_RATIO_DENOM may underflow.

abitmore · 2020-05-11T22:39:33Z

libraries/chain/db_market.cpp

+ * @param feed the debt asset's price feed
+ * @returns the max short squeeze price
+ */
+price database::get_max_short_squeeze_price( const fc::time_point_sec& block_time, const price_feed& feed)const


I think it's better to move this function from database class to price_feed class.

I thought about that, but all other HARDFORK code (which is the basis for this function) has been kept away from the protocol code. Should I break that rule?

True, protocol code doesn't include chain/hardfork.hpp. We usually check the time in chain code and convert the result to another meaningful parameter to pass in.

abitmore · 2020-05-11T22:40:36Z

libraries/chain/db_market.cpp

@@ -685,7 +690,7 @@ int database::match( const limit_order_object& bid, const call_order_object& ask
   order_pays = call_receives;

   int result = 0;
-   result |= fill_limit_order( bid, order_pays, order_receives, cull_taker, match_price, false ); // the limit order is taker


It's better to keep the comment.

abitmore · 2020-05-11T22:50:48Z

libraries/chain/db_market.cpp

-   auto issuer_fees = pay_market_fees(&seller, recv_asset, receives, is_maker);
+   asset issuer_fees;
+   if (!is_maker && is_margin_call)
+      issuer_fees = pay_margin_fees(pays.asset_id(*this), receives );


The margin call fee should be stored in the fee field of the fill_order_operation which contains the call order, but not the limit order.

Market fees is unrelated to the margin call fee. The limit order should pay market fee regardless.

abitmore · 2020-05-11T22:55:32Z

libraries/chain/db_market.cpp

+ * @param receives what the call order will receive from the other party (debt)
+ * @param fill_price the price at which the call order will execute
+ * @param is_maker TRUE if the call order is the maker, FALSE if it is the taker
+ * @param is_margin_call TRUE if this method was called due to a margin call


I didn't fully understand what's your definition of "a margin call" nor the logic around this parameter. Looks like it is always equal to !is_maker.

abitmore · 2020-05-11T23:21:18Z

libraries/chain/db_market.cpp

+         || !ba.options.extensions.value.margin_call_fee_ratio.valid())
+      return asset(0);
+   auto ratio = ba.adjusted_mcfr(price_feed);
+   auto amount = detail::calculate_ratio( collateral.amount, ratio );


This is incorrect.
All the specs and additional comments say fee = debt / feed_price * fee_rate, but not collateral * fee_rate, because debt / feed_price != collateral due to MSSR.

abitmore · 2020-05-11T23:47:11Z

libraries/chain/db_market.cpp

+         || price_feed.settlement_price.base.amount == 0
+         || !ba.options.extensions.value.margin_call_fee_ratio.valid())
+      return asset(0);
+   auto ratio = ba.adjusted_mcfr(price_feed);


It's inefficient to adjust the param on every order. Since it's only related to the price feed, it's best to save an adjusted value whenever the price feed changes or the param itself changes.

abitmore · 2020-05-11T23:54:28Z

libraries/protocol/include/graphene/protocol/asset_ops.hpp

@@ -103,6 +104,7 @@ namespace graphene { namespace protocol {

      struct ext
      {
+         fc::optional<uint16_t> margin_call_fee_ratio; // BSIP 74


Although this time it doesn't matter since all parameters are newly added, we should always add new parameters to the end of the structs used by extensions, otherwise it breaks consensus.

By the way, need new code in the cpp file to validate the new parameter.

abitmore · 2020-05-11T23:56:09Z

libraries/protocol/asset.cpp

@@ -283,7 +283,6 @@ namespace graphene { namespace protocol {

      price price_feed::max_short_squeeze_price()const
      {
-         // settlement price is in debt/collateral


I think it's better to keep the comment.

jmjatlanta · 2020-05-12T17:37:04Z

Thank you for your review. Please pause your review while I will attempt to understand then implement your changes.

MichelSantos · 2020-05-12T22:41:24Z

Margin call fees should be always paid by the debt position owner, but not the limit order owner. The fee should be stored in fee field of the virtual fill_order_operation which contains the call order.

My reading of the specification contradicts the idea that the debt position owner should pay the margin call fee. The spec mandates that the call order pays X*MSSR/settlement_price in collateral, and for the limit order receives X*(MSSR-MCFR)/settlement_price in collateral. That difference in collateral is being paid by the limit order owner who is expecting to receive X*MSSR/settlement_price in collateral.

Unless the argument is that, in accordance with " the margin call orders should be placed at the real price without margin call fee to avoid misleading the buyer", the buyer's/limit order owner's expectation of the "real price" is settlement_price/(MSSR-MCFR). If that is the case, is it correct that this "real price" is the match price?

abitmore · 2020-05-12T23:50:40Z

... the buyer's/limit order owner's expectation of the "real price" is settlement_price/(MSSR-MCFR). If that is the case, is it correct that this "real price" is the match price?

Yes.

MichelSantos · 2020-05-18T20:26:29Z

... the buyer's/limit order owner's expectation of the "real price" is settlement_price/(MSSR-MCFR). If that is the case, is it correct that this "real price" is the match price?

Yes.

@abitmore We have a draft of this implementation which is causing two previously existing margin call tests (1 and 2) to fail when BSIP74 activates. BSIP74 is being activated, indirectly, by the these BSIP77 tests when the tests advance to the BSIP77 hardfork activation time, which is after the BSIP74 time.

(Separately we should discuss synchronizing these future activation times to test for potential interactions of new features.)

These two BSIP77 tests are failing because they were expecting the margin call order to be matched against the standing limit order (no conflict here) and filled at the limit order's asking price (conflict here). In these examples, these current tests are expecting "filled core" of 105 which is the minimum being asked by the standing limit order.

However the draft BSIP74 implementation is filling at the effective price of settlement_price/(MSSR-MCFR); in these BSIP77 tests, MCFR = 0, so the effective price is settlement_price/MSSR.

Should we modify these BSIP77 tests with a check about whether BSIP74 is also activated? And then if BSIP74 is also activated in the test, change the test's literal values from, for example, 105 CORE to 110 CORE? Something like

if before_bsip74
  filled_core = 105;
else
  filled_core = 110;

abitmore · 2020-05-18T21:37:08Z

@MichelSantos wrote:

These two BSIP77 tests are failing because they were expecting the margin call order to be matched against the standing limit order (no conflict here) and filled at the limit order's asking price (conflict here). In these examples, these current tests are expecting "filled core" of 105 which is the minimum being asked by the standing limit order.

However the draft BSIP74 implementation is filling at the effective price of settlement_price/(MSSR-MCFR); in these BSIP77 tests, MCFR = 0, so the effective price is settlement_price/MSSR.

IMO, when MCFR is zero, all existing test cases should pass. According to BSIP32, in any case, the matching/filling price is the maker price (also see an earlier comment: #2130 (comment)). BSIP74 doesn't change it. If it is not clear in the specification of BSIP74, I think it's better to make it clear via bitshares/bsips#273.

MichelSantos · 2020-05-19T00:58:13Z

If it is not clear in the specification of BSIP74, I think it's better to make it clear via bitshares/bsips#273.

Follow-up here

abitmore · 2020-05-26T16:33:22Z

Closing in favor of #2180.

jmjatlanta added 5 commits March 30, 2020 14:21

hardfork protection for bsip74

e9e0e7e

HF for BSIP74 happens on time, not next maint

00b2ed1

calculating call order price

8d5aad6

test of normal case complete

fee9fe6

Implement margin call fee

ca6f361

abitmore changed the title ~~Implement BSIP 74~~ Implement BSIP 74: Margin Call Fee Ratio Apr 9, 2020

jmjatlanta marked this pull request as ready for review April 10, 2020 14:06

jmjatlanta added 3 commits April 10, 2020 10:45

do not assert if fee is zero

fa7d4d5

update feature branch with hf changes

a43e18a

Resolve compiler warning

2b120c9

winston-1984 reviewed Apr 11, 2020

View reviewed changes

libraries/chain/db_market.cpp Outdated Show resolved Hide resolved

winston-1984 reviewed Apr 11, 2020

View reviewed changes

libraries/chain/include/graphene/chain/database.hpp Outdated Show resolved Hide resolved

winston-1984 reviewed Apr 11, 2020

View reviewed changes

tests/tests/bitasset_tests.cpp Show resolved Hide resolved

fix/add comments

4fd251c

abitmore added this to the 4.0.0 - Protocol Upgrade Release milestone Apr 13, 2020

Merge hardfork into jmj_bsip74

35d797a

abitmore reviewed Apr 17, 2020

View reviewed changes

jmjatlanta added 2 commits April 17, 2020 11:18

fix misinterpretation of method params

3ea265a

Fix wrong comment

4684e51

abitmore reviewed Apr 17, 2020

View reviewed changes

abitmore added the hardfork label Apr 23, 2020

jmjatlanta added 3 commits April 23, 2020 16:21

match on calls with price feed/(mssr-mcfr)

1aee59c

rollback db_market changes

750c720

marge hardfork to bsip74

9ea7962

jmjatlanta added 2 commits April 30, 2020 18:33

MSSR / MCFR Testing

d11ecbf

Test limit as taker

fad10a6

christophersanborn mentioned this pull request May 2, 2020

Collateral asset fee accumulator for BSIPs 74 and 87 #2159

Merged

3 tasks

jmjatlanta added 7 commits May 3, 2020 17:39

limit maker case

0fae92f

merge in hf changes to jmj_bsip74

3531df9

handle when limit order is taker

96c2d98

Merge hardfork into jmj_bsip74

1424257

call order with insufficient collateral

476e3fd

wrap long lines in tests

efff548

Add check to proposal/asset_create for bsip 74

d062cd2

jmjatlanta mentioned this pull request May 5, 2020

BSIP 74 Specification Clarification bitshares/bsips#273

Closed

jmjatlanta added 4 commits May 11, 2020 13:39

move margin_fee to bitasset struct

eb9a898

tweak code, clean comments

4c0ac06

Merge hardfork into jmj_bsip74

f3f363c

comment fix

7df91b4

abitmore reviewed May 11, 2020

View reviewed changes

charge margin fee to call order

5c2210b

MichelSantos mentioned this pull request May 25, 2020

Implement BSIP 74: Margin Call Fee Ratio #2129

Closed

17 tasks

abitmore removed this from the 4.0.0 - Protocol Upgrade Release milestone May 26, 2020

abitmore closed this May 26, 2020

abitmore deleted the jmj_bsip74 branch May 28, 2020 14:03

Implement BSIP 74: Margin Call Fee Ratio #2130

Implement BSIP 74: Margin Call Fee Ratio #2130

Conversation

jmjatlanta commented Apr 4, 2020 • edited Loading

abitmore commented Apr 10, 2020

jmjatlanta commented Apr 10, 2020 • edited Loading

abitmore commented Apr 10, 2020

jmjatlanta commented Apr 11, 2020

abitmore commented Apr 17, 2020

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta commented May 11, 2020

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta commented May 12, 2020

MichelSantos commented May 12, 2020

abitmore commented May 12, 2020

MichelSantos commented May 18, 2020 • edited Loading

abitmore commented May 18, 2020

MichelSantos commented May 19, 2020

abitmore commented May 26, 2020

jmjatlanta commented Apr 4, 2020 •

edited

Loading

jmjatlanta commented Apr 10, 2020 •

edited

Loading

jmjatlanta Apr 17, 2020 •

edited

Loading

jmjatlanta May 12, 2020 •

edited

Loading

MichelSantos commented May 18, 2020 •

edited

Loading