-
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
Radix Connect for Mobile #1157
Radix Connect for Mobile #1157
Conversation
@@ -109,15 +109,6 @@ | |||
"version" : "11.6.4" | |||
} | |||
}, | |||
{ |
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.
will be replaced with Sargon version once the related PR is merged 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.
Looks good!
Left some minor comments.
Will do more testing after Sargon PR merge.
RadixWallet/Clients/SecureStorageClient/SecureStorageClient+Interface.swift
Show resolved
Hide resolved
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.
Looking very clean!
...DappInteractionFeature/Children/DappOriginVerification/DappInteractionVerifyDappOrigin.swift
Outdated
Show resolved
Hide resolved
...nteractionFeature/Children/DappOriginVerification/DappInteractionVerifyDappOrigin+View.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Features/DappInteractionFeature/Interactor/DappInteractor.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/DeepLinkHandlerClient/DeepLinkHandlerClient+Live.swift
Outdated
Show resolved
Hide resolved
} | ||
|
||
extension DeepLinkHandlerClient { | ||
public typealias HandleDeepLink = () -> Void |
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.
@Sendable
on all function types?
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.
Not needed, as those functions are only called from the main thread.
// MARK: - DeepLinkHandlerClient | ||
/// A client that manages the handling of the received deepLink. | ||
/// It will delegate the deepLink to the respective subcomponent. | ||
public struct DeepLinkHandlerClient: DependencyKey { |
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 should client be Sendable
?
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.
Not needed, it is used only from the main thread really. I could add it for the sake of it, but it will not be useful.
var bufferedDeepLink: URL? | ||
} | ||
|
||
var state = State() |
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.
Im unsure if this passes complete Isolation check? Since state
is references referenced/mutated from multiple closures.
Might be solved with @TaskLocal
?
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.
but all of the closures are not executed in a concurrent context, there will not be two concurrent deepLinks to be handled at the same time.
extension DeepLinkHandlerClient: TestDependencyKey { | ||
public static let previewValue = Self.noop | ||
|
||
public static let testValue = 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.
I think this is not needed any more, if you use @DependencyClient
macro on DeepLinkHandlerClient
.
RadixWallet/Clients/SecureStorageClient/SecureStorageClient+Interface.swift
Show resolved
Hide resolved
...nteractionFeature/Children/DappOriginVerification/DappInteractionVerifyDappOrigin+View.swift
Outdated
Show resolved
Hide resolved
func loadSession(sessionId: SessionId) async throws -> Data? { | ||
try secureStorageClient.loadRadixConnectMobileSession(sessionId) | ||
} | ||
} |
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.
perhaps a small unit tests for this? Just calling save
and then load
and assert the correct session data is loaded?
/// A very thin wrapper around Sargon.RadixConnectMobile. | ||
/// It mainly initializes the `live` version Sargon.RadixConnectMobile and | ||
/// exposes the incomming messages stream. | ||
struct RadixConnectMobile { |
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.
I guess P2P.RTCIncomingMessage
does not exist in Sargon? preventing this struct to be implemented in Swift Sargon?
What if you make this struct generic?
// This will be `Sargon.RadixConnectMobile` (vs `SargonUniFFI.RadixConnectMobile`)
public struct RadixConnectMobile<Storage, Message> where Storage: SessionStorage {
private let radixConnectMobile: SargonUniFFI.RadixConnectMobile // N.B. `SargonUniFFI.`
... incomingMessagesSubject: AsyncReplaySubject<Message> ...
public init(storage: Storage) {
self.radixConnectMobile = SargonUniFFI.RadixConnectMobile.live(sessionStorage: storage)
}
}
extension RadixConnectMobile {
func incomingMessages() async -> AnyAsyncSequence<Message> {
...
}
}
Then it can be placed in Swift Sargon, with all the benefits that entails:
- Easy to write unit tests
- Getting code coverage measurements
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.
I don't quite follow your suggestion, why I would make this generic when it is specific to RadixConnectMobile.
P2P.RTCIncomingMessage
does not exist in Sargon, since it does not yet know about messages from WebRTC or from the Wallet itself; also it is highly specific to iOS.
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.
P2P.RTCIncomingMessage does not exist in Sargon,
that is my point, P2P.RTCIncomingMessage
does not exist in Sargon. But a generic type T
can be declared ("exist") in Sargon. Thus this struct RadixConnectMobile<T>
can exist in Sargon. Which works, since this type does not actually create any T
or read any fields of T
. And having Swift code In Sargon has the above mentioned two advantages:
- Easy to write unit tests
- Getting code coverage measurements
VS the terrible DX of writing unit tests in iOS app repo - without code coverage.
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.
I see, we can get to this at some later point when we can unify the channel of incoming message on both platforms.
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.
Sweet! LGTM
Radix Connect for Mobile implementation.
This PR is the Wallet integration of Sargon's RadixConnectMobile API - radixdlt/sargon#122. All of the RCfM logic is in Sargon, while Wallet is responsible for intercepting the DeepLink and present the request to the user.
What has been done
Demo
https://drive.google.com/file/d/1REicQe4gyIXMAKoifMS2pERMuYmrNsQr/view?usp=sharing&t=73.