-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feature/hero 6 #2354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature/hero 6 #2354
Conversation
…und looping through platforms enum and generic validatePurchase functioniality added
…atepurchase is in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intermediate review feedback, there's some more changes that we'll need here.
apigrpc/apigrpc.proto
Outdated
// Validate Purchase | ||
rpc ValidatePurchase (api.ValidatePurchaseRequest) returns (api.ValidatePurchaseResponse) { | ||
option (google.api.http) = { | ||
put: "/v2/iap/purchase", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a post
to be consistent with other APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to post
build/Dockerfile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We likely want to revert these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
main.go
Outdated
@@ -187,6 +187,7 @@ func main() { | |||
leaderboardRankCache := server.NewLocalLeaderboardRankCache(ctx, startupLogger, db, config.GetLeaderboard(), leaderboardCache) | |||
leaderboardScheduler := server.NewLocalLeaderboardScheduler(logger, db, config, leaderboardCache, leaderboardRankCache) | |||
googleRefundScheduler := server.NewGoogleRefundScheduler(logger, db, config) | |||
xboxRefundPoller := server.NewXboxRefundPoller(logger, db, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be up to the xbox specific plugin that implements the adapter interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the xbox adapter
server/api_purchase.go
Outdated
|
||
func validatePurchaseRequest(ctx context.Context, in *api.ValidatePurchaseRequest, platform iap.Platform, config *IAPConfig) error { | ||
switch platform { | ||
case iap.Apple: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok that we provide well-known defaults for the most common platforms in nakama-common
, but we want the implementation (and Nakama) to be agnostic to a specific provider.
Perhaps the adapter interface could expose a GetProviderString
and GetProvider
functions, or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a ValidateRequest func on the purchaseprovider interface, this will be used to check for the relevant string for each platform, example
func (a *ApplePurchaseProvider) ValidateRequest(in *api.ValidatePurchaseRequest) error {
if a.config.GetApple().GetSharedPassword() == "" {
return status.Error(codes.FailedPrecondition, "Apple IAP is not configured.")
}
return nil
}
server/config.go
Outdated
Xbox *IAPXboxConfig `yaml:"xbox" json:"xbox" usage:"Xbox Configuration."` | ||
Playstation *IAPPlaystationConfig `yaml:"playstation" json:"playstation" usage:"Playstation Configuration."` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration should live in the plugin implementation, Nakama should not be aware of specific provider configurations for the generic IAP adapter API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved into the adapter, have added a func to the config interface in nakama common which gets the configfilepath which is then used to read in the config in the adapter
main.go
Outdated
@@ -215,6 +216,7 @@ func main() { | |||
|
|||
leaderboardScheduler.Start(runtime) | |||
googleRefundScheduler.Start(runtime) | |||
xboxRefundPoller.Start(runtime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved into adapter
server/api_purchase.go
Outdated
persist = in.Persist.GetValue() | ||
} | ||
|
||
validation, err := purchaseProvider.PurchaseValidate(ctx, s.logger, s.db, in.Receipt, userID, persist, s.config.GetIAP()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely be wrapped into a core function that calls the provider's PurchaseValidate
as done here, but also then proceeds to call upsertPurchase
to actually store it, and populate the response including the seenBefore
field.
We may also want to return the persist
flag as part of PurchaseValidate
return values so that we skip storing it and just return the validated output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the upsertPurchase is currently called in the purchaseProvider.PurchaseValidate func, can we have a chat about this tomorrow?
server/xbox_refund_poller.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is just being used for testing, but if not, it should live in the separate implementation of the adapter interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved into adapter
@@ -305,6 +308,8 @@ func (e RuntimeExecutionMode) String() string { | |||
return "purchase_notification_google" | |||
case RuntimeExecutionModeSubscriptionNotificationGoogle: | |||
return "subscription_notification_google" | |||
case RuntimeExecutionModePurchaseNotification: | |||
return "purchase_notification_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return "purchase_notification_" | |
return "purchase_notification" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea here was that i have prepended the platform inside the registerRefundHandler func:
`func (ri *RuntimeGoInitializer) RegisterRefundHandler(platform string, purchaseRefundFn runtime.PurchaseRefundFn, subscriptionRefundFn runtime.SubscriptionRefundFn) error {
if nk, ok := ri.nk.(*RuntimeGoNakamaModule); ok {
if ri.refundFns == nil {
ri.refundFns = make(map[string]runtime.RefundFns)
}
if nk.refundFns == nil {
nk.refundFns = make(map[string]runtime.RefundFns)
}
purchasefuncWrapper := func(ctx context.Context, logger runtime.Logger, db *sql.DB, nk runtime.NakamaModule, purchase *api.ValidatedPurchase, providerPayload string) error {
ctx = NewRuntimeGoContext(ctx, ri.node, ri.version, ri.env, RuntimeExecutionModePurchaseNotification, nil, nil, 0, "", "", nil, "", "", "", "")
return purchaseRefundFn(ctx, ri.logger.WithField("mode", RuntimeExecutionModePurchaseNotification.String()+platform), ri.db, ri.nk, purchase, providerPayload)
}
SubscriptionfuncWrapper := func(ctx context.Context, logger runtime.Logger, db *sql.DB, nk runtime.NakamaModule, subscription *api.ValidatedSubscription, providerPayload string) error {
ctx = NewRuntimeGoContext(ctx, ri.node, ri.version, ri.env, RuntimeExecutionModePurchaseNotification, nil, nil, 0, "", "", nil, "", "", "", "")
return subscriptionRefundFn(ctx, ri.logger.WithField("mode", RuntimeExecutionModePurchaseNotification.String()+platform), ri.db, ri.nk, subscription, providerPayload)
}
refundFns := runtime.RefundFns{
Purchase: purchasefuncWrapper,
Subscription: SubscriptionfuncWrapper,
}
ri.refundFns[platform] = refundFns
nk.refundFns[platform] = refundFns
}
return nil
}`
can still remove if you want though
…eric validate purchase func
…purchase provider validate request func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more comments @jackhellyercw
server/runtime_config.go
Outdated
@@ -33,6 +34,10 @@ func (c *RuntimeConfigClone) GetName() string { | |||
return c.Name | |||
} | |||
|
|||
func (c *RuntimeConfigClone) GetConfigFilePath() []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
server/runtime_config.go
Outdated
@@ -18,6 +18,7 @@ import "github.com/heroiclabs/nakama-common/runtime" | |||
|
|||
type RuntimeConfigClone struct { | |||
Name string | |||
FilePaths []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -2795,6 +2861,82 @@ func (ri *RuntimeGoInitializer) RegisterFleetManager(fleetManager runtime.FleetM | |||
return nil | |||
} | |||
|
|||
func (ri *RuntimeGoInitializer) RegisterPurchaseProvider(platform string, purchaseProvider runtime.PurchaseProvider) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@novabyte I think we want the platform to be an int
, or would we rather use string
to identify them?
server/runtime_go.go
Outdated
} | ||
|
||
if nk, ok := ri.nk.(*RuntimeGoNakamaModule); ok { | ||
if ri.purchaseProviders == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do these initializations when the structs are initialized instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved initializations
iap/facebook.go
Outdated
return nil, runtime.ErrPurchaseProviderFunctionalityNotSupported | ||
} | ||
|
||
func (f *FacebookPurchaseProvider) HandleRefund(ctx context.Context) (http.HandlerFunc, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't return http.HandlerFunc
, instead, it should just return error. The actual implementation of the endpoints should be up to the adapter via custom RPC, and for the adapters that we want to auto-register, we should just wrap this function so that we register it as an http.HandlerFunc
, to keep it backwards compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should the custom RPC in the adapter not just be the handleRefund function? and for the auto registered adapters how do you want to wrap this? do you want to move the code that registers the handleFunc inside of the handleRefund func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a wrapper func around handleRefund
server/config.go
Outdated
@@ -550,6 +552,10 @@ func (c *config) GetName() string { | |||
return c.Name | |||
} | |||
|
|||
func (c *config) GetConfigFilePath() []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
server/config.go
Outdated
@@ -643,6 +649,7 @@ func (c *config) GetRuntimeConfig() (runtime.Config, error) { | |||
|
|||
cn := &RuntimeConfigClone{ | |||
Name: clone.GetName(), | |||
FilePaths: clone.GetConfigFilePath(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
… generic string passed into validatePurchase func, unused comments removed, platform enum moved to nakama common, google handleRefund flow comment out and revert config file path func changes
…he purchase providers and refund funcs maps where the struct is initialized
func RegisterBuiltInIAPPurchaseProviders(nk runtime.NakamaModule, logger runtime.Logger, initializer runtime.Initializer, db *sql.DB, config runtime.IAPConfig, zapLogger *zap.Logger) { | ||
// Apple | ||
provider := iap.NewApplePurchaseProvider(nk, logger, db, config, zapLogger) | ||
if provider != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this check inspect whether a "Apple" provider is registered, and only if not, register the default on?
Same applies for the other providers with default implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checked by the RegisterPurchaseProvider func:
_, exists := ri.purchaseProviders[platform]
if exists {
return errors.New("platform already registered")
}
server/runtime_go.go
Outdated
_, exists := ri.refundFns[platform] | ||
|
||
if exists { | ||
return errors.New("platform already registered") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return errors.New("platform already registered") | |
return fmt.Errorf("refund handler for platform %q already registered", platform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change made
server/api_purchase.go
Outdated
validationPurchases, err := purchaseProvider.PurchaseValidate(ctx, in, userID.String()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
validatedPurchasesResponse, err := handleValidatedPurchases(ctx, s.db, validationPurchases, persist) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to expose this logic under a single function, defined in core_purchase.go
, simply because we'll also have to call this for the Lua and JS runtime functions to be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change made and also have done the same for the subs validate funcs
server/api_purchase.go
Outdated
CreateTime: timestamppb.New(p.CreateTime), | ||
UpdateTime: timestamppb.New(p.UpdateTime), | ||
ProviderResponse: p.RawResponse, | ||
Environment: p.Environment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the RefundTime here (I think it was missing in the original code too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be included here? this is called as part of the validate purchase flow not the refund flow
@jackhellyercw maybe it's best if you rebase against the current master as there's been a few changes thus far. |
…/hero-6 # Conflicts: # apigrpc/apigrpc.pb.go # apigrpc/apigrpc.pb.gw.go # apigrpc/apigrpc.swagger.json # apigrpc/apigrpc_grpc.pb.go # console/console.pb.go # console/console.pb.gw.go # console/console_grpc.pb.go # console/ui/src/app/console.service.ts # go.mod # go.sum # server/console.go # server/core_subscription.go # server/google_refund_scheduler.go # server/runtime.go # server/runtime_go.go # vendor/github.com/heroiclabs/nakama-common/api/api.pb.go # vendor/github.com/heroiclabs/nakama-common/rtapi/realtime.pb.go # vendor/golang.org/x/net/http2/frame.go # vendor/modules.txt
… certain platform, moved handling of validated purchase and subs funcs into core_purchase and core_subs
@jackhellyercw I think the only thing left is to expose the adapters via the Go/Lua/JS runtimes as well. |
@sesposito so is this just for the built in platforms (apple, google, facebook, huawei)? just asking as i've had a quick look at adding a registerPurchaseProvider func for the js runtime and not entirely sure how i would do this. Also not entirely sure how/if i can even add the nakama-console-auth go package to a js runtime server so that i would be able to even use the console platforms |
@jackhellyercw we don't want to allow registration via JS/Lua, we'll provide that via the Go modules, however we do want to expose any registered adapters validation via the runtimes, you should only need to wire the adapters to the several providers and add wrappers for Lua/JS to call them |
created generic validate purchase func, created new flow for apple platform to use the new register purchase provider and refund funcs. Xbox platform also setup up to use this new flow