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 lip-8 #75

Closed
wants to merge 12 commits into from
Closed

Conversation

danprinz
Copy link
Contributor

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

Disclaimer regarding the discovery phase
Extended scope for fund pull pre-approval
Revoke an authorization in more details
@bors-libra
Copy link

❗ before this PR can be merged please make sure that you enable "Allow edits from maintainers".

This is needed for tooling to be able to update this PR in-place so that Github can properly recognize and mark it as merged once its merged into the upstream branch

@JoelMarcey
Copy link

@danprinz Looks like a rebase may be needed.

@bmwill Is there something we need to do with bors here? I don't usually see that warning in PRs to this repo, I don't think.

@bmwill
Copy link

bmwill commented Oct 12, 2020

@JoelMarcey Did we ever resolve if we wanted bors on this repo or not? If not I probably need to just disable bors since IIRC we aren't currently relying on bors for this repo.

@JoelMarcey
Copy link

@bmwill I think I am ok removing bors on this repo for now and revisit later. We have the passive checks to make sure the website builds and we have only CODEOWNERS being able to merge, which helps too.

lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Show resolved Hide resolved
lips/lip-8.md Outdated

| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| Type | str enum | Y | This can be either save_sub_account or consent |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we would want "type". save_sub_account seems to not be overly useful, but perhaps I'm missing something. Isn't it implied that if I wanted to bill someone again, I could try their same subaddress and they would have the option to approve it? Even if I didn't get this approval ahead of time, I could still trigger the transaction, couldn't I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right that "consent" covers the basic option of "save_sub_account".
but we thought it would be nice to have the user approve even the storage of their details (sub-account).
consider the scenario of users who wants to add their Libra wallet as a payment method (to make subsequent payments easier), but still want to authorize every payment. something like a push notification to their mobile app.
the merchant VASP should store their sub-address but without a "real" consent of withdrawing funds.
one may argue that the merchant could save these details without prior approval of the user, but I think that users will want to authorize even this.
the wallet VASPs on the other hand will pop a notification to the user only if the merchant could present such an approval.

does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I'm still not convinced that this is something that merchants/users will want. I could be wrong though. But it feels like there is no real reason for anyone to follow this for save_sub_account. I can just put a checkbox on my page to "save" in the same way and it functionally operates exactly the same as this without needing any off-chain or user action in their wallet app. It just feels like we are adding a feature that isn't actually adding anything practically, but does add some complexity in implementation

lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
danprinz and others added 9 commits October 15, 2020 23:55
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>
@danprinz danprinz requested a review from kphfb October 15, 2020 21:26
lips/lip-8.md Outdated Show resolved Hide resolved
1. The buyer VASP triggers a process with pre-knowledge of the merchant subaddress. e.g., QR/deep links - where the consumers while acting in their wallet provides their VASP the merchant's relevant details (embedded in the QR/link in the checkout page)
2. The merchant gets the user identifier (like Pay ID) and query the buyer side for a new subaddress for this process
For now, we will assume that one of these methods took place and the merchant have the buyer subaddress at hand.

---
# Specification
Copy link
Contributor

Choose a reason for hiding this comment

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

I am missing a high level description of the proposed approach, before we jump into the Specification.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dahliamalkhi what type of thing would you like to see here as compared to what is there in the abstract section?

lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated

| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| Type | str enum | Y | This can be either save_sub_account or consent |
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I'm still not convinced that this is something that merchants/users will want. I could be wrong though. But it feels like there is no real reason for anyone to follow this for save_sub_account. I can just put a checkbox on my page to "save" in the same way and it functionally operates exactly the same as this without needing any off-chain or user action in their wallet app. It just feels like we are adding a feature that isn't actually adding anything practically, but does add some complexity in implementation

lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
lips/lip-8.md Outdated Show resolved Hide resolved
@kphfb
Copy link
Contributor

kphfb commented Nov 4, 2020

Think with the one change I suggest, it hopefully addresses #71 as well. If so, we can close that one once this is updated

danprinz and others added 2 commits November 4, 2020 11:33
Co-authored-by: kphfb <kph@fb.com>
@kphfb
Copy link
Contributor

kphfb commented Nov 11, 2020

I think I'm mostly good with this PR other than I'm still not convinced about "type" and want to chat about that first


This LIP does not contain the initial phase of a sub-account discovery that is required to start negotiating merchant scenarios.
The process described below starts at the phase that both sides (particularly the biller side) have the relevant subaddresses.
For now, we will assume that such a discovery process took place and the merchant have the buyer subaddress at hand.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For now, we will assume that such a discovery process took place and the merchant have the buyer subaddress at hand.
For now, we will assume that such a discovery process took place and the merchant has the buyer subaddress at hand.

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 the normal approach would be to describe a general strawman flow, then explicitly state what is assumed (e.g., that there is a user -> merchant identity exchange process). As I noted in another PR on DIP-8, there's so much boiler plate before getting into what this DIP does, that it ... isn't particularly accessible. You calling this out, means you see it too :).

@@ -101,16 +116,15 @@ For a fund pre-approval request, the [command_type](https://lip.libra.org/lip-1/

### FundPullPreApprovalObject

The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can by updated multiple times. Updating immutable fields with a different value results in a command error, but it is acceptable to re-send the same value.
The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can be updated multiple times. Updating immutable fields with different values results in a command error, but it is acceptable to re-send the same value. It should be noted that initial creation of the FundPullPreApprovalObject can be created or updated by the VASP on either side (buyer or seller).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can be updated multiple times. Updating immutable fields with different values results in a command error, but it is acceptable to re-send the same value. It should be noted that initial creation of the FundPullPreApprovalObject can be created or updated by the VASP on either side (buyer or seller).
The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can be updated multiple times. Updating immutable fields with different values results in a command error, but it is acceptable to re-send the same value. It should be noted that initial creation of the FundPullPreApprovalObject can be created by the VASP on either side (buyer or seller).

| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| address | str | Required for creation | Address of account from which the pre-approval is being requested. Addresses may be single-use or valid for a limited time, and therefore VASPs should not rely on them remaining stable across time or different VASP addresses. The addresses are encoded using bech32. The bech32 address encodes both the address of the VASP as well as the specific user's subaddress. They should be no longer than 80 characters. Mandatory and immutable. For Libra addresses, refer to the "account identifier" section in LIP-5 for format. |
| biller_address | str | Required for creation | Address of account from which billing will happen. Addresses may be single-use or valid for a limited time, and therefore VASPs should not rely on them remaining stable across time or different VASP addresses. The addresses are encoded using bech32. The bech32 address encodes both the address of the VASP as well as the specific user's subaddress. They should be no longer than 80 characters. Mandatory and immutable. For Libra addresses, refer to the "account identifier" section in LIP-5 for format. |
| funds_pre_approval_id | str | Y | Unique reference ID of this pre-approval on the pre-approval initiator VASP (the VASP which originally created this pre-approval object). This value should be unique, and formatted as “<creator_vasp_onchain_address_bech32>_<unique_id>”. For example, ”lbr1pg9q5zs2pg9q5zs2pg9q5zs2pgyqqqqqqqqqqqqqqspa3m_7b8404c986f53fe072301fe950d030de“. Note that this should be the VASP address and thus have a subaddress portion of 0. This field is mandatory on pre-approval creation and immutable after that. Updates to an existing pre-approval must also include the previously created pre-approval ID. |
Copy link
Contributor

Choose a reason for hiding this comment

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

why not call this reference_id to be consistent with lip-1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't be very clear once one will try to use it in a subsequent payment (a reference to the user consent).
IMHO this name is more descriptive.

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 the key thing here is .... how do we distinctly distinguish different objects... a reference_id is supposed to be common to all objects -- if there is a shared object to be created or maintained. Also it might behove us to pivot to DiemId rather than further embracing the jankiness of subaddresses, they are really dizzying.

| funds_pre_approval_id | str | Y | Unique reference ID of this pre-approval on the pre-approval initiator VASP (the VASP which originally created this pre-approval object). This value should be unique, and formatted as “<creator_vasp_onchain_address_bech32>_<unique_id>”. For example, ”lbr1pg9q5zs2pg9q5zs2pg9q5zs2pgyqqqqqqqqqqqqqqspa3m_7b8404c986f53fe072301fe950d030de“. Note that this should be the VASP address and thus have a subaddress portion of 0. This field is mandatory on pre-approval creation and immutable after that. Updates to an existing pre-approval must also include the previously created pre-approval ID. |
| max_cumulative_amount | [CurrencyObject](#currencyobject) | N | Max cumulative amount that is approved for funds pre-approval. This is the sum across all transactions that occur while utilizing this funds pre-approval. |
| description | str | N | Description of the funds pre-approval. May be utilized so show the user a description about the request for funds pre-approval |
| scope | [ScopeObject](#scopeobject) | Y | Technical definition. The parameters of the pre-approval, this contains the expiration time and the amount limits |
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I don't fully understand why we call this scope. Is it a terminology in industry?

| funds_pre_approval_id | str | Y | Unique reference ID of this pre-approval on the pre-approval initiator VASP (the VASP which originally created this pre-approval object). This value should be unique, and formatted as “<creator_vasp_onchain_address_bech32>_<unique_id>”. For example, ”lbr1pg9q5zs2pg9q5zs2pg9q5zs2pgyqqqqqqqqqqqqqqspa3m_7b8404c986f53fe072301fe950d030de“. Note that this should be the VASP address and thus have a subaddress portion of 0. This field is mandatory on pre-approval creation and immutable after that. Updates to an existing pre-approval must also include the previously created pre-approval ID. |
| max_cumulative_amount | [CurrencyObject](#currencyobject) | N | Max cumulative amount that is approved for funds pre-approval. This is the sum across all transactions that occur while utilizing this funds pre-approval. |
| description | str | N | Description of the funds pre-approval. May be utilized so show the user a description about the request for funds pre-approval |
| scope | [ScopeObject](#scopeobject) | Y | Technical definition. The parameters of the pre-approval, this contains the expiration time and the amount limits |
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, it's not in the example below


| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| type | str enum | Y | This can be either save_sub_account or consent |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| type | str enum | Y | This can be either save_sub_account or consent |
| type | str enum | Y | This can be either `save_sub_account` or `consent` |

Comment on lines +154 to +155
| max_cumulative_amount | [ScopedCumulativeAmountObject](#scopedcumulativeamountobject) | N | Max cumulative amount that is approved for funds pre-approval. This is the sum across all transactions that occur while utilizing this pre-approval. |
| max_transaction_amount | [CurrencyObject](#currencyobject) | N | Max transaction amount that is approved for funds pre-approval. This is the maximum amount that may occur in a single transaction while utilizing this funds pre-approval. |
Copy link
Contributor

Choose a reason for hiding this comment

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

better to mention that these params are required for consent

@@ -148,7 +223,7 @@ Valid values are:
* `rejected` - User/VASP did not approve the pre-approval request.
* `closed` - Approval has been closed by the user/VASP and can no longer be used.

**Valid Status Transitions**. Either party in the pre-approval agreement can mutate the status. The status always initially begins as `pending` at which time a user must agree to the pre-approval request. Once the user has reviewed the request, the billee VASP will update the pre-approval status to `valid` (if the user agreed) or `rejected` (if the user rejected the pre-approval).
**Valid Status Transitions**. Either party in the pre-approval agreement can mutate the status. The status always initially begins as `pending` at which time a user must agree to the pre-approval request. Once the user has reviewed the request, the sender VASP (of the buyer) will update the pre-approval status to `valid` (if the user agreed) or `rejected` (if the user rejected the pre-approval).
Copy link
Contributor

Choose a reason for hiding this comment

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

given the updates on Lip-1 recently, we probably need to make this very liner as well.


`abort` may still be used to cancel the authorization early. Once a capture action occurs, the status of the payment will now be updated to `ready_for_settlement`.

**Valid Status Transitions**. `authorized` is now a valid initial value and may be followed by `ready_for_settlement` (upon a successful capture) or `abort` (if one side wishes to cancel the auth).
**Valid Status Transitions**. `authorized` is now a valid initial value and may be followed by `ready_for_settlement` (upon a successful capture) or `abort` (if a valid cancel request was sent).
Copy link
Contributor

Choose a reason for hiding this comment

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

after reading the whole auth/capture section, I still do not understand how the valid status transition. Can we have a detailed diagraph like we are making for lip-1?

In this object the initiator VASP declares its intent for the pre-approval, this can be one of two options:
1. Save the consumer sub-account for future transactions (save_sub_account)- this will enable the initiator VASP (merchant) to charge the sub-account in the future but will require the owner to approve the transaction. When using this option, amount limits are not required
2. Save the consumer sub-account and get a consent for future payments (consent) - this enables the initiator VASP (merchant) to charge the sub-account without any interaction with the owner.
Also, the scope limits the FundPullPreApprovalObject to certain parameters of time and amount. This object can be later mutated by the initiator VASP if needed, but any change requires the target VASP to approve the scope change.
Copy link
Contributor

Choose a reason for hiding this comment

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

what is "sub_account" means?
use name "type" is confusing here, as type indicates the object type, which is protocol level,
but it looks like a business agreement about how the next payment object can be processed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"sub_account" is the consumer identifier within (behind) the VASP.
It might be a good idea to replace it with another term that indicates the wallet user.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, interesting insight, we should determine a uniform fashion to call these out. I am concerned that subaccount doesn't really refer to a client's account and I've heard a lot of confusion across subaddress and child vasps :/

| Field | Type | Required? | Description |
|------- |-------- |----------- |------------- |
| unit | str enum | N | One of: "day", "week", "month", "year" |
| value | int | N | Number of "unit"(s) |
Copy link
Contributor

Choose a reason for hiding this comment

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

what is difference between above 2 fields with "expiration_timestamp"? they are also confusing because different application may have different interpretation about what means of start of the day/week/month/year, hence not accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fields are for limiting (scoping) the fund pulls from the consumer VASP. It can't be expressed using an expiration time, as it might be "not more than $100 a week".
Regarding the interpretation of week start, duration, etc. - we think of it as a sliding time window, but one may think it's a calendar timeframe.
I will add a more detailed description of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

unit - reset_period?
value - amount_per_period

also I would probably move max_amount and currency to the same level since currency refers to both...

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.

Really excited to see the enthusiasm in the space! And it makes it painfully difficult how hard it can be to work effectively in github for doc related reviews :P

I think the biggest contribution here is to enable the notion of periodic resettable limits within a single approval --- that makes some sense. I'm not aware of whether or not this is a feature that existing payment providers offer, so would we potentially be increasing the barrier to entry? With that being said, I do like the concept and am wondering if it should be a different command or a follow up.


This LIP does not contain the initial phase of a sub-account discovery that is required to start negotiating merchant scenarios.
The process described below starts at the phase that both sides (particularly the biller side) have the relevant subaddresses.
For now, we will assume that such a discovery process took place and the merchant have the buyer subaddress at hand.
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 the normal approach would be to describe a general strawman flow, then explicitly state what is assumed (e.g., that there is a user -> merchant identity exchange process). As I noted in another PR on DIP-8, there's so much boiler plate before getting into what this DIP does, that it ... isn't particularly accessible. You calling this out, means you see it too :).

---
# Specification
---

# Fund Pull Pre-Approval
### *VERSION SUPPORT: Supported only in version 1 of off-chain APIs*

Establishes a relationship between sender and recipient where the recipient can then pull funds from the sender without sender approving each transaction. This allows recipient to bill the sender without sender approving each payment. This relationship exists between a subaddress on the biller side and a subaddress on the sender side. After this request is POSTed, the target VASP can use out-of-band methods to determine if this request should be granted. If the target VASP chooses to allow the relationship to be established, the biller can create a payment object and POST to the billed party’s VASP to request funds. The “funds_pull_approval_id” object must then match the ID established by this request.
Establishes a relationship between the sender and recipient where the recipient can now pull funds from the sender without the sender approving each transaction. This allows the recipient to bill the sender without the sender approving each payment. This relationship exists between a subaddress on the biller side and a subaddress on the sender's side. After this request is POSTed, the target VASP can use out-of-band methods to determine if this request should be granted. If the target VASP chooses to allow the relationship to be established, the biller can create a payment object and POST to the billed party’s VASP to request funds. The “funds_pull_approval_id” object must then match the ID established by this request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's another assumption, an out-of-band process, I think having a strawman definition of this protocol broken down into bullets would be more accessible. We could even allude to one such method for doing the exchanges.

"amount": 1000,
"currency": "LBR"
"funds_pre_approval_id": "lbr1pg9q5zs2pg9q5zs2pg9q5zs2pgyqqqqqqqqqqqqqqspa3m_7b8404c986f53fe072301fe950d030de",
"scope": {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the value of having another data structure? could we amend the PR description or is there a tracking doc for this?

@@ -101,16 +116,15 @@ For a fund pre-approval request, the [command_type](https://lip.libra.org/lip-1/

### FundPullPreApprovalObject

The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can by updated multiple times. Updating immutable fields with a different value results in a command error, but it is acceptable to re-send the same value.
The structure in this object can be a full pre-approval or just the fields of an existing pre-approval object that need to be changed. Some fields are immutable after they are defined once (see below). Others can be updated multiple times. Updating immutable fields with different values results in a command error, but it is acceptable to re-send the same value. It should be noted that initial creation of the FundPullPreApprovalObject can be created or updated by the VASP on either side (buyer or seller).
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems kinda weird to me, it feels like it would make more sense for this to be something like a client submits a pre-approval to the merchant's VASP. There shouldn't be any mutable fields either it is agreeable or not. If it is not, return back some error that the service can then address. If the services should support follow up pre-approvals, those should be separate interactions not building on the same "object". I should have asked this on Kevin's PR, but I'll ask it on yours instead.

| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| address | str | Required for creation | Address of account from which the pre-approval is being requested. Addresses may be single-use or valid for a limited time, and therefore VASPs should not rely on them remaining stable across time or different VASP addresses. The addresses are encoded using bech32. The bech32 address encodes both the address of the VASP as well as the specific user's subaddress. They should be no longer than 80 characters. Mandatory and immutable. For Libra addresses, refer to the "account identifier" section in LIP-5 for format. |
| biller_address | str | Required for creation | Address of account from which billing will happen. Addresses may be single-use or valid for a limited time, and therefore VASPs should not rely on them remaining stable across time or different VASP addresses. The addresses are encoded using bech32. The bech32 address encodes both the address of the VASP as well as the specific user's subaddress. They should be no longer than 80 characters. Mandatory and immutable. For Libra addresses, refer to the "account identifier" section in LIP-5 for format. |
| funds_pre_approval_id | str | Y | Unique reference ID of this pre-approval on the pre-approval initiator VASP (the VASP which originally created this pre-approval object). This value should be unique, and formatted as “<creator_vasp_onchain_address_bech32>_<unique_id>”. For example, ”lbr1pg9q5zs2pg9q5zs2pg9q5zs2pgyqqqqqqqqqqqqqqspa3m_7b8404c986f53fe072301fe950d030de“. Note that this should be the VASP address and thus have a subaddress portion of 0. This field is mandatory on pre-approval creation and immutable after that. Updates to an existing pre-approval must also include the previously created pre-approval ID. |
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 the key thing here is .... how do we distinctly distinguish different objects... a reference_id is supposed to be common to all objects -- if there is a shared object to be created or maintained. Also it might behove us to pivot to DiemId rather than further embracing the jankiness of subaddresses, they are really dizzying.

In this object the initiator VASP declares its intent for the pre-approval, this can be one of two options:
1. Save the consumer sub-account for future transactions (save_sub_account)- this will enable the initiator VASP (merchant) to charge the sub-account in the future but will require the owner to approve the transaction. When using this option, amount limits are not required
2. Save the consumer sub-account and get a consent for future payments (consent) - this enables the initiator VASP (merchant) to charge the sub-account without any interaction with the owner.
Also, the scope limits the FundPullPreApprovalObject to certain parameters of time and amount. This object can be later mutated by the initiator VASP if needed, but any change requires the target VASP to approve the scope change.
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, interesting insight, we should determine a uniform fashion to call these out. I am concerned that subaccount doesn't really refer to a client's account and I've heard a lot of confusion across subaddress and child vasps :/

| Field | Type | Required? | Description |
|------- |-------- |----------- |------------- |
| unit | str enum | N | One of: "day", "week", "month", "year" |
| value | int | N | Number of "unit"(s) |
Copy link
Contributor

Choose a reason for hiding this comment

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

unit - reset_period?
value - amount_per_period

also I would probably move max_amount and currency to the same level since currency refers to both...

Comment on lines +291 to +297
## Cancellation

This LIP describes two independent phases of a payment - pre-approval and auth/capture.

The first one (pre-approval) may be canceled by either side (buyer or seller). A reasonable scenario is when a consumer wishes to cancel a subscription (buyer cancel) or asks a merchant app to remove the user wallet from the list of payment methods (seller cancel).

The second (authorization) could be canceled only by the biller (merchant) as it holds a guarantee of funds availability if requested. This has no reason to be canceled by the buyer. A request for such cancellation from the buyer should be rejected.
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't there be a follow up command to cancel?

I think having a clean story of how these pieces fit together would be immensely valuable for this DIP. As a super noob to the space, I'm struggling a bit to see how all these fit together.

Base automatically changed from master to main March 10, 2021 22:40
@davidiw
Copy link
Contributor

davidiw commented Apr 9, 2021

feel free to re-open but this is pretty stale now

@davidiw davidiw closed this Apr 9, 2021
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.

10 participants