Sphereon-Opensource / SIOP-OID4VP

Self Issued OpenID Provider v2 (SIOP) with optional OpenID for Verifiable Presentations (OpenID4VP)
Apache License 2.0
77 stars 25 forks source link

Feature/vdx 148/add verifier information for wallet clients #34

Closed hrehman-sphereon closed 1 year ago

hrehman-sphereon commented 1 year ago

looks good to me, just something I forgot to mention yesterday, please add some assertion for the new added values in the RP and OP test suits

Some assertions added, for example, in the jest case titled : 'return a reference url'

nklomp commented 1 year ago

As mentioned in a comment I would expect a builder that has a seperate optional argument for a language tag and a required argument. The builder than builds the complete tags

codecov-commenter commented 1 year ago

Codecov Report

Merging #34 (7436a87) into release/v2.0.0 (bc94de5) will increase coverage by 0.86%. The diff coverage is 100.00%.

@@                Coverage Diff                 @@
##           release/v2.0.0      #34      +/-   ##
==================================================
+ Coverage           83.06%   83.92%   +0.86%     
==================================================
  Files                  30       31       +1     
  Lines                1352     1425      +73     
  Branches              351      361      +10     
==================================================
+ Hits                 1123     1196      +73     
  Misses                222      222              
  Partials                7        7              
Flag Coverage Δ
unittest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/main/schemas/AuthenticationRequestOpts.schema.ts 100.00% <ø> (ø)
.../main/schemas/AuthenticationResponseOpts.schema.ts 100.00% <ø> (ø)
...rc/main/schemas/DiscoveryMetadataPayload.schema.ts 100.00% <ø> (ø)
...in/schemas/RPRegistrationMetadataPayload.schema.ts 100.00% <ø> (ø)
src/main/types/SIOP.types.ts 97.65% <ø> (ø)
src/main/AuthenticationRequestRegistration.ts 83.78% <100.00%> (+3.78%) :arrow_up:
src/main/AuthenticationResponseRegistration.ts 100.00% <100.00%> (ø)
src/main/OP.ts 88.63% <100.00%> (+0.83%) :arrow_up:
src/main/PresentationExchange.ts 78.30% <100.00%> (-0.21%) :arrow_down:
src/main/functions/LanguageTagUtils.ts 100.00% <100.00%> (ø)
... and 2 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

hrehman-sphereon commented 1 year ago

As mentioned in a comment I would expect a builder that has a seperate optional argument for a language tag and a required argument. The builder than builds the complete tags

@nklomp

test/IT.spec.ts

'RP and OP interaction should' 'succeed when calling each other in the full flow' line 149.

From this same example we do allow user to set the variable. We do not have a builder method for eavery single field of the registration object. Would you like to have builder method for these 3 fields or all fields of registration ?

hrehman-sphereon commented 1 year ago

The builder than builds the complete tags

What does it mean ? we do not have field for every single language possible (cross product) and for every single field. So ... do we need to get rid of [x: string]: any and add fixed fields ? I hope this will be against the RFC as well as I don't think that's what you mean...

so, can you please explain ?