feat(crowdnode): Linking New Online account [NMI-917] #506

Syn-McJ · 2023-02-14T05:33:35Z

Issue being fixed or feature implemented

If the user had created an account using the blockchain API, they still might want to create an online account for an additional recovery option.

What was done?

UI for Email input screen
Signing Email using the private key of the reserved address
Tracking Email status for the new online account

How Has This Been Tested?

QA

Breaking Changes

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

…e-new-online-account

…count

Syn-McJ · 2023-02-14T05:35:26Z

DashWallet/Sources/UI/CrowdNode/CrowdNodeModel.swift

+    func signAndSendEmail(email: String) async throws -> Bool {
+        guard !crowdNode.accountAddress.isEmpty else { return false }
+        emailForAccount = email
+
+        let wallet = DWEnvironment.sharedInstance().currentWallet
+        let result = await wallet.seed(withPrompt: NSLocalizedString("Sign the message", comment: "CrowdNode"), forAmount: 1)
+
+        if !result.1 {
+            let key = wallet.privateKey(forAddress: crowdNode.accountAddress, fromSeed: result.0!)
+            let signResult = await key?.signMessageDigest(email.magicDigest())
+
+            if signResult?.0 == true {
+                let signature = (signResult!.1 as NSData).base64String()
+                try await crowdNode.registerEmailForAccount(email: email, signature: signature)
+                return true
+            }
+        }
+
+        return false
+    }


We sign the email that we send with the private key of our account address - that's how CrowdNode links those two together.

Syn-McJ · 2023-02-14T05:36:12Z

DashWallet/Sources/UI/CrowdNode/Online/CrowdNodeWebViewController.swift

+        if let email = email, let url = navigationAction.request.url?.absoluteString {
+            let fullSuffix = "\(kSignupSuffix)&loginHint=\(email)"
+
+            if (url.hasPrefix(loginPrefix) && !url.hasSuffix(fullSuffix) && !url.hasSuffix(kCallbackSuffix)) {
+                let redirectUrl = "\(url)\(fullSuffix)"
+                let urlRequest = URLRequest(url: URL(string: redirectUrl)!)
+                webView.load(urlRequest)
+                previousUrl = url
+
+                return (WKNavigationActionPolicy.cancel, preferences)
+            } else if (previousUrl.hasPrefix(loginPrefix) && url.hasPrefix(accountPrefix)) {
+                // Successful signup
+                viewModel.finishSignUpToOnlineAccount()
+            }
+
+            previousUrl = url
+        }


We have to manually track webview transition to the profile screen to detect successful signup.

Syn-McJ · 2023-02-14T05:36:54Z

DashWallet/Sources/UI/CrowdNode/Portal/CrowdNodePortalViewController.swift

+        let backButton = UIButton(type: .custom)
+        backButton.frame = .init(x: 0, y: 0, width: 30, height: 30)
+        backButton.setImage(UIImage(systemName: "arrow.backward"), for: .normal)
+        backButton.tintColor = .white
+        backButton.imageEdgeInsets = .init(top: 0, left: -10, bottom: 0, right: 0)
+        backButton.addTarget(self, action: #selector(backButtonAction), for: .touchUpInside)
+        navigationItem.leftBarButtonItem = UIBarButtonItem(customView: backButton)


Seems like the only way to fix the back button and make it white is to recreate it here.

We can also change the tintColor of the 'original' back button:

navigationItem.leftBarButtonItem!.customView!.tintColor = .white

This solution is better, but there is a problem that we can only do this at viewDidAppear which doesn't look good. Let's keep your implementation for now and I will improve BaseNavigationController and add extra property into NavigationBarDisplayable protocol, like backButtonTintColor and we will be able to change the back button color per view controller without adding a custom button.

Syn-McJ · 2023-02-14T05:38:40Z

DashWallet/Sources/UI/CrowdNode/Views/OutlinedTextField.swift

+    @objc func onEditingBegin() {
+        let path = UIBezierPath(roundedRect: self.layer.bounds.inset(by: UIEdgeInsets(top: -2, left: -2, bottom: -2, right: -2)), cornerRadius: 14)
+        outerBorder.path = path.cgPath
+        self.layer.addSublayer(outerBorder)
+        self.layer.borderColor = UIColor.dw_dashBlue().cgColor
+    }


I figured I'll keep the external border as a sublayer - that way we can implement the focused, unfocused, and error states according to the design and have it all contained in a single control for reusability sake.
https://www.figma.com/file/azdJACb5WmivxYVhB5q46F/Design-system%E3%83%BBAndroid?node-id=3014%3A14441&t=K4J99XOQ6jVwlIWC-0

@Syn-McJ Sound good — this approach works fine as well. The only thing is that we should not use onEditingBegin to calculate the rect for the path, this could be done within layoutSubviews or drawRect methods:

override func layoutSubviews() { super.layoutSubviews() let path = UIBezierPath(roundedRect: bounds.inset(by: UIEdgeInsets(top: -2, left: -2, bottom: -2, right: -2)), cornerRadius: 14) outerBorder.path = path.cgPath }

To change the visibility of outerBorder layer, I suggest just to toggle isHidden flag, when onEditingBegin or onEditingEnd is being invoked, instead of adding/removing for the view stack.

Makes sense

tikhop · 2023-02-14T06:40:08Z

DashWallet/Sources/Models/CrowdNode/CrowdNode.swift


-    private func waitForApiAddressConfirmation(primaryAddress: String, apiAddress: String) async -> DSTransaction {
+    private func waitForApiAddressConfirmation(primaryAddress: String, apiAddress: String) async -> (tx: DSTransaction, didWait: Bool) {


@Syn-McJ I'm not sure I understand this change, why do we need to return a flag from async method that shows whether it was invoked synchronously or not in order to check availability of confirmation. I think, it's better to keep it as it was and has some additional flag that tells whether we need to invoke notifyIfNeeded.

@tikhop the problem is that after the wallet reset / the network is changed, we don't have a good way to determine if we need to show this notification or not. Any other flag that we keep will be erased.

The boolean here isn't about how the method was invoked, but whether we had to wait for the confirmation or not. If we waited - means the confirmation was still in process and we need to inform the user that it's finished. If not - means the confirmation was already done and the user has already received the notification.

@Syn-McJ I would keep it simple and use the flag instead. If user resets the wallet, the flag is also reseted then it's fine to show notification once again.

Showing the notification again is the issue that was raised during QA and that we're trying to fix :)
Specifically, the user would see "Your account is confirmed" every time the network is changed.

I can probably do this with a flag but in a slightly different way.

Okay, you have one flag for all networks and if I try to create on mainnet and then switch on testnet you need to reset the flag as well, otherwise you can't display a notification if we created an account once on a different network. We can keep it as it is, but I think a better solution is to have a different flag per network since you can have two accounts with different networks.

This doesn't sound like "keep it simple" to me :)
Anyway, I'll do the flag solution and trigger it when the user initiates the confirmation. Then we'll only show the notification if the flag is set.

tikhop · 2023-02-14T06:43:06Z

DashWallet/Sources/Models/CrowdNode/CrowdNode.swift

@@ -62,6 +62,8 @@ public class CrowdNodeObjcWrapper: NSObject {

 private let kValidStatus = "valid"


@Syn-McJ I see that we are passing the status as a string, but since we have a predefined statuses it would be better to have a string enum.

The OnlineAccountState and these statuses are different - we have the first to keep the state of the app, while these are just constants to check which response had CrowdNode API returned.

I don't think we should mix them since that could be confusing.

DashWallet/Sources/Models/CrowdNode/CrowdNode+UserDefaults.swift

tikhop

Looks good except a few things.

Syn-McJ · 2023-02-14T13:27:09Z

DashWallet/Sources/Models/CrowdNode/CrowdNode.swift

@@ -758,11 +800,40 @@ extension CrowdNode {
                notifyIfNeeded(message: NSLocalizedString("Your CrowdNode address has been validated, but verification is required.", comment: "CrowdNode"))
            } else if status.lowercased() == kConfirmedStatus {
                changeOnlineState(to: .done)
-                notifyIfNeeded(message: NSLocalizedString("Your CrowdNode address has been confirmed.", comment: "CrowdNode"))
+
+                if prefs.shouldShowConfirmedNotification {


Actually, I forgot about this case, so there was probably a bug with the previous solution.

tikhop

Looks great!

Andrei Ashikhmin added 14 commits February 6, 2023 20:44

feat: online account info UI

82dac5f

fix: back button color on Portal

abe9754

Merge branch 'feature/crowdnode-restore-online' into feature/crowdnod…

662d5eb

…e-new-online-account

feat: online account email screen

de9f398

Merge branch 'feature/crowdnode-restore-online' into feature/crowdnod…

a563a70

…e-new-online-account

feat: outlined textfield

a11d168

fix: outlined textfield colors / focus

0b1c724

feat: email input validation and error behavior

21fe22e

fix: jumping additional info height

9e104ff

feat: email signing and registration

0f21ce3

feat: webview signup flow

bad4f64

fix: updating online item

8cd9a1c

fix: try not to show notifications after network switch / rescan

0dddc30

Merge branch 'feature/crowdnode' into feature/crowdnode-new-online-ac…

13d6016

…count

Syn-McJ requested a review from tikhop February 14, 2023 05:33

Syn-McJ self-assigned this Feb 14, 2023

Syn-McJ commented Feb 14, 2023

View reviewed changes

tikhop reviewed Feb 14, 2023

View reviewed changes

DashWallet/Sources/Models/CrowdNode/CrowdNode+UserDefaults.swift Show resolved Hide resolved

tikhop suggested changes Feb 14, 2023

View reviewed changes

Andrei Ashikhmin added 3 commits February 14, 2023 15:27

fix: caching user defaults

45124d7

fix: calculating outline path in layoutSubviews

1a00467

fix: keep a flag for confirmation notification

89e3121

Syn-McJ requested a review from tikhop February 14, 2023 13:25

Syn-McJ commented Feb 14, 2023

View reviewed changes

tikhop approved these changes Feb 15, 2023

View reviewed changes

Syn-McJ merged commit f2093ea into dashpay:feature/crowdnode Feb 15, 2023

Syn-McJ deleted the feature/crowdnode-new-online-account branch August 3, 2023 13:38


		private func waitForApiAddressConfirmation(primaryAddress: String, apiAddress: String) async -> DSTransaction {
		private func waitForApiAddressConfirmation(primaryAddress: String, apiAddress: String) async -> (tx: DSTransaction, didWait: Bool) {

		@@ -62,6 +62,8 @@ public class CrowdNodeObjcWrapper: NSObject {

		private let kValidStatus = "valid"

feat(crowdnode): Linking New Online account [NMI-917] #506

feat(crowdnode): Linking New Online account [NMI-917] #506

Uh oh!

Conversation

Syn-McJ commented Feb 14, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

Syn-McJ Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tikhop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tikhop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Syn-McJ Feb 14, 2023 •

edited

Loading

Syn-McJ Feb 14, 2023 •

edited

Loading

Syn-McJ Feb 14, 2023 •

edited

Loading

Syn-McJ Feb 14, 2023 •

edited

Loading