Improve `parse_and_sign`

[jseagrave21]

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

• Improves parse_and_sign functionality

• Add test coverage

• This PR verifies that #697 is resolved.

• Improves the instructions for updating test fixtures

NOTE: Required an update to the test fixtures:

-updated fixtures- notif_fixtures_v9.tar.gz fixtures_v9.tar.gz

How did you solve this problem? Trial and Error. Special thanks to @dauTT and @ixje for helping me through.

How did you make sure your solution works? Unittest NOTE: Due to permission errors I was not able to run make test so I am not sure how the changes in the fixtures (mainly the small advancement in height) has affected other tests. I was thinking it might work after the fixtures are uploaded to AWS.

-updated-

• make test works athough neo-boa will need to be updated.

Are there any special changes in the code that we should be aware of? Updated the unittest fixtures

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)

[ixje]

I've uploaded the files but it doesn't help.

[I 181115 11:46:03 BlockchainFixtureTestCase:70] downloading fixture notification database from https://s3.us-east-2.amazonaws.com/cityofzion/fixtures/notif_fixtures_v9.tar.gz. this may take a while E[I 181115 11:46:04 LevelDBBlockchain:114] leveldb unavailable, you may already be running this process: b'IO error: lock /home/travis/.neopython/fixtures/test_chain/LOCK: already held by process'

Initially I thought this was because you created the fixtures while the level DB was open, so I manually removed the LOCK file from the archives, but it still seems to fail. My only recommendation is to get it working locally with make test first. 99 out of 100 if it doesn't build locally it will also not build on github.

[jseagrave21]

Okay, well I followed the documentation in tests.py to create the fixtures, which you can see wasn't perfect to begin with. If there are other errors in this process that aren't obvious, then I don't know what they are. @dauTT could you provide some help perhaps?

If needed, I can provide the exact steps I took to create the fixtures. And describe exactly what I was trying to accomplish in the new fixtures: mainly, the creation of a 2-signature-required multisig address between neo-test1-w.wallet and neo-test2-w.wallet and sending 5 NEO from neo-test2-w.wallet's default address to its multisig address.

[dauTT]

@jseagrave21 , in regards to your note:

Due to permission errors I was not able to run make test so I am not sure how the changes in the fixtures (mainly the small advancement in height) has affected other tests. I was thinking it might work after the fixtures are uploaded to AWS.

You can test everything locally before uploading the fixtures to AWS. Both fixures files (notif_fixtures_v9.tar.gz, fixtures_v9.tar.gz) need to be located here :

run:

make test

to see if everything passes. If not, you will need to fix the failing test cases.

[jseagrave21]

okay @ixje, please upload the new fixtures. When I ran make test I have to run it as root but I don't think this will carry over. make test does pass but neo-boa will need to be updated it looks like. Also, I noticed quite a few of these warnings, usually when running insufficient funds tests:

[W 181116 16:13:07 Wallet:1063] Wait for your wallet to be synced before doing transactions. To check enter 'wallet' and look at 'percent_synced', it should be 100. Also the blockchain should be up to the latest blocks (see Progress). Issuing 'wallet rebuild' restarts the syncing process.

I am not sure if these will carry over or how to make them go away. At least make test passes now.

[ixje]

I'm curious to learn why the wallet address of wallet-1 (in the neo-boa PR) needs updating. Any idea? I mean as far as you explained your approach I can't see why that is needed as I expect only an additional multi-sig address and some new TX's related to it. So it worries me a bit as long as I don't understand the root cause for that.

Do these "wallet sync" warnings exist in the current build/other PRs?

[jseagrave21]

Unless I am mistaken, the wallet address for wallet-1 in the neo-boa test was wrong. It doesn't match the address from neo-test1-w.wallet from the WalletFixtureTestCase

The only time I've seen the wallet sync warnings during make test is with this new build.

[ixje]

Ok, I now wonder if the address was wrong how there were no tests failing 🤔

Regarding warnings; I think what probably needs to happen is opening all wallets, make sure they're all synced to the latest block in the chain, then close and save.

[jseagrave21]

Yes, I checked that. They were all at 100% before I exited then stopped the docker container. Is there a step I missed?

[jseagrave21]

@ixje It looks like you were right. I went and ran np-prompt -u without the container running and reopened all the wallets. Now I am not seeing any "wallet sync" warnings.

[jseagrave21]

@ixje Should I add a note to the tests.rst instructions mentioning this extra step in case wallet fixtures were updated? I am not sure how I would phrase it.

[ixje]

yeah it would be great if it can be added as a step/reminder in these instructions: https://neo-python.readthedocs.io/en/latest/tests.html#fixtures-guidelines

[jseagrave21]

This is ready for the new fixtures to be uploaded to AWS. Everything should work now.

[ixje]

the fixtures contain LOCK files which will make LevelDB think the chain is already open

[jseagrave21]

There are LOCK files in the current fixtures. And make test works. Will Travis still fail?

[coveralls]

Coverage increased (+0.6%) to 86.886% when pulling da8cf8d40d00c52804141e8e7571fcb583b06217 on jseagrave21:parse_and_sign into 347f64870973c33ccf7911dd1f59372405b837c8 on CityOfZion:development.

[jseagrave21]

@localhuman @ixje This PR is ready for your review please note that the change here https://github.com/CityOfZion/neo-python/pull/716/files#diff-354f30a63fb0907d4ad57269548329e3R23 was implemented to show neo-boa successfully pass make test with the current build. I will revert this change as soon as you have reviewed the PR and told me to.

[jseagrave21]

I am planning on using these tests as the basis for the refactored tests for parse_and_sign; mentioned here: https://github.com/CityOfZion/neo-python/pull/726#issuecomment-441996896

[jseagrave21]

@metachris @localhuman please review this PR. I just updated it and it is working as intended. Please note I have edited .travis.yml to look at my neo-boa patch-1 to show that the build is successful. Upon merging of my neo-boa PR, I will update .travis.yml to its original state.

This PR is necessary for establishing the new test fixtures so I can test the functionality I am working on for glaiming GAS from multisig addresses as mentioned by @ThomasLobker in # _neopython in Slack.