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

Fixes a few AATH failures #1897

Merged
merged 5 commits into from
Aug 16, 2022
Merged

Fixes a few AATH failures #1897

merged 5 commits into from
Aug 16, 2022

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Aug 12, 2022

Fixes:

  • one 023-did-exchange error
  • 0434-oob errors
  • one 0183-revocation error

ianco added 2 commits August 12, 2022 09:20
Signed-off-by: Ian Costanzo <[email protected]>
@ianco ianco requested a review from swcurran August 12, 2022 18:46
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #1897 (331ff16) into main (d407c48) will decrease coverage by 0.03%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##             main    #1897      +/-   ##
==========================================
- Coverage   93.79%   93.76%   -0.04%     
==========================================
  Files         539      539              
  Lines       34084    34105      +21     
==========================================
+ Hits        31970    31978       +8     
- Misses       2114     2127      +13     

@swcurran
Copy link
Contributor

Helpful to know which ones are fixed :-). I'll check it out on main and this branch.

Signed-off-by: Ian Costanzo <[email protected]>
@@ -424,7 +424,8 @@ async def revoke(request: web.BaseRequest):
body["notify"] = body.get("notify", context.settings.get("revocation.notify"))
notify = body.get("notify")
connection_id = body.get("connection_id")
notify_version = body.get("notify_version", "v1_0")
body["notify_version"] = body.get("notify_version", "v1_0")
notify_version = body["notify_version"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question about this change - basically it changes aca-py behaviour to always send a revocation notification if the startup option is set (--notify-revocation) vs only sending if a notify_version is explicitly set in the revocation payload.

I assume that if you are notifying then you always want to notify.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that is right. And presumably if notify_version is not set, it should default to something -- e.g. the latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would think that is right. And presumably if notify_version is not set, it should default to something -- e.g. the latest version?

v1_0 is the default (it was the default before however the value wasn't getting passed through to the actual notification code so no notification was getting sent)

@ianco ianco merged commit 5df3bc8 into openwallet-foundation:main Aug 16, 2022
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.

4 participants