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

wallet compatibility issue #1323

Merged
merged 12 commits into from Oct 16, 2018

Conversation

Projects
4 participants
@oxarbitrage
Member

oxarbitrage commented Sep 13, 2018

#1307

  • Expose account_id_to_string so it can be called from inside wallet api functions, this is not reflected to clients so they can't execute it from the command line.
  • Apply a fix to get_account_balances only as i want to make sure the solution is good enough before going forward.
  • Added Todo comments to remove code after hardfork. Will add this to all the calls that need to be modified.

Please review so i can apply in more places. Will add commits to this same pull request, 1 per function modified.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 13, 2018

Forgot to mention, this works in the nodes with the new version and also in older ones using account id or account_name.

@abitmore abitmore added this to In progress in Feature release (201810) via automation Sep 13, 2018

* @param id the ID of the account to be converted to string
* @returns string representation of account ID
*/
string account_id_to_string(account_id_type id) const;

This comment has been minimized.

@abitmore

abitmore Sep 13, 2018

Member

Since we already have a function in the impl class, I think we don't need it here.

This comment has been minimized.

@oxarbitrage

oxarbitrage Sep 14, 2018

Member

silly me, fixed here cab7ff4 thanks.

* Todo: remove the next 2 lines and change always_id to id in remote call after next hardfork
*/
auto account = get_account(id);
auto always_id = account_id_to_string(account.id);

This comment has been minimized.

@abitmore

abitmore Sep 13, 2018

Member

We can call my->account_id_to_string(id) directly.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 14, 2018

list of wallet calls to be reviewed/changed in this pull request to be more defensive until next HF:

  • resync - No changes needed
  • get_account - No changes needed
  • import_balance - No changes needed
  • create_committee_member
  • get_witness - No changes needed
  • get_committee_member - No changes needed
  • create_witness - No changes needed
  • get_vesting_balances
  • vote_for_committee_member
  • vote_for_witness
  • list_account_balances
  • get_account_history
  • get_account_history_by_operations
  • get_relative_account_history
@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 28, 2018

@abitmore i was able to finish reviewing and adding code for compatibility issue here. please take a look when you get the time. thank you very much.

@ryanRfox ryanRfox requested a review from jmjatlanta Oct 12, 2018

@jmjatlanta

The code looks good, just some long-ish lines. As GitHub limits us to 123 columns before we need to scroll horizontally, can you limit your code to match?

I tested some methods against MAINNET, and all methods listed in the ticket against TESTNET (node.testnet.bitshares.eu). All worked fine. I am not sure what version of witness_node that TESTNET node is running, but will test again if I find an older one.

@@ -2997,7 +3043,7 @@ vector<operation_detail> wallet_api::get_account_history(string name, int limit)
int page_limit = skip_first_row ? std::min( 100, limit + 1 ) : std::min( 100, limit );
vector<operation_history_object> current = my->_remote_hist->get_account_history( name, operation_history_id_type(),
vector<operation_history_object> current = my->_remote_hist->get_account_history( always_id, operation_history_id_type(),

This comment has been minimized.

@jmjatlanta

jmjatlanta Oct 12, 2018

It seems Github is limiting line lengths to 123 characters without scrolling. Please adjust ( it is currently cut off at operation_history_id_ty )

if(start == 0)
start = stats.total_ops;
else
start = std::min<uint32_t>(start, stats.total_ops);
while( limit > 0 )
{
vector <operation_history_object> current = my->_remote_hist->get_relative_account_history(name, stop, std::min<uint32_t>(100, limit), start);
vector <operation_history_object> current = my->_remote_hist->get_relative_account_history(always_id, stop, std::min<uint32_t>(100, limit), start);

This comment has been minimized.

@jmjatlanta

jmjatlanta Oct 12, 2018

Line length. Gitub cuts it off at "always_id, stop, std::min"

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 15, 2018

thank you for the test @jmjatlanta

Feature release (201810) automation moved this from In progress to In Testing Oct 15, 2018

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 15, 2018

Isn't problems with spacing fun? Here's a good (my opinion) talk on the subject: https://www.youtube.com/watch?v=ZsHMHukIlJY

Feature release (201810) automation moved this from In Testing to In progress Oct 15, 2018

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 15, 2018

@jmjatlanta i am all in favor of implementing code styles practices and clean code. Another one i like is "Clean Code. A Handbook of Agile Software Craftsmanship" by Robert C. Martin. It goes in a very similar direction.

I just changed some long function calls and definitions at 496c622
This is all arguments in new line and 6 spaces(2 tabs) indentation for them, is a method that looks clear and can scale; recommended in the video as an option.

We should discuss this and other similar at #1318
We cant follow any guide as a bible but we can take some ideas from them and write a set of rules for us.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 15, 2018

One of the things all clean code guides recommended is small functions that do only one thing. This is a rule that in our codebase is pretty hard to follow, i practiced this in one of the refactor of ES plugin, they are now all very short functions that try to do only one thing, only one return path and have the less number of arguments possible.
I think that is great but hard to apply everywhere.

Apart from that there are other rules we will find hard to follow, we need to make our own specific rules.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Oct 15, 2018

One of the things all clean code guides recommended is small functions that do only one thing. This is a rule that in our codebase is pretty hard to follow, i practiced this in one of the refactor of ES plugin, they are now all very short functions that try to do only one thing, only one return path and have the less number of arguments possible.
I think that is great but hard to apply everywhere.

Apart from that there are other rules we will find hard to follow, we need to make our own specific rules.

@oxarbitrage agree that small functions that do only one thing with as less args as possible is better than huge functions, etc.

In general the simplest solutions are the best always ! At the same time it isn't so easy to find the simplest solutions always :)

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 15, 2018

There was a lot of discussions at this year's cppcon about code simplicity. It was a big talking point for Herb Sutter and Kate Gregory.

The great things about standards is there are so many to choose from. :-D

I am all for what works, and what is maintainable. I am not for religious wars about whether a bracket should be on this line on the next. If we find something that works, stick to it the majority of the time, and are all understanding when we break the rules because of some legit reason, I'm okay.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 15, 2018

Yea, the brackets thing is just what i wrote here #1318 (comment)

To me they are all the same, however if we want to make some style guide we should select 1 form(or more, or allow all) and use it the majority of the times as you said. I am ok with that, don't need to be a bible.

@jmjatlanta

All looks good. Thanks @oxarbitrage

Feature release (201810) automation moved this from In progress to In Testing Oct 16, 2018

@oxarbitrage oxarbitrage merged commit 65f7eee into bitshares:develop Oct 16, 2018

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment