[ABW-1472] Allow user to toggle Verbose / Summary when making ledger requests #474

CyonAlexRDX · 2023-05-04T14:58:41Z

Jira ticket: ABW-1472

Description

Adds a setting (in Profile.AppPrefences.Display) to change Ledger Hardware wallet display mode during signing, either .verbose or .summary, since we only have those two possible values (and we have not identified the need for a third one) we can represent it as a Toggle (bool).

In SignWithFactorSourcesOfKindLedger we now read this mode from Profile using appPreferencesClient

Video

trim.193A3FC2-65AC-4392-95BE-2E4521AB6176.MOV

PR submission checklist

I have tested account to account transfer flow and have confirmed that it works

kugel3 · 2023-05-04T20:15:21Z

Sources/Clients/FactorSourcesClient/FactorSourcesClient+Interface.swift

@@ -49,6 +49,12 @@ extension SigningFactors {
 	}
 }

+extension FactorSourcesClient {
+	public func getFactorSources(ofKind kind: FactorSourceKind) async throws -> Set<FactorSource> {


I suppose we don't want this to be nonempty, since it can legitimately be empty, but one could argue that it should still be an identified array, since the set will only guarantee that it's unique with respect to the whole factor source (it seems like it uses synthesized hashing), but we actually know that it's unique also with respect to id alone, which is stronger.

Since it's a getter the set doesn't sacrifice any safety w.r.t this function, but perhaps the consumer would benefit from the extra information. Or not.

👍 changed to IdentfiedArrayOf and upgraded FactorSourcesClient a bit with useful convenience query methods

kugel3 · 2023-05-04T20:16:35Z

Sources/Features/GeneralSettings/GeneralSettings+View.swift

@@ -3,6 +3,8 @@ import FeaturePrelude
 extension GeneralSettings.State {
 	var viewState: GeneralSettings.ViewState {
 		.init(
+			hasAnyLedgerHardwareWalletFactorSources: hasAnyLedgerHardwareWalletFactorSources,


I'd argue that "any" is superfluous here

Sources/Features/InspectProfileFeature/InspectProfile+View.swift

kugel3 · 2023-05-04T20:21:18Z

...g/Children/Factors/SignWithFactorSourcesOfKindLedger/SignWithFactorSourcesOfKindLedger.swift

 			displayHashOnLedgerDisplay: true
 		))
 	}
 }
+
+extension FactorSource.LedgerHardwareWallet.SigningDisplayMode {
+	// seperation so that we do not accidentally break profile or RadixConnect


Separation in the sense that we have two identical enums?

kugel3 · 2023-05-04T20:22:27Z

Sources/Profile/Factor/FactorSource+Kinds/FactorSource+LedgerHardwareWallet.swift

@@ -9,6 +9,12 @@ extension FactorSource {
 			case nanoSPlus = "nanoS+"
 			case nanoX
 		}
+
+		public enum SigningDisplayMode: String, Sendable, Hashable, Codable {


I would prefer to break this out instead of copying it, but maybe there is no natural place to put it

I started using the same enum, but I think it might be something that might bite us in the ass later, which is why I for LedgerDevice enum as well against every fiber of my being chose to duplicate the models. This kind of repetition hurts my soul, but I still think it is the right choice here. The reason is that yes they do share the same Codable values now, but logically they are different models. One model is a CAP21 contract between wallet and Connector Extension, the other is the CRUCIAL internals of Profile, which if accidentally changed, breaks the whole app.

We are iterating both domains a lot still, thus it feels safest to keep them apart.

Does that make sense?

OK that makes sense

kugel3 · 2023-05-04T20:25:26Z

Sources/Features/GeneralSettings/GeneralSettings+View.swift

@@ -11,6 +13,11 @@ extension GeneralSettings.State {
 // MARK: - GeneralSettings.View
 extension GeneralSettings {
 	public struct ViewState: Equatable {
+		let hasAnyLedgerHardwareWalletFactorSources: Bool
+
+		/// only to be displayed if `hasAnyLedgerHardwareWalletFactorSources` is true


I would not have been able to resist the temptation to make an enum here

enum LedgerHardwareWalletFactorSourcesPresent { case zero case some(displayMode: Mode) }

which would probably just make things more complex

yeah I started out doing something like that too, but was not really worth it

kugel3 · 2023-05-04T20:26:49Z

Sources/Features/GeneralSettings/GeneralSettings+View.swift

+			ToggleView(
+				title: "Verbose Ledger transaction signing",
+				subtitle: "When signing with your Ledger hardware wallet, should all instructions be displayed?",
+				isOn: viewStore.binding(


Since you're only using the binding in here, you could create it outside and pass it in. This has no practical benefit, it's merely philosophical since this view is very unlikely to be reused...

I think unnecessary variables in ResultBuilders are ugly :D matter of taste I guess...

kugel3 · 2023-05-04T20:28:31Z

Sources/Features/GeneralSettings/GeneralSettings.swift

 		var preferences: AppPreferences?
-
+		var hasAnyLedgerHardwareWalletFactorSources: Bool = false


I suppose the properties should also be public, strictly speaking?

yes you are right

kugel3 · 2023-05-04T20:29:51Z

Sources/Features/GeneralSettings/GeneralSettings.swift

@@ -36,6 +49,13 @@ public struct GeneralSettings: Sendable, FeatureReducer {
 			return .fireAndForget {
 				try await appPreferencesClient.updatePreferences(preferences)
 			}
+
+		case let .useVerboseModeToggled(useVerboseMode):


Couldn't we just put the mode here?

the problem with bindings and mapping them, it is a hassel... I feel the logic of mapping from Bool (which Toggle uses) to DisplayMode is a job for the reducer, not the view.

Ah yes if it's the one that goes into the binding then sure, bool makes more sense

kugel3

Looks great, I just have some pedantic comments

kugel3 · 2023-05-05T08:25:34Z

Sources/Clients/FactorSourcesClientLive/FactorSourcesClient+Live.swift

 				guard softwareAccounts.allSatisfy({ $0.accountType == .software }) else {
 					assertionFailure("Unexpectedly received hardware account, unable to verify.")
 					return nil
 				}
 				do {
-					let factorSourceIDs = try await getFactorSources(ofKind: .device)
+					// cannot use `getFactorSources:ofKind`


Out of curiosity, why not?

because we are inside the init of Self and getFactorSource:ofKind is a non static func, I myself silly enough tried to call it, which ofc does not work, since we are in the process of init Self :D

one could complicate matters with static func but deemed it not worth it.

CyonAlexRDX added 3 commits May 4, 2023 16:57

WIP

add9f4e

WIP

178e990

WIP

e357de4

CyonAlexRDX marked this pull request as ready for review May 4, 2023 17:58

CyonAlexRDX requested review from GhenadieVP and kugel3 May 4, 2023 18:02

kugel3 reviewed May 4, 2023

View reviewed changes

Sources/Features/InspectProfileFeature/InspectProfile+View.swift Show resolved Hide resolved

kugel3 reviewed May 4, 2023

View reviewed changes

kugel3 approved these changes May 4, 2023

View reviewed changes

CyonAlexRDX added 6 commits May 5, 2023 08:15

WIP

102da7c

WIP

f735217

WIP

7f148fe

WIP

52fcd8e

swiftformat 0.51.9

b249a2d

WIP

983f92b

kugel3 reviewed May 5, 2023

View reviewed changes

CyonAlexRDX merged commit 01a9b9a into main May 5, 2023

CyonAlexRDX deleted the ABW-1472_settings_for_ledgerHardwareDevice_displayMode branch May 5, 2023 10:27

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABW-1472] Allow user to toggle Verbose / Summary when making ledger requests #474

[ABW-1472] Allow user to toggle Verbose / Summary when making ledger requests #474

CyonAlexRDX commented May 4, 2023 •

edited

Loading

kugel3 May 4, 2023 •

edited

Loading

CyonAlexRDX May 5, 2023

kugel3 May 4, 2023

kugel3 May 4, 2023

kugel3 May 4, 2023

CyonAlexRDX May 5, 2023

kugel3 May 5, 2023

kugel3 May 4, 2023

CyonAlexRDX May 5, 2023

kugel3 May 4, 2023 •

edited

Loading

CyonAlexRDX May 5, 2023

kugel3 May 4, 2023

CyonAlexRDX May 5, 2023

kugel3 May 4, 2023

CyonAlexRDX May 5, 2023

kugel3 May 5, 2023

kugel3 left a comment

kugel3 May 5, 2023

CyonAlexRDX May 5, 2023

CyonAlexRDX May 5, 2023

		var preferences: AppPreferences?

		var hasAnyLedgerHardwareWalletFactorSources: Bool = false

[ABW-1472] Allow user to toggle Verbose / Summary when making ledger requests #474

[ABW-1472] Allow user to toggle Verbose / Summary when making ledger requests #474

Conversation

CyonAlexRDX commented May 4, 2023 • edited Loading

Description

Video

PR submission checklist

kugel3 May 4, 2023 • edited Loading

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

kugel3 May 4, 2023 • edited Loading

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

kugel3 left a 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

CyonAlexRDX commented May 4, 2023 •

edited

Loading

kugel3 May 4, 2023 •

edited

Loading

kugel3 May 4, 2023 •

edited

Loading