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

add --return-type to override test invoke return parameter type #913

Merged
merged 4 commits into from
Mar 22, 2019
Merged

add --return-type to override test invoke return parameter type #913

merged 4 commits into from
Mar 22, 2019

Conversation

hal0x2328
Copy link
Member

What current issue(s) does this address, or what feature is it adding?
Contract invocation results are displayed using the default return type of the contract, even though some operations will return different types than the default, requiring the user to manually convert results, e.g. from bytearray to integer

How did you solve this problem?
Added a --return-type argument to the sc invoke command to allow the user to override the contract's default return type during a test invoke

How did you make sure your solution works?
Manual testing

Are there any special changes in the code that we should be aware of?
No

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)

Copy link
Contributor

@jseagrave21 jseagrave21 left a comment

Choose a reason for hiding this comment

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

This is a great addition. Will the code support the string equivalents of the ParameterTypes? I ask because I know it is a goal for users to be able to specify the input and output types with their string equivalents (i.e. "Integer" is acceptable, not just "02")
See #781

for item in params:
if '--return-type' in item:
params.remove(item)
return_type = item.replace('--return-type=', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend adding some logic here to ensure only acceptable ParameterTypes are allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked myself when adding this option, does it add more value to add some sort of input sanity check rather than just educating the user through hitting an exception when they enter a bad parameter type? This is for advanced users after all. Personally I like the feedback loop approach, it doesn't really have any negative impact and it cuts down on the amount of code that needs to be maintained.

Copy link
Member

Choose a reason for hiding this comment

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

While true that it reduces code maintenance, we put in a lot of effort to get rid of exception throwing due to user input errors when we refactored the cli. I personally don't think that an advanced option or tool justifies not handling exceptions. In this case it's also as simple as doing

try:
   [ContractParameter.AsParameterType(ContractParameterType.FromString(return_type), item).ToJson() for item in results]
except ValueError:
    # inform user

@hal0x2328
Copy link
Member Author

hal0x2328 commented Mar 17, 2019

This is a great addition. Will the code support the string equivalents of the ParameterTypes? I ask because I know it is a goal for users to be able to specify the input and output types with their string equivalents (i.e. "Integer" is acceptable, not just "02")
See #781

It does support using the string equivalents but I don't use those in the example because the deployment convention has always been hex notation, both in neo-gui and neo-python, and all the documented examples use the hex notation. Unless it changes everywhere users are still going to need to know the mapping between hex and string parameter notation.

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.

See #913 (comment)
Please add exception handling and add a test case for coverage. Thanks!

@hal0x2328
Copy link
Member Author

Added exception handler and unit tests.

@hal0x2328 hal0x2328 requested a review from ixje March 22, 2019 14:37
@ixje
Copy link
Member

ixje commented Mar 22, 2019

Thanks!

@ixje ixje merged commit f467f5e into CityOfZion:development Mar 22, 2019
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.

3 participants