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.

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

Change call_order_update_operation to return order_id #1269 #1352

Merged
merged 2 commits into from Oct 1, 2018

Conversation

cogutvalera
Copy link
Member

PR for #1269 "Change call_order_update_operation to return order_id"

Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

I am not sure what should be done in this case. Should we be returning an ID of something that no longer exists? If no, what should we return?


if( new_debt == 0 )
{
FC_ASSERT( new_collateral == 0, "Should claim all collateral when closing debt position" );
d.remove( *call_obj );
return void_result();
return call_order_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return the id of a deleted object?

Copy link
Member Author

Choose a reason for hiding this comment

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

why not ? :) IMHO the id of a deleted object may be helpful for looking history of that object, even it is better than return nothing ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, makes sense to return it.

@@ -264,13 +267,12 @@ void_result call_order_update_evaluator::do_apply(const call_order_update_operat
_bitasset_data->current_feed.maintenance_collateral_ratio );
call.target_collateral_ratio = o.extensions.value.target_collateral_ratio;
});
call_order_id = call_obj->id;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is superfluous, modify() will not change the id.

@cogutvalera
Copy link
Member Author

cogutvalera commented Oct 1, 2018

@pmconrad @jmjatlanta

any other changes must be here or we can approve and merge this PR ?

Thanks a lot !

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Thanks!

@pmconrad pmconrad merged commit 293e8da into bitshares:develop Oct 1, 2018
@cogutvalera
Copy link
Member Author

Thanks for review ! Thank you very much for your time and efforts !

@abitmore abitmore added this to In progress in Feature release (201810) via automation Oct 1, 2018
@abitmore abitmore added this to the 201810 - Feature Release milestone Oct 1, 2018
@abitmore abitmore moved this from In progress to Done in Feature release (201810) Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants