-
Notifications
You must be signed in to change notification settings - Fork 375
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
Implement an FFI to fetch API IP addresses using mullvad-api #7644
base: main
Are you sure you want to change the base?
Conversation
The completion cookie should now be stuffed in a `SwfitCompletionHandler`, which will ensure that it will be only ever used once.
Now `mullvad_api_get_addresses` returns a cancel handle. It should be wrapped in a class on the swift side so that it is deallocated appropriately. But calling it should allow for cancelling any in-flight requests. It doesn't wait for the task to actually stop - but it we can make it do it.
351f538
to
a12bc5b
Compare
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.
Reviewed 24 of 32 files at r1, 6 of 7 files at r2.
Reviewable status: 29 of 32 files reviewed, 6 unresolved discussions
ios/build-rust-library.sh
line 57 at r2 (raw file):
if [ $IS_SIMULATOR -eq 0 ]; then # Hardware iOS targets rustup target add aarch64-apple-ios
nit
As discussed with the team, this was only a temporary fix for devs, we should remove there 2 new lines.
ios/MullvadRustRuntime/MullvadApiCompletion.swift
line 16 at r2 (raw file):
let completionBridge = Unmanaged<MullvadApiCompletion> .fromOpaque(completionCookie) .takeUnretainedValue()
this should be takeRetainedValue()
otherwise we are leaking memory with every request.
ios/MullvadVPN/AddressCacheTracker/AddressCacheTracker.swift
line 122 at r2 (raw file):
return Date() // return _nextScheduleDate()
Reinstate the commented line
ios/MullvadTypes/AsyncExample.swift
line 0 at r2 (raw file):
Delete this ?
ios/MullvadREST/ApiHandlers/RESTRustNetworkOperation.swift
line 149 at r2 (raw file):
extension MullvadApiResponse { public func restError() throws -> REST.Error? {
This should either throw
or return an Optional
value, not both.
ios/MullvadRustRuntime/MullvadApiContext.swift
line 18 at r2 (raw file):
if context._0 == nil { throw NSError(domain: "", code: 0)
nit
We should have a custom error code that can identify why the request failed maybe ?
This way, debugging potential errors will be less of a hassle
We should test out if using the mullvad-api crate is feasible. An FFI that allows for calling out to https://api.mullvad.net/app/documentation/#/paths/~1v1~1api-addrs/get
We should then see if this works when:
Code can be throwaway for now, but if we can make it production ready, we should.
The Swift interface should look similar to the one below:
Said interface would probably have to be implemented by passing a continuation to Rust for when the result is fetched.
This task should be owned by an iOS team member, but they should feel free to reach out to any of the Rust developers for help and to pair-program on this task.
As part of this, we should also implement unit tests at least for the Rust parts.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)