WalletService misrepresents locally-managed address keys as watch-only addresses

[placer14]

When the WalletService requests getStoredAddresses, it incorrectly labels the addresses as a watch-only address.

Acceptance Criteria for this fix:

• [x] Check that a WatchedScript is not accidentally re-added as a watch-only address on top of an already defined local key. (Fix proposal in https://github.com/OpenBazaar/multiwallet/pull/103)
• [x] Migrate data which has been incorectly marked as WatchOnly (Fixed by https://github.com/OpenBazaar/multiwallet/pull/111)
• [x] A unit test asserting that AddWatchedScript should never add a script which already represents a key from the appropriate wallet. This may take some refactoring, but the idea would be that if the script represents a key returns in WalletService.km.GetKeys(), that it should never be indicated as watch-only. (Fixed by https://github.com/OpenBazaar/multiwallet/pull/116)

[hoffmabc]

@rodkeys we mentioned having this second checkbox item as part of the fix. Do you mind taking a look at incorporating that as well?

A unit test asserting that AddWatchedScript should never add a script which already represents a key from the appropriate wallet. This may take some refactoring, but the idea would be that if the script represents a key returns in WalletService.km.GetKeys(), that it should never be indicated as watch-only.

[rodkeys]

I'm cool with writing the unit test but I need a little direction

A unit test asserting that AddWatchedScript should never add a script which already represents a key from the appropriate wallet. This may take some refactoring, but the idea would be that if the script represents a key returns in WalletService.km.GetKeys(), that it should never be indicated as watch-only.

I'm asked to write a unit test to make sure that this won't happen but AFAICT there's currently nothing stopping this from happening in the code.

Chris' fix (https://github.com/OpenBazaar/multiwallet/pull/103/commits/ac178e7c483d2f9be684f04f2d951b5e95a0c8cc) ignored WatchedScripts that correspond to already-existing keys, but it did not stop them from being added to the db.

Am I supposed to add a check in the code for WatchedScripts corresponding to existing keys and then write a unit test after that?

[cpacia]

I don't know if it's worth making sure they;'re not added to the db. I suppose either way is OK.

[placer14]

I'm asked to write a unit test to make sure that this won't happen but AFAICT there's currently nothing stopping this from happening in the code.

Yes, there's nothing preventing this from happening. I was implying that we'd add it by writing the unit test (and making it pass). (Red-Green testing)

Create the scenario that we know will fail, then improve the Put to check the input isn't already part of existing Keys(), then add it.

[placer14]

Attn: There's the bit of trying to share the KeyManager state with the Put request. Please keep those as orthogonal as possible.

[rodkeys]

I don't know if it's worth making sure they;'re not added to the db. I suppose either way is OK.

I don't think it's critical for getting this immediate release ready*.

Yes, there's nothing preventing this from happening. I was implying that we'd add it by writing the unit test (and making it pass). (Red-Green testing)

Create the scenario that we know will fail, then improve the Put to check the input isn't already part of existing Keys(), then add it.

Got it, thanks for the clarification, makes sense.