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

Add feature for renaming and deleting aids #241

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

rodolfomiranda
Copy link
Collaborator

PR that follows KERIA PR #218

@rodolfomiranda rodolfomiranda marked this pull request as draft March 30, 2024 11:54
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.94%. Comparing base (4c0072f) to head (c67f41b).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
src/keri/app/credentialing.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   82.88%   82.94%   +0.06%     
==========================================
  Files          46       46              
  Lines        4212     4217       +5     
  Branches     1040     1041       +1     
==========================================
+ Hits         3491     3498       +7     
- Misses        692      694       +2     
+ Partials       29       25       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rodolfomiranda rodolfomiranda marked this pull request as ready for review March 30, 2024 12:37
@rodolfomiranda
Copy link
Collaborator Author

integrations test will fail until KERIA PR is merged

Copy link
Collaborator

@lenkan lenkan left a comment

Choose a reason for hiding this comment

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

Nice!

Open question... since we are changing the API anyways. Is there currently any benefit of using the event type as an object field key rather than just relying on the event payload t property? I'll illustrate with an example what I mean:

Currently:

POST /identifiers/name/events
{
  "rot": { ...rot payload },
  "sigs": [...sigs]
}

POST /identifiers/name/events
{
  "ixn": { ...ixn payload },
  "sigs": [...sigs]
}

Rather than:

POST /identifiers/name/events
{
  "event": { t: "rot/ixn", ...rot/ixn payload },
  "sigs": [...sigs]
}

The switch on backend would be to check body.event.t === "ixn" or body.event.t === "rot".

src/keri/app/aiding.ts Outdated Show resolved Hide resolved
src/keri/app/aiding.ts Outdated Show resolved Hide resolved
@lenkan lenkan mentioned this pull request Apr 11, 2024
@lenkan lenkan changed the base branch from development to main April 12, 2024 19:51
@lenkan
Copy link
Collaborator

lenkan commented Apr 14, 2024

@rodolfomiranda There is something wrong after the upgrade to 0.2.0-dev0. I tried to debug the multisig-vlei test and found that after the client of GAR1 resolves the oobi for the multisig QVI aid, they still don't have the QVI aid in the contact list.

To get this merged we need to figure out what the problem is there. Perhaps it would be better to create a separate PR for just the upgrade to 0.2.0-dev0.

@lenkan
Copy link
Collaborator

lenkan commented Apr 14, 2024

@rodolfomiranda Did a little bit more digging, and what we get back from the long running operation for resolving the QVI AID is just ({ oobi: "oobi url" }), rather than the expected response of the current state of the contact. This only happens here:

https://github.com/WebOfTrust/keria/blob/7ee5ba84c9aed5d2d21fa48d8241d39b06aadb93/src/keria/core/longrunning.py#L239, which only happens if the obr.cid is not in kevers. Any ideas?

@rodolfomiranda
Copy link
Collaborator Author

Any ideas?

I've been debugging a lot and saw the same but also found that the problem starts to happens after a new ixn or rot event is applied to the multisig. In the multisig-vlei-issuanse test, that happens after the ixn for the delegation in GEDA aid; in the multisig test that happens after rotating the multisig aid. In both cases, if you comment out the event (the delegation in the vlei test or the rotation in the multisig test), the problem not longer appears. If not, seems that the habs got corrupted.

I think it's related to the changes that were applied in keripy and keria to track multisig states, such as the smids and rmids as states. It works well at inception, but something is wrong after and ixn or rot event. I'm still troubleshooting...

@lenkan
Copy link
Collaborator

lenkan commented Apr 14, 2024

Any ideas?

I think it's related to the changes that were applied in keripy and keria to track multisig states, such as the smids and rmids as states. It works well at inception, but something is wrong after and ixn or rot event. I'm still troubleshooting...

Cross posting for visibility.

There are probably more issues then. In #252, I managed to reproduce a similar situtation for single sig only, where the problem is that a delegator cannot resolve an oobi of the delegatee.

@lenkan
Copy link
Collaborator

lenkan commented Oct 17, 2024

@rodolfomiranda I believe this has been superseded by a bunch of other merged PRs now. Should we close this one?

@lenkan lenkan added the stale Stale issues or PRs label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants