Update process_transaction #680

jseagrave21 · 2018-10-25T06:59:25Z

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

Adds logic in case the tx fails to relay
Maintains parody with process_transaction from Send.py
- neo-python/neo/Prompt/Commands/Send.py
  
  Line 206 in fd5e1d9
  
  relayed = NodeLeader.Instance().Relay(tx)

-update-

Maintains parody with neo-cli RPC method
Ensures a tx sent via RPC is saved to the wallet

How did you solve this problem?
trial and error

How did you make sure your solution works?
unittest

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)

Merge CoZ Development into jseagrave21 Development

@localhuman

* Update ExtendedJsonRpcApi.py - add fix provided by @localhuman so original methods are returned as well as extended methods

…s thrown. (CityOfZion#663)

…yOfZion#661) * Add the option -u (unittest-net) to prompt.py * Add unittest guildeline and add the smart contract source codes (UnitTest-SM.zip) to the fixtures package

for compatibility

@localhuman

* Fix ExtendedJsonRpcApi (CityOfZion#662) * Update ExtendedJsonRpcApi.py - add fix provided by @localhuman so original methods are returned as well as extended methods * Mute expected test stacktrace and clearly identify why an exception is thrown. (CityOfZion#663) * Add guideline for adding tests to the neo-privnet-unittest image (CityOfZion#661) * Add the option -u (unittest-net) to prompt.py * Add unittest guildeline and add the smart contract source codes (UnitTest-SM.zip) to the fixtures package * Add raw transaction building examples (CityOfZion#665) * Update neo-boa version to fix core building test

Merge CoZ Development into jseagrave21 Development

- add logic in case the tx is not relayed

- Add test in case tx fails to relay

coveralls · 2018-10-25T07:04:21Z

Coverage increased (+0.0009%) to 83.745% when pulling 085f040 on jseagrave21:update-process_transaction into b7ae417 on CityOfZion:development.

ixje

I will leave the PR open for now so you can decide if you want to suggest the change to the C# project. If you do so and once it's accepted we can merge this. If you choose not to, then we'll have to close this. Please let me know your intentions

ixje · 2018-10-25T08:16:55Z

neo/api/JSONRPC/JsonRpcApi.py

+                self.wallet.SaveTransaction(tx)
+                return tx.ToJson()
+            else:
+                raise JsonRpcError(-32001, "Could not relay tx %s " % tx.Hash.ToString())


Unfortunately accepting this change would make the behaviour of calls like sendtoaddress deviate from the C# implementation, which is not what we want. See:
/~https://github.com/neo-project/neo/blob/dbe688b7b1d64975128b8aefb5c3ee692df07cd5/neo/Network/RPC/RpcServer.cs#L525-L532

jseagrave21 · 2018-10-25T10:55:22Z

@ixje I will try to suggest the change to C# but I may fail because I don't know C#. Perhaps a good compromise would be to just include self.wallet.SaveTransaction(tx) because this was missing from the original implementation and present in neo-cli /~https://github.com/neo-project/neo/blob/dbe688b7b1d64975128b8aefb5c3ee692df07cd5/neo/Network/RPC/RpcServer.cs#L528

jseagrave21 · 2018-10-26T00:09:52Z

@ixje I looked at the neo-project RPC and I just don't feel comfortable tinkering since I am not set up to test the code and I don't really know how to code in C#. I still think that this line /~https://github.com/CityOfZion/neo-python/pull/680/files#diff-f9e9d3e0fbf0fb287940a8d009a379b2R560 is equivalent to this line /~https://github.com/neo-project/neo/blob/dbe688b7b1d64975128b8aefb5c3ee692df07cd5/neo/Network/RPC/RpcServer.cs#L528 and should be added. I would be happy to eliminate the additonal logic in case the tx fails to relay. What do you think?

jseagrave21 · 2018-10-29T05:31:34Z

@ixje Based on my reading of the purpose of self.wallet.SaveTransaction(tx) here

neo-python/neo/Wallets/Wallet.py

Lines 1102 to 1109 in 773e57e

    
               def SaveTransaction(self, tx): 
        
                   """ 
        
                   This method is used to after a transaction has been made by this wallet.  It updates the states of the coins 
        
                   In the wallet to reflect the new balance, but the coins remain in a ``CoinState.UNCONFIRMED`` state until 
        
                   The transaction has been processed by the network. 
        
                   The results of these updates can be used by overriding the ``OnSaveTransaction`` method, and, for example 
        
                   persisting the results to a database.

I really think we should at least add this line.

ixje · 2018-11-05T09:32:40Z

Sounds reasonable. I hope @localhuman can shed a quick light here because I'm far from familiar with that part of the neo-python code and I think he can probably do that in a minute or so

jseagrave21 · 2018-11-11T01:38:11Z

Based on our discussion, I will update the PR accordingly.

- revert original changes but maintain parody with neo-cli and include `self.wallet.SaveTransaction(tx)` to ensure the tx is saved to the wallet - see /~https://github.com/neo-project/neo/blob/dbe688b7b1d64975128b8aefb5c3ee692df07cd5/neo/Network/RPC/RpcServer.cs#L528

- remove test for failure to relay

for compatibility

Merge neo-python development into jseagrave21 update-process_transaction

jseagrave21 · 2018-11-11T01:50:10Z

Okay @localhuman this is ready for your review. A brief recap: Originally, this PR was to bring the RPC method to parody with process_transaction from Send.py. However, this would take the RPC method out of parody with neo-cli. So, I have just updated process_transaction enough to bring it to parody with the neo-cli RPC method. Also, I believe this is an important piece of code to include for anyone who is sending tx via RPC. cc @ixje

ixje · 2018-11-14T11:41:42Z

👍

jseagrave21 and others added 14 commits October 9, 2018 20:38

Merge pull request #38 from CityOfZion/development

13e979b

Merge CoZ Development into jseagrave21 Development

Merge pull request #40 from CityOfZion/development

e41e950

Merge CoZ Development into jseagrave21 Development

Merge pull request #41 from CityOfZion/development

7db54f4

Merge CoZ Development into jseagrave21 Development

Fix ExtendedJsonRpcApi (CityOfZion#662)

54a4f3f

* Update ExtendedJsonRpcApi.py - add fix provided by @localhuman so original methods are returned as well as extended methods

Mute expected test stacktrace and clearly identify why an exception i…

0f96dc3

…s thrown. (CityOfZion#663)

Add guideline for adding tests to the neo-privnet-unittest image (Cit…

1e5b4dc

…yOfZion#661) * Add the option -u (unittest-net) to prompt.py * Add unittest guildeline and add the smart contract source codes (UnitTest-SM.zip) to the fixtures package

Update CHANGELOG.rst

8febcf1

for compatibility

Update CHANGELOG.rst

c7a790f

Merge pull request #50 from CityOfZion/development

32a16e3

Merge CoZ Development into jseagrave21 Development

Merge pull request #53 from CityOfZion/development

3c8232d

Merge CoZ Development into jseagrave21 Development

Update JsonRpcApi.py

9077b2f

- add logic in case the tx is not relayed

Update test_json_rpc_api.py

723449e

- Add test in case tx fails to relay

Update CHANGELOG.rst

745f51a

ixje reviewed Oct 25, 2018

View reviewed changes

jseagrave21 added 5 commits November 10, 2018 17:41

Update test_json_rpc_api.py

231bc15

- remove test for failure to relay

Update CHANGELOG.rst

aada381

for compatibility

Merge pull request #67 from CityOfZion/development

e041f36

Merge neo-python development into jseagrave21 update-process_transaction

Update CHANGELOG.rst

41f96b2

Merge branch 'development' into update-process_transaction

085f040

ixje merged commit 82afc0f into CityOfZion:development Nov 14, 2018

jseagrave21 deleted the update-process_transaction branch November 14, 2018 15:45

jseagrave21 mentioned this pull request Dec 9, 2018

Fix unconfirmed coins when sending via RPC #745

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update process_transaction #680

Update process_transaction #680

jseagrave21 commented Oct 25, 2018 •

edited

Loading

coveralls commented Oct 25, 2018 •

edited

Loading

ixje left a comment

ixje Oct 25, 2018

jseagrave21 commented Oct 25, 2018 •

edited

Loading

jseagrave21 commented Oct 26, 2018 •

edited

Loading

jseagrave21 commented Oct 29, 2018 •

edited

Loading

ixje commented Nov 5, 2018

jseagrave21 commented Nov 11, 2018

jseagrave21 commented Nov 11, 2018

ixje commented Nov 14, 2018

Update process_transaction #680

Update process_transaction #680

Conversation

jseagrave21 commented Oct 25, 2018 • edited Loading

coveralls commented Oct 25, 2018 • edited Loading

ixje left a comment

Choose a reason for hiding this comment

ixje Oct 25, 2018

Choose a reason for hiding this comment

jseagrave21 commented Oct 25, 2018 • edited Loading

jseagrave21 commented Oct 26, 2018 • edited Loading

jseagrave21 commented Oct 29, 2018 • edited Loading

ixje commented Nov 5, 2018

jseagrave21 commented Nov 11, 2018

jseagrave21 commented Nov 11, 2018

ixje commented Nov 14, 2018

jseagrave21 commented Oct 25, 2018 •

edited

Loading

coveralls commented Oct 25, 2018 •

edited

Loading

jseagrave21 commented Oct 25, 2018 •

edited

Loading

jseagrave21 commented Oct 26, 2018 •

edited

Loading

jseagrave21 commented Oct 29, 2018 •

edited

Loading