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

Implement an FFI to fetch API IP addresses using mullvad-api #7644

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Feb 11, 2025

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:

  • Connected over 4g
  • Connected over WiFi
  • Connected to a VPN when IAN is set up
  • Blocked state when IAN is set up
  • Proxies?

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:

protocol AddressFetching {
  func fetchApiAddresses() async throws -> [MullvadEndpoint]
}

Said interface would probably have to be implemented by passing a continuation to Rust for when the result is fetched.

/// Swift side for the continuation
struct FFIContinuation {
  let callback: UnsafeMutable<...>
}

@convention(c) func fetch_api_addresses_result(continuation: UnsafePointer<FFIContinuation>)

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

rablador and others added 17 commits February 11, 2025 11:46
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.
@rablador rablador added the iOS Issues related to iOS label Feb 11, 2025
@rablador rablador self-assigned this Feb 11, 2025
@rablador rablador force-pushed the urlsession branch 4 times, most recently from 351f538 to a12bc5b Compare February 14, 2025 12:46
Copy link
Contributor

@buggmagnet buggmagnet left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants