Anoncreds schema endorsement #2715

jamshale · 2024-01-16T22:51:49Z

Adds the flow for schema endorsement.

The endorsement flow is triggered by the agent having an author role or by manually setting the create_transaction_for_endorser option request param.

The anoncreds registry submits the transaction based on wallet type, and the post processing (storing the schema in the wallet) is triggered by the transaction manager using a new anoncreds schema event.

The endorsement integration tests are run with anoncreds wallets on the endorser and author roles. Added unit tests mostly on the registry register schema flow.

ianco · 2024-01-18T17:52:11Z

Overall looks good.

I think we need to move where the ledger write actually happens, tho. Right now the transaction manager writes to the ledger (https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/endorse_transaction/v1_0/manager.py#L428) and then emits the event that we listen for (https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/endorse_transaction/v1_0/manager.py#L526).

For anoncreds, I think the ledger write needs to move into our new (anoncreds) event handler method.

jamshale · 2024-01-18T18:02:44Z

@ianco Ok, thanks. I will look into doing that, and add some unit tests and ping you when I'm finished.

ianco

looks good!

dbluhm

Super quick comment, will come back with a more detailed review.

aries_cloudagent/anoncreds/default/did_web/registry.py

jamshale · 2024-01-23T23:38:03Z

The latest commit removes the extending of the registry interface, and instead implements the txn_submit method from within the legacy_indy registry. Author triggered txn_submits in the indy ledger and endorser manager complete_transaction create an instance of the LegacyIndyRegistry to submit the transactions.

This will still end up submitting the transaction from the indy ledger but will make sure that ledger is injected from the anoncreds registry.

One drawback is that because the transaction manager and ledger are imported from the ledger routes module on initialization, importing the LegacyIndyRegistry from the top level caused a circular import exception. To get around this the Registry is only imported when needed during anoncred transaction submit operations.

dbluhm

Apologies for taking so long to look deeper on this PR; I feel bad about making some comments early, requesting changes, and then requesting some more after you made those changes... But I think we're not quite aligned on vision for the endorser support yet.

Happy to discuss these points more.

aries_cloudagent/anoncreds/routes.py

aries_cloudagent/anoncreds/issuer.py

aries_cloudagent/anoncreds/events.py

aries_cloudagent/anoncreds/default/legacy_indy/registry.py

dbluhm · 2024-01-24T18:43:26Z

I fear I'm coming across as harsh but I think there were some mistakes made in the first iteration of endorser support in ACA-Py that really need to be corrected. As it stands, I think we're inheriting a bit too much of those mistakes in this iteration.

ianco

Not completely happy with how the endorsed transaction is getting saved ... I don't like the (circular) dependency between the transaction manager and Indy registry, and would prefer the ledger transaction gets saved in the event handler somehow (limits the specific ledger dependencies) but let's accept this PR and then re-visit in the next endorser PR.

jamshale · 2024-01-24T19:19:28Z

I fear I'm coming across as harsh but I think there were some mistakes made in the first iteration of endorser support in ACA-Py that really need to be corrected. As it stands, I think we're inheriting a bit too much of those mistakes in this iteration.

I think that's fair. A lot of this is new to me so I appreciate the feedback. Some of the things you mentioned I had been confused about. I don't think it should take to long.

jamshale · 2024-01-25T19:14:10Z

I think I'm getting a grip on the old implementation and how we want it to change with anoncreds. This should be way better now.

One thing I noticed is that the GET /schema/{id} endpoint doesn't return the state, so you can't see it's finished though the api even though the record is updated. Not sure if that should be change as another ticket.

I added a state for transaction_requested and I used a random uuid for the job-id. Wasn't sure if this was exactly what we wanted to do.

ianco · 2024-01-29T23:57:56Z

One thing I noticed is that the GET /schema/{id} endpoint doesn't return the state, so you can't see it's finished though the api even though the record is updated. Not sure if that should be change as another ticket.

Isn't there a ledger transaction id that isn't available until the schema is written to the ledger?

jamshale · 2024-01-30T00:01:24Z

One thing I noticed is that the GET /schema/{id} endpoint doesn't return the state, so you can't see it's finished though the api even though the record is updated. Not sure if that should be change as another ticket.

Isn't there a ledger transaction id that isn't available until the schema is written to the ledger?

That should still be there, yes. This is just for the anoncreds Result object. It has a state field I am setting to transaction_requested and then finished when it is written to the ledger. You can't ever see the finished state from the current GET api. Only the inner anoncreds object.

jamshale · 2024-01-30T00:16:52Z

POST /schema

{
  "job_id": "db16fd5f8c094d62ba3d1722dacb2467",
  "schema_state": {
    "state": "transaction_requested",
    "schema_id": "WgWxqztrNooG92RXvxSTWv:2:author-schema:1.1",
    "schema": {
      "issuerId": "WgWxqztrNooG92RXvxSTWv",
      "attrNames": [
        "score"
      ],
      "name": "author-schema",
      "version": "1.1"
    }
  },
  "registration_metadata": {
    "txn": {
      "state": "request_sent",
      "created_at": "2024-01-30T00:08:21.960157Z",
      "updated_at": "2024-01-30T00:08:21.968250Z",
      "trace": false,
      "transaction_id": "7e664eb2-2ed0-460e-8411-b6858aeb2fa2",
      "_type": "https://didcomm.org/sign-attachment/1.0/signature-request",
      "signature_request": [
        {
          "context": "did:sov",
          "method": "add-signature",
          "signature_type": "default",
          "signer_goal_code": "aries.transaction.endorse",
          "author_goal_code": "aries.transaction.ledger.write"
        }
      ],
      "signature_response": [],
      "timing": {
        "expires_time": null
      },
      "formats": [
        {
          "attach_id": "4cef3a8f-b910-4794-9326-6f0a638d683c",
          "format": "dif/endorse-transaction/[email protected]"
        }
      ],
      "messages_attach": [
        {
          "@id": "4cef3a8f-b910-4794-9326-6f0a638d683c",
          "mime-type": "application/json",
          "data": {
            "json": "{\"endorser\": \"6bX4vukUZxxGKm7f5bBkmN\", \"identifier\": \"FB5yHWKaZk59hiKqjJKEHs\", \"operation\": {\"data\": {\"attr_names\": [\"score\"], \"name\": \"author-schema\", \"version\": \"1.1\"}, \"type\": \"101\"}, \"protocolVersion\": 2, \"reqId\": 1706573301954441235, \"signature\": \"5ihP9oXmgg6WRRMmNjHiiiP6W3r3S5xVfJFCru3BUzHSqiV6gkmRhvo3UgDzsTmQHU2Z4zp91hwU3cKHtLsG9xWA\"}"
          }
        }
      ],
      "meta_data": {
        "context": {
          "job_id": "db16fd5f8c094d62ba3d1722dacb2467",
          "schema_id": "WgWxqztrNooG92RXvxSTWv:2:author-schema:1.1"
        }
      },
      "connection_id": "2e64c27e-5c35-4090-87ef-a48e4db3f2ba"
    }
  }
}

GET /schems/{id}

{  
 "schema": {
    "issuerId": "FB5yHWKaZk59hiKqjJKEHs",
    "attrNames": [
      "score"
    ],
    "name": "author-schema",
    "version": "1.1"
  },
  "schema_id": "FB5yHWKaZk59hiKqjJKEHs:2:author-schema:1.1",
  "resolution_metadata": {
    "ledger_id": null
  },
  "schema_metadata": {
    "seqNo": 1234
  }
}

dbluhm

A few comments but I think this is still a great first set of changes. I'm okay with my comments being ignored in the interest of unblocking this from moving forward!

dbluhm · 2024-01-30T19:56:09Z

aries_cloudagent/anoncreds/default/legacy_indy/registry.py

            schema_state=SchemaState(
-                state=SchemaState.STATE_FINISHED,
+                state=SchemaState.STATE_TRANSACTION_REQUESTED,


This should be the generic STATE_WAIT.

👍 I had second guessed adding a more specific state here. Wait makes sense.

dbluhm · 2024-01-30T20:05:30Z

aries_cloudagent/anoncreds/routes.py

-        description="Connection identifier (optional) (this is an example)",
+        description="""
+            Connection identifier (optional) (this is an example)
+            You can set this is you know the endorsers connection id you want to use.
+            If not specified then the agent will attempt to find an endorser connection.
+        """,
        required=False,
        example=UUIDFour.EXAMPLE,
    )

+    create_transaction_for_endorser = fields.Bool(
+        description="""
+            Create transaction for endorser (optional, default false). 
+            Use this for agents who don't specify an author role but want to 
+            create a transaction for an endorser to sign.""",
+        required=False,
+        example=False,
+    )
+


I think this is a good use of options. However, I wonder if the first option might be better handled by having an established default endorser (kind of like how we set our public DID and that persists across calls). And I'm honestly not sure what the second one means; is this perhaps a case where we could simplify the interface by sticking to a reasonable default? In what circumstances would we set this value to True?

Yes. These were from the previous implementation.

I thought endorser_connection_id was so you could use a different endorser. It's not needed and the default endorser connection is found without it. Maybe we can remove it.

The create_transaction_for_endorser is when an agent want's to do a transaction but doesn't have the author role set. I'm not sure if this needs to be supported but the integration tests use them.

I think removing these options if we can should be done later as another smaller PR. I'll leave them for now.

dbluhm · 2024-01-30T20:17:09Z

aries_cloudagent/protocols/endorse_transaction/v1_0/manager.py

+            if self._profile.settings.get("wallet.type") == "askar-anoncreds":
+                event_bus = self.profile.inject(EventBus)
+                await event_bus.notify(
+                    self._profile,
+                    SchemaRegistrationFinishedEvent.with_payload(meta_data),
+                )
+            else:
+                await notify_schema_event(self._profile, schema_id, meta_data)


What do you think of just calling AnonCredsIssuer.finish_schema directly?

I think that's a good idea. I was just following the existing pattern here but that would be simpler.

jamshale · 2024-01-30T20:54:51Z

A few comments but I think this is still a great first set of changes. I'm okay with my comments being ignored in the interest of unblocking this from moving forward!

I'll update it. I'd rather address them here before the next PR which will be a lot bigger.

Signed-off-by: jamshale <[email protected]>

sonarqubecloud · 2024-01-30T22:31:41Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

jamshale · 2024-01-30T23:15:03Z

@dbluhm Addressed the comments now.

jamshale requested a review from ianco January 16, 2024 23:48

jamshale force-pushed the main branch from 644da3c to 98d0047 Compare January 18, 2024 19:05

jamshale marked this pull request as ready for review January 19, 2024 18:34

jamshale changed the title ~~WIP - Anoncreds schema endorsement~~ Anoncreds schema endorsement Jan 19, 2024

jamshale force-pushed the main branch from 3bf2717 to f566004 Compare January 22, 2024 16:39

jamshale requested a review from dbluhm January 22, 2024 16:54

ianco previously approved these changes Jan 22, 2024

View reviewed changes

dbluhm requested changes Jan 22, 2024

View reviewed changes

aries_cloudagent/anoncreds/default/did_web/registry.py Outdated Show resolved Hide resolved

jamshale dismissed ianco’s stale review via 6e0cc98 January 23, 2024 22:53

jamshale force-pushed the main branch from f566004 to 6e0cc98 Compare January 23, 2024 22:53

jamshale force-pushed the main branch from 6e0cc98 to 057f425 Compare January 24, 2024 17:07

dbluhm requested changes Jan 24, 2024

View reviewed changes

ianco previously approved these changes Jan 24, 2024

View reviewed changes

jamshale dismissed ianco’s stale review via 2a02d09 January 25, 2024 18:58

jamshale force-pushed the main branch from 6c01bac to 2a02d09 Compare January 25, 2024 18:58

jamshale force-pushed the main branch from ccfb286 to 6e9ab07 Compare January 29, 2024 23:51

jamshale requested a review from dbluhm January 29, 2024 23:55

jamshale force-pushed the main branch from 6e9ab07 to 84db9f2 Compare January 30, 2024 00:39

dbluhm previously approved these changes Jan 30, 2024

View reviewed changes

Anoncreds - schema endorsement

15e65c6

Signed-off-by: jamshale <[email protected]>

Use wait state / finish schem directly with anoncreds issuer

74b3830

Signed-off-by: jamshale <[email protected]>

jamshale dismissed dbluhm’s stale review via 74b3830 January 30, 2024 22:24

jamshale force-pushed the main branch from ed3760b to 74b3830 Compare January 30, 2024 22:24

Fix test

3a16212

Signed-off-by: jamshale <[email protected]>

dbluhm approved these changes Jan 30, 2024

View reviewed changes

jamshale merged commit 670db01 into openwallet-foundation:main Jan 31, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anoncreds schema endorsement #2715

Anoncreds schema endorsement #2715

jamshale commented Jan 16, 2024 •

edited

Loading

ianco commented Jan 18, 2024

jamshale commented Jan 18, 2024

ianco left a comment

dbluhm left a comment

jamshale commented Jan 23, 2024

dbluhm left a comment

dbluhm commented Jan 24, 2024

ianco left a comment

jamshale commented Jan 24, 2024

jamshale commented Jan 25, 2024 •

edited

Loading

ianco commented Jan 29, 2024

jamshale commented Jan 30, 2024 •

edited

Loading

jamshale commented Jan 30, 2024 •

edited

Loading

dbluhm left a comment

dbluhm Jan 30, 2024

jamshale Jan 30, 2024

dbluhm Jan 30, 2024

jamshale Jan 30, 2024

dbluhm Jan 30, 2024

jamshale Jan 30, 2024

jamshale commented Jan 30, 2024

sonarqubecloud bot commented Jan 30, 2024

jamshale commented Jan 30, 2024

Anoncreds schema endorsement #2715

Anoncreds schema endorsement #2715

Conversation

jamshale commented Jan 16, 2024 • edited Loading

ianco commented Jan 18, 2024

jamshale commented Jan 18, 2024

ianco left a comment

Choose a reason for hiding this comment

dbluhm left a comment

Choose a reason for hiding this comment

jamshale commented Jan 23, 2024

dbluhm left a comment

Choose a reason for hiding this comment

dbluhm commented Jan 24, 2024

ianco left a comment

Choose a reason for hiding this comment

jamshale commented Jan 24, 2024

jamshale commented Jan 25, 2024 • edited Loading

ianco commented Jan 29, 2024

jamshale commented Jan 30, 2024 • edited Loading

jamshale commented Jan 30, 2024 • edited Loading

dbluhm left a comment

Choose a reason for hiding this comment

dbluhm Jan 30, 2024

Choose a reason for hiding this comment

jamshale Jan 30, 2024

Choose a reason for hiding this comment

dbluhm Jan 30, 2024

Choose a reason for hiding this comment

jamshale Jan 30, 2024

Choose a reason for hiding this comment

dbluhm Jan 30, 2024

Choose a reason for hiding this comment

jamshale Jan 30, 2024

Choose a reason for hiding this comment

jamshale commented Jan 30, 2024

sonarqubecloud bot commented Jan 30, 2024

Quality Gate passed

jamshale commented Jan 30, 2024

jamshale commented Jan 16, 2024 •

edited

Loading

jamshale commented Jan 25, 2024 •

edited

Loading

jamshale commented Jan 30, 2024 •

edited

Loading

jamshale commented Jan 30, 2024 •

edited

Loading