Fix: RevRegEntry Transaction Endorsement

[shaangill025]

• resolve Possible endorsement processing bug -- RevRegEntry transaction not being signed #2441

[swcurran]

Is this checking to see if there is an endorser connection? Is so, the error message should be more explicit, I think.

[shaangill025]

Yes, to check that the connection_id if provided when calling POST /revocation/revoke exists. If no connection_id is provided and the profile is author role then connection_id will be retrieved from endorser.endorser_alias which will always exist. Maybe I can change it to No endorser connection record .... ?

[esune]

Do these changes also account for the scenario where the revocation entry is not published contextually to the call to the /revocation/revoke endpoint, but rather using the /revocation/publish-revocations endpoint to publish in bulk?

[shaangill025]

Do these changes also account for the scenario where the revocation entry is not published contextually to the call to the /revocation/revoke endpoint, but rather using the /revocation/publish-revocations endpoint to publish in bulk?

Thanks for pointing this out, now it does.

[swcurran]

Not for this issue, but doesn’t the “revoke and publish” and the “publish” endpoints use a common “publish” method? I would think they would...

[shaangill025]

but doesn’t the “revoke and publish” and the “publish” endpoints use a common “publish” method?

• /revocation/revoke
  

  aries-cloudagent-python/aries_cloudagent/revocation/manager.py

  Line 150 in 2c901a9

  await issuer_rr_upd.send_entry(self._profile)

• /revocation/publish-revocations
  

  aries-cloudagent-python/aries_cloudagent/revocation/manager.py

  Line 242 in 2c901a9

  await issuer_rr_upd.send_entry(self._profile)

[dbluhm]

Any word on when we can look at getting the failing tests resolved?

[esune]

Any word on when we can look at getting the failing tests resolved?

@shaangill025 is looking into them, I'm following-up today to see where we are at

[andrewwhitehead]

It looks like the new unit tests were written before rebasing on some changes to the test framework. The fix seems to be just renaming 'async_mock' to 'mock' in test_manager.py.

[andrewwhitehead]

In the integration test it looks like Acme receives a notification that their credential was revoked, but when checking the status it hasn't been updated yet. Could be a timing issue or an unpublished update?

[swcurran]

Which is definitely related to the code being changed.

[esune]

In the integration test it looks like Acme receives a notification that their credential was revoked, but when checking the status it hasn't been updated yet. Could be a timing issue or an unpublished update?

That's what I was wondering. It looks like the workflow was re-run and now only 1 scenario is failing, which seems to be a good candidate for timing issues (but I admit I have not had a chance to look into the test code yet to be 100% sure).

[esune]

Changes look good to me. If @dbluhm can take a peek and confirm I did not miss anything I think it's good to go 👍🏻

[dbluhm]

I'm not thrilled about the continued propagation of endorser_conn_id and write_ledger through several interfaces but I think that's unavoidable until we have endorser support behind the ledger agnostic AnonCreds interface. LGTM.

[dbluhm]

Do we want to try to get this out in a patch release separate from the AnonCreds-RS changes being made on main (in addition to merging this to main)?

[esune]

Do we want to try to get this out in a patch release separate from the AnonCreds-RS changes being made on main (in addition to merging this to main)?

Yes, I think this was the strategy we decided to go with. Re-poinintg this to the 0.11.x branch and merging the result back to main might be the easiest way to go rather than cherry-picking commits, but either way will work.

The base branch was changed.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information