[refactor-prompt] Add wallet token send

[ixje]

What current issue(s) does this address, or what feature is it adding?
add wallet token send to move #623 forward

How did you solve this problem?
add command, refactor token_send

How did you make sure your solution works?
made tests, make test

Are there any special changes in the code that we should be aware of?
yes, refactored token_send to parameterise it. Also it's now throwing exceptions instead of printing errors because this is a typical function that others would potentially like to re-use. Print statements do not allow others to easily handle invalid arguments.

Please check the following, if applicable:

• Did you add any tests?
• Did you run make lint?
• Did you run make test?
• Are you making a PR to a feature branch or development rather than master?
• Did you add an entry to CHANGELOG.rst? (if not, please do)

TODO:

• remove isValidPublicAddress() helper function once a new neo-python-core release is out and update requirements.txt

[coveralls]

Coverage increased (+0.05%) to 84.996% when pulling d8e6e96 on ixje:token_send into 9f9b1fe on CityOfZion:refactor-prompt.

[jseagrave21]

I noticed that you are verifying the printouts. Should we have been doing that all along?

[ixje]

I personally find it more reliable. I think past reviews have shown that merely looking at a False result doesn't always mean that the business logic is failing on what you expected.

[jseagrave21]

That is true. Let me know in the reviews if you'd like to make these kind of tests standard. Also, if it is worth updating previous tests.

[ixje]

description to be fixed

[LysanderGG]

A couple of things to double check.

The parameter checking is pretty complete, maybe we should do a second path on the already migrated commands sometime.

[ixje]

maybe we should do a second path on the already migrated commands sometime.

I don't understand what you mean with "second path". Perhaps it's a typo?

[LysanderGG]

I meant "second pass" 😅
Going back to the already implemented commands to improve the parameters checking, so this is not for this PR in particular.

[ixje]

Are you referring to the repeated (pseudo):

if len(arguments) < count(mandatory args):
    error
if len(arguments) > count(mandatory + optional):
   error 

If so, I think we could put some helper function in the base class that iterates over the ParamDesc objects , counts the optional + mandatory args and boiler plates the above. I thought about it, but decided to skip for a later moment

[LysanderGG]

I mean as general, your verification code here seems way more robust/user friendly than in most of the other commands.

If so, I think we could put some helper function in the base class that iterates over the ParamDesc objects , counts the optional + mandatory args and boiler plates the above.

I did something similar some time ago but removed it because it was leading to some unreachable code
#733 (comment)

Let's add it to the todo and decide at the end.

Again this was not a problem about your PR but rather a good point that we should consider applying to the other commands 😄

[LysanderGG]

A couple of documentation errors

[LysanderGG]

I think it's None if it fails

[ixje]

do_token_transfer return False. See:

neo-python/neo/Prompt/Commands/Tokens.py

Lines 388 to 393 in 1b66e9f

	return False
	return InvokeContract(wallet, tx, fee)
	print("could not transfer tokens")
	return False

[LysanderGG]

My bad!
I checked something and it was returning None, I don't know what I checked 🤔