-
Notifications
You must be signed in to change notification settings - Fork 316
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
Local receipt parsing + IntroEligibility #302
Conversation
@@ -55,16 +55,18 @@ | |||
debugServiceExtension = "internal" | |||
allowLocationSimulation = "YES" | |||
notificationPayloadFile = "WatchExample Extension/PushNotificationPayload.apns"> | |||
<BuildableProductRunnable |
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.
this just makes the schemes runnable on network devices.
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.
feel free to skip.
@@ -11,17 +11,17 @@ import StoreKit | |||
|
|||
public class IntroEligibilityCalculator: NSObject { | |||
private let productsManager: ProductsManager | |||
private let localReceiptParser: LocalReceiptParser |
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.
It's not technically local
if it receives NSData
, the data could come from anywhere.
Purchases/SwiftSources/LocalReceiptParsing/BasicTypes/ASN1ObjectIdentifier.swift
Outdated
Show resolved
Hide resolved
return Set(productIdentifiers) | ||
} | ||
|
||
var asDict: [String: Any] { |
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.
this bit is just to make debugging easier.
cancellationDate = 1712, | ||
isInTrialPeriod = 1713, | ||
isInIntroOfferPeriod = 1719, | ||
promotionalOfferIdentifier = 1721 |
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.
promotionalOfferIdentifier
, isInTrialPeriod
and productType
seem to work and are useful, but they aren't documented.
I made them optional since they're not documented, just in case they change in the future.
private extension UInt8 { | ||
func maskForRange(_ range: UInt8) -> UInt8 { | ||
guard 0 <= range && range <= 8 else { fatalError("range must be between 1 and 8") } | ||
switch range { |
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.
https://twitter.com/ctrlshifti/status/1288745146759000064?s=21 😂
but I couldn't think of an alternative that was easier to understand at a glance, and it's just a few cases
} | ||
|
||
private extension ReceiptParser { | ||
func findASN1Container(withObjectId objectId: ASN1ObjectIdentifier, |
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.
this would be reused if we wanted other parts of the receipt, like the signature
@@ -0,0 +1,369 @@ | |||
{ |
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.
I figured it might be useful to have the file around for testing purposes, but it differs enough from the actual receipt that I'll probably replace it with a JSON representation of the actual receipt.
let receiptData = sampleReceiptData() | ||
|
||
expect { | ||
let receipt = try ReceiptParser().parse(from: receiptData) | ||
print(receipt.description) | ||
return receipt | ||
} .notTo(throwError()) |
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.
this is clearly not testing anything yet, next up is working on testing everything thoroughly
d560942
to
e1e68d5
Compare
45e0446
to
af73e08
Compare
let intData = [UInt8](data) | ||
|
||
let asn1Container = try containerBuilder.build(fromPayload: ArraySlice(intData)) | ||
guard let receiptASN1Container = try findASN1Container(withObjectId: .data, inContainer: asn1Container) else { |
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.
This .data
confused me a little bit with the data
from parameters
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.
👍 I'll make it more explicit and also rename data
|
||
struct ASN1Container { | ||
let containerClass: ASN1Class | ||
let containerType: ASN1Type |
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.
I thought the Type was composed by identifier + class + encoding type. But here, containerType
is actually the identifier? I was expecting this ASN1Type to be another struct that had an identifier, a class and an encoding type. That might be too much, but I think containerType
should be at least indicate that it is the identifier of the type, and not the whole type.
if container.encodingType == .constructed { | ||
var currentPayload = container.internalPayload | ||
for internalContainer in container.internalContainers { | ||
currentPayload = currentPayload.dropFirst(internalContainer.totalBytes) |
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.
I am a little bit confused about this dropFirst
. Why is it needed?
Also, you're only using currentPayload
in line 44. The next iteration, it's applying the dropFirst
on the currentPayload
created in the previous iteration? Why?
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.
per our convo the other day, I'm going to refactor this to remove the currentPayload
stuff since it's no longer needed - it was needed before I extracted this method.
let lengthBit = firstByte.bitAtIndex(0) | ||
let isShortLength = lengthBit == 0 | ||
|
||
let firstByteValue = UInt(firstByte.valueInRange(from: 1, to: 7)) |
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.
Since this is only used in the case that's not short length, I would move this inside the else branch. I would probably do:
let totalLengthOctets = Int(firstByte.valueInRange(from: 1, to: 7))
Since that's the same right?
let containerType = try extractType(byte: firstByte) | ||
let length = try extractLength(data: payload.dropFirst()) | ||
let identifierTotalBytes = 1 | ||
let metadataBytes = identifierTotalBytes + length.totalBytes |
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.
Why this +1?
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.
I would probably call this numberOfMetadataBytes
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.
I am not convinced about the naming for totalBytes
and identifierTotalBytes
, I had to dig in the code to understand that it is the number of bytes used to represent the length or the identifier
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.
I'm not convinced about the naming for any of these either XD I just kept finding more similar stuff to add on and naming got difficult. I'll try to improve it.
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.
Updated naming here, lmk what you think
struct ASN1ContainerBuilder { | ||
|
||
func build(fromPayload payload: ArraySlice<UInt8>) throws -> ASN1Container { | ||
guard payload.count >= 2, |
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.
Why this >= 2
?
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.
Oh, is it because it needs to be > 1
, being the first byte the type
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.
It might actually need to be >= 3, but I haven't been able to find docs about it.
there has to be at least one byte for identifier and one for length, I'm not sure whether the value needs to have at least one byte.
|
||
struct ASN1Length { | ||
let value: UInt | ||
let totalBytes: Int |
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.
Why is value UInt
and totalBytes Int
?
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.
no good reason tbh. At first I was using UInt a lot more since the inability for the values to be negative made sense, but it's a total hassle because most useful methods require Int
(like the array slicing methods dropFirst
, prefix
, etc).
I'll refactor value
to Int
since it removes the need for quite a bit of casting.
let identifierTotalBytes = 1 | ||
let metadataBytes = identifierTotalBytes + length.totalBytes | ||
|
||
guard payload.count - metadataBytes >= Int(length.value) else { throw ReceiptReadingError.asn1ParsingError } |
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.
What is this check trying to prevent? Shouldn't it be something like if metadataBytes != Int(length.value)
instead?
var promotionalOfferIdentifier: String? | ||
|
||
for internalContainer in container.internalContainers { | ||
guard internalContainer.internalContainers.count == 3 else { fatalError() } |
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.
Maybe adding a message in that fatalError would help
guard let internalContainer = container.internalContainers.first else { fatalError() } | ||
let receiptContainer = try containerBuilder.build(fromPayload: internalContainer.internalPayload) | ||
for receiptAttribute in receiptContainer.internalContainers { | ||
let typeContainer = receiptAttribute.internalContainers[0] |
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.
In the InAppPurchaseBuilder, there's a check for the number of internalContainers which I think would be helpful to add here
let nonOptionalOriginalPurchaseDate = originalPurchaseDate, | ||
let nonOptionalIsInIntroOfferPeriod = isInIntroOfferPeriod, | ||
let nonOptionalWebOrderLineItemId = webOrderLineItemId else { | ||
throw ReceiptReadingError.inAppParsingError |
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.
I think this is very smart
func build(fromPayload payload: ArraySlice<UInt8>) -> ASN1ObjectIdentifier? { | ||
guard let firstByte = payload.first else { fatalError("invalid object identifier") } | ||
|
||
var oidBytes: [UInt] = [] |
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.
can we rename this oid
?
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.
absolutely. I don't know what got into me.
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.
also, bytes is wrong - it makes you expect [UInt8]
instead of [UInt]
.
Purchases/SwiftSources/LocalReceiptParsing/TPInAppReceipt/LICENSE
Outdated
Show resolved
Hide resolved
aae2652
to
1576d18
Compare
b6913e8
to
0917ac7
Compare
let containerIdentifier = try extractIdentifier(byte: firstByte) | ||
let length = try extractLength(data: payload.dropFirst()) | ||
let bytesUsedForIdentifier = 1 | ||
let bytesUsedForMetadata = bytesUsedForIdentifier + length.bytesUsedForLength |
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.
I think this naming looks better yes
8402f0d
to
610f4bf
Compare
e6502e6
to
f880bc8
Compare
let receipt = try receiptParser.parse(from: receiptData) | ||
let purchasedProductIdsWithIntroOffers = receipt.purchasedIntroOfferOrFreeTrialProductIdentifiers() | ||
|
||
let allProductIdentifiers = candidateProductIdentifiers.union(purchasedProductIdsWithIntroOffers) |
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.
join all product ids so that we only issue one products request, we separate them again in the completion block
CALL_IF_SET_ON_MAIN_THREAD(receiveEligibility, result); | ||
completionBlock:(RCReceiveIntroEligibilityBlock)receiveEligibility | ||
{ | ||
[self receiptData:^(NSData *data) { |
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.
the logic here should eventually be moved out of purchases and into something like a higher-level intro eligibility manager, which decides whether to use the local or backend version.
This PR is gigantic enough as it is, though.
@@ -16,46 +16,6 @@ import Purchases | |||
|
|||
class StoreKitRequestFetcher: XCTestCase { | |||
|
|||
class MockProductResponse: SKProductsResponse { |
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.
moved to separate file for reuse
@@ -0,0 +1,369 @@ | |||
{ |
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.
this isn't used, but it's a useful reference when writing tests for the other sample, since it has the /verifyReceipt
version of the same file
protocol BuildableReceiptAttributeType { | ||
var rawValue: Int { get } | ||
} | ||
extension InAppPurchaseAttributeType: BuildableReceiptAttributeType {} | ||
extension ReceiptAttributeType: BuildableReceiptAttributeType {} |
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.
this is a swift trick - it makes it easy for a method to accept values for either enum, like a union
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.
equivalent of python type hinting's Union
import Foundation | ||
@testable import Purchases | ||
|
||
class ASN1ObjectIdentifierEncoder { |
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.
this ended up being tougher than the decoder.
it's also less efficient since I didn't go crazy optimizing given that it's only for tests.
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.
I did manually test that it outputs the correct result for all known object identifiers.
@vegaro this is finally ready for review! let me know if you'd like me to split it up into smaller pieces, though, since tests ended up being huge. |
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.
Haven't looked at the tests part yet
Purchases/Public/RCPurchases.m
Outdated
operationDispatcher:(RCOperationDispatcher *)operationDispatcher { | ||
|
||
operationDispatcher:(RCOperationDispatcher *)operationDispatcher | ||
introEligibilityCalculator:(RCIntroEligibilityCalculator *)introEligibilityCalculator { |
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.
introEligibilityCalculator:(RCIntroEligibilityCalculator *)introEligibilityCalculator { | |
introEligibilityCalculator:(RCIntroEligibilityCalculator *)introEligibilityCalculator { |
} | ||
do { | ||
let receipt = try receiptParser.parse(from: receiptData) | ||
let purchasedProductIdsWithIntroOffers = receipt.purchasedIntroOfferOrFreeTrialProductIdentifiers() |
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.
let purchasedProductIdsWithIntroOffers = receipt.purchasedIntroOfferOrFreeTrialProductIdentifiers() | |
let purchasedProductIdsWithIntroOffersOrFreeTrials = receipt.purchasedIntroOfferOrFreeTrialProductIdentifiers() |
let allProductIdentifiers = candidateProductIdentifiers.union(purchasedProductIdsWithIntroOffers) | ||
|
||
productsManager.products(withIdentifiers: allProductIdentifiers) { allProducts in | ||
let purchasedProductsWithIntroOffers = allProducts.filter { purchasedProductIdsWithIntroOffers.contains($0.productIdentifier) } |
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.
let purchasedProductsWithIntroOffers = allProducts.filter { purchasedProductIdsWithIntroOffers.contains($0.productIdentifier) } | |
let purchasedProductsWithIntroOffersOrFreeTrials = allProducts.filter { purchasedProductIdsWithIntroOffers.contains($0.productIdentifier) } |
} | ||
|
||
enum ASN1Identifier: UInt8, CaseIterable { | ||
case endOfContent = 0x00 |
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.
Does this have to be 0x00
or can it be just 0
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.
good catch
if isShortLength { | ||
return ASN1Length(value: firstByteValue, bytesUsedForLength: 1) | ||
} else { | ||
let totalLengthOctets = firstByteValue |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for (index, internalContainer) in container.internalContainers.enumerated() { | ||
if internalContainer.containerIdentifier == .objectIdentifier { | ||
let objectIdentifier = objectIdentifierParser.build(fromPayload: internalContainer.internalPayload) | ||
if objectIdentifier == objectId && index < container.internalContainers.count - 1 { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
it's just to make sure that there is a next container available after the object identifier.
let productsAlreadyCached = self.cachedProductsByIdentifier.filter { key, _ in identifiers.contains(key) } | ||
if productsAlreadyCached.count == identifiers.count { | ||
let productsAlreadyCachedSet = Set(productsAlreadyCached.values) | ||
NSLog("skipping products request because products were already cached. products: \(identifiers)") |
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.
We are using NSLog
in this file. Should we copy the logs utils we have to be able to use them in this swift files?
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.
once we get the new RCLog stuff Tina's working on merged, we can come back and refactor this to use it
|
||
if let existingHandlers = self.completionHandlers[identifiers] { | ||
NSLog("found an existing request for products: \(identifiers), appending to completion") | ||
self.completionHandlers[identifiers] = existingHandlers + [completion] |
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.
we should probably synchronize the access to completionHandlers
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.
no need - that's what the queue is doing.
things on this file can only be executed atomically in the queue. it's a way of handling this stuff in Swift that's pretty neat.
guard let requestProducts = self.productsByRequests[request] else { fatalError("couldn't find request") } | ||
guard let completionBlocks = self.completionHandlers[ | ||
requestProducts | ||
] else { fatalError("couldn't find completion") } |
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.
I think the formatting broke here
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.
good catch
resultDict[product.productIdentifier] = product | ||
} | ||
|
||
cachedProductsByIdentifier.merge(productsByIdentifier) { (_, new) in new } |
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.
cachedProductsByIdentifier
should probably be synchronized too right?
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.
we could force these operations into the queue, but there's no way to access them publicly anyway, only through the public methods, and those do force you into the queue, so it should be thread-safe already
45603b8
to
5de8034
Compare
I'll need to rebase, but I'm holding off until the other PRs are merged or this one gets approved to minimize conflicts |
completion: @escaping ([String: NSNumber], Error?) -> ()) { | ||
invokedCheckTrialOrIntroductoryPriceEligibility = true | ||
invokedCheckTrialOrIntroductoryPriceEligibilityCount += 1 | ||
invokedCheckTrialOrIntroductoryPriceEligibilityParameters = ( |
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.
I think the formatting broke here
invokedCheckTrialOrIntroductoryPriceEligibilityParameters = ( | |
invokedCheckTrialOrIntroductoryPriceEligibilityParameters = (receiptData, candidateProductIdentifiers) |
|
||
override func start() { | ||
startCalled = true | ||
DispatchQueue.main.async { |
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.
why is this async?
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.
so that we can test what happens when two requests are running simultaneously. also, so it's closer to what would happen in a prod environment.
we could even add an optional delay... I didn't find a need for it, but it might not be a bad addition
@@ -43,4 +43,6 @@ | |||
#include <Purchases/RCProductInfo.h> | |||
#include <Purchases/RCProductInfoExtractor.h> | |||
#include <Purchases/RCOperationDispatcher.h> | |||
#include <Purchases/RCIntroEligibilityCalculator.h> |
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.
You've probably explained me this 1000 times, but why do we need this here? Don't the tests use the swift headers?
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.
LOL I don't think I have for this particular one - this is kind of a workaround because the initWith...
method in RCPurchases
won't be recognized by the compiler at compile-time without the symbol being imported.
I don't like it one bit, though, and we (luckily) remove the need for it in #316. But until we rebase from that one, it's needed.
payloadArray.insert(constructedEncodingByte, at: 0) | ||
|
||
let containerLenghtByte: UInt8 = UInt8(3) | ||
payloadArray.insert(containerLenghtByte, at: 1) |
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.
Do you need to add the container length in this test? Also there is a typo in length
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.
well, there has to be a header and a length for it to be a valid asn1. Without it, validations would fail
|
||
expect { try self.containerBuilder.build(fromPayload: payload) }.to(throwError()) | ||
} | ||
} |
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.
this is very intense, I can't imagine what you went through writing this. good job
UInt8(UInt8(subContainer1InternalPayload.count))] | ||
+ subContainer1InternalPayload | ||
let subContainer2Payload: [UInt8] = [UInt8(0b1), | ||
UInt8(UInt8(subContainer2InternalPayload.count))] |
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.
Why is this UInt8(UInt8
?
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.
probably cause I had a tough day 😂 I'll clean it up
func testBuildGetsExpiresDate() { | ||
let expirationDate = Date.from(year: 2020, month: 7, day: 4, hour: 5, minute: 3, second: 2) | ||
let expirationDateContainer = containerFactory | ||
.receiptAttributeContainer(attributeType: ReceiptAttributeType.expirationDate, |
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.
.receiptAttributeContainer(attributeType: ReceiptAttributeType.expirationDate, | |
.receiptAttributeContainer(attributeType: ReceiptAttributeType.expirationDate, expirationDate) |
func testDateFromBytesReturnsNilIfItCantBeParsedIntoDate() { | ||
guard let stringAsBytes = "some string that isn't a date".data(using: .ascii) else { fatalError() } | ||
expect(ISO3601DateFormatter.shared.date(fromBytes: ArraySlice(stringAsBytes))).to(beNil()) | ||
} |
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.
Should we also test what happens if the ArraySlice is empty?
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.
yep! I'll add.
expect(UInt8(0b10000000).bitAtIndex(0)) == 1 | ||
expect(UInt8(0b10000000).bitAtIndex(1)) == 0 | ||
expect(UInt8(0b00100000).bitAtIndex(2)) == 1 | ||
expect(UInt8(0b11111111).bitAtIndex(3)) == 1 |
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.
Since all of them are 1, the test is not really proving that it's getting the right one right? If I understand correctly, this should be:
expect(UInt8(0b00010000).bitAtIndex(3)) == 1
I might be getting this all wrong lol. Also, it's missing the case where you check for the 8th bit, you're only checking for 7 of the bits.
I don't think it's a big deal though
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.
lol yep, fixing
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.
I am just a little bit confused by ASN1ObjectIdentifierEncoder but other than that EXTREMELY GOOD JOB!
LOL I'm confused by that one as well. I'm doing a clean-up pass on it and I'll ask for a re-review |
* Preparing for next version * Update RCSystemInfo.m * Update .jazzy.yaml * Update .jazzy.yaml * back to 3.6.0-SNAPSHOT added TPInAppReceipt pod, added logic to get intro eligibility from receipt finished main logic for intro eligibility small fixes to retention in closures replaced pod dependency with embedded classes added TPInAppReceipt license clean up on intro eligibility calculator updated the logic to get products info so that only one SKProductsRequest gets issued added using the cache in productsManager cleanup: used DI for the IntroEligibilityCalculator fixed TPInAppReceipt compatibility with watchOS added syncing with a queue + support for multiple concurrent requests to product manager remove api key specified os version availability for intro calculation in detail ensured that data can't be nil before sending it to parser also covered the case where receipt data is not null, but is empty added missing import remove pod from podspec added files for local testing WIP interpreting bytes from receipt added parsing of data length, added methods to make parsing individual bits and bit ranges easier added more generalized code to get asn1 containers cleanup: moved methods into UInt8 extensinos more cleanup: moved method into ASN1Container init made constructor recursive, fixed issues with indexes more iteration fixes small cleanup started unpacking InAppReceiptAttributes more decoding of attributes more decoding of attributes decoded missing attributes added algorithm for decoding object identifier started cleanup: moved stuff to a separate class cleanup, separated objectIdentifierParser WIP extracting stuff into classes added methods for easier print debugging more cleanup added more (undocummented) fields to in app purchase moved to using our own solution instead of TPInAppReceipt for intro eligibility removed TPInAppReceipt cleanup, renames, removed unused files separated inAppPurchase into its own class split extraction logic into several factories moved to using a single date formatter method naming clenaup separated ASN1ObjectIdentifier from factory small update to use DI for the date formatter more cleanup: unified duplicated logic to convert from bytes to different value types renamed file from previous commit moved more responsibility to the apple receipt factory, simplified the AppleReceipt class moved logic for querying the apple receipt into the apple receipt class moved more responsibility to the inAppPurchaseFactory and away from InAppPurchase removed redundant class LocalReceiptParser removed all force unwraps and replaced assumptions with more robust error handling and throwing custom exceptions split code into builders, basic types and utilities renamed factory -> builder renamed factory -> builder renamed Utilities -> Data Converters removed unused extractableValueType protocols added UInt8 extensions tests, simplified guard conditions added tests for ArraySlice<UInt8>+Extensions added tests for iOS3601 date formatter renamed ASN1Type -> ASN1Identifier. Added first tests for ASN1ContainerBuilder added more tests for asn1ContainerBuiler added tests for short length asn1 containers fixed rebase issues cleanup added tests for container length, added description to ASN1 errors added more details to error cases, added tests for container length removed unused property added more tests for length and internal payload made internal containers `let` since it's never modified outside of construction. added tests for asn1ContainerBuilder's building of internal containers - removed receiptAsData.txt, renamed sample receipt, added tests that compare directly against the sample receipt and serve as integration tests for local receipt parsing. small cleanup changed type of ASN1Length.value from UInt -> Int inApp -> inAppPurchase purchasedIntroOfferProductIdentifiers -> purchasedIntroOfferOrFreeTrialProductIdentifiers clean up naming oid -> objectIdentifier more naming cleanup WIP adding container factory to make tests easier added more factory methods added more logic to be able to compose receipts for testing purposes. added tests for minimal attributes in the receipt added more tests for minimal attributes added test for expires date cleanup in container factory added base code to be able to compose in app purchase containers for testing purposes added tests for minimal attributes in in-app purchases added tests for optional attributes adds tests to ensure that in-app purchase build fails if attributes are missing, fixed tests not running for in-app purchase builder. fixed bug in tests added ASN.1 object identifier encoding to be able test decoding added tests for all known object identifiers added checks for empty and invalid object identifiers cleanup added tests to check that in app purchases are correctly added to the receipt simplified ASN1ObjectIdentifierBuilder logic fixed rebase issues added some tests for ReceiptParser renamed the methods in container factory removing the redundant build prefix added remaining tests for ReceiptParser moved containerFactory into TestHelpers cleanup in IntroEligibilityCalculator, added class for tests replaced classes with mocks added more tests updated intro eligibility calculator swift name so name lookup works correctly in tests updated Swift name for other internal swift classes so we won't run into lookup issues with them added test to check that only one SKProducts call is issued added tests to check that eligibility is calculated correctly removed redundant file SKProduct+TestExtensions, replaced with mockSKProduct extracted mockProductsRequest into separate file for reuse added more tests added final tests for productsManager fixed rebase issue added link to docs for object identifiers added link to apple docs for apple receipt fields cleanup: extracted method to build internal containers added docs and extracted method for variable length quantity decoder added a couple of comments
0266da2
to
5fb0247
Compare
I noticed this featured is in the PurchasesCoreSwift module and not the Purchases... does this mean it is not meant for public use? I am using another local receipt parser nu for legacy reasons and would like to remove that dependency. |
Hmmm.... looks like the parse method isn't public. Is this intentional? I'd love to be able to use RevenueCat to parse local receipts. Today I have to keep another library around just to be able to do this. In some cases, the local receipt returns an original version that is different from the purchaserInfo and since my app has been converted from a paid app to freemium I am afraid to upset old customers by removing the local receipt parser. |
The local receipt parser isn't public, indeed, and it's intentional.
@sipersso I hope this answers your questions. As for making the migration, I'd encourage you to contact our support to ensure that your transition goes smoothly. They'll be able to help you work out any kinks. |
@aboedo Thanks! Then I'll avoid using it and reach out to support for guidance. |
Documentation on apple receipt format:
https://www.notion.so/revenuecat/Receipt-parsing-iOS-c3221c5591a045a7bab2780f6c819664
Overview of the solution:
ASN1 and containers
The receipt is stored as an ASN1Container, which recursively holds asn1 containers. Every component and every field in the receipt is stored as an ASN1 container.
Their basic structure is a TLV triplet - Type, Length, Value.
The type is identifier + encoding type (primitive vs constructed, tells you whether it's recursive)
The length is one or multiple bytes, and they indicate the length of the Value
The Value can be one of numerous ASN1 types, and it might hold other ASN1 containers.
Code structure:
The code is split into the main entrypoint, Basic Types, Builders and Data Converters.
Receipt Parser
The main entry point for local receipt parsing is
ReceiptParser
.This class has one public method, which parses an
AppleReceipt
from a binary stream of data, and returns if possible, throws if the data can't be interpreted as a receipt.To get the payload of the receipt,
ReceiptParser
interprets the binary stream as an ASN1 container, and then looks inside (recursively) for a specific container: anobjectIdentifier
of typedata
. If it's found, the container immediately after it will hold the payload.Basic types
The basic types (
AppleReceipt
,ASN1Container
,ASN1ObjectIdentifier
,InAppPurchase
) are just data models with only the needed properties, and querying methods in the case ofAppleReceipt
.Builders
Each of the builders holds the logic to create basic types from binary payloads or ASN1 containers.
Data Converters
The files in this group hold converters and general converters for binary types into Swift types.