-
Notifications
You must be signed in to change notification settings - Fork 649
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
Prevent current > max supply and call_order_update proposal #1566
Conversation
d.adjust_balance( o.funding_account, o.delta_debt ); | ||
|
||
// Deduct the debt paid from the total supply of the debt asset. | ||
d.modify(*_dynamic_data_obj, [&](asset_dynamic_data_object& dynamic_asset) { | ||
d.modify( *_dynamic_data_obj, [ this, &o ]( asset_dynamic_data_object& dynamic_asset ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ASSERTs were originally within the d.modify. Moved to before d.adjust_balance, as it seems to make more sense. Please correct me if the logic is flawed.
Also, some branches do not contain the verification that the current supply is non-negative. Perhaps that check is done elsewhere?
{ | ||
ilog( "20181206A: proposal contains call_order_update at ${bt}: ${op}", ("bt",block_time)("op",v) ); | ||
FC_ASSERT( block_time < fc::time_point::now() - fc::seconds(30), "Soft fork - preventing proposal with call_order_update!"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the interpretation is: For CNY, prediction markets can have proposals with call_order_update. Otherwise, we ASSERT. I believe the softfork protection should be changed to hardfork protection (backdated to late Dec.?)
for( const op_wrapper& op : v.proposed_ops ) | ||
{ | ||
op.op.visit( *this); | ||
if ( nested_soft_fork && op.op.which() == operation::tag<proposal_update_operation>().value ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will reword this section based on answers to questions above.
Not ready for review. Currently examining the approach.
This is a fairly direct port of the patch. PR comments indicate the areas I have questions.