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

fix: offer creation improvements #159

Merged

Conversation

TimoGlastra
Copy link
Collaborator

Some improvements / cleanup to the credential offer creation process:

  • Take the input grants as leading, and only set default values if a required prop is not provided. Currently the grants objects are overwritten in some places, which means that a) you can't add custom properties and b) the authorization_server in e.g. the pre auth flow was removed when the offer was created
  • do not put the whole credential offer uri in the credential_offer_uri param, this was leading to weird behaviour where sometimes the value would be the whole uri openid-credential-offer://credential_offer_uri=<uri> and sometimes just <uri>. So now the object just contains the uri that hosts the credential offer, not the uri that is scanned by the wallet (it's quite confusing that the credential offer uri can contain a credential_offer_uri param, so it was somtimes interchanged).
  • Correctly encode the credential_offer_uri param (Fixes credential_offer_uri= hosted credential url should be uri encoded in the credential offer #102)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 74.19355% with 16 lines in your changes missing coverage. Please review.

Project coverage is 49.10%. Comparing base (56b16a3) to head (fa2b31b).
Report is 907 commits behind head on develop.

Files with missing lines Patch % Lines
...kages/issuer/lib/functions/CredentialOfferUtils.ts 77.77% 10 Missing ⚠️
packages/issuer/lib/VcIssuer.ts 64.70% 5 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (56b16a3) and HEAD (fa2b31b). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (56b16a3) HEAD (fa2b31b)
6 0
unittest 6 1
Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #159       +/-   ##
============================================
- Coverage    89.22%   49.10%   -40.12%     
============================================
  Files           17       74       +57     
  Lines          668     4975     +4307     
  Branches       170     1730     +1560     
============================================
+ Hits           596     2443     +1847     
- Misses          69     2529     +2460     
  Partials         3        3               
Flag Coverage Δ
unittest 49.10% <74.19%> (-40.12%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TimoGlastra
Copy link
Collaborator Author

Hey @nklomp do you have a chance to look at this PR this week?

Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We always use accolades to prevent bugs in case someone had a rough night ;)

packages/issuer/lib/functions/CredentialOfferUtils.ts Outdated Show resolved Hide resolved
packages/issuer/lib/functions/CredentialOfferUtils.ts Outdated Show resolved Hide resolved
}
}
return { credential_offer, credential_offer_uri, scheme, baseUri, grants: credential_offer.grants }
if (grants) credential_offer.grants = grants
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accolades

@nklomp
Copy link
Contributor

nklomp commented Oct 24, 2024

@TimoGlastra Small nudge, since you requested for these PRs to be merged

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra
Copy link
Collaborator Author

done

@nklomp nklomp merged commit f1a3b75 into Sphereon-Opensource:develop Oct 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

credential_offer_uri= hosted credential url should be uri encoded in the credential offer
2 participants