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

Update DIP-10 #151

Merged
merged 7 commits into from
Mar 30, 2021
Merged

Update DIP-10 #151

merged 7 commits into from
Mar 30, 2021

Conversation

sunmilee
Copy link
Contributor

Updating DIP-10 to reflect the current PayAddress MVP spec. Changes include:

  • Include using CSV file for domain distribution
  • Include Reference ID exchange
  • Remove UserObject and ProfileURI

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.

A lot to chew on, I'm not convinced I have the best wording or structure in mind, so consider this a conversation. Also what's up with the conflicts on the tests? weird :(

@sunmilee sunmilee requested review from xli and davidiw March 24, 2021 02:25
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.

Okay, this is coming together really well, I think the next push should probably make this ready for more reviews

@sunmilee sunmilee requested a review from tzakian March 24, 2021 05:56
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.

after tweaks, ready to approve

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.

Looks good to me!

@davidiw
Copy link
Contributor

davidiw commented Mar 30, 2021

Let's make an issue for this DIP where folks can continue to give feedback.

@davidiw davidiw merged commit 8806be3 into diem:main Mar 30, 2021
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.

Thanks for making substantial improvements to this DIP!

I wanted to offer some high level comments:

  • I would add a brief introduction to the effect of:

    This DIP has two components.

    The first part is a 2-level namespace for VASP user addresses, consisting of a username and a VASP name. It includes a method for registering VASPs name(s) on the Diem blockchain.

    The second part is a standard framework for initiating a payment transfer to a recipient whose address has already been communicated to an originator (by means left outside the scope of this DIP). The protocol standard arranges for the originator and recipient VASPs to coordinate a transaction referenceID. Other off-chain information exchanges can occur as described in other DIPs (e.g., in DIP-1) and can be embedded within the proposed framework.

  • I would refrain from claims about greater privacy, and any other unsubstantiated privacy statements

  • I am not supportive of the (two part) name Diem ID, since it it not clear it will be the only identity we will have on Diem. I would prefer something more specific, e.g., VAEP (VASP Endpoint) or the like.

  • The user story is cute, but we should emphasize this is only for demonstration purposes, it is NOT part of the proposed standard

  • I wonder whether we should permit more characters in VAEP in order to be compatible with addresses on other blockchains?

davidiw pushed a commit to davidiw/lip that referenced this pull request Apr 1, 2021
* DIP-10

* Rename to Pay Address

* Edits

* address comments

* Addressing comments and reordering

* Travel rule and payment metadata

* attribute table
@davidiw
Copy link
Contributor

davidiw commented Apr 1, 2021

@dahliamalkhi, great feedback, routing it to: #156

Copy link
Contributor

@longbowlu longbowlu left a comment

Choose a reason for hiding this comment

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

I think we should also talk about what happens when a reference id is used multiples times. Do we expect the receiver to refund?

tzakian pushed a commit to tzakian/lip that referenced this pull request Apr 20, 2021
* DIP-10

* Rename to Pay Address

* Edits

* address comments

* Addressing comments and reordering

* Travel rule and payment metadata

* attribute table
sunmilee added a commit to sunmilee/dip that referenced this pull request Apr 28, 2021
* DIP-10

* Rename to Pay Address

* Edits

* address comments

* Addressing comments and reordering

* Travel rule and payment metadata

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

* Rename to Pay Address

* Edits

* address comments

* Addressing comments and reordering

* Travel rule and payment metadata

* attribute table
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.

9 participants