fix: ignore duplicate record errors on add key #2447
Conversation
This is a bandaid solution to prevent routing keys from being stored for multiple DIDs. It would be nice to correct behavior of the DIDDoc class to not turn routing keys into values in the publicKey list and then claiming that the DID is the controller (with no evidence). However, given the impending replacement of this behavior with did:peer, it doesn't seem worth it to go through that effort right now. Signed-off-by: Daniel Bluhm <[email protected]>
|
I'm working on some additional unit tests but assuming existing tests pass as I expect, testing this in your regression test setup would be helpful @WadeBarnes |
|
Actually, I may have spoken too confidently. Sorry! I'm not seeing the results I expected. I'll continue testing. |
Signed-off-by: Daniel Bluhm <[email protected]>
|
I didn't see the results I expected because I initially tried to get the code to misbehave in a way I no longer permitted; worked around this in my test by circumventing the protections I added to recreate the conditions I believe we saw in those regression tests. If I've correctly deduced those conditions, I think this is now ready for testing. |
|
Reiterating that while this is a bandaid, this will no longer be an issue once we get did peer fixes in. If we choose to migrate old records for stored DID documents, I think we should also reconstruct the key-to-DID mapping to get rid of old records of routing keys getting mapped to DIDs when they shouldn't have been. @Jsyro @swcurran |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Daniel -- when |
@swcurran as far as I understand the circumstances of what led to the duplicate records errors, I believe ignoring the error and raising a warning will not have a negative impact on message processing. The reason being that it is the routing keys that have duplicate records. These routing keys really have no reason to be stored in the key-to-DID lookup since we will never receive a message from a connection with those keys. If we did receive a message from those keys, prior to this fix we would have failed to process it anyways because we would have received that same duplicate record error. So on the whole, there's no change in how this scenario is handled when compared to the behavior before the introduction of #2409 |
|
Perfect — thanks! -rc2 on the way. |
|
@dbluhm - Looks like we're not out of the woods yet. In AATH, the Aries Framework Go tests that had been passing failed, as did the Findy Tests. I'll create a new issue and try to get the data needed to review them. |
This is a bandaid solution to prevent routing keys from being stored for multiple DIDs. It would be nice to correct behavior of the DIDDoc class to not turn routing keys into values in the publicKey list and then claiming that the DID is the controller (with no evidence). However, given the impending replacement of this behavior with did:peer, it doesn't seem worth it to go through that effort right now.
cc @swcurran @WadeBarnes and others