Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEP-6 wallet support #444

Merged
merged 5 commits into from
Dec 31, 2017
Merged

NEP-6 wallet support #444

merged 5 commits into from
Dec 31, 2017

Conversation

slipo
Copy link
Contributor

@slipo slipo commented Dec 30, 2017

What current issue(s) from Trello/Github does this address?
#421

What problem does this PR solve?
All projects should follow the NEP-6 wallet standard so they can share wallets

How did you solve this problem?
Some notes:
I leave the keys.json storage file behind, just in case.

When we upgrade the wallets we only have labels and encrypted keys. This means that we can’t fill in the address fields until they log into the accounts.

I first tried to use neon-js which was pretty cool but it throws an exception if you try to export an Account without an address. It wasn’t too much work to do it all custom in the end.

Try to use naming inline with the NEP-6 standard (wallets contain accounts, in settings you add accounts not keys)

How did you make sure your solution works?
Automated + manual tests

QA Tests worth doing (any others?)

  • Starting with legacy key file, it should migrate to NEP-6. Delete an account, restart app and make sure that account is still missing.
  • Starting with no wallet, test recovering NEP-6 wallet
  • Starting with no wallet, test recovering legacy wallet
  • Adding accounts after the two above
  • Starting with no wallet, test adding 2 accounts, logging into both works
  • Recovering NEP-6 / Legacy after already having accounts in the wallet
  • Logging into recovered legacy account
  • Logging into recovered NEP-6 account
  • Deleting all those accounts
  • Recovering same recovery file multiple times (won’t allow key to be in wallet twice)
  • Export everything, delete all accounts, import

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

Is there anything else we should know?
Having wallet accounts without public addresses could pose a challenge if other programs the user is importing into don't support that. There's not really anything we can do though.

  • Unit tests written?

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 41.805% when pulling bfe2090 on slipo:nep6 into 7885ea2 on CityOfZion:dev.

@slipo
Copy link
Contributor Author

slipo commented Dec 30, 2017

Per this issue:
electron-userland/electron-builder#2260

I upgraded electron-builder and now the build works.

storage.set('keys', data)
setKeys(data)
storage.get('userWallet', (error, data) => {
data.accounts.map((account, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do some error handling here?
if (error) { ... } else { delete wallet account }
Also can we use something like:
const keys = reject(data.accounts, { key });
Also, why do we need to check the label when deleting? isn't a key is unique?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 41.391% when pulling 06803aa on slipo:nep6 into 7885ea2 on CityOfZion:dev.

@@ -29,31 +35,183 @@ describe('generateWallet module tests', () => {

describe('newWallet tests', () => {
const payload = account
const expectedAction = Object.assign({}, { payload }, { type: NEW_WALLET })
const expectedAction = Object.assign({}, { payload }, { type: NEW_WALLET_ACCOUNT })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an Object.assign here?
Can it be -
const expectedAction = { payload, type: NEW_WALLET_ACCOUNT }

}
}

export const createEmptyWallet = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this -

export const createEmptyWallet = ({
  ...DEFAULT_WALLET,
  scrypt: DEFAULT_SCRYPT
})

export const walletHasKey = (wallet, key) => {
let foundKey = false

each(wallet.accounts, (account) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use .some - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

wallet.accounts.some((account) => account.key === key)

@coveralls
Copy link

Coverage Status

Coverage increased (+1.7%) to 41.246% when pulling bb1ab24 on slipo:nep6 into 7885ea2 on CityOfZion:dev.

@evgenyboxer evgenyboxer merged commit 54a2ed3 into CityOfZion:dev Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants