Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Improve parameter parsing [02] #973

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

jseagrave21
Copy link
Contributor

@jseagrave21 jseagrave21 commented Jun 29, 2019

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 Improve parameter parsing #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 efb6ef4
I think these are good changes seeing as they mirror the results if the --i flag was used.

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
Copy link

coveralls commented Jun 29, 2019

Coverage Status

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

expected_cost = Fixed8(1046600000)
expected_gas = Fixed8.FromDecimal(1.0)
expected_cost = Fixed8.FromDecimal(15.466)
expected_gas = Fixed8.FromDecimal(6.0)
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why the GAS costs rises with 5 GAS? That really seems like a lot for (assuming) just an input type parsing difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is the original code pushed the big_str as bytes and the updated code pushes it as a bytearray. Here are the input values just prior to being pushed onto the stack:

Original:

b'abababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababab' Length: 2048
b'6b657931' Length: 8
b'7075745f35' Length: 10

Updated:

bytearray(b'abababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababab') Length: 2048
b'6b657931' Length: 8
b'7075745f35' Length:10

Copy link
Member

Choose a reason for hiding this comment

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

Using the development branch, I get a result of 1.0 GAS used when invoking with b’abababab…' and 6.0 GAS used when invoking with b’ghghghgh…' so 5 GAS is the difference between parsing that size of an argument as a hex string vs parsing it as an ascii string. The changes in the test_gas_costs.py would seem to be correct if the parser is no longer arbitrarily converting strings to binary just because they look like hexadecimal values.

- based on discussion from @hal0x2328 and @ixje in Slack
- adding to PR CityOfZion#973 because the change is so small
@jseagrave21
Copy link
Contributor Author

@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
Copy link
Member

ixje commented Jul 22, 2019

note to self; check ceil and gas change impact

@@ -331,7 +345,6 @@ def test_invoke(script, wallet, outputs, withdrawal_tx=None,
# print("Used %s Gas " % engine.GasConsumed().ToString())

consumed = engine.GasConsumed() - Fixed8.FromDecimal(10)
consumed = consumed.Ceil()
Copy link
Member

Choose a reason for hiding this comment

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

https://docs.neo.org/docs/en-us/tooldev/advanced/tx_execution.html#b02a262b

Assign Math.max(0, GasConsumed - 10).Ceiling() to InvocationTransaction.Gas as transaction's system fee. Note that system provides 10 Gas for free in each InvocationTransaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hal0x2328 could you record your findings for GAS costs from Slack here for discussion? It would appear either the current implementation of neo-python (with Ceil rounding) is correct per the above documentation, or the documentation is outdated and no longer being utilized in neo-cli.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. In that case, I will remove this change.

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

While testing

neo> sc build_run test-sc.py Fale False False 0510 02 bla [1234]
Saved output to test-sc.avm
Could not execute command: name 'bla' is not defined
  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/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 473, in test_deploy_and_invoke
    param, abort = PromptUtils.verify_params(ContractParameterType(iarg), invoke_args[index])
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Utils.py", line 325, in verify_params
    res = eval(param, {"__builtins__": {'bytearray': bytearray, 'bytes': bytes}}, {})
  File "<string>", line 1, in <module>
NameError: name 'bla' is not defined

test-sc.py

def Main(operation, args):
    if args[0] ==b'\x01\x02\x03\x04':
        return 1
    return 0

For some reason we must now wrap strings in double quotes "", so to fix this you must wrap bla in quotes. This was previously not necessary.


Also, if you provide a data that does not match the specified contract parameter type it used to throw exceptions that got caught and replaced with a warning e.g.

neo> sc build_run test-sc.py Fale False False 0510 02 "bla" "hoi"
Saved output to test-sc.avm
[I 190910 14:40:10 LevelDBImpl:45] Created DB at /Users/erik/.neopython/Chains/debugstorage
run `sc build_run help` to see supported queries

now it throws exceptions:

neo> sc build_run test-sc.py Fale False False 0510 02 "bla" "hoi"                                                                                                                                                                                                                               
Saved output to test-sc.avm 
Could not execute command: Please provide a list
  File "/Applications/PyCharm.app/Contents/helpers/pydev/pydevd.py", line 2060, in <module>
    main()
  File "/Applications/PyCharm.app/Contents/helpers/pydev/pydevd.py", line 2054, in main
    globals = debugger.run(setup['file'], None, None, is_module)
  File "/Applications/PyCharm.app/Contents/helpers/pydev/pydevd.py", line 1405, in run
    return self._exec(is_module, entry_point_fn, module_name, file, globals, locals)
  File "/Applications/PyCharm.app/Contents/helpers/pydev/pydevd.py", line 1412, in _exec
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "/Applications/PyCharm.app/Contents/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
  File "./neo/bin/prompt.py", line 409, in <module>
    main()
  File "./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 "./neo/bin/prompt.py", line 234, in run
    traceback.print_stack()
Traceback (most recent call last):
  File "./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 473, in test_deploy_and_invoke
    param, abort = PromptUtils.verify_params(ContractParameterType(iarg), invoke_args[index])
  File "/Users/erik/Documents/code/test-issues/seagrave/neo/Prompt/Utils.py", line 338, in verify_params
    raise Exception("Please provide a list")
Exception: Please provide a list

I'm actually not happy with the error message it used to print either as it's useless. The example uses a str as second argument ("hoi") while it expects an array type (as indicated by contract parameter type 10). There is more information available in the code

raise Exception("Please provide a list")
else:
raise Exception("Unknown param type %s " % ptype.name)

but we're not using it.


another issue

neo> sc build_run test-sc.py Fale False False 0512 02 "bla" hoi
Saved output to test-sc.avm
[I 190910 14:48:09 LevelDBImpl:45] Created DB at /Users/erik/.neopython/Chains/debugstorage
Could not execute command: 18 is not a valid ContractParameterType
  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/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 473, in test_deploy_and_invoke
    param, abort = PromptUtils.verify_params(ContractParameterType(iarg), invoke_args[index])
  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: 18 is not a valid ContractParameterType

It shouldn't throw exceptions but just use the exception information and notify that we have specified an invalid contract type.


overall

param, abort = PromptUtils.verify_params(ContractParameterType(iarg), params[index])

does not handle exceptions at all, where as the originally used PrompUtils.gather_params does
except KeyboardInterrupt: # Control-C pressed: exit
return None, True
except Exception as e:
print("Could not parse param as %s : %s " % (ptype, e))
if do_continue:
return gather_param(index, param_type, do_continue)

That's just what I happen to find and it is already making me uncomfortable

jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this pull request Sep 12, 2019
@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Sep 12, 2019

@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.

if isinstance(res, list):
return res, False
except Exception:
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pass is necessary in case there is no exception thrown but eval does not evaluate a list object.

jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this pull request Sep 12, 2019
@ixje
Copy link
Member

ixje commented Sep 16, 2019

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants