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

add last_vote_time to account stats object #1449

Merged
merged 6 commits into from
Dec 16, 2018

Conversation

oxarbitrage
Copy link
Member

Pull for #1393

I am not very sure about the default value of account parameter: https://github.com/oxarbitrage/bitshares-core/blob/5d1c15cdd7faa955463902ea1348ba302eb65681/libraries/chain/account_evaluator.cpp#L60

I did it that way so i can call verify_account_votes without the extra parameter at account creation: https://github.com/oxarbitrage/bitshares-core/blob/5d1c15cdd7faa955463902ea1348ba302eb65681/libraries/chain/account_evaluator.cpp#L162

Open to suggestions.

jmjatlanta
jmjatlanta previously approved these changes Nov 26, 2018
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.

Tested with Ubuntu 18.10 / Boost 1.67. Good coverge on Boost test. Thanks!

db.modify(stats_obj, [&](account_statistics_object &obj) {
obj.last_vote_time = db.head_block_time();
});
}
Copy link
Member

@abitmore abitmore Nov 26, 2018

Choose a reason for hiding this comment

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

This means every time when do_evaluate is called (E.G. by validate_transaction API ?), the data will be updated. But do_evaluate can fail.

Copy link
Member

Choose a reason for hiding this comment

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

Usually as an unwritten principle we don't modify database in do_evaluate, but probably it's practically fine to do so. @pmconrad your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that do_evaluate must not modify the database.
IMO it's a good design decision to separate the various stages of operation evaluation, and we should stick with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

got it, thanks. ill refactor to do the update in do_apply or something different.

@oxarbitrage oxarbitrage mentioned this pull request Nov 29, 2018
34 tasks
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.

Need to bump GRAPHENE_CURRENT_DB_VERSION when adding a field

// last_vote_time is not updated
now = db.head_block_time().sec_since_epoch();
alice_stats_obj = db.get_account_stats_by_owner(alice_id);
BOOST_CHECK(alice_stats_obj.last_vote_time.sec_since_epoch() != now);
Copy link
Contributor

Choose a reason for hiding this comment

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

better check that last_vote_time equals the previous now

@pmconrad pmconrad added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 9a Tiny Effort estimation indicating TBD labels Dec 14, 2018
@pmconrad pmconrad added this to the Future Feature Release milestone Dec 14, 2018
pmconrad
pmconrad previously approved these changes Dec 15, 2018
@pmconrad
Copy link
Contributor

Looks like you have to rebase to avoid the conflict

@oxarbitrage
Copy link
Member Author

fixed conflict in database bump number, need to re approve. thanks.

@oxarbitrage oxarbitrage merged commit ae1cbfa into bitshares:develop Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 9a Tiny Effort estimation indicating TBD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants