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

Create exchange yaml incorrect #434

Open
dlongley opened this issue Nov 27, 2024 · 5 comments
Open

Create exchange yaml incorrect #434

dlongley opened this issue Nov 27, 2024 · 5 comments
Assignees
Labels
ready for PR Issue ready to be resolved via a Pull Request

Comments

@dlongley
Copy link
Contributor

The incorrect section is here:

vc-api/exchanges.yml

Lines 313 to 334 in 5aff850

CreateExchangeRequest:
type: object
description: Object containing information about the exchange to be created.
properties:
exchangeInformation:
type: object
properties:
ttl:
type: string
description: Time to live for the exchange (ms).
variables:
type: array
description: Template variables to be used in the exchange.
items:
type: object
properties:
type:
type: string
description: The type of template.
template:
type: string
description: The template itself.

It should instead look something like this:

    CreateExchangeRequest:
      type: object
      description: Object containing information about the exchange to be created.
      properties:
        ttl:
          type: number
          description: Time to live for the exchange (seconds).
        variables:
          type: object
          description: Template variables to be used in the exchange.
        openId:
          type: object
          description: Optional parameters to enable OID4* protocol for delivery.
@msporny msporny added the ready for PR Issue ready to be resolved via a Pull Request label Jan 7, 2025
@kezike
Copy link

kezike commented Feb 17, 2025

Glad this issue was created, because I noticed some of these as well. I would add for ttl that this property is only useful if there is also a creation date property on the exchange schema. Otherwise, there would be no way of knowing whether the exchange has existed beyond the ttl. Let me know if I should create an issue for this.

@kezike
Copy link

kezike commented Feb 17, 2025

I would also note that the same changes noted by Dave should also be applied in appropriate locations in the specification text.

@dlongley
Copy link
Contributor Author

@kezike,

I would add for ttl that this property is only useful if there is also a creation date property on the exchange schema.

The exchange is considered created at the time that the request is made. This should obviate the need for an additional create date property to be included when it is being created. Does that address the concern?

Or are you talking about when the exchange state is retrieved by a coordinator? If so, if the spec doesn't include an expires property on the exchange state, then that is indeed missing and we should add it.

@kezike
Copy link

kezike commented Feb 18, 2025

@kezike,

I would add for ttl that this property is only useful if there is also a creation date property on the exchange schema.

The exchange is considered created at the time that the request is made. This should obviate the need for an additional create date property to be included when it is being created. Does that address the concern?

Or are you talking about when the exchange state is retrieved by a coordinator? If so, if the spec doesn't include an expires property on the exchange state, then that is indeed missing and we should add it.

Yeah, I am referring to the exchange state retrieval response, so that consumers know whether the exchange has gone stale.

@msporny
Copy link
Contributor

msporny commented Feb 18, 2025

The group discussed this on 2025-02-18 and @dlongley noted that the expires field is used by at least one implementation and we should get that into the specification. @PatStLouis asked if we should have both ttl and expires? @jandrieu noted the pattern that the two separate uses is TTL for cacheable data, valid for 20 days (expires), but check every day (ttl). @dlongley noted that in this case, TTL isn't being used in the way @jandrieu is noting. @msporny noted that expires should be good enough for now. @PatStLouis noted that expires probably addresses all use cases for now. @jandrieu noted we might be talking about bitstring status list and that might be an exchange -- status property in VC might be an exchange. @dlongley other benefit of ttl is server can have a default value for that, you can't have default expiration period (but that can be hidden behind interface where client provides its own expires and server computes it). Regarding BSL and ttl, it's not a good or useful feature, comes into conflict with validUntil -- understand two different uses, but challenging for BSL. A TTL for an exchange doesn't have to match TTL for BSL, don't know if that applies here. @dlongley noted that minimally on the exchange you need expires.

A PR needs to be raised that adds expires and removes ttl. The group will revisit whether the removal of ttl is causing an grief to implementers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for PR Issue ready to be resolved via a Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants