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

Endorser doc updates and some bug fixes #1926

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Sep 2, 2022

Signed-off-by: Ian Costanzo [email protected]

Fixes a couple of issues with DID ATTRIB transactions, and adds some technical docs.

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

Merging #1926 (be4fd96) into main (b72db27) will decrease coverage by 0.08%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##             main    #1926      +/-   ##
==========================================
- Coverage   93.66%   93.58%   -0.09%     
==========================================
  Files         539      539              
  Lines       34219    34272      +53     
==========================================
+ Hits        32052    32073      +21     
- Misses       2167     2199      +32     

Endorser.md Show resolved Hide resolved
Endorser.md Outdated Show resolved Hide resolved
Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

I'd say merge the puml into the Readme before merging. And fix the typo. I will not be able to test directly, so will leave it to @shaangill025 or @andrewwhitehead to provide a code review.

@dbluhm -- not sure how much your team plans on using the endorser, but you might want to do a review as well.

@ianco
Copy link
Contributor Author

ianco commented Sep 2, 2022

@swcurran @frostyfrog @dbluhm there's support for a startup parameter called --auto-promote-author-did, which (for authors) will auto-promote their DID to be their public DID on receipt of the endorser response.

However I now feel that this parameter is pretty useless, and I'd like to get rid of it. I think it may end up causing more problems that it's resolving.

(I mention it in this PR because one of the issues I was addressing - from @CChiu : "i also tried setting the auto-promote-author-did: true in the author's config files but i still get the same issue where the ATTRIB is not written to the ledger when an org is created under the author-endorser flow using v0.7.4." - mentioned this flag and it didn't look like it was working properly.)

The intent was that an author wouldn't need to monitor their endorser responses - if the endorser endorsed their DID transaction, then their aca-py would auto-promote this DID to be their public DID.

However it's only useful in the case where the author already has a public DID, so they can sign their endorser request. If they don't have a public DID yet, then they have to go though some out of band process anyways to register their first public DID (maybe phone a friend?) so this --auto-promote-author-did is pretty useless.

Any opinions on if I can just remove it?

@swcurran
Copy link
Contributor

swcurran commented Sep 2, 2022

Should the scenario of an author creating a DID for itself be handled by the Endorser not signing and returning the DID, but rather creating and executing the transaction? That is the flow that is expected -- the "out of band" process you are mentioning. Basically, handle requests to create DIDs differently from other transactions.

@ianco
Copy link
Contributor Author

ianco commented Sep 2, 2022

Should the scenario of an author creating a DID for itself be handled by the Endorser not signing and returning the DID, but rather creating and executing the transaction?

Possibly, but this is a fairly big change. The author can't *sign" the request, because they have no public DID to sign with. So the endorser process would have to be customized for this one scenario for the author to send an unsigned request, the endorser to write the transaction, and then to send an acknowledgement back. (Which kind of goes against the intent to make the endorser protocol a generic "please sign this" protocol.)

So if we're going to do this I suggest it be a separate protocol (or at least a dedicated function within the endorser protocol).

And there's still the question of whether to keep or get rid of the --auto-promote-author-did param.

@swcurran
Copy link
Contributor

swcurran commented Sep 2, 2022

Fair enough. I think we need that functionality, but agree that it is a separate protocol and Issue/PR from the rest of the endorsing. I would say remove the start up option, and lets add an issue that will extend the work of the endorser to handle this new type of action, with a new protocol.

@ianco ianco marked this pull request as ready for review September 6, 2022 14:16
@ianco
Copy link
Contributor Author

ianco commented Sep 6, 2022

Fair enough. I think we need that functionality, but agree that it is a separate protocol and Issue/PR from the rest of the endorsing. I would say remove the start up option, and lets add an issue that will extend the work of the endorser to handle this new type of action, with a new protocol.

Added a new issue #1929

@ianco ianco merged commit b9f4d7a into openwallet-foundation:main Sep 6, 2022
@dbluhm dbluhm added this to the Version 0.7.5 milestone Oct 19, 2022
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.

5 participants