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

Fix test delegation approval #248

Closed
wants to merge 12 commits into from

Conversation

2byrds
Copy link
Collaborator

@2byrds 2byrds commented May 29, 2024

Delegation Approval enpoint added.
The formatting for test_aiding was updated but the only significant change is the test_identifier_delegation_end test.

@2byrds 2byrds requested a review from pfeairheller May 29, 2024 15:25
@2byrds
Copy link
Collaborator Author

2byrds commented May 29, 2024

@2byrds
Copy link
Collaborator Author

2byrds commented May 29, 2024

Per the discussion on #247 I'm going to adjust the endpoint for the approval on this PR.

@2byrds
Copy link
Collaborator Author

2byrds commented May 29, 2024

Updated per @lenkan suggestion to use a separate endpoint and a PUT instead of POST

2byrds and others added 2 commits May 29, 2024 16:42
pointing to keripy branch for testing purposes. will change the version on a new tag.
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 94.93671% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 92.71%. Comparing base (caad67b) to head (fcaea50).
Report is 11 commits behind head on main.

Files Patch % Lines
src/keria/app/aiding.py 76.59% 11 Missing ⚠️
tests/app/test_aiding.py 98.13% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #248      +/-   ##
==========================================
- Coverage   92.82%   92.71%   -0.12%     
==========================================
  Files          37       36       -1     
  Lines        6982     7093     +111     
==========================================
+ Hits         6481     6576      +95     
- Misses        501      517      +16     

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

@lenkan
Copy link
Contributor

lenkan commented May 30, 2024

Cross posting my comment on #247 for visibility

What is the process?

  1. Delegatee create delegated inception event dip
  2. Delegator receives the inception event details out of band
  3. Delegator create interaction event with delegation information in anchor data.
  4. Delegator approves delegation by posting the same interaction event again?

What is the risk with having step 4 processed automatically?

If it is not possible to do automatically, can we add a flag to step 3 so it becomes

POST /identifiers/name/events
{ 
   ixn: [ anchor data ], 
   approveDelegation: true 
}

Another option if we absolutely need to do do another call to the server, we can move it to another endpoint? What is it that is being created or modified here? Is it a "pending delegation request"?

Note: I am not saying any of my suggestion are objectively better, just trying to trigger discussion about how we design the HTTP API of KERIA so that we can improve it as we move forward.

I think from the dev meeting today it is clear that the desired approach is to not have to do step 4 as a separate API call, but rather as a part of the call that posts the interaction event.

@2byrds
Copy link
Collaborator Author

2byrds commented May 30, 2024

Cross posting my comment on #247 for visibility

What is the process?

  1. Delegatee create delegated inception event dip
  2. Delegator receives the inception event details out of band
  3. Delegator create interaction event with delegation information in anchor data.
  4. Delegator approves delegation by posting the same interaction event again?

What is the risk with having step 4 processed automatically?

If it is not possible to do automatically, can we add a flag to step 3 so it becomes

POST /identifiers/name/events
{ 
   ixn: [ anchor data ], 
   approveDelegation: true 
}

Another option if we absolutely need to do do another call to the server, we can move it to another endpoint? What is it that is being created or modified here? Is it a "pending delegation request"?

Note: I am not saying any of my suggestion are objectively better, just trying to trigger discussion about how we design the HTTP API of KERIA so that we can improve it as we move forward.

I think from the dev meeting today it is clear that the desired approach is to not have to do step 4 as a separate API call, but rather as a part of the call that posts the interaction event.

I agree, let's hear from @pfeairheller and I'll adjust the PR accordingly

Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

A few concerns here...

First, the history on this PR seems odd as it is bringing in some of my commits from 9 days ago.

Second, I don't think REST likes a PUT on a plural endpoint, PUTs require a resource afaik.

Third, I'm not sure why there is the waterfall of excepts at the bottom of the on_put, none of those exceptions can happen in that try block.

Fourth, we have a "delegating.py" file where this code should belong and the endpoint should be "delegation" specific, not "approvals"

Finally (and most importantly) I don't think this is the right approach. If you look at credentialing.py, there are endpoints that do credential specific things that ALSO require an anchoring event. That is the pattern that should be followed for this. A delegation endpoint that accepts either an IXN or ROT that is processed by the IdentifierResource, then a long running operation is returned. A new cue can be submitted to a Doer that then checks the delegables escrow, creates the anchor seal attachment and parses the event and when it is actually committed then declares the operation "done". So you'll need a new long running operation type with code in the Monitor to check for this to be "done" by looking for the successful completion of the delegated event.

My suggestion is to close this PR and open a new one that takes another approach.

setup.py Show resolved Hide resolved
src/keria/app/aiding.py Show resolved Hide resolved
src/keria/app/aiding.py Show resolved Hide resolved
src/keria/app/aiding.py Show resolved Hide resolved
src/keria/app/aiding.py Show resolved Hide resolved
@2byrds
Copy link
Collaborator Author

2byrds commented Jun 4, 2024

Closing now that review comments addressed in new PR #250

@2byrds 2byrds closed this Jun 4, 2024
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

Successfully merging this pull request may close these issues.

3 participants