Fts/484 account storage alert #525

jeden · 2024-11-13T23:12:38Z

No description provided.

- Extracted a reusable `StorageUsageView` from `WalletSettingView` - Added the `StorageUsageView` to `TokenDetailView` - Added `getAccountInfo` cadence - Handled the `insufficientStorage` case in the remote config to display an insufficient storage alert

…the app

…n failure - Refactored `AlertView` to allow for injected custom content - Created `PersistentToastView` - Added storage usage progress in TokenDetailView

jeden · 2024-11-13T23:16:13Z

Storage view

Move token
Move NFT
Send Token
Send NFT
Sign Transaction on browser
Sign Transaction on WalletConnect
Insufficient Storage Alert
Storage Capacity Exceeded Alert (error condition simulated from code)

- Implemented generic and reusable `InsufficientStorageToastView` as a drop-in specifically for showing the insufficient warning - UI fixes

# Conflicts: # FRW.xcodeproj/project.pbxproj # FRW/Modules/NFT/NFTTransferView.swift # FRW/Modules/Wallet/MoveAsset/MoveComponentView.swift # FRW/Modules/Wallet/MoveAsset/MoveNFTsView.swift # FRW/Modules/Wallet/MoveAsset/MoveSingleNFTView.swift # FRW/Modules/Wallet/MoveAsset/ViewModel/MoveNFTsViewModel.swift # FRW/Modules/Wallet/MoveAsset/ViewModel/MoveSingleNFTViewModel.swift # FRW/Modules/Wallet/MoveAsset/ViewModel/MoveTokenViewModel.swift # FRW/Modules/Wallet/Send/WalletSendAmountViewModel.swift # FRW/Modules/Wallet/SideMenu/SideMenuView.swift # FRW/Modules/Wallet/TokenDetail/TokenDetailViewModel.swift # FRW/Services/Manager/Config/RemoteConfig.swift # FRW/Services/Manager/WalletManager.swift # FRW/Services/Network/FlowNetwork.swift

gitguardian · 2024-11-14T03:24:38Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9474120	Triggered	Generic Password	`0a9fd17`	FRW/Modules/MultiRestore/ViewModel/RestoreMultiConnectViewModel.swift	View secret
9474120	Triggered	Generic Password	`0a9fd17`	FRW/Modules/MultiRestore/ViewModel/RestoreMultiConnectViewModel.swift	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

lmcmz · 2024-11-14T05:05:00Z

FRW/Modules/Wallet/TokenDetail/TokenDetailViewModel.swift

+                    try? await WalletManager.shared.fetchBalance()
+                }
+
+                group.addTask {


No need query checkStorageInfo any more, it's included in AccountInfo now

lmcmz · 2024-11-14T05:06:49Z

FRW/Services/Manager/Config/RemoteConfig.swift

@@ -202,6 +203,9 @@ extension RemoteConfigManager {
                }
            case .isIOS:
                return true
+            case .insufficientStorage:


Let's make this func boolValue async, otherwise, the user might missed their alert on home page.

That could potentially delay the UI, and I wouldn't feel comfortable making an enum method async, as it would imply adding logic that should reside elsewhere.
For that reason I opted for using the event bus, so that notifications are reloaded when the account info has been retrieved.

lmcmz · 2024-11-14T05:07:59Z

FRW/Services/Manager/WalletManager.swift

+                self.accountInfo = accountInfo
+            }
+        } catch let error {
+            log.error("[EVM] fetch account info failed.\(error.localizedDescription)")


It's not EVM action, let's put other tag. Such as [WALLET]

A clear case of copy and paste from another line :-)

lmcmz · 2024-11-14T05:10:48Z

FRW/Services/Network/FlowNetwork.swift

+
+    static func checkAccountInfo() async throws -> Flow.AccountInfo {
+        let address = Flow.Address(hex: WalletManager.shared.getPrimaryWalletAddress() ?? "")
+        let cadence = CadenceManager.shared.current.basic?.getAccountInfo?.toFunc() ?? ""


let's throw the error if the address or cadence is empty

Ok. It doesn't seem to exist an error for invalid cadence, so I've added invalidCadence to LLError. But let me know if there's another error that fits better.

lmcmz · 2024-11-14T05:11:46Z

FRW/Services/Network/FlowNetwork.swift

+    static func checkAccountInfo() async throws -> Flow.AccountInfo {
+        let address = Flow.Address(hex: WalletManager.shared.getPrimaryWalletAddress() ?? "")
+        let cadence = CadenceManager.shared.current.basic?.getAccountInfo?.toFunc() ?? ""
+        let response = try await flow.accessAPI.executeScriptAtLatestBlock(cadence: cadence, arguments: [.address(address)]).decode(Flow.AccountInfo.self)


return try await flow.accessAPI.executeScriptAtLatestBlock(cadence: cadence, arguments: [.address(address)]).decode()

lmcmz · 2024-11-14T05:29:31Z

FRW/Services/Manager/WalletManager.swift

+
+    func isStorageInsufficient(for amount: Decimal) -> Bool {
+        guard let accountInfo else { return false }
+        return accountInfo.availableBalance - amount < Self.minDefaultBlance


Storage check doesn't need amount, it should be storageCapacity - storageUsed < 10000

Yeah I think I forgot to update it after we discussed it... btw I'm reimplementing this part using your pseudo-code

lmcmz · 2024-11-14T05:32:33Z

FRW/Modules/Wallet/SideMenu/SideMenuView.swift

@@ -562,6 +586,28 @@ struct SideContainerView: View {
            }
            .offset(x: vm.isOpen ? screenWidth - SideOffset : 0)
        }
+        .customAlertView(


There should be a better place for placing alert.
And it alert shouldn't be configured on SideContainerView.

lmcmz · 2024-11-14T05:43:25Z

FRW/Services/Manager/TransactionManager.swift

+                            switch result.errorCode {
+                            case .storageCapacityExceeded:
+                                let data = InsufficientStorageTransactionFailureData(minimumBalance: WalletManager.shared.minimumStorageBalance)
+                                NotificationCenter.default.post(name: .insufficientStorageTransactionFailure, object: data)


Cause we post the transactionStatusDidChanged event already, we should reuse the same notification

lmcmz · 2024-11-14T05:56:06Z

FRW/Services/Manager/WalletManager.swift

+
+    func isBalanceInsufficient(for amount: Decimal) -> Bool {
+        guard let accountInfo else { return false }
+        return accountInfo.availableBalance - amount - Self.minDefaultBlance < Self.minDefaultBlance


I assume the - Self.minDefaultBlance is the moveFee ?

Correct — I've renamed all constants using the names you provided in your pseudo-code.

lmcmz · 2024-11-14T05:58:32Z

FRW/UI/Component/InsufficientStorageToastView.swift

+        }
+
+        let wm = WalletManager.shared
+        if (isTransferBetweenEvmAndFlow) {


This logic need revisit, I will post the pseudo code in the chat.

I have a double on this logical expression: should it apply both sides (Evm -> Flow and Flow -> Evm), or it is one side only?

lmcmz · 2024-11-14T08:00:56Z

FRW/UI/Component/PersistentToastView.swift

+            .padding(.horizontal, 12)
+            .background(Color(.orange5))
+        }
+        .background(Color.LL.Neutrals.background.opacity(0.95))


I think the opacity is not the same as design

The opacity is defined in the orange5 color. I added the background at line 30 to control transparency, as the content behind the view would make it unreadable. Compare the two:

The alternative is to make sure the content behind doesn't scroll, but that would require the sheet to be taller. Suggestions?

lmcmz · 2024-11-14T08:01:32Z

FRW/UI/Component/InsufficientStorageToastView.swift

+    @EnvironmentObject private var viewModel: ViewModel
+
+    var body: some View {
+        PersistentToastView(message: self.viewModel.variant?.message ?? "", imageRes: .Storage.insufficient)


Can we add slideUp animation when it appears ?

jeden added 3 commits November 7, 2024 23:31

484 Account storage alert

ed20e6d

- Extracted a reusable `StorageUsageView` from `WalletSettingView` - Added the `StorageUsageView` to `TokenDetailView` - Added `getAccountInfo` cadence - Handled the `insufficientStorage` case in the remote config to display an insufficient storage alert

- Fixed bug causing getAccountInfo() to fail right after launching …

29b5e55

…the app

- Added system-wide popup to notify the user about storage transactio…

208bdd9

…n failure - Refactored `AlertView` to allow for injected custom content - Created `PersistentToastView` - Added storage usage progress in TokenDetailView

jeden marked this pull request as draft November 13, 2024 23:13

jeden added 2 commits November 14, 2024 02:06

- Added storage warning to move/send token/nft views

9ecd3ee

- Implemented generic and reusable `InsufficientStorageToastView` as a drop-in specifically for showing the insufficient warning - UI fixes

- Minor fixes

c71e23c

jeden force-pushed the fts/484-account-storage-alert branch from ab313c9 to c71e23c Compare November 14, 2024 03:59

jeden marked this pull request as ready for review November 14, 2024 04:00

jeden requested review from zhouxl and lmcmz November 14, 2024 04:00

lmcmz requested changes Nov 14, 2024

View reviewed changes

jeden linked an issue Nov 15, 2024 that may be closed by this pull request

[FEATURE] Account storage alert #484

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fts/484 account storage alert #525

Fts/484 account storage alert #525

jeden commented Nov 13, 2024

jeden commented Nov 13, 2024 •

edited

Loading

gitguardian bot commented Nov 14, 2024 •

edited

Loading

lmcmz Nov 14, 2024

lmcmz Nov 14, 2024

jeden Nov 14, 2024

lmcmz Nov 14, 2024

jeden Nov 14, 2024

lmcmz Nov 14, 2024

jeden Nov 14, 2024

lmcmz Nov 14, 2024

lmcmz Nov 14, 2024

jeden Nov 15, 2024

lmcmz Nov 14, 2024

lmcmz Nov 14, 2024

lmcmz Nov 14, 2024

jeden Nov 15, 2024

lmcmz Nov 14, 2024

jeden Nov 15, 2024

lmcmz Nov 14, 2024

jeden Nov 15, 2024

lmcmz Nov 14, 2024

Fts/484 account storage alert #525

Are you sure you want to change the base?

Fts/484 account storage alert #525

Conversation

jeden commented Nov 13, 2024

jeden commented Nov 13, 2024 • edited Loading

gitguardian bot commented Nov 14, 2024 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeden commented Nov 13, 2024 •

edited

Loading

gitguardian bot commented Nov 14, 2024 •

edited

Loading