Skip to content

StoreKit1Wrapper: added warning if receiving too many updated transactions#2117

Merged
NachoSoto merged 5 commits into
mainfrom
sk1-wrapper-transaction-log
Dec 20, 2022
Merged

StoreKit1Wrapper: added warning if receiving too many updated transactions#2117
NachoSoto merged 5 commits into
mainfrom
sk1-wrapper-transaction-log

Conversation

@NachoSoto

Copy link
Copy Markdown
Contributor

See #2107. We still don't know what caused that, but it leads to degraded performance (mildly reduced by #2115). To advice of that, in case users are wondering why the app might potentially take excessive CPU usage, I added this warning.

@NachoSoto NachoSoto added the docs label Dec 5, 2022
@NachoSoto NachoSoto requested a review from a team December 5, 2022 18:37
@NachoSoto NachoSoto force-pushed the sk1-wrapper-transaction-log branch from 3ce5325 to 83dffcf Compare December 5, 2022 19:58
@NachoSoto NachoSoto force-pushed the storekit-1-wrapper-process-bg branch from 8d1cce5 to da0b5b8 Compare December 5, 2022 20:02
@NachoSoto NachoSoto force-pushed the sk1-wrapper-transaction-log branch from 83dffcf to 0453633 Compare December 5, 2022 21:52

case let .sk1_payment_queue_too_many_transactions(count):
return "SKPaymentQueue sent \(count) updated transactions. " +
"This high number is unexpected and is likely due to bad state on your device."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔
I mean... this would probably be the sandbox account, right? like, if you use it a lot for tests, it's gonna just grow and grow.
Apple updated so they only keep transactions for 90 days now, but if you open up an old sandbox account on a new device, I wouldn't be surprised if you got a ton of transactions.

I would mostly recommend using a different sandbox account here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for example if I test with one account a ton, then go to a new device and log in with that account, the new device will get all of those transactions coming in at once

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would mostly recommend using a different sandbox account here

That makes sense 👍🏻 I'll update the message to point to that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated it to

This high number is unexpected and is likely due to using an old sandbox account on a new device. If this is impacting performance, using a new sandbox account is recommended.

What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that looks great! One final nitpick: can we ensure that that message is used only in sandbox, and have a different one for production?
The production one could say something about there just being a very high number of transactions.
I wouldn't necessarily call it unexpected, I can imagine that for a few apps they might actually have people with a ton of transactions (2 years of a weekly subscription is already over 100 renewals). I'd maybe just say something about there being a lot of transactions, so performance might be impacted

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, good idea. I thought about it but I thought YAGNI because I had to inject SystemInfo to StoreKit1Wrapper. But I agree it's useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aboedo updated.

Comment thread Sources/Purchasing/StoreKit1/StoreKit1Wrapper.swift Outdated
Base automatically changed from storekit-1-wrapper-process-bg to main December 7, 2022 22:37
@NachoSoto NachoSoto requested review from a team and aboedo December 9, 2022 17:38
@NachoSoto

Copy link
Copy Markdown
Contributor Author

@aboedo updated.

@NachoSoto NachoSoto force-pushed the sk1-wrapper-transaction-log branch 2 times, most recently from e5f6e72 to 11a6a29 Compare December 13, 2022 18:33
…actions

See #2107. We still don't know what caused that, but it leads to degraded performance (mildly reduced by #2115).
To advice of that, in case users are wondering why the app might potentially take excessive CPU usage, I added this warning.
@NachoSoto NachoSoto force-pushed the sk1-wrapper-transaction-log branch from 11a6a29 to debf22c Compare December 13, 2022 19:35
Comment on lines +102 to +108
case let .sk1_payment_queue_too_many_transactions(count, isSandbox):
let messageSuffix = isSandbox
? "This high number is unexpected and is likely due to using an old sandbox account on a new device. " +
"If this is impacting performance, using a new sandbox account is recommended."
: "This is a very high number and might impact performance."

return "SKPaymentQueue sent \(count) updated transactions. " + messageSuffix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:chefskiss:

@NachoSoto NachoSoto merged commit 30c7e9e into main Dec 20, 2022
@NachoSoto NachoSoto deleted the sk1-wrapper-transaction-log branch December 20, 2022 22:08
NachoSoto pushed a commit that referenced this pull request Dec 21, 2022
**This is an automatic release.**

### Bugfixes
* `ErrorUtils.purchasesError(withUntypedError:)`: handle `PublicError`s
(#2165) via NachoSoto (@NachoSoto)
* Fixed race condition finishing `SK1` transactions (#2148) via
NachoSoto (@NachoSoto)
* `IntroEligibilityStatus`: added `CustomStringConvertible`
implementation (#2182) via NachoSoto (@NachoSoto)
* `BundleSandboxEnvironmentDetector`: fixed logic for `macOS` (#2176)
via NachoSoto (@NachoSoto)
* Fixed `AttributionFetcher.adServicesToken` hanging when called in
simulator (#2157) via NachoSoto (@NachoSoto)
### Other Changes
* `Purchase Tester`: added ability to purchase products directly with
`StoreKit` (#2172) via NachoSoto (@NachoSoto)
* `DNSChecker`: simplified `NetworkError` initialization (#2166) via
NachoSoto (@NachoSoto)
* `Purchases` initialization: refactor to avoid multiple concurrent
instances in memory (#2180) via NachoSoto (@NachoSoto)
* `Purchase Tester`: added button to clear messages on logger view
(#2179) via NachoSoto (@NachoSoto)
* `NetworkOperation`: added assertion to ensure that subclasses call
completion (#2138) via NachoSoto (@NachoSoto)
* `CacheableNetworkOperation`: avoid unnecessarily creating operations
for cache hits (#2135) via NachoSoto (@NachoSoto)
* `PurchaseTester`: fixed `macOS` support (#2175) via NachoSoto
(@NachoSoto)
* `IntroEligibilityCalculator`: added log including `AppleReceipt`
(#2181) via NachoSoto (@NachoSoto)
* `Purchase Tester`: fixed scene manifest (#2170) via NachoSoto
(@NachoSoto)
* `HTTPClientTests`: refactored to use `waitUntil` (#2168) via NachoSoto
(@NachoSoto)
* `Integration Tests`: split up tests in smaller files (#2158) via
NachoSoto (@NachoSoto)
* `StoreKitRequestFetcher`: removed unnecessary dispatch (#2152) via
NachoSoto (@NachoSoto)
* `Purchase Tester`: added companion `watchOS` app (#2140) via NachoSoto
(@NachoSoto)
* `StoreKit1Wrapper`: added warning if receiving too many updated
transactions (#2117) via NachoSoto (@NachoSoto)
* `StoreKitTestHelpers`: cleaned up unnecessary log (#2177) via
NachoSoto (@NachoSoto)
* `TrialOrIntroPriceEligibilityCheckerSK1Tests`: use `waitUntilValue`
(#2173) via NachoSoto (@NachoSoto)
* `DNSChecker`: added log with resolved host (#2167) via NachoSoto
(@NachoSoto)
* `MagicWeatherSwiftUI`: updated `README` to point to workspace (#2142)
via NachoSoto (@NachoSoto)
* `Purchase Tester`: fixed `.storekit` config file reference (#2171) via
NachoSoto (@NachoSoto)
* `Purchase Tester`: fixed error alerts (#2169) via NachoSoto
(@NachoSoto)
* `CI`: don't make releases until `release-checks` pass (#2162) via
NachoSoto (@NachoSoto)
* `Fastfile`: changed `match` to `readonly` (#2161) via NachoSoto
(@NachoSoto)
@swrobel

swrobel commented Dec 28, 2022

Copy link
Copy Markdown

Out of curiosity, why wasn't this implemented for StoreKit 2?

@NachoSoto

Copy link
Copy Markdown
Contributor Author

StoreKit 2 uses StoreKit.Transaction.updates, which is an AsyncStream. There's no easy way to detect the number of elements that will be sent through that stream.

@vegaro vegaro added pr:other and removed pr:docs labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants