Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Update stored contacts with deprecated DID Format, respond with SDK Format for outdated Contacts#87

Merged
LeonFLK merged 15 commits intodevelopfrom
lw-contact-did-migration
Nov 27, 2020
Merged

Update stored contacts with deprecated DID Format, respond with SDK Format for outdated Contacts#87
LeonFLK merged 15 commits intodevelopfrom
lw-contact-did-migration

Conversation

@LeonFLK
Copy link
Contributor

@LeonFLK LeonFLK commented Nov 18, 2020

fixes KILTProtocol/ticket#744

When Contact with deprecated format exists in DB we delete on update and respond SDK format.

How to test:

docker-compose up -d mongodb
start old democlient and old services
register Contact with DID in deprecated format
restart services with this PR included
visit http://localhost:3000/contacts
contact DID should be displayed in updated sdk format

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@LeonFLK LeonFLK force-pushed the lw-contact-did-migration branch from f989484 to 8bb243e Compare November 18, 2020 20:05
@LeonFLK LeonFLK requested a review from rflechtner November 18, 2020 20:06
@LeonFLK LeonFLK self-assigned this Nov 18, 2020
@rflechtner
Copy link
Contributor

I'm not sure I get what is going on here. We display the outdated DID format in the new format? I think I remember that the problem was that the old signature is just not valid over the new format. Have we solved this somehow?

@LeonFLK
Copy link
Contributor Author

LeonFLK commented Nov 20, 2020

Hey @rflechtner, sorry that I didn't clarify, though I remember mentioning it to you, the old signature IS valid even for the new format, as the signature is essentially built the same way with the same data and they match .

@rflechtner
Copy link
Contributor

Hey @rflechtner, sorry that I didn't clarify, though I remember mentioning it to you, the old signature IS valid even for the new format, as the signature is essentially built the same way with the same data and they match .

Yeah sorry man I can't seem to remember. So when I run the resulting object through the sdk or the did resolver, they would validate the signature correctly, regardless of whether it was originally part of the old or the new format?

@LeonFLK
Copy link
Contributor Author

LeonFLK commented Nov 20, 2020

No worries, might have been some time ago 🙄 ...the resulting signatures match!

@LeonFLK LeonFLK requested review from rflechtner and removed request for rflechtner November 20, 2020 14:56
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to do what it is supposed to do 👍
I had a hard time reviewing because it was not easy to understand what the original format looked like and what we'd have to do to translate. In-line comments and appropriate typing could help!

@@ -50,9 +67,15 @@ export class MongoDbMContactsService implements ContactsService {
}
private convertToContact(contactDB: ContactDB): Contact {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we adjust the typing so that it reflects the potential conversion from the old to the new format?
the easiest way to do this would probably be

Suggested change
private convertToContact(contactDB: ContactDB): Contact {
private convertToContact(contactDB: ContactDB & {signature?: string} ): Contact {

Copy link
Contributor Author

@LeonFLK LeonFLK Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I really wasn't sure how to deal with "shadow" properties, but it seems like this is a great way to deal with potential shadow properties

return {
metaData,
did,
did: actualDid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess

Suggested change
did: actualDid,
did: contactDB.signature && did ? {...did, signature: contactDB.signature} : did,

would also do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact when you add the optional {signature?: string} to the signature you could even make this more concise by adding signature to

const { metaData, did, publicIdentity } = contactDB

Comment on lines +29 to +31
.deleteOne({
'publicIdentity.address': contact.publicIdentity.address,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment describing why we need to delete the existing entry first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. why an update just won't do the trick?

Copy link
Contributor Author

@LeonFLK LeonFLK Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure, also for here: ran into issues with updating as this keeps the old properties as artifacts or something, it does not exactly replace the stored document, thus not removing the signature outer property.

@LeonFLK LeonFLK merged commit ff51c0b into develop Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants