Improve parameter parsing [02]

jseagrave21 commented 5 years ago

What current issue(s) does this address, or what feature is it adding? NOTE: This PR is a continuation of #972 When manually testing contract invocation and build_run, I noticed that parameters entered via the command line were not parsed identically to those entered with the --i flag. This is because the --i flag used the gather_params helper function and the regular command line parsing did not.

How did you solve this problem? I split out the parameter verification into a separate function: verify_params and added a user_entry flag to limit verification to user entered params.

verifies parameters input via command line and --i flag
ensures parameter input is identical
does not disrupt existing logic which had previously caused many errors in #972

How did you make sure your solution works? manual testing

Are there any special changes in the code that we should be aware of? Yes, new parameter parsing affected some gas costs and one return value from previous tests. See https://github.com/CityOfZion/neo-python/pull/973/commits/efb6ef47887237822d8ad1c844b3a794255bcece I think these are good changes seeing as they mirror the results if the --i flag was used.

Please check the following, if applicable:

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

coveralls commented 5 years ago

Coverage increased (+0.01%) to 85.328% when pulling 23b50e49d5825b3df75fbee92386be9c1a0672b6 on jseagrave21:verify-params into cc6fd894f6c7c395a02e736e2a2b9c374b58a39a on CityOfZion:development.

jseagrave21 commented 5 years ago

@ixje please note make test passes locally. You can see the failure is being caused by the snapshot argument which seems to be from #974.

ixje commented 5 years ago

note to self; check ceil and gas change impact

jseagrave21 commented 5 years ago

@ixje BLUF: I think I have addressed all your comments. Please let me know if I am missing something.

Background: In your first comment, since the string 'bla' is not a bytearray or a bytes object, eval returns a NameError. I have included this case in testing and exceptions during eval or any other returns other than bytearray or bytes output will result in an Exception to provide a bytearray or bytes object.

I address both your second and third comments by incorporating exception handling into the user-entry process.

I was a little unsure of what you meant with Exception('Please provide a list'). This exception is now incorporated in the the exception handling which provides good feedback to the user.

Please let me know if I am missing something and thank you for your thorough review.

ixje commented 5 years ago

I think you're trying to resolve it in a too narrow context. Normally, it's good to not catch exceptions too 'broad', to make sure you don't have hidden issues. In this case I pointed at the difference of PrompUtils.gather_params() vs your verify_params for that specific reason.

The second issue I pointed out is not resolved

neo> sc build_run test-sc.py False False False fe10 02 bla []
Saved output to test-sc.avm
Could not execute command: local variable 'ptype' referenced before assignment
  File "/Users/erik/Documents/code/test-issues/seagrave/venv/bin/np-prompt", line 11, in <module>
    load_entry_point('neo-python', 'console_scripts', 'np-prompt')()
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/bin/prompt.py", line 382, in main
    loop.run_forever()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 523, in run_forever
    self._run_once()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 1758, in _run_once
    handle._run()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/events.py", line 88, in _run
    self._context.run(self._callback, *self._args)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/bin/prompt.py", line 234, in run
    traceback.print_stack()
Traceback (most recent call last):
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/Invoke.py", line 477, in test_deploy_and_invoke
    ptype = ContractParameterType(iarg)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 307, in __call__
    return cls.__new__(cls, value)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 555, in __new__
    return cls._missing_(value)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 568, in _missing_
    raise ValueError("%r is not a valid %s" % (value, cls.__name__))
ValueError: 254 is not a valid ContractParameterType

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/bin/prompt.py", line 220, in run
    cmd.execute(arguments)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/SC.py", line 44, in execute
    return self.execute_sub_command(item, arguments[1:])
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/CommandBase.py", line 75, in execute_sub_command
    return self.__sub_commands[id].execute(arguments)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/SC.py", line 80, in execute
    tx, result, total_ops, engine = BuildAndRun(arguments, PromptData.Wallet)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/BuildNRun.py", line 72, in BuildAndRun
    debug_map=debug_map, invoke_attrs=invoke_attrs, owners=owners, enable_debugger=enable_debugger)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/BuildNRun.py", line 96, in DoRun
    enable_debugger=enable_debugger, user_entry=True)
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Commands/Invoke.py", line 488, in test_deploy_and_invoke
    print("Could not parse param as %s : %s " % (ptype, e))
UnboundLocalError: local variable 'ptype' referenced before assignment

I also think we can improve the error message:

neo> sc build_run test-sc.py False False False 0710 02 bla hoi
Saved output to test-sc.avm
[I 190916 12:19:15 LevelDBImpl:45] Created DB at /Users/erik/.neopython/Chains/debugstorage
Could not parse param as Array : Please provide a list

which param is wrong?

CityOfZion / neo-python

Improve parameter parsing [02] #973