Adds `sendmany` feature to Prompt.py #610

jseagrave21 · 2018-09-14T07:28:54Z

What current issue(s) does this address, or what feature is it adding?
Adds sendmany feature to Prompt.py including options for from-addr and change-addr and all previous supported features from send command (e.g. support for multi-sig wallets and user attributes)

How did you solve this problem?
Trial and Error

How did you make sure your solution works?
Quite a bit of personal testing on privatenet and implemented a few tests.

NOTE: A couple of tests rely on WalletFixtureTestCase

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)

To support SendMany

Supports sendmany

To support sendmany

coveralls · 2018-09-14T08:56:36Z

Coverage increased (+0.5%) to 82.042% when pulling 25b9e71 on jseagrave21:SendMany_Prompt into 97fda32 on CityOfZion:development.

jseagrave21 · 2018-09-14T23:24:35Z

@ixje Thank you for telling me about mock!

ixje

This PR seems to have a lot of code duplication from #596 but then for the prompt. It's best to split the common code pieces and make that callable from both the prompt and RPC side.

fix compatibility issues

for compatibility

Merge CoZ Development into jseagrave21 SendMany_Prompt

- implement `construct_send_basic`, `construct_send_many`, and `process_transaction`

- Adds tests for `construct_send_basic`, `construct_send_many`, and overall tests - Improves test coverage

- implement new `send` and `sendmany` functions

- Fix sendmany token test - Add tests for bad precision - Add weird outgoing amount test - Add 'could not send' test

jseagrave21 · 2018-10-05T09:11:17Z

@ixje This PR is ready for another review. Please let me know if I am on the right track.

ixje

I've not looked into your test-cases at this point because I feel that the requested changes need to be addressed first. I'm assuming many of the tests now pass because you use a catch-all try/except structure that filters out some of the real exceptions that should be addressed (like the return signature)

ixje · 2018-10-08T20:05:39Z

neo/Prompt/Utils.py

+        if '--change-addr=' in item:
+            to_remove.append(item)
+            try:
+                change_addr = item.replace('--change-addr=', '')


if the return of this functions expects at best 1 change_addr then we do not need the to_remove list and loop over it. We can do something like:

for item in params: change_addr = None if '--change-addr' in item: params.remove(item) change_addr = item.replace('--change-addr=','') return params, change_addr

This way we stop the moment we found the a change address and avoid unnecessary looping over the remaining params.

What is the Try/except catching exactly? It's generally bad practice to use Exception (==catch all) and we should prefer being more specific (like ValueErroror TypeError)

The format for get_change_addr followed the example of the pre-existing get_from_addr. I will work on improving these utility functions.

ixje · 2018-10-08T20:12:18Z

neo/Prompt/Utils.py

+    return params, change_addr
+
+
+def get_outgoing(params):


same comments as for get_change_addr

if we only return 1 outgoing we don't need to push 1 item in a list.

try to make the exception specific

I think I addressed this issue.

ixje · 2018-10-08T20:13:13Z

neo/bin/prompt.py

-        construct_and_send(self, self.Wallet, arguments)
+        try:
+            contract_tx, scripthash_from, fee, owners, user_tx_attributes = construct_send_basic(self, self.Wallet, arguments)
+        except Exception as e:


what specific errors are we catching?

ixje · 2018-10-08T20:13:22Z

neo/bin/prompt.py

+    def do_send_many(self, arguments):
+        try:
+            contract_tx, scripthash_from, scripthash_change, fee, owners, user_tx_attributes = construct_send_many(self, self.Wallet, arguments)
+        except Exception as e:


same as above

ixje · 2018-10-08T20:17:58Z

neo/Prompt/Commands/Send.py

            return False

+    output = TransactionOutput(AssetId=assetId, Value=f8amount, script_hash=scripthash_to)
+    contract_tx = ContractTransaction(outputs=[output])
+    return contract_tx, scripthash_from, fee, owners, user_tx_attributes


the return signature of a function must be concise. Meaning you can't have success return a 5 argument tuple while failure returns a single boolean. This will exceptions when you try to use the command as follows:

contract_tx, scripthash_from, fee, owners, user_tx_attributes = construct_send_basic(prompt, wallet, bad_args)

bad_args will cause the function to returns False and then you'll get an exception saying something like (from the top of my head):

