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 revocation notification after list entry written to ledger #2812

Merged

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Feb 22, 2024

This fixes the issue with the revocation notification not happening.

There was a couple things that didn't make sense.

  • For the /revoke endpoint if was never calling notify. It was only creating the notification record. This may have been deliberate (see comment), but needs to notify at some point, which it isn't doing.
  • For the /publish-revocations endpoint it was not calling notify when there was a connection_id, but this is only used for explicitly using a specific endorser connection. When in author and preconfigured mode, this parameter isn't even used. I think it was trying to prevent the notify from happening for endorsement scenarios. (see next comment)

comment: right now the revocation notification is happening after requesting endorsement but before the new list entry has been written to the ledger. This is the same for anoncreds. I'm not sure if this is what we actually want. It kinda makes sense to have it immediately when the issuer revokes, but it also makes sense not to notify until after the endorsement has happened and the entry has been written.

Tested with traction and bc wallet.

Still testing some scenarios.

@jamshale jamshale linked an issue Feb 22, 2024 that may be closed by this pull request
@jamshale jamshale marked this pull request as ready for review February 22, 2024 20:42
@jamshale jamshale requested review from swcurran, ianco and esune February 22, 2024 20:47
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

As noted by @jamshale , the notification is triggered before the entry is written to the ledger. I don't think this is correct, as it could lead to scenarios where the notification is sent to the holder, however the credential is technically still valid (revocation checks would return a success) until the entry is actually posted.

@jamshale
Copy link
Contributor Author

☝️ That was what I was thinking. It shouldn't be difficult to change, but I'll wait for a bit any additional feedback before changing it.

@esune
Copy link
Member

esune commented Feb 22, 2024

☝️ That was what I was thinking. It shouldn't be difficult to change, but I'll wait for a bit any additional feedback before changing it.

Sounds good. If @ianco @andrewwhitehead @swcurran want to chime in about this as well we can come up to a consensus and move forward.

@ianco
Copy link
Contributor

ianco commented Feb 22, 2024

Agree with @esune the notification shouldn't happen until after the ledger entry is written, which in the endorser scenario is after the endorsed transaction is received and written. Not sure how this was implemented for this transaction type, but for other scenarios this kind of functionality is in the event handler for whatever event is emitted by the TransactionManager,

(
The ledger is updated here: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/endorse_transaction/v1_0/manager.py#L437

... and then the event is emitted here: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/endorse_transaction/v1_0/manager.py#L772
)

@esune
Copy link
Member

esune commented Feb 22, 2024

Thanks @ianco. I wonder if, when it was first implemented, it was relying on the (now deprecated) endorse-and-write request pattern - either way, this is a good opportunity to clean things up.

@swcurran
Copy link
Contributor

My $0.02CDN — it’s been a question/dilemna for a bit, and we’ve had questions from folks like IDIM about that. I didn’t know exactly when the “notification” occurred.

There is the revoke event (action to mark a credential as pending revocation for a specific credential) and the revocation publish event (batch of one or more pending revocations published to a ledger) There are reasonable arguments both ways on whether the notification should be sent on revoke or publish. The sooner the notification goes the better, but if we notify on publish and then publish a lot to get the notification out sooner, we get a lot more transactions on the ledger.

I think we should probably move the notification should be moved to the end of the publish event, which will likely mean more publishing events so that the notifications go out more frequently.

We also have to make sure that even when we have a combined “revoke and publish”, that the same actions are being performed as when we “revoke” and then sometime later “publish”. The combined is just a convenience.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jamshale
Copy link
Contributor Author

Updated both revocation branches to send the revoke notification after the list is written to the ledger. I think this addresses all the comments.

Will do another pass of manual testing.

@swcurran swcurran merged commit 9f5dda2 into openwallet-foundation:main Feb 23, 2024
7 of 8 checks passed
@swcurran
Copy link
Contributor

Can you please update the PR title to better reflect the change? I'll highlight the change in the changelog. We should check the docs to make sure this is clear -- I didn't know before when notification occurred.

@jamshale
Copy link
Contributor Author

Can you please update the PR title to better reflect the change? I'll highlight the change in the changelog. We should check the docs to make sure this is clear -- I didn't know before when notification occurred.

Yes I will.

I have to check out some failing integration tests too. Wasn't expecting it to get merged so fast. Not sure why they're failing yet.

@jamshale jamshale changed the title Fix - missing revocation notification Create revocation notification after list entry written to ledger Feb 23, 2024
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.

Revocation notification not being triggered
4 participants