Skip to content
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

Extensibility of PaymentResponse.complete(result) #817

Open
adrianhopebailie opened this issue Dec 6, 2018 · 9 comments
Open

Extensibility of PaymentResponse.complete(result) #817

adrianhopebailie opened this issue Dec 6, 2018 · 9 comments

Comments

@adrianhopebailie
Copy link
Collaborator

In his analysis of SRC, @mountainhippo identified the need to have a post-authorization step where the merchant/website notifies the payment handler of the result of the payment.

I believe that this is roughly why we have PaymentResult.complete(result) driven by a similar requirement from Apple Pay (@aestes can confirm).

My question is, how extensible can we make the result argument so we can support other payment methods with this requirement?

result is currently defined as being an optional PaymentComplete enum but I think it would be better if it was more generic.

The ideal would be if payment method specs could define a format for this (JSON serializable object) in the same way they do for their custom payment request and response data.

This would allow an SRC payment method spec to define the exact format of result when a website calls PaymentResponse.complete(result) after a payment.

Concretely this would mean:

  1. Change the definition of the PaymentResponse interface such that PaymentResponse.complete(response) is defined as:
  [NewObject]
  Promise<void> complete(optional object result);

where object is the object type (as used in the definition of PaymentMethodData.data).

  1. Change the definition of the complete() method to include a definition of result argument. Something like the following, copied from PaymentMethodData.data:

An object that provides optional information that might be needed by the supported payment methods. If supplied, it will be JSON-serialized.

  1. Move the definition of the PaymentComplete enum out of the PR API spec and into payment method specs that use it as is (E.g. Apple Pay).

The effect is that any payment method specification can then define 3 custom data formats:

  1. request data, passed in by the website as PaymentMethodData.data
  2. response data, received by the website as PaymentResponse.data
  3. complete data, passed in as result by the website when calling PaymentResponse.complete(result)
@rsolomakhin
Copy link
Collaborator

Thank you for writing this up, @adrianhopebailie ! I agree that "complete data" should be defined in the payent method. For the sake of feature detection, may I also suggest to use a different name for the method that sends over an object instead of enum? I believe the method name confirm(result) has been mentioned.

@ianbjacobs ianbjacobs added the SRC label Dec 17, 2018
@marcoscaceres
Copy link
Member

@adrianhopebailie, to keep things backwards compatible, we could add a second argument.

.complete("success", {detail: {}})

That would keep things backwards compatible, but allow supporting new payment types.

WDYT? Would that work?

@marcoscaceres
Copy link
Member

The underly question here is if PaymentComplete enum values apply to all transactions and it that needs to be extended beyond the three current values? We could expand PaymentComplete with additional things, and it would also be backwards compatible for all current payment methods... engines supporting newer payment methods would then also know about anything we would add to PaymentComplete 🤔...😀👍

@adrianhopebailie
Copy link
Collaborator Author

@rsolomakhin said:

For the sake of feature detection, may I also suggest to use a different name for the method that sends over an object instead of enum?

I worry that makes the API for a developer a little confusing. i.e. When do I call complete and when do I call confirm? That said, I'm not sure how else you could provide a feature detectable difference.

@marcoscaceres said:

to keep things backwards compatible, we could add a second argument.

I think my proposal is already backwards compatible (at least from the perspective of a developer using the API). I've proposed changing the type of the argument from enum to object which I had assumed is non-breaking for any javascript that is calling the API but I don't know enough about the IDL and JS relationship.

The underly question here is if PaymentComplete enum values apply to all transactions and it that needs to be extended beyond the three current values?

I think it would be a mistake to assume we can design anything that "applies to all transactions". Even if that's true now it'll probably not be true for long.

I could live with the two-argument solution, my only reservation is that this allows for ambiguous input (e.g. enum: success, detail: something that suggests failure).

@marcoscaceres
Copy link
Member

I think my proposal is already backwards compatible (at least from the perspective of a developer using the API). I've proposed changing the type of the argument from enum to object which I had assumed is non-breaking for any javascript that is calling the API but I don't know enough about the IDL and JS relationship.

Unfortunately, no. It would break :( For enums, IDL binding layer just converts whatever you pass into it into a string, so passing an object would become "[object Object]" (which would then throw an error, because it's not a known PaymentComplete enum value in a "v1" user agent).

Alternative might be to do (PaymentComplete or Object) (I've not checked if that's allowed in WebIDL). If it works, that would allow current things to keep working as they are, but also support passing the object for any new payment method.

@marcoscaceres
Copy link
Member

@mountainhippo would be great to hear more about the requirements around what needs to be passed.

@adrianhopebailie wrote:

I think it would be a mistake to assume we can design anything that "applies to all transactions". Even if that's true now it'll probably not be true for long.

I'm mindful that we might not be able to model everything that is needed - but using the object IDL type is something our security folks really frown on: even with the JSON serializable check, it still means we need to do extra security checks and data cloning to work with object (why, for instance, with Basic Card, we actually cast it to a BasicCardRequest dictionary). In short: it's a pain to work with, lacks some useful security assurances, etc.

There are a number of ways to deal with the extensibility aspects. If we find a common base, we can use that and extend it using inheritance. But if it's obvious that it won't scale, then yeah... object will be the way to go. But we should never start at object... object is where we land when all else fails.

So, I'd say we start with the use cases and requirements for what this thing needs and see how we go. And object is always there if we really need it at the end of the day.

@adrianhopebailie
Copy link
Collaborator Author

For enums, IDL binding layer just converts whatever you pass into it into a string, so passing an object would become "[object Object]"

Ah, makes sense.

PaymentComplete or Object

This seems like a good direction.

But we should never start at object... object is where we land when all else fails.

The reason I think we have to use the JSON serialisable object is for the same reasons PaymentMethodData.data in the request is an object. This could take any form, based on the payment method.

On the plus side, it's possible for browsers to enforce validation for known payment methods (like basic-card) where there is an IDL definition of the object in the payment method spec.

Would be interesting to explore the possibility of having an IDL definition in the manifest for payment methods that use a URL identifier?

@marcoscaceres
Copy link
Member

Would be interesting to explore the possibility of having an IDL definition in the manifest for payment methods that use a URL identifier?

Would be nice. The challenge is that browser today can only use IDL statically: we literally take IDL as text input, run it through a Python-based processor, and spit out C++... which is then compiled into the browser itself (i.e., no way of doing some kind of JIT compilation today).

So, before we start going too deep into technical solution, I think we should be clear about we might need to support SRC and other things.

@ianbjacobs
Copy link
Collaborator

Let's leave this issue open for now. I expect we will make progress on an SRC payment method in the first quarter of this year, and then we will know if we need any changes for at least that payment method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants