Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

refactor: wallet import options depending on the network #3814

Merged

Conversation

w3ea
Copy link
Contributor

@w3ea w3ea commented May 31, 2021

Summary

https://app.clickup.com/t/jzbxze

Checklist

  • My changes look good in both light AND dark mode
  • The change is not hardcoded to a single network, but has multi-asset in mind
  • I checked my changes for obvious issues, debug statements and commented code
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ArkEcosystem ArkEcosystem deleted a comment from faustbrian May 31, 2021
@faustbrian faustbrian changed the title chore: wallet import options depending on the network refactor: wallet import options depending on the network Jun 1, 2021
Copy link
Contributor

@faustbrian faustbrian left a comment

Choose a reason for hiding this comment

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

This should also respect the default flag to pre-select an option.

@w3ea
Copy link
Contributor Author

w3ea commented Jun 1, 2021

This should also respect the default flag to pre-select an option.

i know about that
https://github.com/ArkEcosystem/platform-sdk/blob/master/packages/platform-sdk-ark/src/networks/shared.ts#L25-L38
but before implement it i have some questions, and i'll ask them in Slack

@faustbrian
Copy link
Contributor

faustbrian commented Jun 3, 2021

ArkEcosystemArchive/platform-sdk@1dee2a9
ArkEcosystemArchive/platform-sdk@3533031
ArkEcosystemArchive/platform-sdk@3e75a26

  • In 8.5.0 there is a breaking change to split up the mnemonic methods instead of relying on a parameter and if branches.
  • In 8.6.0 and 8.7.0 there are breaking changes to merge the non-encryption and encryption methods in the wallet factory.

@faustbrian
Copy link
Contributor

@w3ea what's the status on this?

@w3ea
Copy link
Contributor Author

w3ea commented Jun 8, 2021

@faustbrian working on fixing test
but about psdk 8.6.0 and 8.7.0, you fixed it, right?
i can't see any fromMnemonicWithEncryption method

@faustbrian
Copy link
Contributor

As I stated in the comment before: In 8.6.0 and 8.7.0 there are breaking changes to merge the non-encryption and encryption methods in the wallet factory, thus fromMnemonicWithEncryption no longer exists but is now handled by passing in a password property to fromMnemonic in addition to the usual.

w3ea added 5 commits June 8, 2021 19:58
…nding-on-the-network-#jzbxze' into chore/wallet-import-options-depending-on-the-network-#jzbxze

# Conflicts:
#	src/domains/wallet/pages/ImportWallet/__snapshots__/ImportWallet.test.tsx.snap
@w3ea w3ea marked this pull request as ready for review June 8, 2021 16:02
@faustbrian
Copy link
Contributor

@w3ea what's the status on this?

@w3ea
Copy link
Contributor Author

w3ea commented Jun 11, 2021

@w3ea what's the status on this?

@faustbrian it had completed, but after recently merge it shows some test errors, i should check theme

Copy link
Contributor

@faustbrian faustbrian left a comment

Choose a reason for hiding this comment

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

Missing coverage

@w3ea
Copy link
Contributor Author

w3ea commented Jun 15, 2021

@faustbrian I also wanted to ask about this
we're using ark in the test and as you know we have limit import options from psdk,
https://github.com/ArkEcosystem/desktop-wallet/blob/develop/src/domains/wallet/pages/ImportWallet/ImportWallet.test.tsx#L304-L307
i'm not sure how to mock other coin like btc to coverage all other import options
https://github.com/ArkEcosystem/desktop-wallet/blob/develop/src/domains/wallet/pages/ImportWallet/Step2.tsx#L134-L178

@faustbrian
Copy link
Contributor

For methods that we don't yet actually support you can skip the tests and/or ignore the coverage.

@faustbrian faustbrian merged commit be9e428 into develop Jun 16, 2021
@faustbrian faustbrian deleted the chore/wallet-import-options-depending-on-the-network-#jzbxze branch June 16, 2021 01:35
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

2 participants