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.

Sign up for GitHub

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

Jump to bottom

BSIP38 add target_cr option to call order #838

Merged
merged 26 commits into from
May 2, 2018
Merged

BSIP38 add target_cr option to call order #838

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
82c8d31
BSIP38: add target_CR option to call order #834
abitmore Apr 8, 2018
51a2aff
BSIP38: update limit:call matching logic #834
abitmore Apr 10, 2018
7c84535
BSIP38: update call:limit matching logic #834
abitmore Apr 10, 2018
a60573e
BSIP38: add CLI command borrow_asset_ext #834
abitmore Apr 10, 2018
96b85ad
BSIP38: update rounding when calculating maximums
abitmore Apr 11, 2018
ec00b2a
BSIP38: adjust maximums calculation, add comments
abitmore Apr 12, 2018
7d5db49
BSIP38 tests: add target_CR to database fixture
abitmore Apr 12, 2018
49e5a59
BSIP38 tests: object, validation, hardfork time
abitmore Apr 12, 2018
087e564
BSIP38 tests: add random tests
abitmore Apr 13, 2018
2c61ad9
BSIP38: handle rounding when calculating maximums
abitmore Apr 13, 2018
b4b18e2
Add random tests for asset multiplies price
abitmore Apr 15, 2018
ddb6baa
BSIP38 tests: update to hit more edge cases
abitmore Apr 15, 2018
dd8d7f6
BSIP38 tests: move test case to new file, refactor
abitmore Apr 16, 2018
15b2b16
BSIP38: update algorithm when calculating maximums
abitmore Apr 16, 2018
58542b8
Merge bsip35 changes into bsip38 branch
abitmore Apr 17, 2018
464fd38
Use asset::multiply_and_round_up() in BSIP38
abitmore Apr 17, 2018
e68df58
BSIP38 tests: fewer iterations in call_order_tests
abitmore Apr 17, 2018
a1e9d89
Update comments and dup vars according to review
abitmore Apr 24, 2018
e7f4af8
Merge bsip35 changes into bsip38 branch
abitmore Apr 25, 2018
baaf170
BSIP38: update extensions type; add proposal test
abitmore Apr 25, 2018
d1c3448
BSIP38: get_max... in call_obj returns debt only
abitmore Apr 25, 2018
e2f33f5
Merge hardfork branch into bsip38 branch
abitmore Apr 26, 2018
516228b
BSIP38 tests: proposal test after hard fork
abitmore Apr 26, 2018
6a43799
BSIP38 tests: add extreme case: mcr 1002 mssr 1001
abitmore Apr 28, 2018
182e6ec
BSIP38 tests: add order matching and filling test
abitmore Apr 28, 2018
610dbf5
Fix BSIP38 test case about hard fork time check
abitmore May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libraries/chain/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ add_library( graphene_chain
account_object.cpp
asset_object.cpp
fba_object.cpp
market_object.cpp
proposal_object.cpp
vesting_balance_object.cpp

Expand Down
59 changes: 43 additions & 16 deletions libraries/chain/db_market.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,16 +464,23 @@ bool database::apply_order(const limit_order_object& new_order_object, bool allo
// check if there are margin calls
const auto& call_price_idx = get_index_type<call_order_index>().indices().get<by_price>();
auto call_min = price::min( recv_asset_id, sell_asset_id );
auto call_itr = call_price_idx.lower_bound( call_min );
// feed protected https://github.com/cryptonomex/graphene/issues/436
auto call_end = call_price_idx.upper_bound( ~sell_abd->current_feed.settlement_price );
while( !finished && call_itr != call_end )
while( !finished )
{
auto old_call_itr = call_itr;
++call_itr; // would be safe, since we'll end the loop if a call order is partially matched
// match returns 2 when only the old order was fully filled. In this case, we keep matching; otherwise, we stop.
// assume hard fork core-343 and core-625 will take place at same time, always check call order with least call_price
auto call_itr = call_price_idx.lower_bound( call_min );
if( call_itr == call_price_idx.end()
|| call_itr->debt_type() != sell_asset_id
// feed protected https://github.com/cryptonomex/graphene/issues/436
|| call_itr->call_price > ~sell_abd->current_feed.settlement_price )
break;
// assume hard fork core-338 and core-625 will take place at same time, not checking HARDFORK_CORE_338_TIME here.
finished = ( match( new_order_object, *old_call_itr, call_match_price ) != 2 );
int match_result = match( new_order_object, *call_itr, call_match_price,
sell_abd->current_feed.settlement_price,
sell_abd->current_feed.maintenance_collateral_ratio );
// match returns 1 or 3 when the new order was fully filled. In this case, we stop matching; otherwise keep matching.
// since match can return 0 due to BSIP38 (hard fork core-834), we no longer only check if the result is 2.
if( match_result == 1 || match_result == 3 )
finished = true;
}
}
}
Expand Down Expand Up @@ -574,7 +581,8 @@ int database::match( const limit_order_object& usd, const limit_order_object& co
return result;
}

int database::match( const limit_order_object& bid, const call_order_object& ask, const price& match_price )
int database::match( const limit_order_object& bid, const call_order_object& ask, const price& match_price,
const price& feed_price, const uint16_t maintenance_collateral_ratio )
{
FC_ASSERT( bid.sell_asset_id() == ask.debt_type() );
FC_ASSERT( bid.receive_asset_id() == ask.collateral_type() );
Expand All @@ -583,11 +591,19 @@ int database::match( const limit_order_object& bid, const call_order_object& ask
auto maint_time = get_dynamic_global_properties().next_maintenance_time;
// TODO remove when we're sure it's always false
bool before_core_hardfork_342 = ( maint_time <= HARDFORK_CORE_342_TIME ); // better rounding
// TODO remove when we're sure it's always false
bool before_core_hardfork_834 = ( maint_time <= HARDFORK_CORE_834_TIME ); // target collateral ratio option

bool cull_taker = false;

// TODO if we're sure `before_core_hardfork_834` is always false, remove optional and remove check, always initialize
optional<pair<asset,asset>> call_max_sell_receive_pair; // (collateral, debt)
if( !before_core_hardfork_834 )
call_max_sell_receive_pair = ask.get_max_sell_receive_pair( match_price, feed_price, maintenance_collateral_ratio );

asset usd_for_sale = bid.amount_for_sale();
asset usd_to_buy = ask.get_debt();
// TODO if we're sure `before_core_hardfork_834` is always false, remove the check
asset usd_to_buy = ( before_core_hardfork_834 ? ask.get_debt() : call_max_sell_receive_pair->second );

asset call_pays, call_receives, order_pays, order_receives;
if( usd_to_buy > usd_for_sale )
Expand Down Expand Up @@ -632,7 +648,8 @@ int database::match( const limit_order_object& bid, const call_order_object& ask
int result = 0;
result |= fill_limit_order( bid, order_pays, order_receives, cull_taker, match_price, false ); // the limit order is taker
result |= fill_call_order( ask, call_pays, call_receives, match_price, true ) << 1; // the call order is maker
FC_ASSERT( result != 0 );
// result can be 0 when call order has target_collateral_ratio option set.

return result;
}

Expand Down Expand Up @@ -829,14 +846,14 @@ bool database::fill_call_order( const call_order_object& order, const asset& pay
});

const account_object& borrower = order.borrower(*this);
if( collateral_freed || pays.asset_id == asset_id_type() )
if( collateral_freed.valid() || pays.asset_id == asset_id_type() )
{
const account_statistics_object& borrower_statistics = borrower.statistics(*this);
if( collateral_freed )
if( collateral_freed.valid() )
adjust_balance(borrower.get_id(), *collateral_freed);

modify( borrower_statistics, [&]( account_statistics_object& b ){
if( collateral_freed && collateral_freed->amount > 0 )
if( collateral_freed.valid() && collateral_freed->amount > 0 )
b.total_core_in_orders -= collateral_freed->amount;
if( pays.asset_id == asset_id_type() )
b.total_core_in_orders -= pays.amount;
Expand All @@ -849,7 +866,7 @@ bool database::fill_call_order( const call_order_object& order, const asset& pay
push_applied_operation( fill_order_operation( order.id, order.borrower, pays, receives,
asset(0, pays.asset_id), fill_price, is_maker ) );

if( collateral_freed )
if( collateral_freed.valid() )
remove( order );

return collateral_freed.valid();
Expand Down Expand Up @@ -945,6 +962,7 @@ bool database::check_call_orders(const asset_object& mia, bool enable_black_swan
bool before_core_hardfork_343 = ( maint_time <= HARDFORK_CORE_343_TIME ); // update call_price after partially filled
bool before_core_hardfork_453 = ( maint_time <= HARDFORK_CORE_453_TIME ); // multiple matching issue
bool before_core_hardfork_606 = ( maint_time <= HARDFORK_CORE_606_TIME ); // feed always trigger call
bool before_core_hardfork_834 = ( maint_time <= HARDFORK_CORE_834_TIME ); // target collateral ratio option

while( !check_for_blackswan( mia, enable_black_swan ) && call_itr != call_end )
{
Expand Down Expand Up @@ -983,6 +1001,15 @@ bool database::check_call_orders(const asset_object& mia, bool enable_black_swan
return true;
}

optional<pair<asset,asset>> call_max_sell_receive_pair; // (collateral, debt)
if( !before_core_hardfork_834 )
{
call_max_sell_receive_pair = call_itr->get_max_sell_receive_pair( match_price,
bitasset.current_feed.settlement_price,
bitasset.current_feed.maintenance_collateral_ratio );
usd_to_buy = call_max_sell_receive_pair->second;
}

asset call_pays, call_receives, order_pays, order_receives;
if( usd_to_buy > usd_for_sale )
{ // fill order
Expand Down Expand Up @@ -1028,7 +1055,7 @@ bool database::check_call_orders(const asset_object& mia, bool enable_black_swan
else
order_receives = usd_to_buy.multiply_and_round_up( match_price ); // round up, in favor of limit order

filled_call = true;
filled_call = true; // this is safe, since BSIP38 (hard fork core-834) depends on BSIP31 (hard fork core-343)

if( usd_to_buy == usd_for_sale )
filled_limit = true;
Expand Down
4 changes: 4 additions & 0 deletions libraries/chain/hardfork.d/CORE_834.hf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// bitshares-core issue #834 "BSIP38: add target CR option to short positions"
#ifndef HARDFORK_CORE_834_TIME
#define HARDFORK_CORE_834_TIME (fc::time_point_sec( 1600000000 ))
#endif
3 changes: 2 additions & 1 deletion libraries/chain/include/graphene/chain/database.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,8 @@ namespace graphene { namespace chain {
*/
///@{
int match( const limit_order_object& taker, const limit_order_object& maker, const price& trade_price );
int match( const limit_order_object& taker, const call_order_object& maker, const price& trade_price );
int match( const limit_order_object& taker, const call_order_object& maker, const price& trade_price,
const price& feed_price, const uint16_t maintenance_collateral_ratio );
/// @return the amount of asset settled
asset match(const call_order_object& call,
const force_settlement_object& settle,
Expand Down
10 changes: 9 additions & 1 deletion libraries/chain/include/graphene/chain/market_object.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,20 @@ class call_order_object : public abstract_object<call_order_object>
share_type debt; ///< call_price.quote.asset_id, access via get_debt
price call_price; ///< Collateral / Debt

optional<uint16_t> target_collateral_ratio; ///< maximum CR to maintain when selling collateral on margin call

pair<asset_id_type,asset_id_type> get_market()const
{
auto tmp = std::make_pair( call_price.base.asset_id, call_price.quote.asset_id );
if( tmp.first > tmp.second ) std::swap( tmp.first, tmp.second );
return tmp;
}

/// 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly necessary, but a significant simplification IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed pair.

price feed_price,
const uint16_t maintenance_collateral_ratio )const;
};

/**
Expand Down Expand Up @@ -259,7 +267,7 @@ FC_REFLECT_DERIVED( graphene::chain::limit_order_object,
)

FC_REFLECT_DERIVED( graphene::chain::call_order_object, (graphene::db::object),
(borrower)(collateral)(debt)(call_price) )
(borrower)(collateral)(debt)(call_price)(target_collateral_ratio) )

FC_REFLECT_DERIVED( graphene::chain::force_settlement_object,
(graphene::db::object),
Expand Down
21 changes: 18 additions & 3 deletions libraries/chain/include/graphene/chain/protocol/market.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
#pragma once
#include <graphene/chain/protocol/base.hpp>
#include <graphene/chain/protocol/ext.hpp>

namespace graphene { namespace chain {

Expand Down Expand Up @@ -94,8 +95,6 @@ namespace graphene { namespace chain {
void validate()const;
};



/**
* @ingroup operations
*
Expand All @@ -110,14 +109,26 @@ namespace graphene { namespace chain {
*/
struct call_order_update_operation : public base_operation
{
/**
* Options to be used in @ref call_order_update_operation.
*
* @note this struct can be expanded by adding more options in the end.
*/
struct options_type
{
optional<uint16_t> target_collateral_ratio; ///< maximum CR to maintain when selling collateral on margin call
};

/** 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@abitmore abitmore Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed vector.

extension_type extensions;

account_id_type fee_payer()const { return funding_account; }
void validate()const;
Expand Down Expand Up @@ -214,6 +225,10 @@ FC_REFLECT( graphene::chain::bid_collateral_operation::fee_parameters_type, (fee
FC_REFLECT( graphene::chain::fill_order_operation::fee_parameters_type, ) // VIRTUAL
FC_REFLECT( graphene::chain::execute_bid_operation::fee_parameters_type, ) // VIRTUAL

FC_REFLECT( graphene::chain::call_order_update_operation::options_type, (target_collateral_ratio) )

FC_REFLECT_TYPENAME( graphene::chain::call_order_update_operation::extension_type )

FC_REFLECT( graphene::chain::limit_order_create_operation,(fee)(seller)(amount_to_sell)(min_to_receive)(expiration)(fill_or_kill)(extensions))
FC_REFLECT( graphene::chain::limit_order_cancel_operation,(fee)(fee_paying_account)(order)(extensions) )
FC_REFLECT( graphene::chain::call_order_update_operation, (fee)(funding_account)(delta_collateral)(delta_debt)(extensions) )
Expand Down
26 changes: 18 additions & 8 deletions libraries/chain/market_evaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ void_result call_order_update_evaluator::do_evaluate(const call_order_update_ope
{ try {
database& d = db();

// TODO: remove this check and the assertion after hf_834
if( d.get_dynamic_global_properties().next_maintenance_time <= HARDFORK_CORE_834_TIME )
FC_ASSERT( o.extensions.empty(),
"Can not specify non-empty extensions in call_order_update_operation before hardfork 834." );

_paying_account = &o.funding_account(d);
_debt_asset = &o.delta_debt.asset_id(d);
FC_ASSERT( _debt_asset->is_market_issued(), "Unable to cover ${sym} as it is not a collateralized asset.",
Expand Down Expand Up @@ -221,6 +226,10 @@ void_result call_order_update_evaluator::do_apply(const call_order_update_operat

optional<price> old_collateralization;

optional<uint16_t> new_target_cr; // new target collateral ratio
if( o.extensions.size() > 0 )
new_target_cr = o.extensions.front().value.target_collateral_ratio;

if( itr == call_idx.end() )
{
FC_ASSERT( o.delta_collateral.amount > 0 );
Expand All @@ -232,7 +241,7 @@ void_result call_order_update_evaluator::do_apply(const call_order_update_operat
call.debt = o.delta_debt.amount;
call.call_price = price::call_price(o.delta_debt, o.delta_collateral,
_bitasset_data->current_feed.maintenance_collateral_ratio);

call.target_collateral_ratio = new_target_cr;
});
}
else
Expand All @@ -241,13 +250,14 @@ void_result call_order_update_evaluator::do_apply(const call_order_update_operat
old_collateralization = call_obj->collateralization();

d.modify( *call_obj, [&]( call_order_object& call ){
call.collateral += o.delta_collateral.amount;
call.debt += o.delta_debt.amount;
if( call.debt > 0 )
{
call.call_price = price::call_price(call.get_debt(), call.get_collateral(),
_bitasset_data->current_feed.maintenance_collateral_ratio);
}
call.collateral += o.delta_collateral.amount;
call.debt += o.delta_debt.amount;
if( call.debt > 0 )
{
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Compare the two approaches:

  1. update as is
  • found null in op -> set null in obj
  • found x in op -> set x in obj
  1. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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):

  1. create new position as is
  • found null in op -> set null in obj
  • found x in op -> set x in obj
  1. 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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

});
}

Expand Down
Loading