Nice catch. Can we add an XCUnitTest for this? Maybe have the timestamp to be a very big long long number?

Done. I've added XCUnitTest using current timestamp number value. It's enough to cause the intValue overflow in negative number. If I use really big long long number, the intValue overflow goes positive again and passes the test. I'm not sure of the usefulness of this "not negative int" check in the original code.

We should have some test for the toMap method, can we update the test to include the discount? If we don't have an existing test, do you mind adding a test for the toMap method?

Done. I've added a new test checking the values of the SKPaymentWrapper object and the result of the toMap method explicitly.

We should test that the paymentDiscount is encoded and sent to method channel.

I see your point, that in fact the bug was that the discount parameter was not sent trough the method channel. But I am not sure on how to construct such a test?
In my understanding: now the discount is send trough AppStorePurchaseParam in the buyNonConsumable method, which is not returning a useful result to test. This method constructs SKPaymentWrapper with a discount parameter and sends it to _skPaymentQueueWrapper.addPayment which turns the SKPaymentWrapper to a map (which we test in the newly created test for the toMap method) and sends it trough the method channel. But the channel is not returning useful result for testing either. The encoding of the paymentDiscount is in the toMap method which we test now.
Can you elaborate please?

You might be able to intercept the method call in the FakeStoreKitPlatform and inspect the value?

I got your idea. Thanks! Done in the last commit.

nits, the "optional" keyword is probably not necessary as the nullability of this parameter already indicates it is optional.
However, it might be better to add an explanation what does it mean to be null, something like:

///
/// The value is `null` when the product does not have a discount.

Done. Changed in the last commit.

nits: Alternatively, we can make it non-nullable and initialized it to empty map, this can avoid many unwrapping.

Yes, collections should only be nullable if null and empty are conceptually different.

Done. Changed in the last commit.

Yes, collections should only be nullable if null and empty are conceptually different.

Typo: through

But higher level, the changelog should describe the issue from the client perspective, not the implementation details of the bug.

Changed in the last commit.

This code is checking for a type violation; NSNull is not a NSDictionary, so if this happens the caller has made an invalid call. If you have a path where this is happening, it needs to be fixed at the original argument extraction point, not here.

The original code never transferred through the method channel the discount parameter. So this check was stopped at the !map check, never trying the map.count <= 0 check and directly returning nil afterwards.
What happened when I transferred the discount key through the channel was that, when I was buying a product without discount, it was NULL but it did not stop at the !map it went to the next check map.count <= 0 and crashed.
So here I do not try to check for a type violation, but I am shielding the crashing check that comes next.
As for if it should be done here or in the caller. Why it is better in the caller?
Shouldn't the method take care of the argument's validation, as it does in the checks that come below?

when I was buying a product without discount, it was NULL

You mean it was NSNull, presumably, given the code you added. NULL is a typedef for 0, and is essentially the C equivalent of nil. NSNull is not nil/NULL, it is singleton class.

but it did not stop at the !map

That is expected; a pointer that is pointing to an NSNull instance is not nil.

it went to the next check map.count <= 0 and crashed.

Presumably the crash was an unrecognized selector sent to instance exception, because count is not a method of NSNull.

So here I do not try to check for a type violation

Yes, that is exactly what you are checking for. You are taking a pointer of type NSDictionary and checking whether it contains an NSNull instance instead of an NSDictionary instance. Which would be a type violation.

As for if it should be done here or in the caller. Why it is better in the caller?

This method takes an NSDictionary*, not an id. It is better in the caller because it is a type violation to pass an argument to this method that is not an NSDictionary, and an instance of NSNull is not an NSDictionary.

The comparable Dart code is:

class NSNull {
}

void methodHandler(dynamic args) {
  expectsAMap(args);
}

void expectsAMap(Map? map) {
  if (map != null) {
    print(map.length);
  }
}

void main() {
  NSNull nullArg = NSNull();
  methodHandler(nullArg);
}

The bug is this Dart code is methodHandler passing args to expectsAMap without checking whether it's a Map, not when expectsAMap uses map as if it were its declared type.

Shouldn't the method take care of the argument's validation, as it does in the checks that come below?

Methods should not have to validate that they haven't been called with types that violate their declarations. Other validation (like whether it's nil), sure.

The code below this is also wrong; values with unknown types should be type checked before assigning to a variable declared as a specific type, not after. Either the values can only be NSStrings, in which case the checks are pointless, or (more likely) they could be NSStrings or NSNulls, in which case the variables should be declared id, not NSString*.

I see your point. Thanks for the detailed explanation. Much appreciated! I've pushed a commit where I move the check out of there and into it's caller.

This is a bugfix, so the version should be 0.3.2+3.

This adds a new field to the AppStorePurchaseParam so it should be a new feature. The PR description or CHANGELOG should probably be something like "Supports adding discount information to AppStorePurchaseParam", instead of "fixing".

Thanks for the correction there; I didn't look closely enough at the details and thought it was just populating something that was previously added but not wired up.

Comments should be full grammatically correct sentences, with capitalization and punctuation.

Changed in the last commit

This comment sounds like you attempted to cast with as instead of cast<...>.

True. Changed it to a cast<> now. Thanks!

This is still a type violation, it's just more local. If the right hand side of this assignment is NSNull, then it is incorrect to assign it to an NSDictionary*.

Either this should be an id, or you should use a helper like https://github.com/flutter/plugins/blob/main/packages/file_selector/file_selector_macos/macos/Classes/FileSelectorPlugin.swift#L198-L201 (written as ObjC of course) to do the extract/check/convert/assign in a single line here.

Doing the latter would allow you to remove the extra conditional you had to add at this layer since the called code already handles nil correctly.

(@cyanglaz Any chance you could plan to convert this plugin to Pigeon sometime in the nearish term, like early 2023? It seems like there's a fair amount of this kind of problem in the plugin, and we could eliminate all of it with a Pigeon conversion.)

Done. I did it with ObjC version of the helper that you've shown, one line without the condition check. Thanks! :)