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

DO NOT MERGE; WILL REPLACE SHORTLY - polish 0036 prior to ACCEPTED status #188

Closed
wants to merge 6 commits into from

Conversation

dhh1128
Copy link
Contributor

@dhh1128 dhh1128 commented Aug 15, 2019

Use "Holder" instead of "Prover" during issuance. Simplify to 1 diagram. Add hyperlinks. Mention where ack is triggered by ~please_ack decorator.

dhh1128 added 6 commits August 9, 2019 18:35
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
@dhh1128
Copy link
Contributor Author

dhh1128 commented Aug 15, 2019

@swcurran and @jovfer, I would like your review on this. I spent some time polishing the RFC and made not-quite-trivial, but also not-materially-significant-for-an-implementation-thats-already-written, updates.

@dhh1128
Copy link
Contributor Author

dhh1128 commented Aug 15, 2019

On most of my changes, I'm confident that I'm aligned with the intent of the authors, so the changes are just minor enhancements. The only case where that is not true is that I changed the "Credential Reject" message on the diagram to be a problem-report. This was because the running text of the RFC doesn't document "Credential Reject" as a message anywhere, and I think we intended that rejection to just be a problem report. But I could be wrong on that.

@@ -1,7 +1,7 @@
# Aries RFC 0011: Decorators

- Authors: Daniel Hardman
- Status: [DEMONSTRATED](/README.md#demonstrated)
- Status: [ACCEPTED](/README.md#accepted)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may have leaked from another PR. Do we want to keep this in here, or wait to merge this until the Decorators status change occurs to merge it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Yes, I'll take this out. Sorry about that.

#### Choreography Diagram

<blockquote>
Note: This diagram was made in draw.io. To make changes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these details in here so it's easier for people to reproduce the diagrams and make edits.

Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

These changes look very beneficial. Can we get the Decorators status change removed from here so it can be merged? It seems like this PR proposes the fixes requested by @llorllale , is that true?

@dhh1128
Copy link
Contributor Author

dhh1128 commented Aug 15, 2019

There was already a previously merged PR from Stephen Curran that addressed some of @llorllale 's requested changes. I went a little bit further as part of some other things I was trying to accomplish; hopefully I kept George happy and didn't lose any of @swcurran 's goodness. :-)

@dhh1128 dhh1128 changed the title polish 0036 prior to ACCEPTED status DO NOT MERGE; WILL REPLACE SHORTLY - polish 0036 prior to ACCEPTED status Aug 15, 2019
Copy link
Member

@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.

Looks good to me. @dhh1128 I think you should add yourself as an author on the RFC - these are substantial and helpful updates, although I agree that they do not impact the meaning of the RFC.

Agree with @kdenhartog that the two decorator files need to be removed from the PR.

@dhh1128
Copy link
Contributor Author

dhh1128 commented Aug 15, 2019

Hold on. I'm going to create a new version of this PR that isn't polluted by the other PR that leaked in.

@kdenhartog
Copy link
Contributor

There was already a previously merged PR from Stephen Curran that addressed some of @llorllale 's requested changes. I went a little bit further as part of some other things I was trying to accomplish; hopefully I kept George happy and didn't lose any of @swcurran 's goodness. :-)

Good to hear, I just found what you're referring to in the closed PRs. Good to see this is getting moved statuses.

@dhh1128
Copy link
Contributor Author

dhh1128 commented Aug 16, 2019

Okay, this PR is now replaced by #189 . Closing this one.

@dhh1128 dhh1128 closed this Aug 16, 2019
@dhh1128 dhh1128 deleted the cx branch August 16, 2019 01:49
kdenhartog added a commit that referenced this pull request Aug 18, 2019
fix PR #188 by removing accidental inclusion of different PR
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