[ABW-3821] Pre Authorization general UI #1371
Conversation
matiasbzurovski
changed the title
| import SwiftUI | ||
|
|
||
| // MARK: - TransactionReviewAccounts | ||
| struct TransactionReviewAccounts: Sendable, FeatureReducer { |
There was a problem hiding this comment.
Moved into InteractionReview/Sections/Accounts/Accounts.swift
| @@ -1,22 +0,0 @@ | |||
| struct UnknownDappComponents: FeatureReducer, Sendable { | |||
There was a problem hiding this comment.
Reducer deleted since it is now a plain vanilla SwiftUI view, located in InteractionReview/Destinations/UnknownDappComponents/UnknownDappComponents.swift
| import SwiftUI | ||
|
|
||
| // MARK: - TransactionReviewProofs.View | ||
| extension TransactionReviewProofs { |
There was a problem hiding this comment.
Moved into InteractionReview/Sections/Proofs/Proofs.swift
| import SwiftUI | ||
|
|
||
| // MARK: - TransactionReviewRawTransaction.View | ||
| extension TransactionReviewRawTransaction { |
There was a problem hiding this comment.
Unused view, deleted
RadixWallet/Core/SharedModels/P2P/Dapp/Models/WalletInteraction+Extensions.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Features/PreAuthorizationReview/PreAuthorizationReview.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Features/PreAuthorizationReview/PreAuthorizationReview+View.swift
Show resolved
Hide resolved
This reverts commit 0be24a3.
| /// - `56:02 minutes` / `1:23 minute` | ||
| /// - `34 seconds` / `1 second` | ||
| func formatTime(seconds: Int) -> String { | ||
| typealias S = L10n.PreAuthorizationReview.TimeFormat |
There was a problem hiding this comment.
I would probably not have this formatting logic inside a view, can we move it to Sargon and expose a function?
There was a problem hiding this comment.
or just move it from view layer and add some unit tests?
There was a problem hiding this comment.
I didn't add it in Sargon because it uses localisation strings. However, it does make sense to include unit tests for it, will add.
| extension InteractionReview { | ||
| enum DisplayMode: Sendable, Hashable { | ||
| case detailed | ||
| case raw(String) |
There was a problem hiding this comment.
can we use something more structured than String? Don't we have some UnvalidatedManifest type or something? Maybe we can create a new type or some typealias at least.
I would also put a one line doc on both cases and probably give raw a label: case raw(manifest: ...).
There was a problem hiding this comment.
We cannot use UnvalidatedManifest since in TransactionReview we are dealing with a TransactionManifest (to which we append the fee & guarantees configurations done at this view), while at PreAuthroizationReview we deal with a SubintentManifest.
I will add the raw label, but I think adding a type around the String is an overkill, since this is enum is just internally to display the RawManifestView.
| // MARK: - InteractionReview.Accounts | ||
| extension InteractionReview { | ||
| @Reducer | ||
| struct Accounts: Sendable, FeatureReducer { |
There was a problem hiding this comment.
I'd prefer if we could avoid calling reducers the same thing as models, makes searches (or possible find replace all) tricker, can it be called AccountsSection or similar? I dont feel too strongly about this though, so feel free to ignore
There was a problem hiding this comment.
I'd prefer sticking as it is. The find & replace is already pretty bad on Xcode, meaning that even if we name it AccountsSection, it will still break if we have other AccountsSections anywhere else. When I just renamed the send case to submitTransaction, Xcode wanted to update tests that were doing viewStore.send().
Then, keeping it short-named and enclosed under InteractionReview namespace makes it easier to follow IMO.
| @@ -131,6 +134,9 @@ extension DappToWalletInteraction { | |||
| // transactions | |||
| case .send: | |||
| 0 | |||
| // preAuthorization | |||
There was a problem hiding this comment.
I would probably rename send to submitTransaction or similar. We now have subintent and then having just send looks weird. send was short for sendTransasction - and sendTransaction I've always disliked, I think it should be called submitTransaction and then we can call subintent signSubintent perhaps - because we actually do NOT submit the signed subintent, the Dapp does.
There was a problem hiding this comment.
100% agree (always disliked send as well). Updating
JIRA ticket: ABW-3821
JIRA ticket: ABW-3823
Description
Adds
PreAuthorizationReview, as well as logic to present it from the given dApp interaction.How to review
The first thing I would do is go over
TransactionReview&TransactionReview+View. No additional behavior has been added to this flow, but you will see a lot of code has been migrated into the newInteractionReview.Sections. This module contains the shared components amongTransactionReviewandPreAuthorizationReview.Once the "common code" refactoring has been reviewed, I'd review the new code over
PreAuthorizationReview&PreAuthorizationReview+View.