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.

Sign up for GitHub

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

Jump to bottom

ADD: SLIP39 wallet and unit tests #2899

Merged
merged 4 commits into from
Apr 28, 2021
Merged

ADD: SLIP39 wallet and unit tests #2899

merged 4 commits into from
Apr 28, 2021

Conversation

limpbrains
Copy link
Collaborator

No description provided.

@limpbrains limpbrains added the WIP label Apr 2, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 2, 2021

This pull request introduces 2 alerts when merging 74376fc into 5c8b1bc - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

Overtorment
Overtorment requested changes Apr 8, 2021
View reviewed changes
class/wallets/abstract-hd-electrum-wallet.js Outdated Show resolved Hide resolved
class/wallets/abstract-hd-wallet.js Outdated Show resolved Hide resolved
class/wallets/abstract-wallet.js Outdated Show resolved Hide resolved
class/wallets/hd-aezeed-wallet.js Outdated Show resolved Hide resolved
class/wallets/slip39-wallets.js Outdated Show resolved Hide resolved
class/wallets/slip39-wallets.js
static type = 'SLIP39legacyP2PKH';
static typeReadable = 'SLIP39 Legacy (P2PKH)';

_getSeed = SLIP39Mixin._getSeed;
Copy link
Member

Choose a reason for hiding this comment

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

ok thats smart. traits in js

package.json
"react-native-blue-crypto": "git+https://github.com/Overtorment/react-native-blue-crypto.git",
"react-native-camera": "3.43.0",
"react-native-crypto": "2.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

im almost onboard with this and removing blue modules crypto, but i need to do some experiments on this first

screen/wallets/details.js Show resolved Hide resolved
tests/unit/slip39-wallets.test.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@Overtorment
Copy link
Member

also, whats wrong with e2e and appetize..?

Overtorment
Overtorment requested changes Apr 9, 2021
View reviewed changes
Copy link
Member

@Overtorment Overtorment left a comment

Choose a reason for hiding this comment

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

appetize/e2e still crash on launch

class/app-storage.js Show resolved Hide resolved
class/wallets/abstract-hd-electrum-wallet.js Outdated Show resolved Hide resolved
@limpbrains limpbrains mentioned this pull request Apr 11, 2021
Closed
@lgtm-com
Copy link

lgtm-com bot commented Apr 11, 2021

This pull request introduces 16 alerts when merging 92c546e into 8e7064a - view on LGTM.com

new alerts:

  • 14 for Superfluous trailing arguments
  • 2 for Useless assignment to local variable

@limpbrains
Copy link
Collaborator Author

appetize/e2e still crash on launch

Apparently, slip39 depends on BigInt. It is available in RN on iOS, but not on Android.

I've moved slip39 lib to blue_modules, now Babel transpiles it and because of @babel/plugin-syntax-bigint it work.

But now tests are failing.

I think I'm going to fork slip39, replace BigInt with bignumber.js.

@Overtorment
Copy link
Member

moving it to blue_modules and doing all the patches there is a solid way to go.
you could even add unit tests to test specifically slip39 - to make sure it works. and even add it to selftest page to check it in runtime

@Overtorment
Copy link
Member

e2e still fails btw

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2021

This pull request introduces 9 alerts when merging 0f715e9 into 602f16f - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2021

This pull request introduces 9 alerts when merging 94a3637 into 602f16f - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2021

This pull request introduces 9 alerts when merging 9495933 into 602f16f - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@limpbrains
Copy link
Collaborator Author

I don't think I should change original src of slip39 to pass DeepScan or LGTM bot.

With all changes that I've made to slip39 it now doesn't require full crypto support. I can rollback react-native-crypto thing, or should we keep it ?

@limpbrains limpbrains removed the WIP label Apr 14, 2021
@Overtorment
Copy link
Member

what does your heart tell you? lets give react-native-crypto a chance?

ps. conflicts

@Overtorment
Copy link
Member

image

@lgtm-com
Copy link

lgtm-com bot commented Apr 15, 2021

This pull request introduces 9 alerts when merging e70ac56 into 8f74260 - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@limpbrains
Copy link
Collaborator Author

what does your heart tell you? lets give react-native-crypto a chance?

Yes, let's keep it

@Overtorment
Copy link
Member

conflicts

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2021

This pull request introduces 9 alerts when merging 142c65e into 9b2fe7d - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 9 alerts when merging b9195d3 into 51edd49 - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@GladosBlueWallet
Copy link
Collaborator

♫ This was a triumph. I'm making a note here: HUGE SUCCESS ♫

[android in browser] https://appetize.io/app/66rpxhq0q6f9b9nkweuh7bzk2g?device=pixel4

download apk

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 9 alerts when merging b3fc598 into a46d801 - view on LGTM.com

new alerts:

  • 4 for Useless assignment to local variable
  • 2 for Overwritten property
  • 1 for Expression has no effect
  • 1 for Variable not declared before use
  • 1 for Duplicate property

@GladosBlueWallet GladosBlueWallet merged commit 511d0ca into master Apr 28, 2021
@GladosBlueWallet
Copy link
Collaborator

Unbelievable. You, [subject name here], must be the pride of [subject hometown here]!

@GladosBlueWallet GladosBlueWallet deleted the limpbrains-sss branch April 28, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Overtorment Overtorment Overtorment approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@limpbrains @Overtorment @GladosBlueWallet