From 9710711d93ba4df525827d2f1dba4b5e9b1335b7 Mon Sep 17 00:00:00 2001 From: Stephen Heaps Date: Thu, 2 Mar 2023 10:43:10 -0500 Subject: [PATCH 1/3] Add loading state to `SuggestedNetworkView`. Add completion closure workaround to add network dapp request; changes behaviour of bug when failing to add a network to get stuck in loading state instead of dismissing. --- .../Crypto/Stores/CryptoStore.swift | 32 +++++++++++++- .../SuggestedNetworkView.swift | 43 +++++++++++++------ .../WalletHostingViewController.swift | 4 +- 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift index 852cbca1982..c3d4b61c5cc 100644 --- a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift +++ b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift @@ -386,12 +386,34 @@ public class CryptoStore: ObservableObject { return pendingRequest != nil } - func handleWebpageRequestResponse(_ response: WebpageRequestResponse) { + private struct AddNetworkCompletion { + let chainId: String + let completion: (_ error: String?) -> Void + } + // Helper to store the completion block of an Add Network request. + // The completion closure is handled in `onAddEthereumChainRequestCompleted` + // when we determine if the chain was added successfully or not. + private var addNetworkWebpageRequestCompletion: AddNetworkCompletion? + + func handleWebpageRequestResponse( + _ response: WebpageRequestResponse, + completion: ((_ error: String?) -> Void)? = nil + ) { switch response { case let .switchChain(approved, originInfo): rpcService.notifySwitchChainRequestProcessed(approved, origin: originInfo.origin) case let .addNetwork(approved, chainId): - rpcService.addEthereumChainRequestCompleted(chainId, approved: approved) + // for add network request, approval requires network call so we must + // wait for `onAddEthereumChainRequestCompleted` to know success/failure + if approved, let completion { + // store `completion` closure until notified of `onAddEthereumChainRequestCompleted` event + addNetworkWebpageRequestCompletion = .init(chainId: chainId, completion: completion) + rpcService.addEthereumChainRequestCompleted(chainId, approved: approved) + } else { // not approved, or no completion closure provided. + completion?(nil) + rpcService.addEthereumChainRequestCompleted(chainId, approved: approved) + } + return case let .addSuggestedToken(approved, contractAddresses): walletService.notifyAddSuggestTokenRequestsProcessed(approved, contractAddresses: contractAddresses) case let .signMessage(approved, id): @@ -406,6 +428,7 @@ public class CryptoStore: ObservableObject { walletService.notifySignAllTransactionsRequestProcessed(approved, id: id, signatures: nil, error: nil) } pendingRequest = nil + completion?(nil) } public func rejectAllPendingWebpageRequests() { @@ -491,6 +514,11 @@ extension CryptoStore: BraveWalletJsonRpcServiceObserver { } public func onAddEthereumChainRequestCompleted(_ chainId: String, error: String) { + if let addNetworkWebpageRequestCompletion { + addNetworkWebpageRequestCompletion.completion(error.isEmpty ? nil : error) + self.addNetworkWebpageRequestCompletion = nil + prepare() + } } public func onIsEip1559Changed(_ chainId: String, isEip1559: Bool) { diff --git a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift index ab9a5ed8fd5..97f8bce3418 100644 --- a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift +++ b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift @@ -24,6 +24,7 @@ struct SuggestedNetworkView: View { @State private var isPresentingNetworkDetails: CustomNetworkModel? @State private var customNetworkError: CustomNetworkError? + @State private var isLoading: Bool = false @ScaledMetric private var blockieSize = 24 private let maxBlockieSize: CGFloat = 72 @@ -253,7 +254,7 @@ struct SuggestedNetworkView: View { Alert( title: Text(error.errorTitle), message: Text(error.errorDescription), - dismissButton: .default(Text(Strings.OKString)) + dismissButton: .default(Text(Strings.OKString), action: onDismiss) ) }) ) @@ -288,30 +289,48 @@ struct SuggestedNetworkView: View { } } .buttonStyle(BraveOutlineButtonStyle(size: .large)) - Button(action: { // approve - handleAction(approved: true) - }) { - HStack { - Image(braveSystemName: "brave.checkmark.circle.fill") - Text(actionButtonTitle) - .multilineTextAlignment(.center) + .disabled(isLoading) + WalletLoadingButton( + isLoading: isLoading, + action: { // approve + handleAction(approved: true) + }, + label: { + HStack { + Image(braveSystemName: "brave.checkmark.circle.fill") + Text(actionButtonTitle) + .multilineTextAlignment(.center) + } } - } + ) .buttonStyle(BraveFilledButtonStyle(size: .large)) + .disabled(isLoading) } private func handleAction(approved: Bool) { + isLoading = true switch mode { case let .addNetwork(networkInfo): cryptoStore.handleWebpageRequestResponse( - .addNetwork(approved: approved, chainId: networkInfo.chainId) + .addNetwork(approved: approved, chainId: networkInfo.chainId), + completion: { error in + isLoading = false + if let error, !error.isEmpty { + customNetworkError = .failed(errorMessage: error) + return + } + onDismiss() + } ) case .switchNetworks: cryptoStore.handleWebpageRequestResponse( - .switchChain(approved: approved, originInfo: originInfo) + .switchChain(approved: approved, originInfo: originInfo), + completion: { _ in + isLoading = false + onDismiss() + } ) } - onDismiss() } } diff --git a/Sources/BraveWallet/WalletHostingViewController.swift b/Sources/BraveWallet/WalletHostingViewController.swift index b9c2e37c2ed..b62181d968a 100644 --- a/Sources/BraveWallet/WalletHostingViewController.swift +++ b/Sources/BraveWallet/WalletHostingViewController.swift @@ -68,8 +68,8 @@ public class WalletHostingViewController: UIHostingController { presentingContext: presentingContext ) ) - rootView.dismissAction = { [unowned self] in - self.dismiss(animated: true) + rootView.dismissAction = { [weak self] in + self?.dismiss(animated: true) } rootView.openWalletURLAction = { [unowned self] url in (self.presentingViewController ?? self).dismiss(animated: true) { [self] in From be26d67d0a4df48064bd3c275cdadc67ebc7e8ae Mon Sep 17 00:00:00 2001 From: Stephen Heaps Date: Thu, 2 Mar 2023 16:19:12 -0500 Subject: [PATCH 2/3] Fix for the case where Add Network modal is dismissed while loading (ex. when failing to add network, waiting for timeout). --- .../Crypto/Stores/CryptoStore.swift | 5 ++- .../SuggestedNetworkView.swift | 32 ++++++++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift index c3d4b61c5cc..455afaf062f 100644 --- a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift +++ b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift @@ -386,14 +386,14 @@ public class CryptoStore: ObservableObject { return pendingRequest != nil } - private struct AddNetworkCompletion { + struct AddNetworkCompletion { let chainId: String let completion: (_ error: String?) -> Void } // Helper to store the completion block of an Add Network request. // The completion closure is handled in `onAddEthereumChainRequestCompleted` // when we determine if the chain was added successfully or not. - private var addNetworkWebpageRequestCompletion: AddNetworkCompletion? + var addNetworkWebpageRequestCompletion: AddNetworkCompletion? func handleWebpageRequestResponse( _ response: WebpageRequestResponse, @@ -517,7 +517,6 @@ extension CryptoStore: BraveWalletJsonRpcServiceObserver { if let addNetworkWebpageRequestCompletion { addNetworkWebpageRequestCompletion.completion(error.isEmpty ? nil : error) self.addNetworkWebpageRequestCompletion = nil - prepare() } } diff --git a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift index 97f8bce3418..9841ddbe1af 100644 --- a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift +++ b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift @@ -258,6 +258,20 @@ struct SuggestedNetworkView: View { ) }) ) + .onAppear { + // this can occur when Add Network is dismissed while still loading... + // we need to show loading state again, and handle success/failure response + if case let .addNetwork(network) = mode, + let pendingCompletion = cryptoStore.addNetworkWebpageRequestCompletion, + pendingCompletion.chainId == network.chainId { + self.isLoading = true + // overwrite the completion closure with a new one for this new view instance + cryptoStore.addNetworkWebpageRequestCompletion = .init( + chainId: network.chainId, + completion: handleAddNetworkCompletion + ) + } + } } private var actionButtonTitle: String { @@ -313,14 +327,7 @@ struct SuggestedNetworkView: View { case let .addNetwork(networkInfo): cryptoStore.handleWebpageRequestResponse( .addNetwork(approved: approved, chainId: networkInfo.chainId), - completion: { error in - isLoading = false - if let error, !error.isEmpty { - customNetworkError = .failed(errorMessage: error) - return - } - onDismiss() - } + completion: handleAddNetworkCompletion ) case .switchNetworks: cryptoStore.handleWebpageRequestResponse( @@ -332,6 +339,15 @@ struct SuggestedNetworkView: View { ) } } + + private func handleAddNetworkCompletion(_ error: String?) { + isLoading = false + if let error, !error.isEmpty { + customNetworkError = .failed(errorMessage: error) + return + } + onDismiss() + } } #if DEBUG From 1cabf891bc8232e88b5d92112926a44380d9e2a6 Mon Sep 17 00:00:00 2001 From: Stephen Heaps Date: Fri, 3 Mar 2023 16:11:08 -0500 Subject: [PATCH 3/3] Address PR comment. Store `AddNetworkCompletion` closures in a dictionary for each chain_id incase we received a new Add Chain request while one is loading. --- .../Crypto/Stores/CryptoStore.swift | 19 ++++++++----------- .../SuggestedNetworkView.swift | 8 ++------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift index 455afaf062f..0a80499bedd 100644 --- a/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift +++ b/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift @@ -386,14 +386,11 @@ public class CryptoStore: ObservableObject { return pendingRequest != nil } - struct AddNetworkCompletion { - let chainId: String - let completion: (_ error: String?) -> Void - } - // Helper to store the completion block of an Add Network request. - // The completion closure is handled in `onAddEthereumChainRequestCompleted` + // Helper to store the completion block of an Add Network dapp request. + typealias AddNetworkCompletion = (_ error: String?) -> Void + // The completion closure(s) are handled in `onAddEthereumChainRequestCompleted` // when we determine if the chain was added successfully or not. - var addNetworkWebpageRequestCompletion: AddNetworkCompletion? + var addNetworkDappRequestCompletion: [String: AddNetworkCompletion] = [:] func handleWebpageRequestResponse( _ response: WebpageRequestResponse, @@ -407,7 +404,7 @@ public class CryptoStore: ObservableObject { // wait for `onAddEthereumChainRequestCompleted` to know success/failure if approved, let completion { // store `completion` closure until notified of `onAddEthereumChainRequestCompleted` event - addNetworkWebpageRequestCompletion = .init(chainId: chainId, completion: completion) + addNetworkDappRequestCompletion[chainId] = completion rpcService.addEthereumChainRequestCompleted(chainId, approved: approved) } else { // not approved, or no completion closure provided. completion?(nil) @@ -514,9 +511,9 @@ extension CryptoStore: BraveWalletJsonRpcServiceObserver { } public func onAddEthereumChainRequestCompleted(_ chainId: String, error: String) { - if let addNetworkWebpageRequestCompletion { - addNetworkWebpageRequestCompletion.completion(error.isEmpty ? nil : error) - self.addNetworkWebpageRequestCompletion = nil + if let addNetworkDappRequestCompletion = addNetworkDappRequestCompletion[chainId] { + addNetworkDappRequestCompletion(error.isEmpty ? nil : error) + self.addNetworkDappRequestCompletion[chainId] = nil } } diff --git a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift index 9841ddbe1af..9612bd67727 100644 --- a/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift +++ b/Sources/BraveWallet/Panels/Add Suggested Network/SuggestedNetworkView.swift @@ -262,14 +262,10 @@ struct SuggestedNetworkView: View { // this can occur when Add Network is dismissed while still loading... // we need to show loading state again, and handle success/failure response if case let .addNetwork(network) = mode, - let pendingCompletion = cryptoStore.addNetworkWebpageRequestCompletion, - pendingCompletion.chainId == network.chainId { + cryptoStore.addNetworkDappRequestCompletion[network.chainId] != nil { self.isLoading = true // overwrite the completion closure with a new one for this new view instance - cryptoStore.addNetworkWebpageRequestCompletion = .init( - chainId: network.chainId, - completion: handleAddNetworkCompletion - ) + cryptoStore.addNetworkDappRequestCompletion[network.chainId] = handleAddNetworkCompletion } } }