abitmore commented on this pull request.
The test case was not broken, so this PR is more an optimization than a fix.
There were efforts to include all consensus related tests into `chain_test`. Moving tests outside of `chain_test` will likely make them be ignored and may cause unexpected consequences.
> @@ -248,6 +248,11 @@ void_result call_order_update_evaluator::do_apply(const call_order_update_operat
call_obj = &*itr;
old_collateralization = call_obj->collateralization();
old_debt = call_obj->debt;
+ auto new_collateral = call_obj->collateral + o.delta_collateral.amount;
+ auto new_debt = *old_debt + o.delta_debt.amount;
+ // Forbid zero collateral with nonzero debt (avoids divide by zero when calculating call price below)
+ FC_ASSERT(!(new_collateral == 0 && new_debt != 0), “Cannot have zero collateral and nonzero debt”);
“zero collateral with nonzero debt” is covered by this test case: https://github.com/bitshares/bitshares-core/blob/c6b28b2f0534f3a5710543a9d6f029a614432382/tests/tests/operation_tests.cpp#L146-L147
`price::call_price()` will generate a division-by-zero exception, then the exception will be captured and converted to `fc::exception`, then be caught by callers. So I think this change is more an optimization rather than a fix. No?
> @@ -145,6 +145,7 @@ BOOST_AUTO_TEST_CASE( call_order_update_test )
GRAPHENE_REQUIRE_THROW( borrow( dan, bitusd.amount(80000), asset(0)), fc::exception );
BOOST_TEST_MESSAGE( “attempting to claim all collateral without paying off debt” );
GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000)), fc::exception );
+ GRAPHENE_REQUIRE_THROW( cover( dan, bitusd.amount(0), asset(20000-1)), fc::exception );
What is this new test for? Actually it will throw an exception due to triggering a blackswan which is not allowed when updating a call position. It worth testing, but better add a message above it.
This post was last modified on July 7, 2018, 2:51 pm