TypeError: 'bool' object is not iterable

because it's expecting an tuple (which is iterable) to unpack 5 values from and instead gets a boolean

ixje · 2018-10-08T20:18:21Z

neo/Prompt/Commands/Send.py


        print("sending with fee: %s " % fee.ToString())
+    return contract_tx, scripthash_from, scripthash_change, fee, owners, user_tx_attributes


same remark on function return signature as before

for compatibility

- test new `send` and `sendmany` implementation

- Implement new `send` and `sendmany` methods

- Remove unnecessary try/except logic

jseagrave21 · 2018-10-09T09:56:21Z

@ixje Okay, this PR is ready for another review

ixje

I need some input how to correctly operate the command as I can't seem to get it working. I want to try using and abusing it to see it's behaviour

ixje · 2018-10-09T17:43:25Z

neo/bin/prompt.py

@@ -142,6 +142,7 @@ class PromptInterface:
                'wallet split {addr} {asset} {unspent index} {divide into number of vins}',
                'wallet close',
                'send {assetId or name} {address} {amount} (--from-addr={addr}) (--fee={priority_fee})',
+                'sendmany (--outgoing={number outgoing}) (--change-addr={addr}) (--from-addr={addr}) (--fee={priority_fee})',


we have to think of a clearer way to explain this function. I had to read the code to understand that outgoing is a number that collects the outputs_array of the C# description. For command arguments placing something between () parentheses means it's an optional argument. I'm pretty sure that's not the case here. I also can't get it to work:

neo> sendmany 0 neo neo Asset id not found neo> sendmany 1 neo neo Asset id not found neo> sendmany 1 neo Not enough arguments neo> sendmany 1 neo wut Asset id not found neo> sendmany Not enough arguments neo> sendmany --outgoing=0 neo bla Asset id not found

--outgoing is the only required command because it sets up how many iterations (transactions) prompt will ask for. I will update with what might be a better description. The test send_many_good_simple illustrates how the function should work https://github.com/jseagrave21/neo-python/blob/b528e230e64ead9da59ed696f14023b6d5039222/neo/Prompt/Commands/tests/test_send_command.py#L336

You can see that after neo> sendmany the only arg passed is 2 https://github.com/jseagrave21/neo-python/blob/b528e230e64ead9da59ed696f14023b6d5039222/neo/Prompt/Commands/tests/test_send_command.py#L340

Everything that follows is prompted.

ixje · 2018-10-09T17:50:39Z

neo/Prompt/Commands/Send.py

-                                     change_address=None,
-                                     fee=fee,
-                                     from_addr=scripthash_from)
+def process_transaction(prompter, wallet, contract_tx, scripthash_from=None, scripthash_change=None, fee=None, owners=None, user_tx_attributes=None):


can you please remove the prompter argument. It's not used. Same goes for all other functions in the Send file, they have prompter argument that's not used

- make tests compatible with Send.py without `prompter` argument

- remove the `self` argument from `construct_send_basic`, `construct_send_many`, `process_transaction`, and `parse_and_sign`

- remove `get_outgoing` logic and replace with `get_arg`

- update for simplified outgoing logic

- fix indentation so `sendmany` iterates correctly

jseagrave21 · 2018-10-09T23:45:01Z

@ixje Okay, I think this PR is ready for another review. Below is what I envision a complex implementation of sendmany should look like in prompt:

neo> sendmany 2 --change-addr=AWygZ1B5c3GDiLL6u5bHSVU45Ya1SVGX9P --from-addr=AK2nJJpJr6o664CWJKi1QRXjqeic2zRp8y --fee=0.
005
Outgoing Number  1
Asset to send: neo
Address to: APRgMZHZubii29UXF9uFa6sohrsYupNAvx
Amount to send: 500
Outgoing Number  2
Asset to send: gas
Address to: APRgMZHZubii29UXF9uFa6sohrsYupNAvx
Amount to send: 100
sending with fee: 0.005
[Password]> ***
[I 181009 16:39:06 Transaction:617] Verifying transaction: b'fef0bdb63d6e16141b839ef730f09519e47657aba1acce3acbbb922cd9a33140'
Relayed Tx: fef0bdb63d6e16141b839ef730f09519e47657aba1acce3acbbb922cd9a33140

neo> tx fef0bdb63d6e16141b839ef730f09519e47657aba1acce3acbbb922cd9a33140
{
    "txid": "0xfef0bdb63d6e16141b839ef730f09519e47657aba1acce3acbbb922cd9a33140",
    "size": 437,
    "type": "ContractTransaction",
    "version": 0,
    "attributes": [
        {
            "usage": 32,
            "data": "23ba2703c53263e8d6e522dc32203339dcd8eee9"
        }
    ],
    "vout": [
        {
            "n": 0,
            "asset": "0xc56f33fc6ecfcd0c225c4ab356fee59390af8560be0e930faebe74a6daff7c9b",
            "value": "500",
            "address": "APRgMZHZubii29UXF9uFa6sohrsYupNAvx"
        },
        {
            "n": 1,
            "asset": "0x602c79718b16e442de58778e148d0b1084e3b2dffd5de6b7b16cee7969282de7",
            "value": "100",
            "address": "APRgMZHZubii29UXF9uFa6sohrsYupNAvx"
        },
        {
            "n": 2,
            "asset": "0xc56f33fc6ecfcd0c225c4ab356fee59390af8560be0e930faebe74a6daff7c9b",
            "value": "99999390",
            "address": "AWygZ1B5c3GDiLL6u5bHSVU45Ya1SVGX9P"
        },
        {
            "n": 3,
            "asset": "0x602c79718b16e442de58778e148d0b1084e3b2dffd5de6b7b16cee7969282de7",
            "value": "25903.995",
            "address": "AWygZ1B5c3GDiLL6u5bHSVU45Ya1SVGX9P"
        }
    ],
    "vin": [
        {
            "txid": "0x205d2a897b8adc1e1401a04c20a018eff5cf5324833f37c839aeb8a96f62fcd4",
            "vout": 2
        },
        {
            "txid": "0x205d2a897b8adc1e1401a04c20a018eff5cf5324833f37c839aeb8a96f62fcd4",
            "vout": 3
        }
    ],
    "sys_fee": "0",
    "net_fee": "0.005",
    "scripts": [
        {
            "invocation": "40690317762c605c1ee412bf5c3ce61dddf55c61aa47532babdf67c6a75582a4e371900f22eed299a5f7aad08c2258b3368207bee7fb7616369f944169b5208fbf",
            "verification": "21031a6c6fbbdf02ca351745fa86b9ba5a9452d785ac4f7fc2b7548ca2a46c4fcf4aac"
        }
    ],
    "height": 9426,
    "unspents": [
        {
            "n": 0,
            "asset": "0xc56f33fc6ecfcd0c225c4ab356fee59390af8560be0e930faebe74a6daff7c9b",
            "value": "500",
            "address": "APRgMZHZubii29UXF9uFa6sohrsYupNAvx"
        },
        {
            "n": 1,
            "asset": "0x602c79718b16e442de58778e148d0b1084e3b2dffd5de6b7b16cee7969282de7",
            "value": "100",
            "address": "APRgMZHZubii29UXF9uFa6sohrsYupNAvx"
        },
        {
            "n": 2,
            "asset": "0xc56f33fc6ecfcd0c225c4ab356fee59390af8560be0e930faebe74a6daff7c9b",
            "value": "99999390",
            "address": "AWygZ1B5c3GDiLL6u5bHSVU45Ya1SVGX9P"
        },
        {
            "n": 3,
            "asset": "0x602c79718b16e442de58778e148d0b1084e3b2dffd5de6b7b16cee7969282de7",
            "value": "25903.995",
            "address": "AWygZ1B5c3GDiLL6u5bHSVU45Ya1SVGX9P"
        }
    ]
}

ixje

Almost there, just a few little touch ups

ixje · 2018-10-10T18:49:16Z

neo/Prompt/Utils.py

@@ -85,13 +85,14 @@ def get_asset_id(wallet, asset_str):


 def get_asset_amount(amount, assetId):
-    f8amount = Fixed8.TryParse(amount)
+    f8amount = Fixed8.TryParse(amount, require_positive=True)
    if f8amount is None:
        print("invalid amount format")


can you remove this print statement, or change it to logger.debug(). Reason being is that we get double error messages:

Outgoing Number 1 Asset to send: neo Address to: AGBjDZDoRgkBnxPV6N8dH66h2mwZGCwJ6B Amount to send: -1 invalid amount format invalid amount neo> quit

the alternative is to remove the "print(invalid amount" in Send.py itself. Either way we shouldn't have 2 error messages for the same problem

Since this is the utility function I will leave this message and use logger.debug() for the message in Send.py.

ixje · 2018-10-10T18:49:27Z

neo/Prompt/Utils.py

    if f8amount is None:
        print("invalid amount format")
+        return False

    elif f8amount.value % pow(10, 8 - Blockchain.Default().GetAssetState(assetId.ToBytes()).Precision) != 0:
        print("incorrect amount precision")


same as above

Since this is the utility function I will leave this message and logger.debug() for the message in Send.py.

ixje · 2018-10-10T18:53:47Z

neo/Prompt/Commands/Send.py

+
+    scripthash_to = lookup_addr_str(wallet, address_to)
+    if scripthash_to is None:
+        print("invalid address")


we get a double invalid error message; please keep only one (see the description I gave regarding double value error messages as well, I noticed that one first so was more verbose in my description there)

Address to: wertyu Not correct Address, wrong length. invalid address

ixje · 2018-10-10T19:24:45Z

neo/Prompt/Commands/tests/test_send_command.py

+            self.assertTrue(res)  # verify successful tx
+
+            json_res = res.ToJson()
+            self.assertEqual(self.watch_addr_str, json_res['vout'][0]['address'])  # verify correct address_to


Is it just me going crazy or have i seen this before somewhere? It feels like I'm reviewing something I've done before in another PR. Note this is not a request to change something, I think I just seem to have lost track

Probably. I derived these tests from my learning experience from implementing sendmany for JsonRpcApi

ixje · 2018-10-10T19:29:11Z

neo/Prompt/Commands/tests/test_send_command.py


-        self.assertEqual(1, len(res.Attributes))
+            self.assertTrue(res)
+            self.assertEqual(2, len(res.Attributes))


can you please add a comment above saying:

By default the script_hash of the transaction sender is added to the TransactionAttribute list, therefore the Attributes length is `count` + 1

We only have to do this once, but it's something that raises eyebrows if you read this code without it. Because it looks like you add just 1 attribute and then test for 2

for compatibility

Merge CoZ Development into jseagrave21 SendMany_Prompt

- transfers redundant `print` notifications to `logger.debug()`

- add clarifying comment

jseagrave21 · 2018-10-10T21:39:59Z

Okay, in addition to your requested changes I also sent the print("insufficient funds") to logger.debug() because I noticed an error is already raised, which provided an even better description including the assetId.

ixje · 2018-10-11T09:53:13Z

Thanks! 🥇

ixje · 2018-10-11T14:55:09Z

nm, I found a new option in the github web UI for squashing PRs 💯

jseagrave21 added 8 commits September 14, 2018 00:01

Update CHANGELOG.rst

af4604d

Add get_outgoing and get_change_addr

6afb312

To support SendMany

Add support for sendmany

ba461b5

Add construct_and_send_many

76a78b1

Supports sendmany

Add tests for sendmany

c44e3c6

Update CHANGELOG.rst

ac62558

Add tests for sendmany

84be107

Add construct_and_send_many

d896786

To support sendmany

Add more tests for sendmany

d597d9a

Add a couple more tests for sendmany

6f7dd84

ixje suggested changes Sep 24, 2018

View reviewed changes

ixje mentioned this pull request Sep 24, 2018

Add sendmany RPC method #596

Closed

5 tasks

jseagrave21 added 9 commits October 5, 2018 00:44

Update CHANGELOG.rst

96f86db

fix compatibility issues

Update test_send_command.py

a790241

for compatibility

Merge pull request #33 from CityOfZion/development

76df6e2

Merge CoZ Development into jseagrave21 SendMany_Prompt

Update CHANGELOG.rst

1946f31

Update Send.py

84a44eb

- implement `construct_send_basic`, `construct_send_many`, and `process_transaction`

Update test_send_command.py

bcbfd90

- Adds tests for `construct_send_basic`, `construct_send_many`, and overall tests - Improves test coverage

Update prompt.py

f57ee4f

- implement new `send` and `sendmany` functions

Add provision if outgoing is None

0ef8620

Update test_send_command.py

fdc3aef

- Fix sendmany token test - Add tests for bad precision - Add weird outgoing amount test - Add 'could not send' test

jseagrave21 mentioned this pull request Oct 7, 2018

Fix a couple of tests #652

Merged

4 tasks

ixje suggested changes Oct 8, 2018

View reviewed changes

jseagrave21 added 4 commits October 9, 2018 02:11

Update CHANGELOG.rst

e3dfed5

for compatibility

Update Send.py

17fdc3a

for compatibility

Update Send.py

c808041

for compatibility

Update Send.py

baeca78

for compatibility

jseagrave21 added 3 commits October 9, 2018 02:35

Update test_send_command.py

dd4667c

- test new `send` and `sendmany` implementation

Update prompt.py

5cacbbd

- Implement new `send` and `sendmany` methods

Update Send.py

ee6c37c

- Remove unnecessary try/except logic

ixje suggested changes Oct 9, 2018

View reviewed changes

jseagrave21 added 9 commits October 9, 2018 12:20

Improve sendmany description

6e35e68

Remove unused prompter argument

2dd0c29

Update test_send_command.py

047db6c

- make tests compatible with Send.py without `prompter` argument

Update prompt.py

4117b85

- remove the `self` argument from `construct_send_basic`, `construct_send_many`, `process_transaction`, and `parse_and_sign`

Simplify sendmany description further

6a59681

Remove get_outgoing

984c9b3

Update Send.py

a54e3ce

- remove `get_outgoing` logic and replace with `get_arg`

Update test_send_command.py

b528e23

- update for simplified outgoing logic

Update Send.py

72255f1

- fix indentation so `sendmany` iterates correctly

ixje suggested changes Oct 10, 2018

View reviewed changes

jseagrave21 added 5 commits October 10, 2018 14:25

Update CHANGELOG.rst

9e33cfd

for compatibility

Merge pull request #39 from CityOfZion/development

5a7eb9a

Merge CoZ Development into jseagrave21 SendMany_Prompt

Update CHANGELOG.rst

295ee3a

Update Send.py

ed6e3a1

- transfers redundant `print` notifications to `logger.debug()`

Update test_send_command.py

25b9e71

- add clarifying comment

ixje approved these changes Oct 11, 2018

View reviewed changes

ixje merged commit 173342a into CityOfZion:development Oct 11, 2018

jseagrave21 deleted the SendMany_Prompt branch October 11, 2018 15:08

nunojusto mentioned this pull request Dec 5, 2018

Error when running docker_run_and_create_wallet.sh CityOfZion/neo-privatenet-docker#62

Closed

jseagrave21 mentioned this pull request Dec 5, 2018

Update send methods for scripts CityOfZion/neo-privatenet-docker#63

Merged

nunojusto mentioned this pull request Dec 23, 2018

Fix python construct invocation when claiming GAS and NEO CityOfZion/neo-local#145

Merged

3 tasks


		print("sending with fee: %s " % fee.ToString())
		return contract_tx, scripthash_from, scripthash_change, fee, owners, user_tx_attributes

Adds sendmany feature to Prompt.py #610

Adds sendmany feature to Prompt.py #610

Uh oh!

Conversation

jseagrave21 commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseagrave21 commented Sep 14, 2018

Uh oh!

ixje left a comment

Choose a reason for hiding this comment

Uh oh!

jseagrave21 commented Oct 5, 2018

Uh oh!

ixje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 commented Oct 9, 2018

Uh oh!

ixje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 commented Oct 9, 2018

Uh oh!

ixje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseagrave21 commented Oct 10, 2018

Uh oh!

ixje commented Oct 11, 2018

Uh oh!

ixje commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Adds `sendmany` feature to Prompt.py #610

Adds `sendmany` feature to Prompt.py #610

jseagrave21 commented Sep 14, 2018 •

edited

Loading

coveralls commented Sep 14, 2018 •

edited

Loading

jseagrave21 Oct 9, 2018 •

edited

Loading

jseagrave21 Oct 10, 2018 •

edited

Loading

jseagrave21 Oct 10, 2018 •

edited

Loading

ixje commented Oct 11, 2018 •

edited

Loading