Is returning PurchaseGranted at the end bad practice?

Help and Feedback Scripting Support
,
#1

I have noticed that in many code samples in the Developer Hub as well as production environments, the very bottom of a ProcessReceipt function returns Enum.ProductPurchaseDecision.PurchaseGranted. This means that if Enum.ProductPurchaseDecision.NotProcessedYet isn’t returned anywhere in the code, the purchase will be marked as granted.

local function ProcessReceipt(ReceiptInfo)
    if not something then
        return Enum.ProductPurchaseDecision.NotProcessedYet
    end

    return Enum.ProductPurchaseDecision.PurchaseGranted
end

I’ve just been thinking to myself; isn’t it bad practice to do something like this? A code structure like this means that if you don’t explicitly write conditions where a purchase should be denied, then it’ll be marked as processed.

Improperly written code or lack of handlers for various products could lead to purchases being marked as granted, whether anything happened or not. This could potentially lead to dishonouring of purchases, which falls under scamming. That’s trouble waiting at the corner, as well as moderation.

3 Likes
#2

I’d like to say yes. I always return NotProcessedYet at the bottom with validity checks above. It makes much more sense than the alternative.

2 Likes
#3

While returning at the end isn’t nessisarily broken, I feel you should have some validity check. This makes it feel safer, even if it really is doing the same thing.

local function ProcessReceipt(ReceiptInfo)
     if validPurchaseCondition then
        return Enum.ProductPurchaseDecision.PurchaseGranted
    end    
return Enum.ProductPurchaseDecision.NotProcessedYet
end
#4

Why not just assume a purchase is invalid until proven valid? This way by default you can return NotProcessedYet if the code executes to end of the function scope? Why use an extra else if?

Disclaimer, I’ve never used it yet. But I have been formulating my own systems around it. And this would be my approach.

#5

Sorry, wasn’t sure if it returned that value by default. Then again, I didnt need a else if. I edited by above reply.

#6

It doesn’t return the value by default.

I was simply pointing out doing a default return manually. Using your code it would look a bit like this.

local function ProcessReceipt(ReceiptInfo)
     if validPurchaseCondition then
        return Enum.ProductPurchaseDecision.PurchaseGranted
    end

  return Enum.ProductPurchaseDecision.NotProcessedYet
end
#7

That code and my else if do the same thing; Your’s just looks a lot nicer. It’s honestly preference.

1 Like
#8

On top of looking nicer, it actually reduces another scope. Where as your code now has a specified location for code to be executed.

local function ProcessReceipt(ReceiptInfo)
     if validPurchaseCondition then
        -- Scope Here
        return Enum.ProductPurchaseDecision.PurchaseGranted
     else if not validPurchaseCondition then
        -- Scope here
        return Enum.ProductPurchaseDecision.NotProcessedYet
    end    
end

Presentation aside, you have to now maintain two blocks of code, you can argue it does the same thing but in all honesty, it makes the code harder to maintain. Considering this is a master function for all developer purchases that will be in your game this one function will get large pretty quickly and even if broken up into different functions or modules, at the end of the day depending on the project its still gonna be of a decent size.

So while preference does have a say in it. Assuming a purchase will fail versus a purchase will be granted will save you a lot of headache and debugging hours. Let alone put a stop to the possibility of granting a user access to monetized content that they haven’t actually purchased. So I’d say that returning PurchaseGranted in any case by default is bad practice.

It’s similar to not sanitizing input from a client. Though this is all my opinion, and everyone has a right to their own opinion.

2 Likes
#9

One of the main things I’m worried about is the way in which I write the script. While I have a lot of concerns about returning PurchaseGranted by default, there’s also to take into account that you may want to create separate functionality for different things.

For example, in the code example under the ProcessReceipt documentation, they run several operations; checking if the unique purchase has been made before (I absolutely hate the way they use pcall), run the product handler then record the purchase to a DataStore.

Since ProcessReceipt is a callback, the function ends the moment any ProductPurchaseDecision is returned. That means that depending on your code structure, you may miss out on being able to perform a lot of the functions such as what’s included above.

The following text is highlighted in red at the top of ProcessReceipt:

Care and caution are highly recommended in implementing this callback as small mistakes run the risk of failed sales.

It may be a bit of a hassle to cover for all failing scenarios and explicitly return NotProcessedYet, though I find that may be one of the core “care and caution” methods. At the same time, it’s not entirely possible (I don’t think) to cover for all failing scenarios and a purchase might end up getting blankly processed.

Returning PurchaseGranted by default may not be a wise idea, but if it’s done so, perhaps you want to be very cautious and make sure to block purchases in failing circumstances as much as possible?

2 Likes
#10

A lot of people tend to overlook two crucial aspects to the .ProcessReceipt callback:

  1. receiptInfo.PurchaseId is a unique identifier of the purchase - .ProcessReceipt will pass the same identifier after the player leaves and reenters the game. You will have to temporarily store this identifier in your DataStorage to fully validate purchases from granting duplicates.

  2. You can actually yield .ProcessReceipt for as long as you need to, even with a

while wait() do // If player leaves during this, you can "return Enum.ProductPurchaseDecision.NotProcessedYet"
  if condition == true then
    break
  end
end
...
return Enum.ProductPurchaseDecision.PurchaseGranted

…Until you can validate that the purchase was granted and successfully stored in the DataStore (:UpdateAsync() always returns uncached data. Yes, :UpdateAsync() can be used to fetch DataStore data).

3 Likes