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.

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

Call order and bitAsset related code refactory #1306

Merged
merged 11 commits into from Sep 10, 2018

Conversation

Projects
2 participants
@abitmore
Member

abitmore commented Sep 6, 2018

While working on #1270, I ended up refactoring quite some code. Since the change is getting large, for easier code review in the future, I'm now creating this PR.

@abitmore abitmore added this to In progress in Feature release (201810) via automation Sep 6, 2018

@abitmore abitmore requested a review from pmconrad Sep 6, 2018

Feature release (201810) automation moved this from In progress to In Testing Sep 9, 2018

@pmconrad

Looks good, thanks.

modify( borrower_statistics, [&]( account_statistics_object& b ){
if( collateral_freed.valid() && collateral_freed->amount > 0 && collateral_freed->asset_id == asset_id_type() )
b.total_core_in_orders -= collateral_freed->amount;
modify( get_account_stats_by_owner(order.borrower), [&collateral_freed,&pays]( account_statistics_object& b ){

This comment has been minimized.

@pmconrad

pmconrad Sep 9, 2018

Only call modify if pays.asset_id == asset_id_type() (this also implies collateral_freed.asset_id == core if collateral_freed is defined)

});
const account_object& borrower = order.borrower(*this);
// adjust balance and/or account statistics
if( collateral_freed.valid() || pays.asset_id == asset_id_type() )

This comment has been minimized.

@pmconrad

pmconrad Sep 9, 2018

Add condition && collateral_freed.amount != 0?

This comment has been minimized.

@abitmore

abitmore Sep 9, 2018

Member

Actually the code is a bit messy here for potential better performance.

  • The OR check here is for only updating the object once when possible;
  • in most cases collateral_freed.amount is non-zero so it doesn't make much sense to add a check;
    • in addition, adjust_balance() will check if the delta is zero;
  • actually there is a chance that we don't need to execute the modify, since actually pays.asset_id and collateral_freed->asset_id are the same, that means if we don't need to update total_core_in_orders due to pays, we won't need to update it due to collateral_freed. I will improve this. (Just saw your comment above)
Show resolved Hide resolved libraries/chain/market_evaluator.cpp
database::fill_call_order() minor optimization
and added comment in call_order_update_evaluator::do_evaluate()

Feature release (201810) automation moved this from In Testing to In progress Sep 9, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 9, 2018

@pmconrad please review again. Thanks a lot.

Feature release (201810) automation moved this from In progress to In Testing Sep 10, 2018

@abitmore abitmore merged commit 0e86ff5 into develop Sep 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Sep 10, 2018

@abitmore abitmore deleted the call-order-refactory branch Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment