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

Bugfix open newly created wallet #584

Merged
merged 2 commits into from Aug 30, 2018

Conversation

localhuman
Copy link
Collaborator

@localhuman localhuman commented Aug 30, 2018

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

  • Wallets created after the PeeWee updates have a MigrationState of b'1' instead of '1' as previously, which made it impossible to open a newly created wallet.

How did you solve this problem?

  • This change coerces the value to an integer and compares MigrationState to 1 so that both versions will work.

How did you make sure your solution works?

  • manual testing

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

  • I would not be surprised if there are other similar bugs related to the wallet.

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)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.167% when pulling a905c85 on bugfix-open-newly-created-wallet into 6341296 on development.

@jseagrave21
Copy link
Contributor

jseagrave21 commented Aug 30, 2018

This is because of my peewee PR. I remember having a problem with this. I removed it after @ixje asked me about it here:
525b106

#524 (review)

I couldn't remember why it was there 🙁

@jseagrave21
Copy link
Contributor

I am sorry if I introduced more issues 😢

@jseagrave21
Copy link
Contributor

jseagrave21 commented Aug 30, 2018

This comment explains what changed in peewee3. #524 (review) (click 'show outdated')

@localhuman
Copy link
Collaborator Author

No worries! I think better tests could have caught the issue earlier. Mostly I just wanted to open my wallet 😆 . I'll keep this PR open as I look for anything similar.

@jseagrave21
Copy link
Contributor

jseagrave21 commented Aug 30, 2018

Okay. I tried to include some snippets of the change to help. Mostly, is seems like the code authors used to prefer to store everything as a string because peewee didn't mind and would decode or not as needed. With peewee3, they added BlobField and basically took away the original functionality of CharField and StringField.

@localhuman localhuman merged commit 48e1f25 into development Aug 30, 2018
@jseagrave21
Copy link
Contributor

@localhuman during testing I am seeing these notifications:

neo.api.JSONRPC.test_json_rpc_api.JsonRpcApiTestCase) ... Could not save stored data <Model: Key> instance matching query does not exist:
SQL: SELECT "t1"."Id", "t1"."Name", "t1"."Value" FROM "key" AS "t1" WHERE ("t1"."Name" = ?) LIMIT ? OFFSET ?
Params: ['PasswordHash', 1, 0]
Could not save stored data <Model: Key> instance matching query does not exist:
SQL: SELECT "t1"."Id", "t1"."Name", "t1"."Value" FROM "key" AS "t1" WHERE ("t1"."Name" = ?) LIMIT ? OFFSET ?
Params: ['IV', 1, 0]
Could not save stored data <Model: Key> instance matching query does not exist:
SQL: SELECT "t1"."Id", "t1"."Name", "t1"."Value" FROM "key" AS "t1" WHERE ("t1"."Name" = ?) LIMIT ? OFFSET ?
Params: ['MasterKey', 1, 0]
Could not save stored data <Model: Key> instance matching query does not exist:
SQL: SELECT "t1"."Id", "t1"."Name", "t1"."Value" FROM "key" AS "t1" WHERE ("t1"."Name" = ?) LIMIT ? OFFSET ?
Params: ['MigrationState', 1, 0]
Could not save stored data <Model: Key> instance matching query does not exist:
SQL: SELECT "t1"."Id", "t1"."Name", "t1"."Value" FROM "key" AS "t1" WHERE ("t1"."Name" = ?) LIMIT ? OFFSET ?
Params: ['Height', 1, 0]

I am wondering if this is another issue with peewee? Since the "Could not save stored data" announcement comes from

def SaveStoredData(self, key, value):
        k = None
        try:
            k = Key.get(Name=key)
            k.Value = value
            k.save()
        except Exception as e:
            print("Could not save stored data %s " % e)

What do you think?

@ixje ixje deleted the bugfix-open-newly-created-wallet branch October 2, 2018 18:42
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.

None yet

3 participants