Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contact not added correctly after a previous deletion #292

Open
lenkan opened this issue Sep 9, 2024 · 6 comments
Open

Contact not added correctly after a previous deletion #292

lenkan opened this issue Sep 9, 2024 · 6 comments

Comments

@lenkan
Copy link
Collaborator

lenkan commented Sep 9, 2024

Steps to reproduce

  1. Create wallet with identifier X and authorize the agent end role
  2. Create wallet with identifier Y and authorize the agent end role
  3. Resolve the OOBI of identifier Y from the wallet of identifier X, use an alias to ensure that the contact is added
  4. GET /contacts/{prefix} => verify that the correct alias and id is returned.
  5. DELETE /contacts/{prefix}
  6. Resolve the OOBI of identifier Y again, also using an alias.
  7. GET /contacts/{prefix} => verify that the correct alias and id is returned.

Actual result

HTTP 404 is returned.

Notes

See reproduction https://github.com/nordlei/vlei-sandbox/blob/main/src/issues/contact-not-added-after-deletion.test.ts

git clone git@github.com:nordlei/vlei-sandbox.git
cd vlei-sandbox
docker compose run --rm --build test npm start src/issues/contact-not-added-after-deletion.test.ts

It seems like the contact attributes are removed from the contact.

See test

import { beforeAll, expect, test } from "vitest";
import { TestWallet } from "./test-wallet.ts";
import { randomUUID } from "crypto";

const wallet1 = new TestWallet({
  alias: "alias1",
});

const wallet2 = new TestWallet({
  alias: "alias2",
});

function sleep(ms: number) {
  return new Promise((resolve) => setTimeout(resolve, ms));
}

beforeAll(async () => {
  await Promise.all([wallet1.init(), wallet2.init()]);
});

test("Resolve OOBI, verify contact", async () => {
  let operation = await wallet1.client.oobis().resolve(await wallet2.generateOobi(), wallet2.identifier.name);
  operation = await wallet1.client.operations().wait(operation);

  expect(operation).toMatchObject({ done: true });

  expect(await wallet1.client.contacts().get(wallet2.identifier.prefix)).toMatchObject({
    id: wallet2.identifier.prefix,
    alias: wallet2.identifier.name,
  });
});

test("Delete contact, resolve OOBI, get contact", async () => {
  await wallet1.client.contacts().delete(wallet2.identifier.prefix);

  let operation = await wallet1.client.oobis().resolve(await wallet2.generateOobi(), wallet2.identifier.name);
  operation = await wallet1.client.operations().wait(operation);

  expect(operation).toMatchObject({ done: true });

  // Need to add this sleep for the contact to become available, even though the operation is finished
  // await sleep(1000);

  expect(await wallet1.client.contacts().get(wallet2.identifier.prefix)).toMatchObject({
    id: wallet2.identifier.prefix,
    alias: wallet2.identifier.name,
  });
});

It looks like I need to add a delay after the operation has resolved before the contact is properly added to the list of contacts.

@lenkan lenkan changed the title Contact not added correctly after a deletion Contact not added correctly after a previous deletion Sep 9, 2024
@iFergal
Copy link
Collaborator

iFergal commented Sep 19, 2024

The problem here is the long running operation is related to the OOBI and not the contact. The OOBI has been previously resolved (and is never removed (we don't unresolve for contact deletion)) so the operation is done: True immediately - while the actual second client call to oobi().resolve() is still processing again and creating the new contact.

I think the cleanest solution might be to have a new OpTypes.contact that is returned if the alias is specified instead of OpTypes.oobi. What do you think?

@lenkan
Copy link
Collaborator Author

lenkan commented Sep 19, 2024

The problem here is the long running operation is related to the OOBI and not the contact. The OOBI has been previously resolved (and is never removed (we don't unresolve for contact deletion)) so the operation is done: True immediately - while the actual second client call to oobi().resolve() is still processing again and creating the new contact.

I think the cleanest solution might be to have a new OpTypes.contact that is returned if the alias is specified instead of OpTypes.oobi. What do you think?

Thanks for putting some thought into it! I agree that would be cleaner. I think we can even take it one step further even - what if the "resolve oobi" is completely independent from contacts. If you want to add a contact you would POST / PUT a contact resource that contains alias and AID. The creation of contact would fail if the oobi is not yet resolved.

This is what we have to do often times anyway if we want to add more attributes to the contact.

If we do it that way, the resolving of an oobi would never affect the contact database.

What do you think?

@iFergal
Copy link
Collaborator

iFergal commented Sep 19, 2024

Yeah I wouldn't be opposed to that since the second call would be necessary anyway for the attributes, and the OOBI resolution is idempotent if it fails before creating the contact.

This is inherited from keripy though so we'd have to see if it makes sense with @pfeairheller and others.

@2byrds
Copy link
Collaborator

2byrds commented Sep 24, 2024

From the dev meeting today:
There is a desire to distill keripy to the reference impl functionality. Contacts is not part of KERI and could be managed in KERIA. This is a similar conversation to other keripy related code like kli, witness, etc. It is a matter of community members contributing that migration out of keripy and into another project. If it is something useful across multiple projects (keripy users, KERIA users, etc) then a new intermediate project that could be used by all, is preferred. Perhaps a shared or monorepo could help to separate but compose these components.

@iFergal
Copy link
Collaborator

iFergal commented Sep 24, 2024

For now at minimum we could create the new op type. But if we are moving towards removing contacts from KERI in the long term, I think it makes sense to stop mixing OOBIs and contacts already as @lenkan has suggested.

@iFergal
Copy link
Collaborator

iFergal commented Oct 15, 2024

@lenkan I looked at this yesterday a little more closely. The POST/PUT contact APIs are synchronous and once you get the 200 everything is done, so there's no op type.

I could add a new op type, but not sure it's the right move until we know if OOBIs and contacts will be decoupled. Would rather not add it and then remove it later.

You could mimic it for now by not passing an alias to the OOBI resolution, and using POST to create it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants