Feat/4086 anoncreds indy accum fix with endorsement by jamshale · Pull Request #4090 · openwallet-foundation/acapy

jamshale · 2026-03-16T22:02:13Z

Pull request overview

This PR refactors anoncreds revocation recovery by moving legacy Indy–specific recovery logic into the anoncreds/default/legacy_indy implementation, and adds an anoncreds-specific event path to trigger recovery when an endorsed revocation list update fails.

Changes:

Emit a new anoncreds event when endorsed revocation list updates fail, and add a legacy-indy event subscriber to trigger recovery/retry logic.
Move revocation recovery implementation code into anoncreds/default/legacy_indy/recover.py, leaving only shared exception(s) in anoncreds/revocation/recover.py.
Update tests and registry code to use the new recovery entry points.

jamshale · 2026-03-16T22:04:58Z

I think this is getting pretty close. I originally thought I could do this easier but it ended up being quite a bit of refactoring.

I can add more unit tests for the retry logic and there's one other thing I want to go over.

jamshale · 2026-03-17T20:14:44Z

I found another problem when testing self endorsement. The new event driven anoncreds revocation module was swallowing the exception from the update transaction. This prevents that from happening and then emits the same event as the failed endorsement transaction to get the flow working correctly.

I'd still like to add a bit more unit test coverage....

jamshale · 2026-03-17T22:07:15Z

This could be reviewed now. I've been focusing mostly on manually testing this because you need to create a ledger error for it to occur.

I'll try and get a couple more unit tests in to boost it up.

jamshale · 2026-03-18T16:12:21Z

I think this probably has enough unit testing now. Most of the missing code is exception paths and the main fix and retry function which would require a lot of mocking and the individual functions are unit tested.

jamshale · 2026-03-18T19:21:47Z

-            rev_list.rev_reg_def_id,
-            txn_record_type=GET_REVOC_REG_ENTRY,
-        )
+        ledger = profile.inject_or(BaseLedger)


I still don't really understand this yet but this solves the unknown did problem with multi-tenant and multi-ledger scenarios. Apparently you're only supposed to use IndyLedgerRequestsExecutor when reading from the ledger?

I'll keep looking for a bit if there's a reasonable way to fix this pattern. It's very easy to create bugs.

esune

Changes look good from the little I know about anoncreds/revocation. There's a couple test failures that need attention, other than that I think it's good.

…o legacy_indy plugin Signed-off-by: jamshale <jamiehalebc@gmail.com>

Signed-off-by: jamshale <jamiehalebc@gmail.com>

jamshale · 2026-03-19T00:54:06Z

Tests fixed now. The last small change needed updated mocking.

PatStLouis

I think this looks good. I won't put this as a blocker, but I think there could probably be some more comments in the code and these functions to capture the flow and how things work together for future maintainers while its fresh in memory. This is a complex sequence of operations.

PatStLouis · 2026-03-19T12:50:22Z

-async def generate_ledger_rrrecovery_txn(genesis_txns: str, rev_list: RevList) -> dict:
-    """Generate a new ledger accum entry, using the wallet value if revocations ahead of ledger."""  # noqa: E501
-    registry_from_ledger, prev_revoked = await fetch_txns(
+async def generate_ledger_rrrecovery_txn(genesis_txns: str, rev_list: RevList):


can we fix this function name spelling?

I thought this was a deliberate name for something like revocation registry recovery transaction, but if it is not I agree we should fix it

This was just copied straight from the existing implementation. But, yes I might as well change it with the rest of the refactor.

PatStLouis · 2026-03-19T12:52:00Z

-    registry_from_ledger, prev_revoked = await fetch_txns(
+async def generate_ledger_rrrecovery_txn(genesis_txns: str, rev_list: RevList):
+    """Generate a new ledger accum entry, based on wallet vs ledger revocation state."""
+    new_delta = None


this variable seems pointless? its only being set to none and returned as none?

PatStLouis · 2026-03-19T12:57:51Z

+    return endorser_did, connection_record
+
+
+async def _track_retry(cache: BaseCache, accum: str):


I'm confused by this function, is the accum the retry value? what's the logic behind this?

jamshale · 2026-03-19T15:56:56Z

I think this looks good. I won't put this as a blocker, but I think there could probably be some more comments in the code and these functions to capture the flow and how things work together for future maintainers while its fresh in memory. This is a complex sequence of operations.

I'll do a round of commenting... I agree it's a complicated sequence of events and there's 2 separate flows for self endorsement and using an endorser which makes it quite confusing.

esune · 2026-03-19T16:14:20Z

I think this looks good. I won't put this as a blocker, but I think there could probably be some more comments in the code and these functions to capture the flow and how things work together for future maintainers while its fresh in memory. This is a complex sequence of operations.

I'll do a round of commenting... I agree it's a complicated sequence of events and there's 2 separate flows for self endorsement and using an endorser which makes it quite confusing.

For the future: I know endorsement is pretty much baked into some of the core functionality, as a longer-term strategy we could consider refactoring it and extracting it to a plugin - using an event-driven pattern.

jamshale · 2026-03-19T16:22:36Z

For the future: I know endorsement is pretty much baked into some of the core functionality, as a longer-term strategy we could consider refactoring it and extracting it to a plugin - using an event-driven pattern.

I think the endorsement protocol and module is so indy specific it should be taken out as a wider legacy_indy plugin. This would include all of the indy and ledger code and the anoncreds plugin. This would be a large plugin but would make using legacy indy way simpler and would make acapy core way simpler. I was working on this for a while after Christmas and there's some challenges but I think it's doable. I don't see a big benefit in just moving the endorsement protocol itself into a plugin.

Signed-off-by: jamshale <jamiehalebc@gmail.com>

sonarqubecloud · 2026-03-19T17:46:32Z

Quality Gate failed

Failed conditions
75.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

…ation#4090) * feat: Trigger revocation recovery with endorsement / Move indy code to legacy_indy plugin Signed-off-by: jamshale <jamiehalebc@gmail.com> * fix: Co-pilot suggestions Signed-off-by: jamshale <jamiehalebc@gmail.com> * fix: Better session management and more null object handling Signed-off-by: jamshale <jamiehalebc@gmail.com> * chore: Remove indy specific usage from anoncreds manager Signed-off-by: jamshale <jamiehalebc@gmail.com> * Some fixes and hardening Signed-off-by: jamshale <jamiehalebc@gmail.com> * Remove comment Signed-off-by: jamshale <jamiehalebc@gmail.com> * Refactor / Add self endorsment support / Unit tests Signed-off-by: jamshale <jamiehalebc@gmail.com> * fix: Use old ruff version to format Signed-off-by: jamshale <jamiehalebc@gmail.com> * Small fixes Signed-off-by: jamshale <jamiehalebc@gmail.com> * Add a bit more unit test coverage Signed-off-by: jamshale <jamiehalebc@gmail.com> * bit more test coverage Signed-off-by: jamshale <jamiehalebc@gmail.com> * Add another test Signed-off-by: jamshale <jamiehalebc@gmail.com> * fix: Don't use IndyLedgerRequestsExecutor when posting to ledger Signed-off-by: jamshale <jamiehalebc@gmail.com> * Fix unit tests Signed-off-by: jamshale <jamiehalebc@gmail.com> * Refactor and additional commenting Signed-off-by: jamshale <jamiehalebc@gmail.com> * Fix mocks Signed-off-by: jamshale <jamiehalebc@gmail.com> --------- Signed-off-by: jamshale <jamiehalebc@gmail.com>

swcurran · 2026-03-24T14:47:37Z

+                revocation_list=[1, 0, 0, 0],
+            ),
+            recovery_txn={"value": "txn"},
+            endorser_did=None,


@jamshale -- I'm comparing the LTS PR to this PR and saw these warnings in here from SonarCloud about type issues. I'm assuming you saw them, but just wanted to be sure.

jamshale linked an issue Mar 16, 2026 that may be closed by this pull request

Anoncreds Indy - Accumulator fix with endorsement #4086

Closed

jamshale force-pushed the feat/4086-anoncreds-indy-accum-fix-with-endorsement branch from 72e9d3b to 69b9a07 Compare March 17, 2026 19:52

jamshale requested review from PatStLouis and esune March 17, 2026 22:05

jamshale force-pushed the feat/4086-anoncreds-indy-accum-fix-with-endorsement branch from 104a750 to c288588 Compare March 18, 2026 19:17

jamshale commented Mar 18, 2026

View reviewed changes

esune reviewed Mar 19, 2026

View reviewed changes

jamshale added 14 commits March 19, 2026 00:40

feat: Trigger revocation recovery with endorsement / Move indy code t…

62ab6dc

…o legacy_indy plugin Signed-off-by: jamshale <jamiehalebc@gmail.com>

fix: Co-pilot suggestions

ce8e5dc

Signed-off-by: jamshale <jamiehalebc@gmail.com>

fix: Better session management and more null object handling

04ffcfd

Signed-off-by: jamshale <jamiehalebc@gmail.com>

chore: Remove indy specific usage from anoncreds manager

ff4afe5

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Some fixes and hardening

dcaf70f

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Remove comment

58aa2df

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Refactor / Add self endorsment support / Unit tests

482d755

Signed-off-by: jamshale <jamiehalebc@gmail.com>

fix: Use old ruff version to format

7f852c2

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Small fixes

c891df6

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Add a bit more unit test coverage

280d4bb

Signed-off-by: jamshale <jamiehalebc@gmail.com>

bit more test coverage

014966f

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Add another test

e1566a2

Signed-off-by: jamshale <jamiehalebc@gmail.com>

fix: Don't use IndyLedgerRequestsExecutor when posting to ledger

9e8b823

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Fix unit tests

ac9fa9e

Signed-off-by: jamshale <jamiehalebc@gmail.com>

jamshale force-pushed the feat/4086-anoncreds-indy-accum-fix-with-endorsement branch from 9cf8338 to ac9fa9e Compare March 19, 2026 00:40

PatStLouis reviewed Mar 19, 2026

View reviewed changes

jamshale added 2 commits March 19, 2026 17:28

Refactor and additional commenting

72c2886

Signed-off-by: jamshale <jamiehalebc@gmail.com>

Fix mocks

de51ef9

Signed-off-by: jamshale <jamiehalebc@gmail.com>

PatStLouis approved these changes Mar 20, 2026

View reviewed changes

jamshale merged commit 5c18edc into openwallet-foundation:main Mar 20, 2026
11 of 12 checks passed

swcurran reviewed Mar 24, 2026

View reviewed changes

		return endorser_did, connection_record


		async def _track_retry(cache: BaseCache, accum: str):

Conversation

jamshale commented Mar 16, 2026

Pull request overview

Uh oh!

jamshale commented Mar 16, 2026

Uh oh!

jamshale commented Mar 17, 2026

Uh oh!

jamshale commented Mar 17, 2026

Uh oh!

jamshale commented Mar 18, 2026

Uh oh!

jamshale Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

esune left a comment

Choose a reason for hiding this comment

Uh oh!

jamshale commented Mar 19, 2026

Uh oh!

PatStLouis left a comment

Choose a reason for hiding this comment

Uh oh!

PatStLouis Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

esune Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

jamshale Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

PatStLouis Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

PatStLouis Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

jamshale commented Mar 19, 2026

Uh oh!

esune commented Mar 19, 2026

Uh oh!

jamshale commented Mar 19, 2026

Uh oh!

sonarqubecloud Bot commented Mar 19, 2026

Quality Gate failed

Uh oh!

Uh oh!

swcurran Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants