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.

Sign up for GitHub

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

Safer check of .appStore vs .testFlight build, and enables TESTFLIGHT deployment #76

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Safer check of .appStore vs .testFlight build, and enables TESTFLIGHT deployment #76

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a86a52a
According to comments to Mike's PR
johnqh Jan 30, 2024
b6880f3
Change back to use "sandboxReceipt". Even though it is undocumented, …
johnqh Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions Utilities/Utilities/_Utils/Installation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,29 @@ public class Installation {
case appStore
case jailBroken // potentially side-loaded
}

// This is private because the use of 'appConfiguration' is preferred.
private static let isTestFlight = Bundle.main.appStoreReceiptURL?.lastPathComponent == "sandboxReceipt"


private static let isAppStore = {
if let receipt: URL = Bundle.main.appStoreReceiptURL {
var error: NSError?
if (receipt as NSURL).checkResourceIsReachableAndReturnError(&error), error == nil {
return true
}
}
return false
}()
Copy link
Contributor

@mike-dydx mike-dydx Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think my PR has the same logical outcomes, but without using the checkResourceIsReachableAndReturnError API (which is a synchronous round trip)

This method synchronously checks if the file at the provided URL is reachable. Checking reachability is appropriate when making decisions that do not require other immediate operations on the resource, such as periodic maintenance of user interface state that depends on the existence of a specific document. For example, you might remove an item from a download list if the user deletes the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only different is with app reviewer.

We don't know what receipt file URL they have. If it is "sandboxReceipt", with previous logic, they will end up in.appStore. But if we check if it is reachable (essentially check if the receipt file is present), then reviewer will end up in .testFlight.

Copy link
Contributor

@mike-dydx mike-dydx Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my PR
if Bundle.main.appStoreReceiptURL?.lastPathComponent == "sandboxReceipt" is true, then outcome is .testfFlight✅

if Bundle.main.appStoreReceiptURL?.lastPathComponent != "sandboxReceipt" is true, then outcome is .appStore unless jailbroken or isDebug is true✅

if Bundle.main.appStoreReceiptURL?.lastPathComponent == nil is true, then outcome is .appStore unless jailbroken or isDebug is true. To be safe in the case that app review does not have an appStoreReceiptURL, this outcome should be .testFlight so this is incorrect ❌

so my PR has a potential issue depending on app review binary

for your PR
if Bundle.main.appStoreReceiptURL?.lastPathComponent == "sandboxReceipt" is true, then outcome is .appStore if checkResourceIsReachableAndReturnError is true (i think this might be incorrectly true? since the appStoreReceiptURL might still be reachable with checkResourceIsReachableAndReturnError) ❌

if Bundle.main.appStoreReceiptURL?.lastPathComponent != "sandboxReceipt" is true, then outcome is .appStore if checkResourceIsReachableAndReturnError is true ✅

if Bundle.main.appStoreReceiptURL?.lastPathComponent == nil is true, then outcome is .testFlight if checkResourceIsReachableAndReturnError is true ✅

so your PR has a potential issue depending on app review binary as well. This is only if the sandbox receipt url is reachable

I think we might need to combine both of ours to be 100% safe?

    public static var source: Source {
        if isJailBroken {
            return .jailBroken
        } else if isDebug {
            return .debug
        } else if isTestFlight {
            //reached if `Bundle.main.appStoreReceiptURL?.lastPathComponent == "sandboxReceipt"`
            return .testFlight
        } else if isAppStore {
            //reached if `Bundle.main.appStoreReceiptURL?.lastPathComponent != "sandboxReceipt"`
            return .appStore
        } else {
            //reached if `Bundle.main.appStoreReceiptURL?.lastPathComponent == nil`
            return .testFlight 
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additionally, I cannot think of a scenario where Bundle.main.appStoreReceiptURL != nil && checkResourceIsReachableAndReturnError == false

so maybe we can remove the synchronous checkResourceIsReachableAndReturnError to reduce app start-up time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is checkResourceIsReachableAndReturnError, which is true if the receipt file exists. This is battle tested, in our v3 (and many apps before I used it in dYdX)
"sandboxReceipt" is undocumented and can change anytime.

Copy link
Contributor Author

@johnqh johnqh Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-dydx Like I said in Huddle, I think using "sandboxReceipt" should work. Worst case is that if Apple changes the receipt name (or Apple review has a different receipt name), the TestFlight/Reviewer app behaves like App Store download. If that happens, we can deal with it later. I reverted my changes back to use "sandboxReceipt" check


// This can be used to add debug statements.
static var isDebug: Bool {
#if DEBUG
return true
return true
#else
return false
return false
#endif
}

private static var isJailBroken: Bool = {
#if targetEnvironment(simulator)
return false
Expand All @@ -54,20 +64,19 @@ public class Installation {
}
#endif
}()

public static var source: Source {
return .appStore
if isJailBroken {
return .jailBroken
} else if isDebug {
return .debug
} else if isTestFlight {
return .testFlight
} else {
} else if isAppStore {
return .appStore
} else {
return .testFlight
}
}

public static var isSimulator: Bool = {
#if targetEnvironment(simulator)
return true
Expand Down
4 changes: 2 additions & 2 deletions Utilities/Utilities/_Utils/_FeatureFlags/CompositeFeatureFlagsProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ public class CompositeFeatureFlagsProvider: NSObject & FeatureFlagsProtocol {

public func flag(feature: String?) -> Any? {
switch Installation.source {
case .appStore, .jailBroken, .testFlight:
case .appStore, .jailBroken:
return remote?.flag(feature: feature)
case .debug:
case .debug, .testFlight:
if let localFlag = local?.flag(feature: feature) {
return localFlag
} else {
Expand Down
4 changes: 3 additions & 1 deletion dydx/dydxStateManager/dydxStateManager/AbacusStateManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,10 @@ public final class AbacusStateManager: NSObject {
switch Installation.source {
case .appStore:
deployment = "MAINNET"
case .testFlight, .debug, .jailBroken:
case .debug, .jailBroken:
deployment = "TESTNET"
case .testFlight:
deployment = "TESTFLIGHT"
Comment on lines +158 to +159
Copy link
Contributor

@mike-dydx mike-dydx Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the value/benefit of TESTFLIGHT ?

I am assuming the TESTFLIGHT deployment contains mainnet and testnet environments

If app store review build results in testFlight (we are not certain), then they will see those as the default selectable environments. App review might say "you cannot expose beta/test environments to users" and reject. I think apple might like to see that test environment access is behind special submission instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TESTFLIGHT deployment would have both mainnet and testnet, and default to mainnet, so unless reviewer enables debug and then switch to testnet environment, he is on mainnet.

It should be something like:

"TESTFLIGHT": {
         "environments": [
            "dydxprotocol-mainnet"
            "dydxprotocol-testnet"
         ],
         "default": "dydxprotocol-mainnet"
      },

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I am forgetting that the menu is hidden unless the Debug flag is enabled via QR code. I will get this updated in v4-web as well.

Copy link
Contributor Author

@johnqh johnqh Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am doing a PR on v4-web for this

}
appConfigs = AppConfigs.companion.forApp
}
Expand Down
Loading