Fixed PayPal payment provider returning "Invalid cost amount" when it shouldn't.

[DragonByte Tech]

Affected version

2.0.5

I've been receiving the "Invalid cost amount" error message for some select payment receipts with a taxation rate set.

I modified the relevant line of PayPal.php to read
$state->logMessage = 'Invalid cost amount: ' . (round(floatval($state->costAmount), 2) - round(floatval($state->taxAmount), 2)) . ' is not equal to ' . round(floatval($cost), 2) . ' or "' . strval($state->costCurrency) . '" is not equal to "' . $currency . '"';

This is the message that gets logged:
Error: Invalid cost amount: 44.95 is not equal to 44.95 or "USD" is not equal to "USD"

This is the IPN returned:

array(35) {
  ["mc_gross"] => string(5) "53.94"
  ["protection_eligibility"] => string(10) "Ineligible"
  ["payer_id"] => string(13) "<>"
  ["tax"] => string(4) "8.99"
  ["payment_date"] => string(25) "06:56:00 May 01, 2018 PDT"
  ["payment_status"] => string(9) "Completed"
  ["charset"] => string(12) "windows-1252"
  ["first_name"] => string(4) "<>"
  ["mc_fee"] => string(4) "1.86"
  ["notify_version"] => string(3) "3.9"
  ["custom"] => string(32) "<>"
  ["payer_status"] => string(8) "verified"
  ["business"] => string(28) "<>"
  ["quantity"] => string(1) "1"
  ["verify_sign"] => string(56) "<>"
  ["payer_email"] => string(21) "<>"
  ["txn_id"] => string(17) "<>"
  ["payment_type"] => string(7) "instant"
  ["payer_business_name"] => string(10) "<>"
  ["last_name"] => string(5) "<>"
  ["receiver_email"] => string(28) "<>"
  ["payment_fee"] => string(4) "1.86"
  ["shipping_discount"] => string(4) "0.00"
  ["receiver_id"] => string(13) "<>"
  ["insurance_amount"] => string(4) "0.00"
  ["txn_type"] => string(10) "web_accept"
  ["item_name"] => string(31) "<>"
  ["discount"] => string(4) "0.00"
  ["mc_currency"] => string(3) "USD"
  ["item_number"] => string(0) ""
  ["residence_country"] => string(2) "GB"
  ["shipping_method"] => string(7) "Default"
  ["transaction_subject"] => string(0) ""
  ["payment_gross"] => string(5) "53.94"
  ["ipn_track_id"] => string(13) "<>"
}

The cost_amount column in the xf_purchase_request table contains 44.95.

I am running PHP 7.2.

 

[DragonByte Tech]

This may be caused by inconsistent behaviour in the round function.

I discovered that if I do round($val, 2); then run \XF::dumpSimple($val); it will display for instance float(39.95) as you would expect. However, when running the variable through json_encode, the floating point precision error would resurface, even though I explicitly asked for it to be chopped down to 2 decimal places.

I have changed all instances of round($val, 2); to sprintf("%.2f", $val); and that stores the information in the correct format after being ran through json_encode.

I wonder if there may be an additional issue related to MySQL wherein the DECIMAL(10,2) data type that we use still stores the floating point precision internally. If that is true, then that would explain why internally the cost validation fails, even though visually it doesn't.

I will post back after further payments have been completed with the sprintf change to see if that resolves the issue.

(This issue isn't directly applicable to the UserUpgrades as it is, but if/when you ever add taxation support, this issue may become relevant.)


Fillip

 

[DragonByte Tech]

VICTORY! I found the solution!

First of all, I want to add more details. I changed the logMessage code to this:
$state->logMessage = 'Invalid cost amount: ' . json_encode([round($state->costAmount, 2), round($state->taxAmount, 2), round($cost, 2)]);
which produced the following log message:

Error: Invalid cost amount: [30.190000000000001278976924368180334568023681640625,5.2400000000000002131628207280300557613372802734375,"24.95"]

(The log message was for a different IPN than the one in my OP)

Long story short (and lots of lost skin around my fingernails later), what worked for me was to change the relevant part of /src/XF/Payment/PayPal.php to this:

                $costValidated = (
                    round(($state->costAmount - $state->taxAmount), 2) == round($cost, 2)
                    && $state->costCurrency == $currency
                );

Since the costAmount and taxAmount have already been filtered by the input filterer, I don't see how this change could produce any form of problem, not to mention it has received live testing

@Chris D do you see any problems with this change?


Fillip

 

[Chris D]

Looks ok, but I've only give it a very quick glance.

Quite annoying, actually, as @Jake B. reported something similar to me recently and it may well be the same thing, and I couldn't see the problem with it at the time.

 

[DragonByte Tech]

Looks ok, but I've only give it a very quick glance.

Quite annoying, actually, as @Jake B. reported something similar to me recently and it may well be the same thing, and I couldn't see the problem with it at the time.

Sorry I couldn't find the problem prior to the release of 2.0.5 PayPal slows down the IPN re-send rate to the point where it was impossible to actually live-test changes. This time I just happened to catch the tail end of a transaction which was then followed by a new affected one that I caught live.

I'll continue to monitor future transactions and report back when we receive any more transactions with sales tax that doesn't end in 0 or 5.


Fillip

 

[Jake B.]

The issue I'm currently running into is that cost_amount is actually being stored as 0 regardless of what is entered, but only one a single user's install which is making it a bit difficult to pin down exactly why

 

[Chris D]

Ah so it is different. That’s sort of good (not for you obviously)

 

[Jake B.]

PayPal slows down the IPN re-send rate to the point where it was impossible to actually live-test changes.

Forgot to mention, this is a perfect use case for Ngrok, you can reply any request

You'll need to disable the step where it verifies with paypal that the connection was actually made by them, but other than that it should work

 

[DragonByte Tech]

You'll need to disable the step where it verifies with paypal that the connection was actually made by them, but other than that it should work

What I normally do, when I have the IPN data, is fake the $_POST contents which lets me just F5 to retry

I couldn't figure out how to disable the verification step without also breaking the site since I wanted to test it live, so I just said sod it and let it go


Fillip

 

[DragonByte Tech]

Update: Multiple transactions with different tax amounts (both with and without tax) have gone through successfully with the above change implemented

If possible, it would be great if the change could be added to XF 2.0.6.

Thanks!


Fillip

 

[Xon]

The problem is XF2 is doing a floatpoint operation and not rounding the outputs when doing the comparison. The real issue is that XF is using floats internally on what should be a decimal quantity.

Adding rounding before subtraction may be losing enough precision that it bugs out. But doing rounding after can also do it too.

I think the better solution be casting to integer amounts of cents rather than implicitly dollars. Or make MySQL do the lifting for decimals.

 

[DragonByte Tech]

Adding rounding before subtraction may be losing enough precision that it bugs out.

Adding rounding before subtraction loses enough precision that it bugs out 75% of the time, in my experience.

But doing rounding after can also do it too.

Sure, but errors in floating point precision is usually 4-7 decimals down the line (e.g. 24.99000000000064367564562134312456312) and applying rounding will not prevent that from reading 24.99.

The reason why rounding before subtraction loses more precision is probably the fact that when you subtract one float from another float, even a float that's been chopped, you re-apply the precision error.

In other words, round((0.66 - 0.33), 2) == 0.33 is true, but (round(0.33), 2) - round(0.33, 2)) == 0.33 is false, because the result of (round(0.33), 2) - round(0.33, 2)) is a float that does not quite equal 0.33.

I think the better solution be casting to integer amounts of cents rather than implicitly dollars. Or make MySQL do the lifting for decimals.

That is not the problem nor the solution. The problematic code is taking input from PayPal, which is a string, as you can see in my OP. Therefore, you have 2 strings that need to be numbers, and then one subtracted from the other.

The resulting value will then need to be compared to a decimal value fetched directly from MySQL (where the data type is DECIMAL(10,2)). Why round() is applied to the MySQL value as well as the PayPal values is probably a "better safe than sorry" measure rather than an explicit need.


Fillip

 

[Xon]

Something like https://github.com/moneyphp/money which does decimal like handling in php would be useful for money handling.

The resulting value will then need to be compared to a decimal value fetched directly from MySQL (where the data type is DECIMAL(10,2)). Why round() is applied to the MySQL value as well as the PayPal values is probably a "better safe than sorry" measure rather than an explicit need.

The decimal from mysql is cast to a string in php (mostly). If you do operations on the value it will become a float.

 

[Chris D]

I've just gone with the described fix from @DragonByte Tech for now.

The next release, of course, is a beta, so there will be a bit of time to test this.

You're correct in that there may be better solutions. Certainly Stripe uses integer values for currency amounts which is a good approach. I've made a note of it for future reference.