Add did:indy transaction version 2 support #3253
Conversation
jamshale
changed the title
|
The security alerts are nothing. http used in the scenario tests. Not sure how to ignore it yet. |
|
I like what you're doing in this PR -- really appreciate the wallet startup cleanup as well. Question: how do we get the DID onto the ledger? Are we saying this is handled out of band? |
|
It's the same way as a did:sov. You use the /did/indy/create and then post it to /wallet/did/public. There's no way to start up a fresh agent with a seed and a did:indy currently. Doing that with a seed still creates a did:sov. Edit: oh, to get it on the ledger you just post the did:indy:12345 and the verkey. So, yes I think that would be out of band. |
There was a problem hiding this comment.
You're doing as the Romans do here but I think this logic ought to be captured directly in DID registration/creation endpoints. This "centralized" logic made sense when there was a single endpoint for creating DIDs but, "decentralizing" the logic, as we've talked about doing and as you've done here for did:indy, I don't think it's necessary for this to all be in one place.
aries_cloudagent/wallet/askar.py
Outdated
| if did_info.method != SOV and did_info.method != INDY: | ||
| raise WalletError( | ||
| "Setting DID endpoint is only allowed for did:sov or did:indy DIDs" | ||
| ) |
There was a problem hiding this comment.
Setting DID Endpoint in this way will get picked up by the legacy resolution support in Indy VDR but doesn't get picked up by the universal resolver, just as an aside. Ideally, we set this by updating the nym with diddocContent rather than with an attrib. We can address that in the future, though, of course.
|
jamshale
force-pushed
the
feat/3224
branch
2 times, most recently
from
a226b5f
to
0ff3979
Compare
|
Given the 5 errors found (examples with an “http” protocol instead of “https” — would it be easiest to just add an “s” in the indicated places, even though it is irrelevant? |
I think we should be able to disable this rule in the sonarcloud account or |
|
I think the sonarcloud should just be safe to ignore. It's annoying, because we should be able to mark things a safe in sonarcloud but it requires an admin configuring things. Might try and figure it out on a personal account and then ping Ry. |
acapy_agent/wallet/crypto.py
Outdated
| if method == SOV: | ||
| return bytes_to_b58(verkey[:16]) | ||
| if method == INDY: | ||
| return f"did:indy:{bytes_to_b58(hashlib.sha256(verkey).hexdigest()[:16])}" |
There was a problem hiding this comment.
why hexdigest rather than digest ? Seems that to get an equivalent NYM as askar would, digest does the job while hexdigest generates a different NYM.
There was a problem hiding this comment.
I wasn't aware askar used digest, and honestly not super confident about some stuff I'm doing here. I'll change it to digest and make sure it's working.
|
|
I agree with pretty much all the points here, except a couple small points:
I'll do a bit of work here and try and add the extra create options as minimally as possible so they can be changed without too much trouble. |
What's the use-case for this ? As we already feed the seed when calling /did/indy/create, and, thus, the key pair already generated, it seems redundant to feed it again at startup after the did has been provisionned post-deployment via the API. Or am I missing something here ? |
|
If you have multiple local dids, you can tell it which one to use as the public did with the seed when starting up. So you could create a did:sov and a did:indy and tell it which one should be the public (active) did on startup. Do you want to start a brand new agent with the seed option and have it create the did locally at the same time? This is so you can avoid doing it with the endpoints when you initialize an agent? If that's the case I think it will need to be addressed with #3240. |
That's how we use --seed and how I interpreted its intent. I thought the public did status was stored and persisted and once a did had been promoted to public, it would stay public even after agent was restarted. Then again, what SHOULD the --seed parameter be used for. As you'll read in my comments on issue 3240, for pretty much any did methods other than sov or indy, the association between seed and did/NYM makes no sens at all. seed, at its core, is a notion purely related to the generation of key pairs. ONLY did:indy (and to some extent did:sov) can manage to mix seed and did (via the NYM beeing generated from the public key part of the key pair) |
Even there, if I have multiple local did, and I want to specify the one I want to use as public, then I should specify the did to promote public rather than the seed. You're right that this is probably better suited in 3240. In the scope of this PR, I'd just exclude anything related with --seed (and indy) and consider --seed to be a pure did:sov parameter for now. And then see what happens in 3240. Otherwise it easily gets confusing for those who do use --seed to provision a new (public) did and expect it to work for a did:indy. |
|
I don't really think the --seed parameter should be used, but I'm not really sure of the history of it. I think that's what that ticket #3240 is trying to figure out. I think the agent controller should create or ensure the did's are correct on startup. That's what the demo and integration tests do. The public did is persisted, but it can be changed from one did to another. If you wanted to ensure a particular did:indy was the public did on startup you could use the --seed parameter. But yes, if you want to startup and create the did using the --seed parameter on a brand new agent then it would still create a did:sov. This PR was mostly focused on adding the ability to create and use did:indy dids and allowing the seed parameter was a bit of an aside. So maybe that could get removed. |
|
When I create a NYM transaction on an indy ledger with NYM transaction version == 2 To keep it simple for now, a suggestion would be for the 'seed' parameter to be mandatory when the 'did' parameter is included in a given |
|
Most probably this is out of scope for this PR but here's my thoughts. Please let me know if I should document this elsewhere. Given I start acapy not providing the A did:indy is necessarely a public DID. Unless I'm mistaken, there is no use-case (and technically no logic) in a private did:indy. Since now One could even argue that having to POST on |
|
The public did concept does need to be discussed. My goal here was to maintain the same process as did:sov, but I do agree with your points. The did registration in general is something we are going to be looking at improving in the near future. Likely with ideas from this spec https://identity.foundation/did-registration/. I don't want to do too much in this PR as there will be changes based on the upcoming discussions. I will address the comments about the seed option and maybe we can get it merged and then have follow up work around the did registration and public did stuff. I think the |
|
I'm going to update this and try and get it merged. I think this one did:indy method could be added to the core for now. Eventually removed with the other did:indy stuff as a plugin. |
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
|
This adds the ability to create a
did:indywith transaction version 2 algorithm. https://hyperledger.github.io/indy-did-method/#nym-transaction-version.indymethod not supported (same as before) and tell them to use the new endpoint.