BSIP38 add target_cr option to call order #838

abitmore · 2018-04-10T21:46:42Z

This is PR for #834, aka BSIP38: Add target collateral ratio option to short positions.

The code is ready for review~~, but still need (lots of) test cases~~.

wrapping a call_order_update_operation with extension in a proposal is not allowed before hard fork
difficult edge cases e.g. mssr=1001 and mcr=tcr=1002 or something
matching then generate block

pmconrad · 2018-04-23T19:35:08Z

libraries/chain/include/graphene/chain/protocol/market.hpp

      /** this is slightly more expensive than limit orders, this pricing impacts prediction markets */
      struct fee_parameters_type { uint64_t fee = 20 * GRAPHENE_BLOCKCHAIN_PRECISION; };

      asset               fee;
      account_id_type     funding_account; ///< pays fee, collateral, and cover
      asset               delta_collateral; ///< the amount of collateral to add to the margin position
      asset               delta_debt; ///< the amount of the debt to be paid off, may be negative to issue new debt
-      extensions_type     extensions;
+
+      typedef vector<extension<options_type>> extension_type; // use a vector here to be compatible with old JSON


Wrapping the extension in a 1-element-vector is ugly. Is it really worth it?
I suppose that everyone is ignoring the extensions at this time, so I'd tend to sacrifice JSON compatibility for future readability.

Sacrificing JSON compatibility means all UI / bot / client software need to update before any API node is updated (before and after hard fork), and need to use different JSON format for different version of nodes (before hardfork). It affects not only reading from the blockchain, but also broadcasting new transactions. Giving that we have many different clients now, I don't think it's a good idea to break the compatibility. (I personally is using a 2016 build of bitshares-ui and don't want to upgrade :P)

Actually for clients that submit extensions, an empty array is treated like an empty object: https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/include/graphene/chain/protocol/ext.hpp#L169

So we're breaking compatibility only when reading from the chain, and even there it won't matter in client languages that treat objects and arrays mostly the same (like JavaScript).

So old clients can still broad transactions with new nodes even without the vector. That makes sense. I'll change this as suggested.

Removed vector.

pmconrad · 2018-04-23T19:39:30Z

libraries/chain/market_evaluator.cpp

+            call.call_price  =  price::call_price(call.get_debt(), call.get_collateral(),
+                                                  _bitasset_data->current_feed.maintenance_collateral_ratio);
+         }
+         call.target_collateral_ratio = new_target_cr;


The usual semantics of *_update operations is "if absent leave untouched", while here it is "if absent delete". This is confusing.
Perhaps make the extension field an optional<optional<uint16_t>>, or use a special value like 0 for deletion.

I don't think optional<optional<uint16_t>> can be jsonified correctly.

Since type of that field is optional already, it would be always confusing about how to delete it, IMHO it's easier to treat it as an always-required field, aka always update.

Another approach is to use 2 fields, one for existence and the other for the real value. But I think it's too complicated since the optional type is just for doing this.

I'd still prefer to keep the "if absent leave untouched" semantic. I suppose the option will not be available in the UI (all UI's) very soon, so someone who is used to trade via UI might want to set the tcr once via cli_wallet and continue normal operation using the UI.

Since tcr < 1000 doesn't make sense anyway, you could treat and invalid value (i. e. < 1000) as a deliberate delete request.

I still think it's clearer to just update as is (which is written in BSIP).

Compare the two approaches:

update as is

found null in op -> set null in obj

found x in op -> set x in obj

update by checking if it's a special number

found null in op -> don't change obj (no matter what it was)

found 0 in op -> set null in obj

found x(>0) in op -> set x(>0) in obj

IMHO the 2nd one will lead to more confusion.

Another thing related is that call_order_update_operation is not only for updating a call order, but also for creating and removing, although removing is irrelevant here.

If the option is in "if absent leave untouched" semantic, the user may expect that the system will "remember" the option when a position is closed (either intendedly or unintendedly), also sometimes may forget to set when recreating a new position, both may lead to unintended behavior.

Compare the two approaches (I think the 1st one is better):

create new position as is

found null in op -> set null in obj

found x in op -> set x in obj

create new position checking if it's a special number

found null in op -> set null in obj?

found 0 in op -> set null in obj

found x(>0) in op -> set x(>0) in obj

Well, seems we have to agree to disagree (again ;-) ).

BSIP is authoritative though, so I'm OK with leaving it as it is.

Perhaps propose a BSIP change? E.G. drop optional from object, use value 0 as "not set" instead.

pmconrad · 2018-04-23T19:43:18Z

libraries/chain/include/graphene/chain/market_object.hpp

+
+      /// Calculate maximum quantity of collateral to sell and debt to cover to meet @ref target_collateral_ratio.
+      /// @return a pair of assets, the first item is collateral to sell, the second is debt to cover
+      pair<asset, asset> get_max_sell_receive_pair( price match_price,


Why return both? You only ever use the second member of the return value.

Actually, in the beginning, I only returned the first member; while coding I noticed that the second is needed, so changed it to return a pair; at last I found the first is not being used in main code, but is heavily used in test cases, in addition I'm not sure whether it will be useful for potential new APIs, so decided to leave it there.

Do you think it's necessary to change it to only return the second?

Not strictly necessary, but a significant simplification IMO.

Removed pair.

pmconrad · 2018-04-23T20:16:13Z

libraries/chain/market_object.cpp

+
+max_amount_to_sell = ( (debt + x) * target_CR - collateral * feed_price )
+                     / (target_CR * match_price - feed_price)
+                   = ( (debt + x) * tCR / DENOM - collateral * fp_debt_amt / fp_coll_amt + target_CR * x )


The last product in this line (target_CR*x) is too much.

Thanks. Updated.

pmconrad · 2018-04-23T20:30:29Z

libraries/chain/market_object.cpp

+                                                                 const uint16_t maintenance_collateral_ratio )const
+{ try {
+   // make sure feed_price is in collateral / debt format
+   if( feed_price.base.asset_id != call_price.base.asset_id )


This check should always be true (feed price is defined as debt/collateral, see asset.hpp).
(That's just a remark. Probably a good idea to be defensive here.)

Yes, the caller may pass in a reversed price pair.

pmconrad · 2018-04-23T21:45:02Z

libraries/chain/market_object.cpp

+   if( feed_price.base.asset_id != call_price.base.asset_id )
+      feed_price = ~feed_price;
+
+   FC_ASSERT( feed_price.base.asset_id == call_price.base.asset_id


Should always be true, by definition.
(That's just a remark. Probably a good idea to be defensive here.)

pmconrad · 2018-04-23T21:51:58Z

tests/tests/operation_tests.cpp

+      GRAPHENE_REQUIRE_THROW( borrow( alice, bitusd.amount(100000), core.amount(400000), 1750 ), fc::exception );
+      GRAPHENE_REQUIRE_THROW( borrow( alice, bitusd.amount(100000), core.amount(400000), 1751 ), fc::exception );
+      GRAPHENE_REQUIRE_THROW( borrow( alice, bitusd.amount(100000), core.amount(400000), 65535 ), fc::exception );
+


Please add a test that wraps a call_order_update with extension in a proposal.

It's planned but not yet done. Will do definitely.

Added proposal tests.

pmconrad · 2018-04-23T21:55:16Z

tests/tests/call_order_tests.cpp

+         " sell full (perfect), sell full (<0.01%), sell full (<0.1%),sell full (<1%), sell full (other), ...,"
+         " sell some (perfect), sell some (<0.01%), sell some (<0.1%),sell some (<1%), sell some (other), ... ]" );
+   idump( (total)(count) );
+


Please add some tests for the difficult edge cases. I suppose mssr=1001 and mcr=tcr=1002 or something will be problematic.

Added 1001/1002 to random tests. The result shows that ~99.6% will cover full debt.

pmconrad · 2018-04-23T22:02:06Z

libraries/chain/protocol/market.cpp

+   if( extensions.size() > 0 )
+   {
+      const auto& options = extensions.front().value;
+      size_t count = 0; // use a count variable for future expansions


I wouldn't make assumptions about the logic of future extensions. Just check that tcr is valid right now, instead of counting stuff. (See account extensions for example.)

IMHO nonempty is basic requirement for extensions field. Additional validation may or may not apply when adding new fields (no more validation this time since TCR is optional). Btw there are more fields to be added as discussed in bitshares/bsips#51, rather than counting after added them, I decided to count from now on.

In regards to account extensions, since it's not using a container which can jsonify to an array, but replaced flat_set<static_variant<X>> with extension<Y> directly (thus have broken JSON compatibility), it should be allowed if the extensions field is empty. So there are individual validations about the fields inside.

I used a vector here, so doesn't make sense to allow an empty object inside. The validation code here is for different things.

Even when more fields are added, the actual count is irrelevant, as long as it is > 0.
If we remove the vector wrapper, count==0 will have to be accepted, which makes the whole check here useless.

Removed vector, so removed validation.

pmconrad · 2018-04-23T22:03:33Z

libraries/chain/protocol/asset.cpp

@@ -97,15 +97,19 @@ namespace graphene { namespace chain {
         const asset& a = *this;
         if( a.asset_id == b.base.asset_id )
         {
+            // TODO replace exception type with price_multiplication_undefined?


Please remove these unrelated comments. If you think this might be a good idea, open a github issue.

Just realized that the definitions of those exceptions are commented out. Will remove the unrelated comments.

abitmore · 2018-04-28T18:30:13Z

More test cases added. Ready for final review.

pmconrad · 2018-05-01T19:02:21Z

tests/tests/operation_tests.cpp

+         tx.operations.push_back( prop );
+         db.current_fee_schedule().set_fee( tx.operations.back() );
+         set_expiration( db, tx );
+         db.push_transaction(trx, ~0);


You're pushing the wrong transaction here... :-)

Good catch 💯

This kind of bug is hard to detect, especially when the code is in a lambda, changing [&] to [this] here doesn't help. :-(

What's best practice?

abitmore added 4 commits April 10, 2018 17:16

BSIP38: add target_CR option to call order #834

82c8d31

BSIP38: update limit:call matching logic #834

51a2aff

BSIP38: update call:limit matching logic #834

7c84535

BSIP38: add CLI command borrow_asset_ext #834

a60573e

abitmore added the hardfork label Apr 10, 2018

abitmore added this to the 201805 - Consensus Changing Release milestone Apr 10, 2018

abitmore assigned pmconrad and abitmore Apr 10, 2018

abitmore requested review from pmconrad and oxarbitrage April 10, 2018 21:46

abitmore added 4 commits April 11, 2018 07:35

BSIP38: update rounding when calculating maximums

96b85ad

BSIP38: adjust maximums calculation, add comments

ec00b2a

BSIP38 tests: add target_CR to database fixture

7d5db49

BSIP38 tests: object, validation, hardfork time

49e5a59

abitmore added 9 commits April 13, 2018 16:51

BSIP38 tests: add random tests

087e564

BSIP38: handle rounding when calculating maximums

2c61ad9

Add random tests for asset multiplies price

b4b18e2

BSIP38 tests: update to hit more edge cases

ddb6baa

BSIP38 tests: move test case to new file, refactor

dd8d7f6

BSIP38: update algorithm when calculating maximums

15b2b16

Merge bsip35 changes into bsip38 branch

58542b8

Use asset::multiply_and_round_up() in BSIP38

464fd38

BSIP38 tests: fewer iterations in call_order_tests

e68df58

pmconrad suggested changes Apr 23, 2018

View reviewed changes

abitmore added 3 commits April 24, 2018 06:23

Update comments and dup vars according to review

a1e9d89

Merge bsip35 changes into bsip38 branch

e7f4af8

BSIP38: update extensions type; add proposal test

baaf170

BSIP38: get_max... in call_obj returns debt only

d1c3448

abitmore changed the base branch from bsip35-order-rounding to hardfork April 26, 2018 19:38

abitmore added 4 commits April 26, 2018 15:49

Merge hardfork branch into bsip38 branch

e2f33f5

BSIP38 tests: proposal test after hard fork

516228b

BSIP38 tests: add extreme case: mcr 1002 mssr 1001

6a43799

BSIP38 tests: add order matching and filling test

182e6ec

abitmore requested a review from pmconrad May 1, 2018 15:00

pmconrad suggested changes May 1, 2018

View reviewed changes

Fix BSIP38 test case about hard fork time check

610dbf5

pmconrad approved these changes May 1, 2018

View reviewed changes

abitmore merged commit 14c9786 into hardfork May 2, 2018

abitmore mentioned this pull request May 2, 2018

Implement BSIP38: add target CR option to short positions #834

Closed

abitmore deleted the bsip38-call-target-cr branch May 13, 2018 19:27

pmconrad mentioned this pull request Aug 2, 2018

Custom active permission bitshares/bsips#86

Merged

abitmore mentioned this pull request May 9, 2020

Implement BSIP87: Force-Settlement Fee #2151

Merged

3 tasks

BSIP38 add target_cr option to call order #838

BSIP38 add target_cr option to call order #838

Conversation

abitmore commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore Apr 24, 2018 • 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

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

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

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

abitmore commented Apr 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented Apr 10, 2018 •

edited

Loading

abitmore Apr 24, 2018 •

edited

Loading