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

Require voted entity exists (issue #143) #348

Merged
merged 3 commits into from
Sep 6, 2017

Conversation

takaaki7
Copy link
Contributor

@takaaki7 takaaki7 commented Aug 5, 2017

This PR is work in progress, compilable but have not yet been tested.
I think this simple validation may prevent voting for non-existent entity. (#143)
And also we should check that all votes are unique.(account_options.votes is a flat_set so uniqueness check is unnecessary. )
May I go on with this plan?

const auto& committee_idx = db.get_index_type<committee_member_index>().indices().get<by_vote_id>();
const auto& witness_idx = db.get_index_type<witness_index>().indices().get<by_vote_id>();
for ( auto id : options.votes ) {
switch ( id ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the switch should operate on id.type()

@pmconrad
Copy link
Contributor

pmconrad commented Aug 5, 2017

Of course you may. Thank for taking up the task!

@takaaki7
Copy link
Contributor Author

I've modified!

  • Voted committees/witnesses counts are validated in account.cpp, so not validate here.
  • To avoid duplicating hardfork number with hardforks created in previous repository, append "-2" suffix to issue number.
  • Hardfork time is set at 08/28/2017 @ 12:00am (UTC).

Tested by using original func and vote for non-exist entities as follows.

signed_transaction vote_by_id(string voting_account,
                                        string vote_id,
                                        bool approve,
                                        bool broadcast /* = false */)
{ try {
      account_object voting_account_object = get_account(voting_account);
      voting_account_object.options.votes.insert(vote_id_type(vote_id));
      account_update_operation account_update_op;
      account_update_op.account = voting_account_object.id;
      account_update_op.new_options = voting_account_object.options;

      signed_transaction tx;
      tx.operations.push_back( account_update_op );
      set_operation_fees( tx, _remote_db->get_global_properties().parameters.current_fees);
      tx.validate();

      return sign_transaction( tx, broadcast );
} FC_CAPTURE_AND_RETHROW( (voting_account)(vote_id)(approve)(broadcast) ) }
unlocked >>> vote_by_id nathan "0:10" true true
vote_by_id nathan "0:10" true true
889304ms th_a       wallet.cpp:1908               sign_transaction     ] Caught exception while broadcasting tx 075f27c618eefbce33e149c15ea167f791d860c2:  0 exception: unspecified
Assert Exception: committee_idx.find(id) != committee_idx.end():

@takaaki7 takaaki7 changed the title Require voted entity exists (issue #143)[WIP] Require voted entity exists (issue #143) Aug 13, 2017
@pmconrad
Copy link
Contributor

Good job, thanks!
Tested with HF date in the future - replays successfully
Tested with HF date before 02/2016 - replay fails
Tested with HF date in June 2016 - replays successfully

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.

Please rename the constant to HARDFORK_CORE_143_TIME to make it clear that the number refers to the bitshares-core issue.

@takaaki7
Copy link
Contributor Author

I'm sorry for being too late. I've fixed it.

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.

No worries - thanks!

Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

sorry for the delay. everything looks good, code looks good, test cases also good.
made the same tests as @pmconrad and getting the same results. approved.

@oxarbitrage oxarbitrage merged commit 7863d30 into bitshares:develop Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants