Feature/cwall 199 #107
Feature/cwall 199 #107
Conversation
| requiredProperties: uri.includes('credential_offer_uri=') ? ['credential_offer_uri'] : ['credential_offer'], | ||
| }) as CredentialOfferV1_0_11; | ||
| if (credentialOffer?.credential_offer_uri === undefined && !credentialOffer?.credential_offer) { |
There was a problem hiding this comment.
Why is this removed? There should never be a by value and by reference version together in a URL
There was a problem hiding this comment.
Two lines above there is a method call where the required properties are passed as parameters. The ternary operator is used to decide whether credential_offer_uri or credential_offer is present. The actual check happens inside the method. So the if statement is redundant.
There was a problem hiding this comment.
Yeah, but the question is. This was a rather self-explanatory error message with a really simple condition. That raises the question, why would you even decide to remove that without consulting?
There was a problem hiding this comment.
Okay, please don't edit your response after you already got responses, as now my answer looks completely random.
That is only worse. The check is there to ensure that one option is set. You introduced a ternary that now assumes one is there or the other is there. So the correct approach would be to move that check to the top, so we at least know that only one is actually there and you can do your assumptions on what to pick
There was a problem hiding this comment.
I didn't introduce a ternary. The ternary was already there. I just added the '=', to make it skip the scheme and split the string at the credential offer query param.
| */ | ||
| export function convertURIToJsonObject(uri: string, opts?: DecodeURIAsJsonOpts): unknown { | ||
| if (!uri || (opts?.requiredProperties && !opts.requiredProperties?.every((p) => uri.includes(p)))) { | ||
| throw new Error(BAD_PARAMS); | ||
| } | ||
|
|
||
| if (opts?.arrayTypeProperties && opts.arrayTypeProperties.includes('credential_offer_uri')) { |
There was a problem hiding this comment.
Why does a generic function all of a sudden knows about credential_offers?
There was a problem hiding this comment.
Adding a simple '=' to the required properties fixed the issue
There was a problem hiding this comment.
Hmzz, no sure I like that approach, so please create a TODO. It should probably have been solved by not mixing the outer param with the inner param
There was a problem hiding this comment.
The actual problem here is the input url. Usual url encoders encode the '='. In the examples provided in the specs of OID4VCI the '=' is not encoded. If we receive a fully encoded url it doesn't work.
|
Closing this PR since the changes are obsolete |
|
@maikel-maas What do you mean with that, as that is highly unlikely. This still has outstanding issues as well |
No description provided.