-
Notifications
You must be signed in to change notification settings - Fork 9
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-3230] Use Wallet Interaction models from Sargon #1171
[ABW-3230] Use Wallet Interaction models from Sargon #1171
Conversation
public static func walletInteractionID(for interaction: DappInteractionClient.WalletInteraction) -> Self { | ||
"\(interaction.rawValue)_\(UUID().uuidString)" | ||
} | ||
|
||
public var isWalletAccountDepositSettingsInteraction: Bool { | ||
rawValue.hasPrefix(DappInteractionClient.WalletInteraction.accountDepositSettings.rawValue) | ||
hasPrefix(DappInteractionClient.WalletInteraction.accountDepositSettings.rawValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are now using WalletInteractionId
, which is a typealias of UUID
. Then, how can it contain a prefix that we manually set on line 29?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WalletInteractionId
is a typealias of String
for now. Wrote an explanation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I was navigating to an oudated typealias.
Thanks
RadixWallet/Clients/DappInteractionClient/DappInteractionClient+Live.swift
Outdated
Show resolved
Hide resolved
@@ -36,14 +36,14 @@ extension ROLAClient { | |||
return Self( | |||
performDappDefinitionVerification: { metadata async throws in | |||
_ = try await onLedgerEntitiesClient.getDappMetadata( | |||
metadata.dAppDefinitionAddress, | |||
validatingWebsite: metadata.origin.url | |||
metadata.dappDefinitionAddress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm we are using dAppXYZ
all over the app, was this approved on Sargon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name in Rust is dapp_definition_address
, so it's exported as dappDefinitionAddress
. Do you know any trick to have it like dAppDefinitionAddress
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand dApp is the correct style, on the other hand we did decide to generally write it Dapp, while would lead to dapp...
for instances/enum cases, and Dapp...
for types. But as you point out, we only seem to follow this convention for types.
let requested: P2P.Dapp.Request.PersonaDataRequestItem | ||
let responseValidation: P2P.Dapp.Request.RequestValidation | ||
let requested: DappToWalletInteractionPersonaDataRequestItem | ||
let responseValidation: DappToWalletInteraction.RequestValidation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in Sargon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…t+Live.swift Co-authored-by: matiasbzurovski <164921079+matiasbzurovski@users.noreply.github.com>
@@ -15,14 +15,24 @@ extension P2P { | |||
} | |||
|
|||
public enum Request: Sendable, Hashable, Equatable, Decodable { | |||
case dapp(P2P.Dapp.RequestUnvalidated) | |||
case dapp(DappToWalletInteractionUnvalidated) | |||
} | |||
} | |||
} | |||
|
|||
extension P2P.RTCMessageFromPeer.Request { | |||
public init(from decoder: Decoder) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this to be generally Decodable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed Decodable
|
||
private extension [PersonaData.Entry.Kind: [PersonaData.Entry]] { | ||
func extract<F>(_ kind: PersonaData.Entry.Kind, as: F.Type = F.self) throws -> F? where F: PersonaDataEntryProtocol { | ||
try self[kind]?.first.map { try $0.extract(as: F.self) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we only need the first one? And if we do, perhaps the name should still reflect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality was not introduced in this PR, it was simply moved from another location. I suppose it is a matter of preference, and we can change it when we transition this to Sargon.
@@ -640,7 +640,7 @@ extension DappInteractionFlow { | |||
} | |||
|
|||
func finishInteractionFlow(_ state: State) -> Effect<Action> { | |||
guard let response = P2P.Dapp.Response.WalletInteractionSuccessResponse( | |||
guard let response = WalletToDappInteractionSuccessResponse( | |||
for: state.remoteInteraction, | |||
with: state.responseItems.values.compactMap(/State.AnyInteractionResponseItem.remote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done without using the reflection based CasePath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why? We do not intend to compare associated values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we want to avoid reflection if we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -15,14 +15,24 @@ extension P2P { | |||
} | |||
|
|||
public enum Request: Sendable, Hashable, Equatable, Decodable { | |||
case dapp(P2P.Dapp.RequestUnvalidated) | |||
case dapp(DappToWalletInteractionUnvalidated) | |||
} | |||
} | |||
} | |||
|
|||
extension P2P.RTCMessageFromPeer.Request { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small improvement maybe - we could remove the Codable conformance from Request, and then decode the DappToWalletInteractionUnvalidated separately and embed it in the request afterwards.
Jira ticket: ABW-3230
Description
This PR replaces the Wallet Interaction models with those from Sargon without altering the existing functionality.