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

DIP-10 Updates #164

Merged
merged 9 commits into from
Apr 28, 2021
Merged

DIP-10 Updates #164

merged 9 commits into from
Apr 28, 2021

Conversation

sunmilee
Copy link
Contributor

@sunmilee sunmilee commented Apr 8, 2021

Addressing comments in #156. Main changes are:

  • Added error codes
  • Added error object to response
  • Reworded PII related discussion
  • Remove CSV file
  • Diem ID -> DiemID

davidiw
davidiw previously requested changes Apr 9, 2021
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

nearly there, dismiss review or hit me up when ready for a follow up

@sunmilee sunmilee requested review from davidiw and xli April 16, 2021 19:36
@sunmilee sunmilee dismissed stale reviews from dahliamalkhi and davidiw April 19, 2021 19:20

addressed concern

Copy link
Contributor

@dahliamalkhi dahliamalkhi left a comment

Choose a reason for hiding this comment

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

This is a great, succinct formulation of the DiemID proposal!

I left a bunch of stylistic comments for clarification. I also indicated we remove rules/assumptions on the Diem Group governance, and stick to on-chain roles/permissions.

@sunmilee sunmilee requested a review from dahliamalkhi April 20, 2021 02:38
@sunmilee sunmilee dismissed dahliamalkhi’s stale review April 21, 2021 23:16

comments addressed, awaiting approval

@sunmilee sunmilee force-pushed the dip10 branch 3 times, most recently from 9d8c44d to a31812f Compare April 27, 2021 20:24
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

1 nit and then let's land.

@sunmilee sunmilee force-pushed the dip10 branch 2 times, most recently from 2a5b80e to 2a9cf4d Compare April 28, 2021 19:55
@sunmilee sunmilee requested a review from davidiw April 28, 2021 19:55
@davidiw davidiw merged commit 0ef561f into diem:main Apr 28, 2021
tzakian pushed a commit to tzakian/lip that referenced this pull request May 7, 2021
* DIP-10 Updates

* Address comments and bech32 update

* account identifier

* header fix

* nits

* Dahlia's comments

* rename to referenceIDcommandobject

* address comment

* change header
tzakian pushed a commit to tzakian/lip that referenced this pull request May 7, 2021
* DIP-10 Updates

* Address comments and bech32 update

* account identifier

* header fix

* nits

* Dahlia's comments

* rename to referenceIDcommandobject

* address comment

* change header
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.

6 participants