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

Fix issue 1780: Market fees of settle orders aren't shared to referral program #2132

Merged
merged 10 commits into from
Apr 10, 2020
Merged

Fix issue 1780: Market fees of settle orders aren't shared to referral program #2132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
73321ca
Added settle orders to market fee sharing program
Jul 8, 2019
c2b28e9
optimize access to graphene database
Jul 11, 2019
4cb30cd
Merge pull request #1842 from openledger/issue-1780
abitmore Apr 5, 2020
23008cd
Merge 'hardfork' branch, apply minor fixes
abitmore Apr 9, 2020
7348873
Fix test cases
abitmore Apr 9, 2020
ab3f9b8
Update coding style
abitmore Apr 9, 2020
c32a4e6
Add market fee sharing for instant settlements
abitmore Apr 9, 2020
140f022
Update test cases
abitmore Apr 9, 2020
88727de
Wrap long lines
abitmore Apr 9, 2020
c54302f
Update comments
abitmore Apr 10, 2020
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
21 changes: 16 additions & 5 deletions libraries/chain/asset_evaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,10 +708,10 @@ void_result asset_settle_evaluator::do_evaluate(const asset_settle_evaluator::op
if( bitasset.is_prediction_market )
FC_ASSERT( bitasset.has_settlement(), "global settlement must occur before force settling a prediction market" );
else if( bitasset.current_feed.settlement_price.is_null()
&& ( d.head_block_time() <= HARDFORK_CORE_216_TIME
&& ( d.head_block_time() <= HARDFORK_CORE_216_TIME // TODO check whether the HF check can be removed
|| !bitasset.has_settlement() ) )
FC_THROW_EXCEPTION(insufficient_feeds, "Cannot force settle with no price feed.");
FC_ASSERT(d.get_balance(d.get(op.account), *asset_to_settle) >= op.amount);
FC_ASSERT( d.get_balance( op.account, op.amount.asset_id ) >= op.amount, "Insufficient balance" );

return void_result();
} FC_CAPTURE_AND_RETHROW( (op) ) }
Expand All @@ -735,8 +735,7 @@ operation_result asset_settle_evaluator::do_apply(const asset_settle_evaluator::
{
if( d.get_dynamic_global_properties().next_maintenance_time > HARDFORK_CORE_184_TIME )
FC_THROW( "Settle amount is too small to receive anything due to rounding" );
else // TODO remove this warning after hard fork core-184
wlog( "Something for nothing issue (#184, variant F) occurred at block #${block}", ("block",d.head_block_num()) );
// else do nothing. Before the hf, something for nothing issue (#184, variant F) could occur
}

asset pays = op.amount;
Expand All @@ -755,7 +754,19 @@ operation_result asset_settle_evaluator::do_apply(const asset_settle_evaluator::
obj.settlement_fund -= settled_amount.amount;
});

d.adjust_balance( op.account, settled_amount );
// The account who settles pays market fees to the issuer of the collateral asset after HF core-1780
//
// TODO Check whether the HF check can be removed after the HF.
// Note: even if logically it can be removed, perhaps the removal will lead to a small
// performance loss. Needs testing.
if( d.head_block_time() >= HARDFORK_CORE_1780_TIME )
{
auto issuer_fees = d.pay_market_fees( fee_paying_account, settled_amount.asset_id(d), settled_amount );
Copy link
Contributor

Choose a reason for hiding this comment

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

@abitmore This invocation of pay_market_fees from the evaluator is difficult to resolve and merge with another PR from simple maker-taker fees from BSIP81 because the market fee will be dependent on whether the order is considered to be a maker or taker.

I am attempting to find a reasonable manner to merge these two PRs. Because this line of logic applies during a global settlement where the settlement pool is waiting, is it reasonable to consider this settlement order as a taker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the settlement order is always taker.

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, the settlement order can be maker sometimes if we implemented bsip73.

Copy link
Member Author

Choose a reason for hiding this comment

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

A side note, since there is no virtual fill_order_operation generated for instant settlement, neither the price nor the volume would be in the HTLCV charts.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I merge this before the BSIP73 PR, can I ignore the problem? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I believe 73 will be worked on only after this.

settled_amount -= issuer_fees;
}

if( settled_amount.amount > 0 )
d.adjust_balance( op.account, settled_amount );
}

d.modify( mia_dyn, [&]( asset_dynamic_data_object& obj ){
Expand Down
12 changes: 10 additions & 2 deletions libraries/chain/db_market.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,14 +916,22 @@ bool database::fill_settle_order( const force_settlement_object& settle, const a
{ try {
bool filled = false;

auto issuer_fees = pay_market_fees( nullptr, get(receives.asset_id), receives);
const account_object* settle_owner_ptr = nullptr;
// The owner of the settle order pays market fees to the issuer of the collateral asset after HF core-1780
//
// TODO Check whether the HF check can be removed after the HF.
// Note: even if logically it can be removed, perhaps the removal will lead to a small performance
// loss. Needs testing.
if( head_block_time() >= HARDFORK_CORE_1780_TIME )
settle_owner_ptr = &settle.owner(*this);

auto issuer_fees = pay_market_fees( settle_owner_ptr, get(receives.asset_id), receives );

if( pays < settle.balance )
{
modify(settle, [&pays](force_settlement_object& s) {
s.balance -= pays;
});
filled = false;
} else {
filled = true;
}
Expand Down
4 changes: 4 additions & 0 deletions libraries/chain/hardfork.d/CORE_1780.hf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Market fees of settle orders aren't shared to referral program
#ifndef HARDFORK_CORE_1780_TIME
#define HARDFORK_CORE_1780_TIME (fc::time_point_sec( 1600000000 ) ) // September 13, 2020 3:26:40 PM (GMT)
#endif
4 changes: 2 additions & 2 deletions tests/tests/market_fee_sharing_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,8 +436,8 @@ BOOST_AUTO_TEST_CASE(create_actors)

price price(asset(1, asset_id_type(1)), asset(1));
uint16_t market_fee_percent = 20 * GRAPHENE_1_PERCENT;
auto obj = jill_id(db);
const asset_object jillcoin = create_user_issued_asset( "JCOIN", jill, charge_market_fee, price, 2, market_fee_percent );
const asset_object jillcoin = create_user_issued_asset( "JCOIN", jill, charge_market_fee,
price, 2, market_fee_percent );

const account_object alice = create_account("alice", izzyregistrar, izzyreferrer, 50/*0.5%*/);
const account_object bob = create_account("bob", izzyregistrar, izzyreferrer, 50/*0.5%*/);
Expand Down
Loading