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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ package.addModules([
name: "GeneralSettings",
dependencies: [
"AppPreferencesClient",
"FactorSourcesClient", // check if has any ledgers
],
tests: .no
),
Expand Down Expand Up @@ -254,6 +255,7 @@ package.addModules([
name: "SigningFeature",
featureSuffixDroppedFromFolderName: true,
dependencies: [
"AppPreferencesClient",
"EngineToolkit",
"FactorSourcesClient",
"LedgerHardwareWalletClient",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,40 @@ extension SigningFactors {
}
}

extension FactorSourcesClient {
public func getFactorSource(
id: FactorSourceID,
matching filter: @escaping (FactorSource) -> Bool = { _ in true }
) async throws -> FactorSource? {
try await getFactorSources(matching: filter)[id: id]
}

public func getFactorSource(
id: FactorSourceID,
ensureKind kind: FactorSourceKind
) async throws -> FactorSource? {
try await getFactorSource(id: id) { $0.kind == kind }
}

public func getFactorSource(
of factorInstance: FactorInstance
) async throws -> FactorSource? {
try await getFactorSource(id: factorInstance.factorSourceID)
}

public func getFactorSources(
matching filter: (FactorSource) -> Bool
) async throws -> IdentifiedArrayOf<FactorSource> {
try await IdentifiedArrayOf(uniqueElements: getFactorSources().filter(filter))
}

public func getFactorSources(
ofKind kind: FactorSourceKind
) async throws -> IdentifiedArrayOf<FactorSource> {
try await getFactorSources(matching: { $0.kind == kind })
}
}

// MARK: - UpdateFactorSourceLastUsedRequest
public struct UpdateFactorSourceLastUsedRequest: Sendable, Hashable {
public let factorSourceIDs: [FactorSource.ID]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,16 @@ extension FactorSourcesClient: DependencyKey {

return factorSourceID
},
checkIfHasOlympiaFactorSourceForAccounts: { softwareAccounts in
checkIfHasOlympiaFactorSourceForAccounts: { softwareAccounts -> FactorSourceID? in
guard softwareAccounts.allSatisfy({ $0.accountType == .software }) else {
assertionFailure("Unexpectedly received hardware account, unable to verify.")
return nil
}
do {
// cannot use `getFactorSources:ofKind`
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let factorSourceIDs = try await getFactorSources()
.filter { $0.kind == .device && $0.supportsOlympia }
.filter(\.supportsOlympia)
.filter { $0.kind == .device }
.map(\.id)

for factorSourceID in factorSourceIDs {
Expand Down
4 changes: 2 additions & 2 deletions Sources/Clients/FaucetClient/FaucetClient+Live.swift
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,14 @@ extension FaucetClient: DependencyKey {
}

return Self(
getFreeXRD: getFreeXRD,
getFreeXRD: getFreeXRD,
isAllowedToUseFaucet: isAllowedToUseFaucet,
createFungibleToken: createFungibleToken,
createNonFungibleToken: createNonFungibleToken
)
#else
return Self(
getFreeXRD: getFreeXRD,
getFreeXRD: getFreeXRD,
isAllowedToUseFaucet: isAllowedToUseFaucet
)
#endif // DEBUG
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,13 @@ extension UseFactorSourceClient {
@Dependency(\.factorSourcesClient) var factorSourcesClient
@Dependency(\.secureStorageClient) var secureStorageClient

let allFactorSources = try await factorSourcesClient.getFactorSources()
switch entity.securityState {
case let .unsecured(control):
let factorInstance = control.genesisFactorInstance
guard let deviceFactorSource = allFactorSources[id: factorInstance.factorSourceID], deviceFactorSource.kind == .device else {

guard
let deviceFactorSource = try await factorSourcesClient.getFactorSource(of: factorInstance)
else {
throw FailedToDeviceFactorSourceForSigning()
}

Expand All @@ -92,6 +94,7 @@ extension UseFactorSourceClient {
unhashedDataToSign: unhashedDataToSign,
purpose: purpose
)

guard let signature = signatures.first, signatures.count == 1 else {
throw IncorrectSignatureCountExpectedExactlyOne()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"appPreferences" : {
"display" : {
"fiatCurrencyPriceTarget" : "usd",
"isCurrencyAmountVisible" : true
"isCurrencyAmountVisible" : true,
"ledgerHQHardwareWalletSigningDisplayMode" : "verbose"
},
"gateways" : {
"current" : "https://betanet.radixdlt.com",
Expand Down Expand Up @@ -261,5 +262,5 @@
]
}
],
"version" : 30
"version" : 31
}
2 changes: 1 addition & 1 deletion Sources/EngineToolkit/EngineToolkit/EngineToolkit.swift
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ func prettyPrintResponse(jsonString: String) {
}

func prettyPrint<FailedDecodable: Decodable>(
responseJSONString: String,
responseJSONString: String,
error: Swift.Error,
failedToDecodeInto: FailedDecodable.Type
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public struct CallFunction: InstructionProtocol {

#if swift(<5.8)
public init(
packageAddress: PackageAddress,
packageAddress: PackageAddress,
blueprintName: String,
functionName: String,
@SpecificValuesBuilder buildValues: () throws -> [ManifestASTValue]
Expand All @@ -58,7 +58,7 @@ public struct CallFunction: InstructionProtocol {
}

public init(
packageAddress: PackageAddress,
packageAddress: PackageAddress,
blueprintName: String,
functionName: String,
@SpecificValuesBuilder buildValue: () throws -> ManifestASTValue
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public struct CallMethod: InstructionProtocol {

#if swift(<5.8)
public init(
receiver: ComponentAddress,
receiver: ComponentAddress,
methodName: String,
@SpecificValuesBuilder buildValues: () throws -> [ManifestASTValue]
) rethrows {
Expand All @@ -51,7 +51,7 @@ public struct CallMethod: InstructionProtocol {
}

public init(
receiver: ComponentAddress,
receiver: ComponentAddress,
methodName: String,
@SpecificValuesBuilder buildValue: () throws -> ManifestASTValue
) rethrows {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public struct Array_: ValueProtocol, Sendable, Codable, Hashable {

#if swift(<5.8)
public init(
elementKind: ManifestASTValueKind,
elementKind: ManifestASTValueKind,
@SpecificValuesBuilder buildValues: () throws -> [ManifestASTValue]
) throws {
try self.init(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@ extension AccountList {

private func checkIfDeviceFactorSourceControls(factorInstance: FactorInstance) -> EffectTask<Action> {
.run { send in
let allFactorSources = try await factorSourcesClient.getFactorSources()
guard
let factorSource = allFactorSources[id: factorInstance.factorSourceID]
let factorSource = try await factorSourcesClient.getFactorSource(of: factorInstance)
else {
loggerGlobal.warning("Did not find factor source for factor instance.")
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,7 @@ public struct AddLedgerFactorSource: Sendable, FeatureReducer {
state.isWaitingForResponseFromLedger = false
loggerGlobal.notice("Successfully received response from CE! \(ledgerDeviceInfo) ✅")
return .run { send in
let allFactorSources = try await factorSourcesClient.getFactorSources()
if let existing = allFactorSources.first(where: { $0.id == ledgerDeviceInfo.id }) {
if let existing = try await factorSourcesClient.getFactorSource(id: ledgerDeviceInfo.id) {
await send(.internal(.alreadyExists(existing)))
} else {
// new!
Expand Down
21 changes: 21 additions & 0 deletions Sources/Features/GeneralSettings/GeneralSettings+View.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import FeaturePrelude
extension GeneralSettings.State {
var viewState: GeneralSettings.ViewState {
.init(
hasLedgerHardwareWalletFactorSources: hasLedgerHardwareWalletFactorSources,
useVerboseLedgerDisplayMode: (preferences?.display.ledgerHQHardwareWalletSigningDisplayMode ?? .default) == .verbose,
isDeveloperModeEnabled: preferences?.security.isDeveloperModeEnabled ?? false
)
}
Expand All @@ -11,6 +13,11 @@ extension GeneralSettings.State {
// MARK: - GeneralSettings.View
extension GeneralSettings {
public struct ViewState: Equatable {
let hasLedgerHardwareWalletFactorSources: Bool

/// only to be displayed if `hasLedgerHardwareWalletFactorSources` is true
let useVerboseLedgerDisplayMode: Bool

let isDeveloperModeEnabled: Bool
}

Expand All @@ -36,12 +43,26 @@ extension GeneralSettings {
VStack(spacing: .zero) {
VStack(spacing: .zero) {
isDeveloperModeEnabled(with: viewStore)
if viewStore.hasLedgerHardwareWalletFactorSources {
isUsingVerboseLedgerMode(with: viewStore)
}
Separator()
}
.padding(.medium3)
}
}

private func isUsingVerboseLedgerMode(with viewStore: ViewStoreOf<GeneralSettings>) -> some SwiftUI.View {
ToggleView(
title: "Verbose Ledger transaction signing",
subtitle: "When signing with your Ledger hardware wallet, should all instructions be displayed?",
isOn: viewStore.binding(
Copy link
Contributor

@kugel3 kugel3 May 4, 2023

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

get: \.useVerboseLedgerDisplayMode,
send: { .useVerboseModeToggled($0) }
)
)
}

private func isDeveloperModeEnabled(with viewStore: ViewStoreOf<GeneralSettings>) -> some SwiftUI.View {
ToggleView(
title: L10n.GeneralSettings.DeveloperMode.title,
Expand Down
27 changes: 25 additions & 2 deletions Sources/Features/GeneralSettings/GeneralSettings.swift
Original file line number Diff line number Diff line change
@@ -1,33 +1,46 @@
import AppPreferencesClient
import FactorSourcesClient
import FeaturePrelude

// MARK: - GeneralSettings
public struct GeneralSettings: Sendable, FeatureReducer {
public struct State: Sendable, Hashable {
var preferences: AppPreferences?

public var preferences: AppPreferences?
public var hasLedgerHardwareWalletFactorSources: Bool = false
public init() {}
}

public enum ViewAction: Sendable, Equatable {
case appeared
case useVerboseModeToggled(Bool)
case developerModeToggled(Bool)
}

public enum InternalAction: Sendable, Equatable {
case loadPreferences(AppPreferences)
case hasLedgerHardwareWalletFactorSourcesLoaded(Bool)
}

public init() {}

@Dependency(\.appPreferencesClient) var appPreferencesClient
@Dependency(\.factorSourcesClient) var factorSourcesClient

public func reduce(into state: inout State, viewAction: ViewAction) -> EffectTask<Action> {
switch viewAction {
case .appeared:
return .run { send in
let preferences = await appPreferencesClient.getPreferences()
await send(.internal(.loadPreferences(preferences)))

do {
let ledgers = try await factorSourcesClient.getFactorSources(ofKind: .ledgerHQHardwareWallet)
await send(.internal(.hasLedgerHardwareWalletFactorSourcesLoaded(!ledgers.isEmpty)))
} catch {
loggerGlobal.warning("Failed to load ledgers, error: \(error)")
// OK to display it...
await send(.internal(.hasLedgerHardwareWalletFactorSourcesLoaded(true)))
}
}

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

case let .useVerboseModeToggled(useVerboseMode):
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we just put the mode here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

state.preferences?.display.ledgerHQHardwareWalletSigningDisplayMode = useVerboseMode ? .verbose : .summary
guard let preferences = state.preferences else { return .none }
return .fireAndForget {
try await appPreferencesClient.updatePreferences(preferences)
}
}
}

Expand All @@ -44,6 +64,9 @@ public struct GeneralSettings: Sendable, FeatureReducer {
case let .loadPreferences(preferences):
state.preferences = preferences
return .none
case let .hasLedgerHardwareWalletFactorSourcesLoaded(hasLedgerHardwareWalletFactorSources):
state.hasLedgerHardwareWalletFactorSources = hasLedgerHardwareWalletFactorSources
return .none
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,7 @@ public struct ImportOlympiaLedgerAccountsAndFactorSources: Sendable, FeatureRedu
}

return .run { send in
let allFactorSources = try await factorSourcesClient.getFactorSources()
if let existing = allFactorSources.first(where: { $0.id == olympiaDevice.id }) {
if let existing = try await factorSourcesClient.getFactorSource(id: olympiaDevice.id) {
await send(.internal(.alreadyExists(existing)))
} else {
// new!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ extension DisplayView {
#if os(macOS)
.font(.title)
#endif // os(macOS)
Labeled("Ledger Signing Display Mode", value: display.ledgerHQHardwareWalletSigningDisplayMode.rawValue)
Labeled("Currency", value: display.fiatCurrencyPriceTarget.rawValue)
}
.padding([.leading], leadingPadding)
Expand Down Expand Up @@ -678,8 +679,10 @@ public struct Labeled: SwiftUI.View {
HStack(alignment: .top) {
Text(label)
.fontWeight(.light)
.textSelection(.enabled)
Text(value)
.fontWeight(.bold)
.textSelection(.enabled)
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import AppPreferencesClient
import EngineToolkit
import FactorSourcesClient
import FeaturePrelude
Expand All @@ -21,6 +22,7 @@ public struct SignWithFactorSourcesOfKindLedger: SignWithFactorSourcesOfKindRedu
}

@Dependency(\.ledgerHardwareWalletClient) var ledgerHardwareWalletClient
@Dependency(\.appPreferencesClient) var appPreferencesClient
public init() {}

public func reduce(into state: inout State, viewAction: ViewAction) -> EffectTask<Action> {
Expand Down Expand Up @@ -48,11 +50,22 @@ public struct SignWithFactorSourcesOfKindLedger: SignWithFactorSourcesOfKindRedu
} catch {
loggerGlobal.critical("Failed to hash: \(error)")
}
let ledgerTXDisplayMode: FactorSource.LedgerHardwareWallet.SigningDisplayMode = await appPreferencesClient.getPreferences().display.ledgerHQHardwareWalletSigningDisplayMode
return try await ledgerHardwareWalletClient.sign(.init(
signingFactor: signingFactor,
unhashedDataToSign: state.dataToSign,
ledgerTXDisplayMode: .verbose,
ledgerTXDisplayMode: ledgerTXDisplayMode.mode,
displayHashOnLedgerDisplay: true
))
}
}

extension FactorSource.LedgerHardwareWallet.SigningDisplayMode {
// seperation so that we do not accidentally break profile or RadixConnect
Copy link
Contributor

Choose a reason for hiding this comment

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

Separation in the sense that we have two identical enums?

var mode: P2P.ConnectorExtension.Request.LedgerHardwareWallet.Request.SignTransaction.Mode {
switch self {
case .verbose: return .verbose
case .summary: return .summary
}
}
}
Loading