I am wondering if we should state a bit more explicitly here that there might be more parameters in that object depending on the transaction data type?

```suggestion "transaction_data_hashes_alg": [ "sha-256" ], "example_type_specific_parameter": "some-values" } ```

```suggestion - the referenced Credential(s) are not available in the Wallet ```

```suggestion - is missing required fields for the transaction data type. - the credential_ids does not match - the referenced Credential is not available in the Wallet ```

Could we add an Authorization Request example with some dummy transcation data?

```suggestion - transaction data type is not supported by the wallet - contains an unknown transaction data type value, ```

```suggestion Each document specifying a transaction data type must define the Credential that is being used to authorize the transaction data. The Credential may be a general purpose Credential or issued specifically for the authorization use case. A mechanism for Credential Issuers to express that a particular Credential can be used for authorization of transaction data is out of scope for this specification. ```

```suggestion * `credential_ids`: REQUIRED. Array of strings each referencing a Credential requested by the Verifier that shall be used to authorize the transaction. In Presentation Exchange the string matches the `id` field in the Input Descriptor. In Verifiable Presentation Query Language (defined in (#vp_query)) the string matches the `id` field in the Credential Query. If there is more than one element in the array, the Wallet MUST use only one of those Credentials. ```

Shouldn't this be `transaction_data_hashes` instead of `transaction_data` ? ```suggestion "transaction_data_hashes": [ "fOBUSQvo46yQO-wRwXBcGqvnbKIueISEL961_Sjd4do" ] ```

```suggestion The Wallet MUST ignore any unrecognized parameters, other than the `transaction_data` parameter. One exception to this rule is `transaction_data` parameter, and the wallets that do not support this parameter MUST reject requests that contain it. ```

```suggestion * `credential_ids`: REQUIRED. Array of strings each referencing a Credential requested by the Verifier that may be used to authorize the transaction. In Presentation Exchange the string matches the `id` field in the Input Descriptor. In Verifiable Presentation Query Language (defined in (#vp_query)) the string matches the `id` field in the Credential Query. If there is more than one element in the array, the Wallet MUST use only one of those Credentials. ```

```suggestion * `type`: REQUIRED. String that identifies the type of transaction data . The specific values are out of scope of this specification. It is RECOMMENDED to use collision-resistant names for `type` values. The `type` also determines the allowed further parameters of the `transaction_data` object. ```

```suggestion &request_uri_method=post HTTP/1.1 ```

```suggestion * `transaction_data_hashes_alg`: OPTIONAL. Array of strings each representing a hash algorithm identifier, one of which MUST be used to calculate hashes in `transaction_data_hashes` response parameter. The value of the identifier MUST be a hash algorithm value from the "Hash Name String" column in the IANA "Named Information Hash Algorithm" registry [@IANA.Hash.Algorithms] or a value defined in another specification and/or profile of this specification. If this parameter is not present, a default value of `sha-256` MUST be used. To promote interoperability, implementations MUST support the sha-256 hash algorithm. ``` The Named Information Hash Algorithm registry is not one of the JOSE registries.

I don't think the newline here was intentional? These parts seem to belong together ```suggestion This enables the Wallet to assess the Verifier's capabilities, allowing it to transmit only the relevant capabilities through the `wallet_metadata` parameter in the Request URI POST request. ```

```suggestion The following is a non-normative example of an Authorization Request with a `request_uri_method` parameter (including the additional `client_metadata`): ```

```suggestion &request_uri_method=post ```

```suggestion * `transaction_data_hashes`: Array of hashes, where each hash is calculated using a hash function over the strings received in the `transaction_data` request parameter. Each hash value ensures the integrity of, and maps to, the respective transaction data object. Where in the response this parameter is included is defined by each Credential Format Profile, but it has to be included in the mechanism used for the proof of possession of the Credential that is signed using the user-controlled key. ```

doesn't really matter, but introduces a reference that is not used?

Why are we removing this?

Add backticks: ```suggestion - any of the following is true for at least one object in the `transaction_data` structure: ```

Tiny grammar suggestion: ```suggestion * `transaction_data_hashes_alg`: REQUIRED when this parameter was present in the `transaction_data` request parameter. String representing the hash algorithm identifier used to calculate hashes in `transaction_data_hashes` response parameter. ```

I think we should be clearer what the meaning is when the `credential_ids` array contains more than one item. i.e. does the wallet try and sign using all the credentials it is returning that has an entry in this array, or just the first one? (Not sure it's likely to come up in practice to be honest, but we should probably still be clear.)

Typo? (`transaction_data_alg` doesn't appear anywhere else) - Given the next bullet fully defines the behaviour I think you could actually just remove this sentence entirely: ```suggestion ```

Suggest including a link to the applicable section (plus small suggested rewording): ```suggestion Where to include the`transaction_data_hashes` parameter in the response is specific to each credential format and is defined by the Credential Format Profile, such as those in (#alternative_credential_formats). ```

`[IANA.Hash.Algorithms]` isn't being rendered as a link - we should add `<reference` thing under `{backmatter}`.

I think this is part of the definition of `transaction_data` - adding four spaces to the beginning will cause it to be indented to match the rest of the definition: ```suggestion Each document defining a transaction data type specifies whether that type requires a Credential that is bound to transaction data to be specifically issued for that purpose. How the Credential Issuer expresses that a particular Credential can be used for obtaining user's authorization using transaction data mechanism is out of scope for this specification. ```

I think a conflict happened here, the last sentence about `client_id_scheme` was removed recently, it should be removed again: ```suggestion This enables the Wallet to assess the Verifier's capabilities, allowing it to transmit only the relevant capabilities through the `wallet_metadata` parameter in the Request URI POST request. ```

Trivial grammar suggestion that I think aligns better with what we mean: ```suggestion The Wallet MUST ignore any unrecognized parameters, other than the `transaction_data` parameter. The wallets that do not support the `transaction_data` parameter MUST reject requests that contain it. ```

I don't think this is correct - if you're using `request_uri`/JAR then `transaction_data` would be ignored here and must instead be present in the request object. (I'm similarly dubious about `client_metadata` being present in this example, I think that could just be removed now.) I think a full example that includes both a transaction_data request (including a matching credential request) and a matching response would be helpful, but that could happen in a later PR.

adding one little generalization to the generalization you did (thanks for that generalization btw) ```suggestion * `credential_ids`: REQUIRED. Array of strings each pointing to an identifier that identifies a set of information describing a Credential that the Verifier is requesting transaction data in a particular object to be bound to (e.g., Input Descriptor in Presentation Exchange). ```

I have a feeling there's an unintended consequence here between this PR (if merged after 1.0) and the extensibility PR. I think this language says "a wallet MUST fail a request that includes a `transaction_data` if it doesn't support transaction data". But the other PR says "wallets supporting 1.0 of this spec must accept requests that contain unknown elements, including `transaction_data`. I think we need the first behaviour - i.e. particularly when using the browser api, a wallet that doesn't support transaction_data must never say it can satisfy a request that contains transaction_data. If we agree on that, then I can see two solutions: 1. We add `transaction_data` to 1.0 with language like "wallets MUST reject claims that contain this element". 2. We add something like '[crit](https://www.rfc-editor.org/rfc/rfc7515#section-4.1.11)', but for the body, define that in 1.0, and when 1.1 comes along with transaction_data support then verifiers that want to use it are required to include `"crit": [ "transaction_data" ]` in the request payload.

```suggestion The `transaction_data_hashes` response parameter defined in (#transaction_data) MUST be included in the Key Binding JWT as a top level claim. This means that transaction data mechanism cannot be used with SD-JWT VCs without cryptographic key binding and, therefore, do not use KB JWT. ```

```suggestion The Wallet that received the `transaction_data` parameter in the request MUST include in the response a `transaction_data_hashes` parameter defined below. If the wallet does not support `transaction_data` parameter, it MUST return an error. Where in the response `transaction_data_hashes` parameter is included is specific to each credential format and is defined in a respective Credential Format Profile. ```

Hi, questions: a) Is transaction data different from a consent? b) Transaction data needs to be displayed to the end user, except if it's a M2M exchange. Keeping it generic, can lead to implementation issues. Should at least "display" or similar parameter be defined? c) Should the RAR or PE (presentation definition?) syntax be reused? (https://datatracker.ietf.org/doc/html/rfc9396#name-authorization-request) d) Explicit yes/no: default mode is that the user must authorise all actions, or the transaction fails? e) If user declines 1 action, should it be recorded? f) Is there a need for more elaborate transactions, such as conditional statements (e.g., you must answer all, you must answer to some of the questions), supporting multiple options (e.g., do you want to tip 5, 10 or 20%)? Or for QES: select a signature profile (e.g., advanced, qualified). We introduced something similar about 2 years ago for our B2B/M2M VP exchange (https://hub.ebsi.eu/conformance/learn/verifiable-presentation-exchange#service-to-service-token-flow). We defined a simple key:value approach where, where key is a scope, and value is a VP definition. I believe this matches your definition of transaction_data. In our model, for a given action all conditions must be fulfilled, no conditional statements, no multiple options (like tipping). Thank you!

I'd suggest indenting this and the next line (adding 4 spaces before the `*`) just to make it clearer they belong to the transaction data definition.

> The Wallet MUST refuse to process any unknown transaction data type This makes it hard when you don't know up front what a wallet might support or if we want to define new signing mechanisms and have a transition periods where verifiers can send both. I think we need to clarify 'MUST refuse' too (it could be interpreted as 'ignore' or as 'return an error'). Perhaps: "The wallet MUST return an error unless it supports at least one of the transaction data types"?

need clarify that when transaction_data is used, KB JWT must be present.

```suggestion * `credential_ids`: REQUIRED. Array of strings each pointing to an identifier that identifies a set of information describing a Credential that the Verifier is requesting transaction data in a particular object to be bound to (Input Descriptor in Presentation Exchange). ```

Why add this to the response and not just to the KB-JWT as defined below? If that is what is meant, this should be clarified here.

Should this refer to `tx_data_hashes`?

```suggestion The `transaction_data` response parameter defined in (#transaction_data) MUST be included in the Key Binding JWT as a top level claim. ```

```suggestion - any of the following is true for at least one object in the transaction_data structure: - contains an unknown transaction data type value, - is an object of known type but containing unknown fields, - contains fields of the wrong type for the transaction data type, - contains fields with invalid values for the transaction data type, or - is missing required fields for the transaction data type. ```

Lacks description of how the hashes should be encoded and which hash function to use.

```suggestion The Wallet that received the `transaction_data` parameter in the request MUST include in the response a `tx_data_hashes` parameter defined below: ```

```suggestion The transaction data mechanism enables a binding between the user's identification/authentication and the user’s authorization, for example to complete a payment transaction, or to sign specific document(s) using QES (Qualified Electronic Signatures). This is achieved by signing the transaction data used for user authorization with the user-controlled key used for proof of possession of the Credential being presented as a means for user identification/authentication. ```

Add recommendation to use collision-resistant identifiers?

The object itself is JSON-encoded, right? ```suggestion : OPTIONAL. Array of strings, where each string is a Base64url-encoded JSON-encoded object that contains a typed parameter set with details about the transaction that the Verifier is requesting the End-User to authorize. See (#transaction_data) for details. The Wallet MUST refuse to process any unknown transaction data type or transaction data not conforming to the respective type definition. Each object consists of the following parameters: ``` There may be more elegant ways to express this...

@c2bo need to more explicitly define either in this PR or in a follow up PR how to include this parameter in mdoc deviceResponse.

@martijnharing comment that need to clarify that `if the wallet does not support transaction_data, do not return anything`.

from https://github.com/openid/OpenID4VP/pull/197/files#r1709887017 ```suggestion The Wallet that received `transaction_data` parameter in the request, MUST include in the response a `tx_data_hashes` parameter defined below: ```

I think I might know where the words "ensures the integrity of, and maps to, the respective" came from :) I know one is in the request and one is somewhere in the response but using the same name `transaction_data` for the things in the request and the hashes of those things in the response feels erroneous. Maybe something like `tx_data_hashes` for this response one. Or because it has to be defined by each Credential Format Profile anyway, let the name be defined there too. Maybe.

with the likely coming of the new query language (i.e., #220), do we want to further ingrain the usage of input descriptor at this point?

The link is invalid, I believe `-` should be `_`: ```suggestion `transaction_data` response parameter defined in (#transaction_data) MUST be included in the Key Binding JWT as a top level parameter. ```