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

Straighten out the payment method data flow #382

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Dec 14, 2016

This fixes #338 and helps with #307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app.

Along the way, this fixes #354.

This does not completely fix #307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's #346).


This is on top of #374 and should be merged only after that is.

If this looks correct, then to clarify how the basic card spec actually interacts with the main spec, we'll edit the basic card spec to say:

  • Payment apps for basic card must determine if they support the requested payment method by first performing steps equivalent to parsing the supplied data using JSON.parse, then converting the resulting object to a BasicCardRequest dictionary, and inspecting the results.
  • Payment apps for basic card must supply a result object back to the payment request flow in the form of a BasicCardResponse dictionary.

This will then solve w3c/payment-method-basic-card#20.

@adrianba
Copy link
Contributor

I think this is generally fine but I'd prefer that we don't require data be passed in JSON string form. In our implementation we might present the data to apps already "deserialized" so it won't look like a string to them. I think the spec ought not be so explicit in saying it will be that string form - an equivalent should also be okay. We likely to use an alternative RPC mechanism that isn't a string and this should be acceptable.

@domenic
Copy link
Collaborator Author

domenic commented Dec 14, 2016

Well, in general,

Conformance requirements phrased as algorithms or specific steps may be implemented in any manner, so long as the end result is equivalent.

The important part is that JSON.stringify is performed at a very particular point in the algorithm.

@adrianba
Copy link
Contributor

Well, in general,

Conformance requirements phrased as algorithms or specific steps may be implemented in any manner, so long as the end result is equivalent.

The important part is that JSON.stringify is performed at a very particular point in the algorithm.

That's true, but it looks like the end result here is passing off a JSON string to a payment app.

@domenic
Copy link
Collaborator Author

domenic commented Dec 16, 2016

How would that be observably any different from passing along some other serialization format derived from or containing the equivalent information to that JSON string?

@adrianba
Copy link
Contributor

adrianba commented Jan 4, 2017

My concern is that the way this is written implies that that Payment App spec will expect a JSON string.

@domenic
Copy link
Collaborator Author

domenic commented Jan 4, 2017

Yes, I think it would. It can then transform that JSON string into whatever format is most appropriate for it.

What other serializable data structure would you expect a spec to be able to deal in?

@adrianba
Copy link
Contributor

adrianba commented Jan 5, 2017

I think the transport format should be an implementation detail as long as it isn't exposed. The current language leaves ambiguous whether it would be exposed. I would like to close down that ambiguity.

We discussed in the editors call just now and Marcos is reviewing.

@domenic
Copy link
Collaborator Author

domenic commented Jan 5, 2017

Can you explain how it's ambiguous that it would be exposed? No spec exposes it except to other specs.

We need a well-defined transport format for specs to be able to talk to each other. Specifying an entirely new one seems superfluous.

method</a> are handled by <dfn data-lt=
"payment app|payment apps">payment apps</dfn>. In this specification,
these details are left up to the <a>user agent</a>, but future
specifications may expand on the processing model in more detail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for now - but we should have at least 1 base thing here (probably "basic-card").

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I completely understand the ask here, but maybe this is the same as my "If this looks correct, then to clarify how the basic card spec actually interacts with the main spec, we'll edit the basic card spec to say:" from the PR's OP?

@marcoscaceres
Copy link
Member

Small conflicts

@domenic domenic force-pushed the json-serialize-flow branch from 77e3f54 to 930187d Compare January 10, 2017 18:59
@domenic
Copy link
Collaborator Author

domenic commented Jan 10, 2017

Rebased with conflicts fixed.

This fixes w3c#338 and helps with w3c#307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app.

Along the way, this fixes w3c#354.

This does not completely fix w3c#307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's w3c#346).
@domenic domenic force-pushed the json-serialize-flow branch from 930187d to b5b64a0 Compare January 10, 2017 19:24
domenic added a commit to domenic/webpayments-methods-card that referenced this pull request Jan 10, 2017
This follows w3c/payment-request#382 so that instead of simply defining the dictionaries in this spec, we say how they are used, and how invalid incoming data is processed. (See w3c#20.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants