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

eCommerce P2M Payments: DIP-158 #166

Merged
merged 57 commits into from
May 6, 2021
Merged

eCommerce P2M Payments: DIP-158 #166

merged 57 commits into from
May 6, 2021

Conversation

YinonFirstDAG
Copy link
Contributor

Created DIP-158 containing eCommerce P2M Payments flows.
Resolves #158

kphfb and others added 30 commits March 15, 2021 13:57
Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: kphfb <kph@fb.com>
Copy link
Contributor

@dimroc dimroc left a comment

Choose a reason for hiding this comment

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

Comments inline. At a high level,

  • I would second @davidiw's comment to explicitly capture when we perform attestation.
  • An overarching theme I would like to discuss tomorrow is the assumption that we have transaction data (name, address) for every P2M flow.
  • Do we need to capture the notion of a payment timing out even outside of the auth/capture flow? ie: an approved charge that doesn't settle until a month later.

dips/dip-158.md Outdated
Comment on lines 125 to 138
The following values are added to the status enum of [PaymentActor](https://dip.diem.com/dip-1/#paymentactorobject) object:

* `authorized` - This means the actor has performed all required tasks and funds are locked for a future capture. Note that the actual lock is done by the wallet (sender), so for the receiver this status means that all risk checks were successful and it is willing to accept the payment.
* `needs_init_data` - This means the actor requires basic data to initialize the payment. For the receiver, this status means that the payer data is required. For the sender, it means that the merchant data is required

For completeness purposes, the full list of values is:
* `none`
* `needs_kyc_data`
* `ready_for_settlement`
* `abort`
* `authorized`
* `soft_match`
* `needs_init_data`

Copy link
Contributor

@dimroc dimroc Apr 27, 2021

Choose a reason for hiding this comment

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

authorized was also suggested in DIP 8 so I recognize the reuse here.

nit: needs_init_data I pair with the notion of a step up, when a VASP requests additional information. This could be the case for a VASP even if they receive name and address, depending on configuration, and later on in the flow's lifecycle. Step ups, or asking for more info, is common in the PSP/payment processing space. I don't feel that we have to tie it to the init phase. I would suggest a simple rename of needs_data.

dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated
| action | enum | Y | Populated in the request. This value indicates the requested action to perform. For immediate capture and P2P transfer, "charge" is used. For auth and capture, "auth" and "capture" are now available. "capture" can only be performed after a valid "auth" |
| valid_until | uint | N | Unix timestamp indicating until when the payment can be authorized. Once this time has been reached, the payment cannot be authorized and funds cannot be locked. |
| timestamp | uint | Y | Unix timestamp indicating the time that the payment Command was created.
|
Copy link
Contributor

Choose a reason for hiding this comment

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

When properly rendered, there are a lot of dangling |. You don't want a newline before closing the above row.

CleanShot 2021-04-27 at 19 40 09@2x

dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
@dimroc
Copy link
Contributor

dimroc commented Apr 28, 2021

High level feedback summary

Some of this is redundant to previous comments, consolidating here for readability.

  1. InitCommand is redundant merge data into ensuing command

    1. The ensuing command is actually the NotifyReadCommand we are hoping to cut, therefore let's revisit the InitCommand (discussed later).
      PLACEHOLDER_5a44d617182c876a_0
    2. PLACEHOLDER_5a44d617182c876a_1
  2. Get on the same page on dual attestation timing

  3. Separate states for participants - confusing.

    1. Authorized for sender?
    2. Are these internal, but shared via PaymentActorObject
  4. Resolve the PaymentActorObject issues

    1. Status being part of the object
    2. Using a mutable object with state as an argument doesn’t make the command idempotent. Ie: NOTIFY_READY doesn’t work if recipient or sender has wrong state
    3. Let’s just use the name, address core fields and lean on response
  5. Renaming commands to be less ambiguous and more informative

    1. InfoCommand -> GetInfoCommand
    2. InitCommand -> split into two to prevent ambiguity
      1. AuthorizePaymentCommand
      2. ChargePaymentCommand
    3. ReadyCommand -> NotifyReadyCommand
      1. Notify signal that it’s a sender sent notification
      2. We may want to cut this command altogether
    4. AuthorizedCommand -> NotifyAuthorizedCommand
      1. Prevents ambiguity with AuthorizePaymentCommand
    5. CaptureCommand -> CapturePaymentCommand
    6. VoidCommand -> VoidPaymentCommand
    7. AbortPayment -> AbortPaymentCommand

dips/dip-158.md Outdated
Comment on lines 125 to 138
The following values are added to the status enum of [PaymentActor](https://dip.diem.com/dip-1/#paymentactorobject) object:

* `authorized` - This means the actor has performed all required tasks and funds are locked for a future capture. Note that the actual lock is done by the wallet (sender), so for the receiver this status means that all risk checks were successful and it is willing to accept the payment.
* `needs_init_data` - This means the actor requires basic data to initialize the payment. For the receiver, this status means that the payer data is required. For the sender, it means that the merchant data is required

For completeness purposes, the full list of values is:
* `none`
* `needs_kyc_data`
* `ready_for_settlement`
* `abort`
* `authorized`
* `soft_match`
* `needs_init_data`

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good reason to just strip out this data structure from this exchange and use a dedicated customer and merchant (business) data structure that contains nothing more than name, address, and phone number (and maybe additional valuable, identifying fields)

dips/dip-158.md Outdated Show resolved Hide resolved
Notes:
Agreed that we will create new structures for P2M purposes rather than impose changes on existing structures from 0 < DIP < 2  (did not want to use the actual DIP name :)).
For example, we will NOT extend or change the PaymentActorObject. Instead, we will create a new object suitable for P2M needs (e.g. BusinessActorObject).
Agreed that business_data (i.e. merchant basic details) should be part of the InfoCommand response
Agreed that payer_data (i.e. payer information) should be part of the InitCommand(s) request
Agreed that, for the time being, there is no support for an additional step to ask for more data.
The receiving VASP can either accept or reject the payment based on the information provided.
We mentioned that the receiving VASP should be able to state "extra" information that is required on top of the payer_data (e.g. DOB might be required for buying alcohol) as part of the InfoCommand response.
This will be achieved by an additional optional element (type will be JSON)
A similar optional element will be added to the InitCommand(s) request. This element is to be used by the sending VASP to send the additional information requested (e.g. DOB)
Agreed (with a heavy heart...) to stop using the functionality names and stick with the command types
Agreed to use two different init command types (for each payment type) to replace the single InitCommand
We will change the name of the ReadyCommand to be ReadyNotification
Add dual attestation specification for transactions over the limit
Copy link
Contributor

@dimroc dimroc left a comment

Choose a reason for hiding this comment

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

Went ahead and did another pass. I do think DIP 158 and the changes to DIP 8 would have an easier review process and lifecycle if in separate PRs.

dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-8.md Outdated

For the sake of simplicity, we use the following terms:
* "Buyer VASP" - The VASP who submits payment transactions to the blockchain in order to pay to the other VASP
* "Merchant" - The VASP who recieves the funds (transaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explicitly say Merchant VASP as we have other docs that have a lot of communication between Merchant and Merchant VASP.

dips/dip-8.md Outdated
---

For the sake of simplicity, we use the following terms:
* "Buyer VASP" - The VASP who submits payment transactions to the blockchain in order to pay to the other VASP
Copy link
Contributor

Choose a reason for hiding this comment

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

We use Customer VASP in other DIPs, can we be consistent?

dips/dip-8.md Outdated

| Field | Type | Required? | Description |
|------------------------|---------|------------|------------------------------------------------|
| vasp_address | str | Y | Address of account from which billing will happen. The address is encoded using bech32 and should uniquly identify the merchant. For Diem addresses format, refer to the "account identifiers" section in [DIP-5](https://dip.diem.com/dip-5/#account-identifiers). |
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: uniquly

dips/dip-8.md Outdated
|------------------------|---------|------------|------------------------------------------------|
| vasp_address | str | Y | Address of account from which billing will happen. The address is encoded using bech32 and should uniquly identify the merchant. For Diem addresses format, refer to the "account identifiers" section in [DIP-5](https://dip.diem.com/dip-5/#account-identifiers). |
| funds_pull_pre_approval_id | str | Y | A unique identifier of this pre-approval. Should be a UUID according to RFC4122 with "-"'s included. |
| merchant_name | str | Y | The name of the biller |
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the use of merchant here. Compared to DIP 158 however, our naming seems inconsistent. In DIP 158 we're hesitant to be narrow and tight in our definitions, and use "payment receiver" and the like. It would be great to be consistent.

On that note, should this object be a PaymentReceiverObject? Seems like no if entirely sent over URL.

dips/dip-8.md Outdated Show resolved Hide resolved
@YinonFirstDAG
Copy link
Contributor Author

YinonFirstDAG commented Apr 29, 2021 via email

@dimroc
Copy link
Contributor

dimroc commented May 4, 2021

@YinonFirstDAG After some more due diligence on traditional payment networks, the P2M data exchange is asymmetric: merchants share their name, address always, but customers often get away with just a name and zip code. Payer Data requires an address, but the fields in AddressObject are all optional. What would the flow be like when only needing zip from a customer? What would it be like it if we needed more? I think about needs_init_data and the return of specific keys to indicate missing information.

https://github.com/firstdag/dip/blob/split-dip-8/dips/dip-158.md#payerdataobject

@YinonFirstDAG
Copy link
Contributor Author

YinonFirstDAG commented May 4, 2021 via email

@YinonFirstDAG
Copy link
Contributor Author

YinonFirstDAG commented May 4, 2021 via email

@dimroc
Copy link
Contributor

dimroc commented May 4, 2021

Another item I would like to add to tomorrow's agenda is ISO 8583, the standardized messaging framework between issuers and acquirers, the banks in a traditional payment network.

https://en.wikipedia.org/wiki/ISO_8583

dips/dip-158.md Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
* Removed usage of statuses
dips/dip-158.md Show resolved Hide resolved
Copy link
Contributor

@dimroc dimroc left a comment

Choose a reason for hiding this comment

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

Left some thoughts on copy and naming.

Going to list here some object names that are up for discussion:

  1. BusinessDataObject (or MerchantData?)
  2. PaymentReceiverObject which has 1.
  3. PaymentSenderObject which has 4.
  4. PayerDataObject

dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Show resolved Hide resolved
dips/dip-158.md Outdated Show resolved Hide resolved
dips/dip-158.md Show resolved Hide resolved
dips/dip-158.md Show resolved Hide resolved
danprinz and others added 4 commits May 5, 2021 16:14
* Naming consistency
* Optional redirect URL for QR flow (POS scenario)
Added diagram images
@davidiw davidiw merged commit 4248a66 into diem:main May 6, 2021
tzakian pushed a commit to tzakian/lip that referenced this pull request May 7, 2021
* Update dip-8.md

* Update lip-8.md:

Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lip-8.md

* Apply suggestions from code review

Co-authored-by: kphfb <kph@fb.com>

* PR fixes

* split DIP-8 to funds pre approval and auth/capture

* add dip 158 for p2m charge + auth/capture

* Created DIP-158 (AKA P2M DIP).

* Incorporated additional comments

* Update dip-8.md

* Update lip-8.md:

Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lip-8.md

* Apply suggestions from code review

Co-authored-by: kphfb <kph@fb.com>

* PR fixes

* split DIP-8 to funds pre approval and auth/capture

* add dip 158 for p2m charge + auth/capture

* Created DIP-158 (AKA P2M DIP).

* Incorporated additional comments

* Revert DIP-8 initial changes draft

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Updated based on latest comments from Dahlia

* Added glossary item for Reference ID based on Dahlia's comments.

* Update dips/dip-158.md

Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* Update dips/dip-158.md

Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* Updated based on David W. comments

* Additional comments following meeting with the team.
Notes:
Agreed that we will create new structures for P2M purposes rather than impose changes on existing structures from 0 < DIP < 2  (did not want to use the actual DIP name :)).
For example, we will NOT extend or change the PaymentActorObject. Instead, we will create a new object suitable for P2M needs (e.g. BusinessActorObject).
Agreed that business_data (i.e. merchant basic details) should be part of the InfoCommand response
Agreed that payer_data (i.e. payer information) should be part of the InitCommand(s) request
Agreed that, for the time being, there is no support for an additional step to ask for more data.
The receiving VASP can either accept or reject the payment based on the information provided.
We mentioned that the receiving VASP should be able to state "extra" information that is required on top of the payer_data (e.g. DOB might be required for buying alcohol) as part of the InfoCommand response.
This will be achieved by an additional optional element (type will be JSON)
A similar optional element will be added to the InitCommand(s) request. This element is to be used by the sending VASP to send the additional information requested (e.g. DOB)
Agreed (with a heavy heart...) to stop using the functionality names and stick with the command types
Agreed to use two different init command types (for each payment type) to replace the single InitCommand
We will change the name of the ReadyCommand to be ReadyNotification
Add dual attestation specification for transactions over the limit

* Revert DIP-8 to master version to remove from pull request

* Another cycle of comments incorporated.
Main changes:
* Removed DIP-8 fro the pull request
* Change InfoCommand name
* Added image_url to business_data
* Spelling and grammar issues

* Changed the order of sections and added internal links

* Internal links fix

* Appendix A link

* Added AddressObject and NationalIdObject

* * Introduced PaymentCommandErrorObject
* Removed usage of statuses

* add dual attestation info to charge command response and capture command request

* * Grammar issue
* Naming consistency
* Optional redirect URL for QR flow (POS scenario)

* Added sequence diagrams and partial capture support

* Update dip-158.md

Added diagram images

Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: Karpick-E <59393941+Karpick-E@users.noreply.github.com>
Co-authored-by: Daniel Prinz <daniel@firstdag.com>
Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>
Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>
tzakian pushed a commit to tzakian/lip that referenced this pull request May 7, 2021
* Update dip-8.md

* Update lip-8.md:

Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lip-8.md

* Apply suggestions from code review

Co-authored-by: kphfb <kph@fb.com>

* PR fixes

* split DIP-8 to funds pre approval and auth/capture

* add dip 158 for p2m charge + auth/capture

* Created DIP-158 (AKA P2M DIP).

* Incorporated additional comments

* Update dip-8.md

* Update lip-8.md:

Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lips/lip-8.md

Co-authored-by: kphfb <kph@fb.com>

* Update lip-8.md

* Apply suggestions from code review

Co-authored-by: kphfb <kph@fb.com>

* PR fixes

* split DIP-8 to funds pre approval and auth/capture

* add dip 158 for p2m charge + auth/capture

* Created DIP-158 (AKA P2M DIP).

* Incorporated additional comments

* Revert DIP-8 initial changes draft

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Update dips/dip-158.md

Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>

* Updated based on latest comments from Dahlia

* Added glossary item for Reference ID based on Dahlia's comments.

* Update dips/dip-158.md

Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* Update dips/dip-158.md

Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* Updated based on David W. comments

* Additional comments following meeting with the team.
Notes:
Agreed that we will create new structures for P2M purposes rather than impose changes on existing structures from 0 < DIP < 2  (did not want to use the actual DIP name :)).
For example, we will NOT extend or change the PaymentActorObject. Instead, we will create a new object suitable for P2M needs (e.g. BusinessActorObject).
Agreed that business_data (i.e. merchant basic details) should be part of the InfoCommand response
Agreed that payer_data (i.e. payer information) should be part of the InitCommand(s) request
Agreed that, for the time being, there is no support for an additional step to ask for more data.
The receiving VASP can either accept or reject the payment based on the information provided.
We mentioned that the receiving VASP should be able to state "extra" information that is required on top of the payer_data (e.g. DOB might be required for buying alcohol) as part of the InfoCommand response.
This will be achieved by an additional optional element (type will be JSON)
A similar optional element will be added to the InitCommand(s) request. This element is to be used by the sending VASP to send the additional information requested (e.g. DOB)
Agreed (with a heavy heart...) to stop using the functionality names and stick with the command types
Agreed to use two different init command types (for each payment type) to replace the single InitCommand
We will change the name of the ReadyCommand to be ReadyNotification
Add dual attestation specification for transactions over the limit

* Revert DIP-8 to master version to remove from pull request

* Another cycle of comments incorporated.
Main changes:
* Removed DIP-8 fro the pull request
* Change InfoCommand name
* Added image_url to business_data
* Spelling and grammar issues

* Changed the order of sections and added internal links

* Internal links fix

* Appendix A link

* Added AddressObject and NationalIdObject

* * Introduced PaymentCommandErrorObject
* Removed usage of statuses

* add dual attestation info to charge command response and capture command request

* * Grammar issue
* Naming consistency
* Optional redirect URL for QR flow (POS scenario)

* Added sequence diagrams and partial capture support

* Update dip-158.md

Added diagram images

Co-authored-by: kphfb <kph@fb.com>
Co-authored-by: Karpick-E <59393941+Karpick-E@users.noreply.github.com>
Co-authored-by: Daniel Prinz <daniel@firstdag.com>
Co-authored-by: dahliamalkhi <dahliamalkhi@outlook.com>
Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>
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.

P2M DIP
7 participants