Issue with the current flow of registering anoncreds objects with an external endorser

[berendsliedrecht]

Currently, it is expected to register AnonCreds objects (using an external endorser) with the following code:

      // t = a tenant on the agent

      const { credentialDefinitionState } =
        await t.modules.anoncreds.registerCredentialDefinition<IndyVdrRegisterCredentialDefinitionOptions>(
          {
            credentialDefinition: {
              issuerId: issuerDid,
              schemaId,
              tag,
            },
            options: {
              endorserMode: 'external',
              endorserDid,
              supportRevocation: supportsRevocation,
            },
          },
        );

      if (
        credentialDefinitionState.state !== 'action' ||
        credentialDefinitionState.action !== 'endorseIndyTransaction'
      ) {
        throw new Error(
          `Error registering credentialDefinition: ${
            credentialDefinitionState.state === 'failed'
              ? credentialDefinitionState.reason
              : 'Not Finished'
          }`,
        );
      }

      const signedcredentialDefinitionRequest =
        await this.agentService.endorseTransaction(
          // eslint-disable-next-line @typescript-eslint/ban-ts-comment
          // @ts-expect-error
          credentialDefinitionState.credentialDefinitionRequest,
          endorserDid,
        );

      const credentialDefinitionSubmitResult =
        await t.modules.anoncreds.registerCredentialDefinition<IndyVdrRegisterCredentialDefinitionOptions>(
          {
            options: {
              endorsedTransaction: signedcredentialDefinitionRequest,
              endorserMode: 'external',
              supportRevocation: supportsRevocation,
            },
            credentialDefinition:
              credentialDefinitionState.credentialDefinition,
          },
        );

      if (
        credentialDefinitionSubmitResult.credentialDefinitionState.state !==
        'finished'
      ) {
        throw Error(
          `Error while registering credentialDefinition. Cause: ${JSON.stringify(credentialDefinitionSubmitResult)}`,
        );
      }

      return {
        credentialDefinitionId:
          credentialDefinitionState.credentialDefinitionId,
        ...credentialDefinitionState.credentialDefinition,
      };

In general, for all objects apart from the revocation registry definition, this works "fine". However, when registering the revocation registry definition it has some issues.

1. The anoncreds.registerRevocationRegistryDefinition will be called twice (once to create the transaction and once to actually submit the transaction after it being signed). This is causes anoncreds-rs to be called twice to create the revocation registry definition and also uploads it twice to the tails service. In the registerRevocationRegistryDefinition and registerCredentialDefinition there are two checks that only store the private parts when the state is finished which is fine for now, but something that I would prefer to avoid.
2. Another issue is that the tailsFileLocation value is being reused for the localPath, as provided by anoncreds-rs and for the URL later on. This causes an issue as the uploadTailsFile method returns the URL, but you do NOT want to upload it when the transaction is not yet finished. BUT, we must have the URL in order to create the final transaction, so we are in a chicken-and-egg loop.
3. Currently when this flow is done (create a rev reg def with an external endorser), it will not work when the holder receives the credential. I believe this is because we generate another rev reg def with the second anoncreds.registerRevocationRegistryDefinition call and it will be different data due to the first call being used for the actual transaction (I hope this is in some way clear...).

I have discussed this for a bit with @karimStekelenburg and we came to two conclusions but I am very open to hear other solutions for this.

Separate the create TXN and publish functions

Right now we have the anoncreds.registerSchema which must be called twice with slightly different input and some code will run twice which is not needed. We could split it up into two methods: anoncreds.createSchema and anoncreds.uploadSchema. anoncreds.registerSchema could still exist along side these other two methods for convience (or just when using an internal endorser).

The downside of this is that we will introduce 8 additional methods on the anoncreds public API which will make explaining which one you need rather difficult. However, the current API is already complex to me as well with the additional options. You only get to see them once you know the exactly what you have to do (i.e. use the IndyVdrAnonCreds...Options generic).

Have an option that just creates the TXN and expose a submitRequest on the indyVdr module

This is my current solution for the problem and it makes the API quite a bit easier. The anoncreds.registerSchema will stay the same, but instead of calling it twice, it will only have to be called once. Like so:

      const { revocationRegistryDefinitionState } =
        await t.modules.anoncreds.registerRevocationRegistryDefinition({
          options: {
            endorserMode: 'external',
            endorserDid,
          },
          revocationRegistryDefinition: {
            maximumCredentialNumber,
            credentialDefinitionId,
            tag,
            issuerId: issuerDid,
          },
        });

      if (
        revocationRegistryDefinitionState.state !== 'action' ||
        revocationRegistryDefinitionState.action !== 'endorseIndyTransaction'
      ) {
        throw new Error(
          `Error registering revocation registry definition: ${
            revocationRegistryDefinitionState.state === 'failed'
              ? revocationRegistryDefinitionState.reason
              : 'Not Finished'
          }`,
        );
      }

      const signedRevocationRegistryDefinitionRequest =
        await this.agentService.endorseTransaction(
          // eslint-disable-next-line @typescript-eslint/ban-ts-comment
          // @ts-expect-error
          revocationRegistryDefinitionState.revocationRegistryDefinitionRequest,
          endorserDid,
        );

      await t.modules.indyVdr.submitTransaction(
        signedRevocationRegistryDefinitionRequest,
        issuerDid,
      );

At the bottom there is a new method on the indyVdr module which is submitTransaction, which just takes the signed transaction from the endorser and submits this. An issue with this, is that when the submission fails the wallet already has the private parts for the cred def or rev reg def in the wallet and the tails file is already uploaded. The upside of this is is that it will work, avoid double API calls with different context and does not expose 8 new methods on the anoncreds API.

I understand partly why this API design was chosen, due to this being more of an indyVdr issue and not an AnonCreds issue per se. Right now it does not work nicely when combining anoncreds and indyVdr, which is the most common combination of credential format and registry I'd say.

Feel free to ask any questions regarding this ramble, there is a chance I skipped over some of the details of the issue so I'd be happy to clear that up :).

[TimoGlastra]

The anoncreds.registerRevocationRegistryDefinition will be called twice (once to create the transaction and once to actually submit the transaction after it being signed)

Hmm if the revocation registry definition is already passed, it shouldn't be created another time, we should just use the already existing revocation registry definition. Could we tweak that?

I think for e.g. createCredentialDefinition it is also only called once right? First time the cred def is created, the second time it is submitted, but we don't create two cred defs?

Another issue is that the tailsFileLocation value is being reused for the localPath, as provided by anoncreds-rs and for the URL later on. This causes an issue as the uploadTailsFile method returns the URL, but you do NOT want to upload it when the transaction is not yet finished. BUT, we must have the URL in order to create the final transaction, so we are in a chicken-and-egg loop.

I think it's OK to upload the tails file already before the transaction is completed. Storage is cheap, do we expect this flow to fail very often? Could even do a clean up separately to see if some tails files were never finished and remove them afterwards.

We could add something like getTailsUrl to the tails file service, but this will require URLs to be consistent (so the same tails file should always return the same URL, which is possible if you use e.g. the hash, but not if you want to use UUIDs)

Currently when this flow is done (create a rev reg def with an external endorser), it will not work when the holder receives the credential. I believe this is because we generate another rev reg def with the second anoncreds.registerRevocationRegistryDefinition call and it will be different data due to the first call being used for the actual transaction (I hope this is in some way clear...).

Yes that's bad and shouldn't happen. We should only call registerRevocationRegistryDefinition once and this is a bug in the implementation. As you can see with credential definition we check if the input is a full cred def, and if it is we skip the generation of the cred def. That's why external endorsement works fine for cred defs: https://github.com/openwallet-foundation/credo-ts/blob/d7c2bbb4fde57cdacbbf1ed40c6bd1423f7ab015/packages/anoncreds/src/AnonCredsApi.ts#L253-L255. @auer-martin has run into the same problem and this is how we ended up fixing it. I think we should take the same approach here.

Separate the create TXN and publish functions

I'm open to do this, but we should think about the possible flows a bit beforehand. Different ledgers work differently and we should optimize this API just for Indy external endorsement. Still, if we use the new create and publish functions the options will differ based on the ledger you use, thus still requiring some interfaces and maybe confusing overly generic options.

Have an option that just creates the TXN and expose a submitRequest on the indyVdr module

I'd prefer this approach over the seprate create TXN and publish function on the AnonCreds API. I think it could work. The reason why we didn't go with it is because then we'd already have to store the anoncreds object when you call register the first time, you would be able to use it locally, but then it will fail as it's not registered on the ledger. But I think that's a fair trade-off and probably easier to understand (= if you use external endorsment, you need to make sure that the transaction is endorsed and submitted manually)

For indy vdr it would work, for some other ones we can't be 100% sure. It could be that an identifier is generated by the ledger and thus we can't be certain that if state = action, we can already store the credential definition/schema/revocation registry.

Any thoughts on that?

[TimoGlastra]

I think for 0.5 we should just quickly fix it in the way we have done for cred defs using this method: https://github.com/openwallet-foundation/credo-ts/blob/d7c2bbb4fde57cdacbbf1ed40c6bd1423f7ab015/packages/anoncreds/src/AnonCredsApi.ts#L253-L255, and make sure the private parts are stored already also.

That way it'll at least work, and then we can look at making some big breaking changes refactors of the API to make it simpler (as I agree it's a shit show)

[berendsliedrecht]

Thanks for the elaborate response @TimoGlastra!

I think for 0.5 we should just quickly fix it in the way we have done for cred defs using this method:

Yes, I think that will suffice for now.

We could add something like getTailsUrl to the tails file service, but this will require URLs to be consistent (so the same tails file should always return the same URL, which is possible if you use e.g. the hash, but not if you want to use UUIDs)

This was something I started to implement, but ran into the exact issue you just described, so I did not finish it.

For indy vdr it would work, for some other ones we can't be 100% sure. It could be that an identifier is generated by the ledger and thus we can't be certain that if state = action, we can already store the credential definition/schema/revocation registry.

Yeah that is a difficult one. That was also my main issue with my proposed solution, it is fully focussed on indy vdr.