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

Added Export JSON Keystore screens #3498

Merged

Conversation

eviltofu
Copy link
Contributor

@eviltofu eviltofu commented Dec 2, 2021

Part of: #2719

Testing

Preparation

  1. Tap "Settings", "Advanced".
  2. Expects to see "Advanced" screen with "Export JSON Keystore" option.

Test Password screen

  1. Tap "Export JSON Keystore".
  2. Expects to see "Set Password for JSON Keystore" screen, with the password text field as first responder, the "Export JSON Keystore" button is disabled.

Test Enable/Disable "Export JSON Keystore" button

  1. Type "abc" into the password text field.
  2. Expects "Export JSON Keystore" button to be enabled, password text field does not show password.
  3. Tap the backspace key to delete a character.
  4. Expects "Export JSON Keystore" button to be disabled.

Test character filter (right now only alphanumerics are accepted)

  1. Type "+++" into the password text field.
  2. Expects nothing to happen.

Test show/hide password

  1. Tap the "Eye" button.
  2. Expects the password to be shown in the password text field.
  3. Type "1234567890" into the password text field.
  4. Expects to see "ab1234567890" in the password field.

Test Info screen

  1. Tap "Export JSON Keystore" button.
  2. Expects "Export JSON Keystore Info screen" with some random text displayed.

Test back button

  1. Tap the back button.
  2. Expects to see the "Set Password for JSON Keystore" screen.

Test complete export

  1. Tap "Export JSON Keystore" button.
  2. Tap the "Export Button".
  3. Expects to return to the "Advanced" screen.

Start Mode

In the ExportJsonKeystoreCoordinator class, you can specify which screen to start first via the start function.

Validation rules

The validation rules are a list of rules which can specify the validity of the password. The rules are:

  1. lengthLessThan
  2. lengthLessThanOrEqualTo
  3. lengthMoreThan
  4. lengthMoreThanOrEqualTo
  5. doesNotContain
  6. onlyContain
    Rules 1-4 expect an integer. The code does not check for logic. Rules 5-6 expect CharacterSet data.

Test show export page

If a Watched Wallet

Preparation

You will need an Etherium wallet address. You can get one from Etherscan. The one I choose was 0xCD458d7F11023556cC9058F729831a038Cb8Df9c.

  1. Go to the main screen and tap "Wallet".
  2. Create a watch only wallet by tapping on the upper left icon in the "Wallet" screen.
  3. Expects to see "Wallets" screen with list of wallets.
  4. Tap the add button in the upper right hand corner and select Watch Wallet.
  5. Enter the wallet address.
  6. Expects to see the "Wallet" screen.
  7. Tap on settings and advanced.
  8. Expects not to see the Export functions.

If a Real Wallet

  1. Go to the main screen and tap "Wallet".
  2. Create a wallet by tapping on the upper left icon in the "Wallet" screen.
  3. **Expects to see "Wallets" screen with list of wallets.
  4. Tap the add button in the upper right hand corner and select Create/Import.
  5. Type in the key phase if you are importing a wallet.
  6. Expects to see the "Wallet" screen.
  7. Tap on settings and advanced.
  8. Expects to see the Export functions.

@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 2, 2021

Simulator Screen Shot - iPhone 13 - 2021-12-02 at 20 55 45
Simulator Screen Shot - iPhone 13 - 2021-12-02 at 20 55 51
Simulator Screen Shot - iPhone 13 - 2021-12-02 at 20 56 00
Simulator Screen Shot - iPhone 13 - 2021-12-02 at 20 56 05

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch from eb7a5ac to ff0462a Compare December 2, 2021 15:38
Copy link
Member

@hboon hboon left a comment

Choose a reason for hiding this comment

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

There's a glitch when the 2 new view controllers appear, maybe the background color isn't set? (still .clear?)

Simulator.Screen.Recording.-.iPhone.11.-.2021-12-03.at.12.31.29.mp4

Remember to remove the spurious comments in the git commit message after squashing

@@ -97,6 +97,15 @@
"settings.language.useSystem.title" = "Use System Setting";
"settings.version.label.title" = "Version";
"settings.tokenScriptStandard.title" = "TokenScript Compatibility";
"settings.advanced.exportJSONKeystore.title" = "Export JSON Keystore";
"settings.advanced.exportJSONKeystore.info.title" = "Export JSON Keystore";
"settings.advanced.exportJSONKeystore.info.label" = "Your Keystore JSON";
Copy link
Member

Choose a reason for hiding this comment

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

This should be "Set password".

Copy link
Contributor Author

@eviltofu eviltofu Dec 3, 2021

Choose a reason for hiding this comment

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

Can you be more specific? Or do you mean

"settings.advanced.exportJSONKeystore.password.title" = "Set Password for JSON Keystore";

Copy link
Member

Choose a reason for hiding this comment

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

The button's title which is set to: "settings.advanced.exportJSONKeystore.info.label". That button's title should be "Set password" instead.

@hboon
Copy link
Member

hboon commented Dec 3, 2021

@eviltofu after the changes, do you want to work on those 2 TODOs too?

So we support 2 types of wallets:

A. Private key
B. HD wallets (so there is a seed and seed phrases). This is used to (internally) generate (A)

We don't allow users to generate A anymore, but allow them to import them. Users can import both types of wallets.

The Keystore/JSON file is generated from A.

Look for this to see how to export it from (A):

keystore.exportRawPrivateKeyForNonHdWalletForBackup(forAccount: account, newPassword: newPassword) { [weak self] result in

The lines following it also shows how to export it with the sharing sheet.

For B: To export the Keystore/JSON file from B, we'll have to go from B to A first. But I'll work on this later when you are done. The keystore part is a bit more sensitive and it doesn't support this operation yet.

You can check with something like this:

if keystore.isHdWallet(account: account) {
    //This is B. Leave it. Leave a TODO
} else {
    //This is A (try implementing this)
}

You can verify if it works by importing the JSON file along with the password as a wallet. It should tell you the wallet has already been imported (or exist).

It is also important to check that this is not a watched wallet (no key, no seed phrase) with Wallet.type. So only show this new item in Settings > Advanced if this is .real.

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch from ff0462a to 83565a8 Compare December 3, 2021 13:07
@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 3, 2021

Simulator.Screen.Recording.-.iPhone.13.-.2021-12-03.at.21.09.18.mp4

@hboon
Copy link
Member

hboon commented Dec 7, 2021

@eviltofu this isn't blocked by me, right?

@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 7, 2021

@eviltofu this isn't blocked by me, right?

This is not blocked by you. It's being blocked by the M1 MacBook Air. Once I finish creating the second issue where I report what fails I'll revert back to this.

@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 7, 2021

Screen Shot 2021-12-07 at 10 15 06 PM
Screen Shot 2021-12-07 at 10 15 14 PM
Screen Shot 2021-12-07 at 10 15 17 PM

@hboon
Copy link
Member

hboon commented Dec 7, 2021

Can you not mix in changes related to Dark mode into this PR? Also, I'd rather we not work on Dark mode yet.

@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 8, 2021

Can you not mix in changes related to Dark mode into this PR? Also, I'd rather we not work on Dark mode yet.

@hboon The colours are set up with Dark and Light mode in the semantic way we discussed. We have the colours defined in the .xcassets file. Then a struct with static constants that returns the colour based on the user style (dark or light mode). Was I not suppose to build this?

@hboon
Copy link
Member

hboon commented Dec 8, 2021

Ah, right. You'd wanted to experiment how to build an abstraction for the UI building. The number of modified files had me worried for a minute. Would it be feasible to do this — build this PR without that with 1 commit; it seems like it is almost done. Then follow up with another PR to introduce changes you think is necessary for that abstraction and to support Dark Mode etc?

Then it's easier to explore, tweak that second PR, drop it, or expand the approach if it's good?

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch from 8b57373 to ba216aa Compare December 8, 2021 11:36
@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 8, 2021

Ah, right. You'd wanted to experiment how to build an abstraction for the UI building. The number of modified files had me worried for a minute. Would it be feasible to do this — build this PR without that with 1 commit; it seems like it is almost done. Then follow up with another PR to introduce changes you think is necessary for that abstraction and to support Dark Mode etc?

Then it's easier to explore, tweak that second PR, drop it, or expand the approach if it's good?

@hboon It's completed right now. Do you still want me to remove the UIFactory code? It's pretty straightforward.

@hboon
Copy link
Member

hboon commented Dec 8, 2021

@hboon It's completed right now. Do you still want me to remove the UIFactory code? It's pretty straightforward.

@eviltofu if it's simple, yes, would be good to have a PR without UIFactory etc and focused just on the issue, another introducing UIFactory so we can look at it 1 at a time.

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch from ba216aa to 0173716 Compare December 9, 2021 00:43
@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 9, 2021

@hboon removed UIFactory except for the color xcasset.

@hboon
Copy link
Member

hboon commented Dec 9, 2021

@eviltofu can you help to exclude the UIFactory files too? Seems to be excluded from the project, but they are still in the PR.

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch from 0173716 to 1ce70c6 Compare December 9, 2021 07:27
@eviltofu
Copy link
Contributor Author

eviltofu commented Dec 9, 2021

@hboon removed requested files, merged the current upstream/master, and force pushed. You can ignore the changes for the .xcasset files. I originally moved all the colours to a separate .xcasset file for UIKitFactory and then moved them back when I removed the UIKitFactory files.

@eviltofu eviltofu force-pushed the Advanced_Settings_Export_JSON_Keystore branch 3 times, most recently from 79f4fe9 to ae4315e Compare December 10, 2021 10:08
@eviltofu
Copy link
Contributor Author

@hboon updated Features.

@hboon
Copy link
Member

hboon commented Dec 13, 2021

@eviltofu do you mean this is good for review + merge?

@eviltofu
Copy link
Contributor Author

@eviltofu do you mean this is good for review + merge?

Yes

@hboon hboon force-pushed the Advanced_Settings_Export_JSON_Keystore branch from ae4315e to 48a5db8 Compare December 14, 2021 04:39
@hboon hboon merged commit 899c107 into AlphaWallet:master Dec 14, 2021
@hboon hboon deleted the Advanced_Settings_Export_JSON_Keystore branch December 14, 2021 04:39
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.

2 participants