-
Notifications
You must be signed in to change notification settings - Fork 15
Clarify "request_context" #247
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: main
Are you sure you want to change the base?
Conversation
@@ -462,7 +462,7 @@ To request a Txn-Token the workload invokes the OAuth 2.0 {{RFC6749}} token endp | |||
|
|||
The following additional parameters MAY be present in a Txn-Token Request: | |||
|
|||
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. The parameter SHOULD be present and how the Transaction Token Service uses this parameter is out of scope for this specification. | |||
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. Use of this parameter is RECOMMENDED. |
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.
Rather than specify it this way, should line 463 say: "The following parameter SHOULD be present in a Txn-Token request", and then line 465 can read: "request_context
OPTIONAL. This parameter contains a base64url encoded JSON object with represents the context of this transaction."
The request_details
parameter in this list can be under a new line that says: "The following parameters MAY be present in a Txn-Token request".
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.
Or if we prefer RECOMMENDED
The following additional parameters are RECOMMENDED to be present in a Txn-Token Request:
This implies that both are equally recommended. If one is "more recommended" than the other then we should stay with the structure that Pieter as in this PR.
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 issue that Dan flagged was that the text said that how the transaction token service uses the transaction token is out of scope, but later we actually define how it is used. The goal of this PR was to get rid of the contradiction without changing in the normative requirement (RECOMMENDED and SHOULD have equal levels of compulsion as I understand it).
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.
Do we want request_details
to also be SHOULD/RECOMMENDED? or just request_context
. I'm good with the removal of the other text.
@@ -462,7 +462,7 @@ To request a Txn-Token the workload invokes the OAuth 2.0 {{RFC6749}} token endp | |||
|
|||
The following additional parameters MAY be present in a Txn-Token Request: |
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 following additional parameters MAY be present in a Txn-Token Request: | |
The following additional parameters are RECOMMENDED to be present in a Txn-Token Request: |
@@ -462,7 +462,7 @@ To request a Txn-Token the workload invokes the OAuth 2.0 {{RFC6749}} token endp | |||
|
|||
The following additional parameters MAY be present in a Txn-Token Request: | |||
|
|||
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. The parameter SHOULD be present and how the Transaction Token Service uses this parameter is out of scope for this specification. | |||
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. Use of this parameter is RECOMMENDED. |
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.
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. Use of this parameter is RECOMMENDED. | |
* `request_context` OPTIONAL. This parameter contains a base64url encoded JSON object which represents the context of this transaction. |
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.
@gffletch - after trying a few options, I think your earlier suggestion makes sense to move the normative language out of the parameter description (and also getting rid of the contradiction).
See issue #228
In addition, the "request_context" is optional, so making it a SHOULD be included feels like maybe it is not optional. Instead the PR proposes language that says it is "RECOMMENDED".
@tulshi @gffletch - please review.