Making shop and inventory system less exploitable/more efficient?

Help and Feedback Code Review
,
#1

What does the code do and what are you not satisfied with?
Currently, I have my shop and inventory system work in separate Guis. The shop Gui updates an item information frame with the information of the item button you clicked. When the buy button is clicked, it invokes a remote function to the server to handle purchases. The inventory Gui listens for an event to be fired, which will then create a new button in the player’s inventory of their purchased item. Clicking a item button inside the inventory Gui will open an information frame like the shop, and has an equip button that will change the player’s EquippedItem value.

How (specifically) do you want to improve the code?
I would like to make it less exploitable, or to change my current system to make it more efficient.

Handles shop items and invokes the purchase remotefunction

for _, frameItem in pairs(itemFrame:GetChildren()) do
	if frameItem:IsA("TextButton") then
		frameItem.MouseButton1Click:Connect(function()
			local itemName = frameItem:WaitForChild("ItemName")
			local displayResult = displayInformation:InvokeServer(itemName.Value)
			
			if displayResult == "Buy" then
				buyButton.Text = "Buy"
			else
				buyButton.Text = "Already Owned"
			end
				
			nameLabel.Text = itemName.Value
			informationFrame.Visible = true
		end)
	end
end

buyButton.MouseButton1Click:Connect(function()
	local successfulPurchase = buyItem:InvokeServer(nameLabel.Text)
	
	if successfulPurchase == true then
		buyButton.Text = "Already Owned"
	elseif successfulPurchase == false then
		buyButton.Text = "Insufficient Kills"
		wait(1)
		buyButton.Text = "Buy"
	else
		print("Already owned")
	end
end)

Makes a table of prices for the items and handles the purchase button invoke

local itemPrices = {}

for _, item in pairs(items:GetChildren()) do
	itemPrices[item.Name] = item:FindFirstChild("Price").Value
end

displayInformation.OnServerInvoke = function(player, itemName)
	if not items:FindFirstChild(itemName) then
		return
	end
	
	local playerInventory = serverStorage.PlayerData[player.Name].Inventory
	
	if not playerInventory:FindFirstChild(itemName) then
		return "Buy"
	else
		return
	end
end

buyItem.OnServerInvoke = function(player, itemName)
	if not items:FindFirstChild(itemName) then
		return
	end
	
	local playerInventory = serverStorage.PlayerData[player.Name].Inventory
	
	if not playerInventory:FindFirstChild(itemName) then
		local itemCost = itemPrices[itemName]
		local kills = player.leaderstats.Kills
			
		if kills.Value >= itemCost then
			local purchasedItem = Instance.new("StringValue")
			purchasedItem.Name = itemName
			purchasedItem.Parent = playerInventory
				
			kills.Value -= itemCost
				
			purchaseComplete:FireClient(player, itemName)
				
			return true
		else
			return false
		end
	else
		return
	end
end

Handles inventory and updates when a new button is cloned inside

local function clickedFunction(frameItem)
	if not frameItem:IsA("TextButton") then
		return
	end
	
	frameItem.MouseButton1Click:Connect(function()
		local itemName = frameItem.ItemName.Value
		local textUpdate = updateEquipButton:InvokeServer(itemName)

		if textUpdate == "Equip" then
			equipButton.Text = "Equip"
		else
			equipButton.Text = "Unequip"
		end
		
		nameLabel.Text = itemName
		informationFrame.Visible = true
	end)
end

for _, frameItem in pairs(inventoryFrame:GetChildren()) do
	clickedFunction(frameItem)
end

inventoryFrame.ChildAdded:Connect(function(frameItem)
	clickedFunction(frameItem)
end)

equipButton.MouseButton1Click:Connect(function()
	local result = equipToggle:InvokeServer(nameLabel.Text)
	
	if result == "Equip" then
		equipButton.Text = "Unequip"
	else
		equipButton.Text = "Equip"
	end
end)

purchaseComplete.OnClientEvent:Connect(function(itemName)
	local addedItem = inventoryItem:Clone()
	addedItem.Text = itemName
	addedItem.ItemName.Value = itemName
	addedItem.Parent = inventoryFrame
end)

Handles the equip button invoke

updateEquipButton.OnServerInvoke = function(player, itemName)
	if not items:FindFirstChild(itemName) then
		return
	end
	
	local equippedItem = serverStorage.PlayerData[player.Name].EquippedItem

	if equippedItem.Value ~= itemName then
		return "Equip"
	else
		return
	end
end

equipToggle.OnServerInvoke = function(player, itemName)
	if not items:FindFirstChild(itemName) then
		return
	end
	
	local playerData = serverStorage.PlayerData[player.Name]
	local equippedItem = playerData.EquippedItem
	
	if playerData.Inventory:FindFirstChild(itemName) then
		if equippedItem.Value ~= itemName then
			equippedItem.Value = itemName
			return "Equip"
		else
			equippedItem.Value = "Default"
			return
		end
	end
end
1 Like
#2

This looks pretty solid. You verify prices and currency on the server, as you should. You also pass the name of the item and (sort of) check if it exists.

My suggestion would be having a check for when itemName is not actually a real item. Right now, that would error, which presents exploiters with the potential to spam errors into your server at a fast rate.

Next, your equipToggle actually lacks a check as to whether or not the player owns the item. This might not be a functional issue in itself but it could tie back to error spam.

Besides that, you could always make your system more efficient by having both the client and server keep track of the inventory. The server would keep the real version and tell the player when things are added or removed. That way, you don’t need to query the server every single time you want to know if you should buy an item or not.

3 Likes
#3

Thanks for bringing these too my attention. I just added a check to see if the itemName is inside the player’s inventory on the equipToggle, but I’m confused on what you mean by checking if itemName is an actual item.

Do you mean this?

if items:FindFirstChild(itemName) then
end
#4

Yes. A guard clause like that in buyItem and other functions which access items that just returns if there is no associated item.
Ex:

if not items:FindFirstChild(itemName) then
    return
end
2 Likes
#5

I see. Thanks for the suggestions, I wasn’t sure of a different way to pass the itemName (other than naming each button the name of the item?) so I resulted to using a value inside the button.