Feature/hero 6

[jackhellyercw]

generic purchase provider setup

[sesposito]

Left some comments, some of the params may still require some thought as we go through the implementation details.

[sesposito]

Left some more feedback

[sesposito]

Not sure we need to return this in the response, just in the request, unless you think this could be useful?

[jackhellyercw]

maybe i misunderstood when we spoke the about the previously requested changes, i thought you requested the persist bool to be returned with the response?

[sesposito]

I believe I meant that it should be returned as part of the provider implementation functions, not necessarily in the client response, unless you see value in it?

[jackhellyercw]

ah right okay i get you. I will remove this. This also won't need to be return from the provider implementation functions as we now do the upsert outside of those functions as discussed in the call the other week so persist is no longer passed to the validatePurchase func

[sesposito]

The upsert is done outside of the provider implementation but it may still want to skip storing it, so shouldn't it be returned by the provider so that Nakama knows whether to skip the upsert?

[jackhellyercw]

the persist bool already exists in Nakama as it comes from a parameter on the api endpoint for purchaseValidate. So Nakama does know whether to skip the upsert

[sesposito]

You're right, it's not needed for the enpoints, but we also need to expose the registered adapters through a purchase/subscription validate function in Go/Lua/JS, and whatever these return, also need to be upserted into database, unless persist is false.

[jackhellyercw]

that flow also handles the upsert outside of the provider validate implementation with the persist value being passed in from the nk.purchaseValidate function for example

[sesposito]

Shouldn't be needed as we removed references to uuid.

[jackhellyercw]

currently being used in the storagePurchase and storageSubscription structs for the userID

[sesposito]

@zyro do you think it's ok to keep this dependency in common or should we move to use strings everywhere?

[sesposito]

Shouldn't be needed

[sesposito]

Maybe we should remove these fields that are actually set by upsertPurchase/Subscription

[jackhellyercw]

those fields are being used. example from the existing validatepurchaseApple func:

	purchases, err := iap.UpsertPurchases(ctx, db, storagePurchases)
	if err != nil {
		return nil, err
	}

        validatedPurchases := make([]*api.ValidatedPurchase, 0, len(purchases))
	for _, p := range purchases {
		suid := p.UserID.String()
		if p.UserID.IsNil() {
			suid = ""
		}
		validatedPurchases = append(validatedPurchases, &api.ValidatedPurchase{
			UserId:           suid,
			ProductId:        p.ProductId,
			TransactionId:    p.TransactionId,
			Store:            p.Store,
			PurchaseTime:     timestamppb.New(p.PurchaseTime),
			CreateTime:       timestamppb.New(p.CreateTime),
			UpdateTime:       timestamppb.New(p.UpdateTime),
			ProviderResponse: string(raw),
			SeenBefore:       p.SeenBefore,
			Environment:      p.Environment,
		})
	}

[sesposito]

I think this should only return an error instead of http.HandlerFunc

[jackhellyercw]

Reason the handlerFunc was returned was so that the apple and google notification handler flow still worked. I can remove this but will need to rethink how that is going to be handled

[jackhellyercw]

handleRefund func now only returns error, have added a handleRefundWrapper func to the purchase provider interface to keep backwards compatibility

[sesposito]

You're right, it's not needed for the enpoints, but we also need to expose the registered adapters through a purchase/subscription validate function in Go/Lua/JS, and whatever these return, also need to be upserted into database, unless persist is false.