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.

Sign up for GitHub

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

Jump to bottom

docs(ADR): add verifiable credential ADR 006 #179

Merged
merged 9 commits into from
Aug 3, 2022
Merged

docs(ADR): add verifiable credential ADR 006 #179

merged 9 commits into from
Aug 3, 2022

Conversation

noandrea
Copy link
Contributor

@noandrea noandrea commented Jul 4, 2022 •
edited by barriebyron
Loading

The ADR describes the support for verifiable credentials and credential definitions
and
confirms a new credential module for the Elesto chain

@noandrea noandrea changed the base branch from docs/review-net-upgrade to main July 5, 2022 11:42
@noandrea noandrea force-pushed the docs/ADR-pvc branch 3 times, most recently from 3180919 to 3ca0267 Compare July 6, 2022 19:44
@noandrea noandrea marked this pull request as ready for review July 6, 2022 19:45
@noandrea noandrea requested a review from a team as a code owner July 6, 2022 19:45
@noandrea noandrea added cascading A PR that is targeting another branch that is not main documentation Improvements or additions to documentation and removed cascading A PR that is targeting another branch that is not main labels Jul 6, 2022
PaddyMc
PaddyMc approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@PaddyMc PaddyMc left a comment

Choose a reason for hiding this comment

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

gg wp 🙇

I think this is good enough to merge as draft

@barriebyron barriebyron changed the title docs(ADR): add verfiable credential ADR docs(ADR): add verifiable credential ADR Jul 11, 2022
barriebyron
barriebyron reviewed Jul 11, 2022
View reviewed changes
Copy link
Contributor

@barriebyron barriebyron left a comment

Choose a reason for hiding this comment

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

@noandrea I always enjoy reviewing technical content that teaches me important concepts. This ADR review is my first for elesto, so please provide feedback on how I did and if my questions are helpful.

By working together, you can teach me how I can better help the project and the team! I reviewed the ADR template and several Cosmos SDK ADRs for background.

docs/Explanation/ADR/adr-006-public-verifiable-credential.md Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Outdated Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Outdated Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Outdated Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Outdated Show resolved Hide resolved
@barriebyron
Copy link
Contributor

gg wp 🙇

I think this is good enough to merge as draft

How are draft ADRs indicated?

@PaddyMc
Copy link
Contributor

PaddyMc commented Jul 12, 2022

How are draft ADRs indicated?

The is a section at the top

Also feel free to commit all your changes and we can then merge, thanks for the awesome review 🙇

barriebyron
barriebyron approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@barriebyron barriebyron left a comment

Choose a reason for hiding this comment

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

thanks @PaddyMc for answering questions

docs/Explanation/ADR/adr-006-public-verifiable-credential.md Outdated Show resolved Hide resolved
docs/Explanation/ADR/adr-006-public-verifiable-credential.md Show resolved Hide resolved
singhhp1069
singhhp1069 approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@singhhp1069 singhhp1069 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; only some text issue by @barriebyron has to be addressed.

@barriebyron barriebyron changed the title docs(ADR): add verifiable credential ADR docs(ADR): add verifiable credential ADR 006 Jul 18, 2022
@barriebyron
Copy link
Contributor

hi @singhhp1069 can you help me understand what specific "text issue" has to be addressed?
P.S. I love inline comments in the PR itself

@noandrea noandrea merged commit 3d4fcb1 into main Aug 3, 2022
@noandrea noandrea deleted the docs/ADR-pvc branch August 3, 2022 10:49
noandrea added a commit that referenced this pull request Aug 27, 2022
@noandrea @PaddyMc @barriebyron
docs(ADR): add verifiable credential ADR 006 (#179)
4835e71 
fix(simulation): propagate error for add verification

Co-authored-by: PaddyMc <[email protected]>
Co-authored-by: Barrie Byron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaddyMc PaddyMc PaddyMc approved these changes

@barriebyron barriebyron barriebyron approved these changes

@singhhp1069 singhhp1069 singhhp1069 approved these changes

@Bartkeeper Bartkeeper Awaiting requested review from Bartkeeper

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@noandrea @barriebyron @PaddyMc @singhhp1069