Add support for wallet send, sendmany, sign, open, and close

[jseagrave21]

What current issue(s) does this address, or what feature is it adding?

• supports Refactor prompt commands to be plugin based #623
• adds support for wallet send, sendmany, sign, open, and close

How did you solve this problem?
Trial and Error

How did you make sure your solution works?
manual testing. I haven't added coverage yet.

-update-

I added coverage for send and sendmany. Let me know what you think.

Are there any special changes in the code that we should be aware of?
No. But please check my implementation to see if I am on the right track.

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)

[coveralls]

Coverage increased (+0.3%) to 83.55% when pulling d870595 on jseagrave21:jseagrave21-send,sendmany,sign,open,close into 0d41b7e on CityOfZion:refactor-prompt.

[LysanderGG]

But please check my implementation to see if I am on the right track.

Yes, only a few nitpicks but the general implementation is the way to go.

Thank you for fixing the self.Wallet and other things that had to moved to PromptData !

[LysanderGG]

Can't close be done with other subcommands because it needs a wallet to be opened?

[jseagrave21]

I don't think so because this follows the original implementation (i.e. wallet close closes the open wallet)

[LysanderGG]

CommandWalletClose.execute starts with if PromptData.Wallet:.
The difference will be that instead of silently doing nothing if no wallet is opened, it would print a message.

[jseagrave21]

I guess I am not understanding 🙁 Could you message me on Discord or show me what you mean?

[LysanderGG]

You added close as a "specially handled command" instead of a "normal command".

So for now, there is

elif item == 'close':
    self.execute_sub_command(item)
    return

before

if not wallet:
    print("Please open a wallet")
    return

If you do not handle close a special way, it would be handled in

try:
    self.execute_sub_command(item, arguments[1:])
except KeyError:
    print(f"Wallet: {item} is an invalid parameter")

Therefore we would have the 2 cases:

• a wallet is opened: works as now
• no wallet is opened: print("Please open a wallet") would be executed instead of nothing.

The complete implementation of execute() could be:

def execute(self, arguments):
    wallet = PromptData.Wallet
    item = get_arg(arguments)

    # Create and Open must be handled specially.
    if item in {'create', 'open'}:
        self.execute_sub_command(item, arguments[1:])
        return

    if not wallet:
        print("Please open a wallet")
        return

    if not item:
        print("Wallet %s " % json.dumps(wallet.ToJson(), indent=4))
        return

    try:
        self.execute_sub_command(item, arguments[1:])
    except KeyError:
        print(f"Wallet: {item} is an invalid parameter")

If it is still not clear (I don't find a good way to explain), let's talk on Discord later on.

[jseagrave21]

Okay, I think I understand. You are saying to just keep close as a normal subcommand. Okay. Lemme see what I can do. Thank you for explaining! 🙏

[LysanderGG]

We could regroup all the commands in a set and check if item in xxx. (if close can be moved out, see after)

[LysanderGG]

Should not be @classmethod and use self instead of cls.

[jseagrave21]

See https://github.com/CityOfZion/neo-python/pull/726/files#r236708544

[jseagrave21]

What do you guys think of the method of testing I implemented? I could continue this method of testing for the other methods introduced to ensure coverage is maintained as much as possible while refactoring is completed.

NOTE: Coverage for parse_and_sign will be implemented from #716

[LysanderGG]

I left a few more comments, please let me know what you think.

What do you guys think of the method of testing I implemented?

It looks good to me but I think @ixje should confirm because I haven't looked at the tests before.

[LysanderGG]

Maybe the implementation could be moved to a static method or to PromptData.
It could avoid having to instanciate CommandWalletClose to call execute. (CommandWalletClose().execute() in prompt.py).

What do you think?

[jseagrave21]

This is why I originally had it as a class method. But changed it after your review. Couldn't we just keep it as a class method?

[jseagrave21]

A static method still required me to instantiate the class

[LysanderGG]

I realize that my message was probably confusing, sorry for that.

I was suggesting to keep execute as is (to override execute(self arguments)) and have it call another static method.
Imo a @classmethod should only be used when you want to override it in a child class or when you need to have access to cls.

I think the following should work:

class CommandWalletClose(CommandBase):
...
def execute(self, arguments=None):
    CommandWalletClose.close_wallet()


@staticmethod
def close_wallet():
    if not PromptData.Wallet:
        return False

    path = PromptData.Wallet._path
    PromptData.Prompt.stop_wallet_loop()
    PromptData.Wallet.Close()
    PromptData.Wallet = None
    print("Closed wallet %s" % path)
    return True

It should be possible to call it with CommandWalletClose.close_wallet().
I think it would be even better to have this function in PromptData and call PromptData.close_wallet().

[jseagrave21]

@LysanderGG Thank you. I will give it a shot. I will try it your preferred way.

[LysanderGG]

In my opinion, the original signature execute(self, arguments) should be respected. None could be given as arguments for commands like CommandWalletClose.

[ixje]

the execute() method does not have an enforced return type. Use this to your advantage to return the normal res value from process_transaction as in the original tests
here

this way you don't have to patch stdout and you have far more options for testing correct behaviour. We now threw away good logic in favour of trusting a printed message indicating all went right.

Note that the actual goal isn't coverage. The goal is testing correct behaviour of the functions/modules under different circumstances. coverage is just an indicator that can tell you if you missed any obvious parts in your test scenario's.

• Not having full coverage does indicate you've not tested all scenario's.
• Having full coverage does not indicate you have tested all scenario's.

[jseagrave21]

I tried this originally and all it ever returned was None. Also pylint warned about it in VSC. Do you have any hints?

This is why I moved the test logic to the next test. So, this basic test shows everything works and then I test the original functions thoroughly because they return values other than None

[ixje]

@LysanderGG if you read my comment above you see I assumed we could use the return value of execute(). Atm this doesn't work because for the CommandBase.execute_sub_command doesn't return the value. Do you see/have any objections for changing the behaviour of that? I don't have a full clear mind at this point to see possible limitations and you know this code the best :)

If we return the results of execute commands they become more obvious to test. The other approach is just directly calling a the command class. An example test of the former is:

                    args = ['send', 'neo', self.watch_addr_str, '50']                    
                    command_wallet = Wallet.CommandWallet()
                    res = command_wallet.execute(args)

an example of the latter is

                    args = ['neo', self.watch_addr_str, '50']                    
                    command_wallet_send = Wallet.CommandWalletSend()
                    res = command_wallet_send.execute(args)

[LysanderGG]

@ixje No objection to return the value. I didn't do it because I couldn't think of a use case where it would be useful.

[jseagrave21]

@LysanderGG I have implemented the ability to return data which is very useful for testing. see #726 (comment)
Additionally, I have refactored test_send_command.py and added tests for wallet open and wallet close to test_wallet_commands.py. Also, I introduced what I think would be good return values for your wallet commands. I am open to your suggestions of course. Also, will you be creating the testing for the wallet commands you refactored?

[LysanderGG]

@jseagrave21 Thank you!
I added one comment about CommandWalletClose, the rest looks gtm!

Also, will you be creating the testing for the wallet commands you refactored?

Yes I will do it.
Thank you for clearing out the path for tests!

[jseagrave21]

@LysanderGG @ixje Okay, this PR is ready for a [final] review 😉

[LysanderGG]

Because construct_send_basic can return [contract_tx, scripthash_from, fee, owners, user_tx_attributes] it should return None or False in early returns.

[jseagrave21]

@LysanderGG
@ixje has explained to me that it is undesirable to have a function return both a boolean and say a function. Instead if you just leave return blank, it returns none, which will also be interpreted as False, so all my tests pass.

[jseagrave21]

see #716 (comment)

[LysanderGG]

I agree for boolean and something else (I suggested False because it was this way before).
I think it should return None explicitly then.

When I see return (nothing) I assume that the function doesn't return anything. It can be misleading imo.

@ixje any thoughts?

[ixje]

I'm indifferent when it comes to return or return None. I guess I'm used to the idea that return always returns None under the hood, so to speak. Might as well make it explicit if that clarifies it for all future readers. I'll probably need to slapping on the fingers to remind me of doing that though ;)

[ixje]

The add a few more words regarding return values; at some point I expect to start applying gradual typing. We've had past cases where functions were returning None, bool or a tuple. That's just not workable.

[LysanderGG]

Thanks for the answer, I won't make this as a blocker for this PR, so @jseagrave21 as you want 😄

[jseagrave21]

I would like to leave it this way so the format is consistent with #716. Consistency is what matters most to me so people will be able to follow our example throughout the code.

[LysanderGG]

Should return something here too (and other early returns).

[jseagrave21]

#726 (comment)

[LysanderGG]

Should return something here too (and other early returns).

[jseagrave21]

#726 (comment)

[LysanderGG]

I approve the code, I didn't review the tests.
Thank you very much for your help on this!

Sorry for all the nitpicking 😅

[ixje]

@jseagrave21 don't worry, I'm going to merge in a minute and will then update these return parts as I want to update a couple of descriptions. We learn as we go, and while reviewing and testing your PR I noticed the following below

The readability isn't optimal, so we need to present it differently. I tried out some formatting and came to this.

Nothing you could necessarily do about upfront and no need for you to update this as I already have the code. Just take note of how the commands are labelled as well as how their descriptions look for future commits.

[ixje]

@LysanderGG no worries. I appreciate a critical look. Keeps us all sharp and in the end improves overall quality.

[jseagrave21]

@LysanderGG @ixje thank you for your thorough reviews. Great team effort! 🎉