braintree / braintree_ios

Braintree SDK for iOS
https://developer.paypal.com/braintree/docs/start/hello-client/ios/v5
MIT License
560 stars 296 forks source link

Unify Authorization Types; Use single BTHTTP instance for GW calls #1314

Closed scannillo closed 4 months ago

scannillo commented 4 months ago

Summary

This PR is take 2 of https://github.com/braintree/braintree_ios/pull/1294 (see that PR for full description of issue). The goal is to re-use a single TCP connection b/w the 1st v1/configuration call and subsequent GW API calls. This could shave ~50-100ms of our E2E latency.

https://github.com/braintree/braintree_ios/pull/1294 revealed several tangles in our SDK's networking logic. BTAPIClient was managing 2 instances of BTHTTP, 1 for fetching the config and another for everything else. On top of that, BTHTTP had ternary logic that was hard to reason about and was specific to the v1/configuration call.

This PR has a lot (sorry), but cleaning this up at the Authorization & BTHTTP init level felt like the cleanest way.

Changes

Next

These changes reveal a lot of room for architecture improvements. Let's add the following tasks to our JIRA:

  1. Update .post() methods to require non-optional BTConfiguration parameters
  2. Break apart BTHTTP
    • Separate out raw HTTP networking from BT specific business logic
    • See BT Android (or PPCP iOS/Android) for good example
  3. Move to async throws style of BTAPIClient.fetchConfig() versus optional value in completion

Testing

Checklist

Authors

@scannillo

scannillo commented 4 months ago

Known issue with Unit Tests not completing addressed in https://github.com/braintree/braintree_ios/pull/1324

scannillo commented 4 months ago

I just discovered from manual testing that this PR was not properly sending our core-latency analytic events (the line http?.networkTimingDelegate = self was missing from BTAPIClient). However, our tests did not catch this error and were passing.

We should have testing in place to enforce the new latency analytics functionality, either unit or integration test. (I fixed it for this PR in 2b2bfcee7d9f65fe4dda44b9b631f0f03be42d99)