[SEP-6] Add location_id to withdrawal #1433

Ifropc · 2024-01-27T00:57:01Z

Background

One of partners requires for user to choose a location to be selected. This location will be used to pick up funds

Proposed change

Add location_id to withdrawal requests

philipliu · 2024-01-28T03:28:23Z

ecosystem/sep-0006.md

+### Extra object schema
+
+This object is used to pass any additional info to the anchor. Anchors are free to add any field to this object. Anchors
+should notify wallets about fields they are expecting to receive. However, it's recommended to submit a change to this


How will Anchors communicate this to the wallet? Should the schema be included in the /info response?

JakeUrban · 2024-01-30T00:27:19Z

Its unclear if fields listed in /info need to be SEP-9 or can be custom. The fields dest and dest_extra are defined in the SEP itself, where as others in the example are SEP-9 fields, so its unclear if location_id should be defined somewhere as well. As long as we clarify I think this approach is fine.

I'm not opposed to simply adding an optional location_id argument. It is unclear how using an object like extra would work in the context of GET URL arguments.

JakeUrban

I'm not 100% convinced we should be adding a types map to deposit info objects.

I understand that this approach makes sense because the withdraw objects use the same approach, but withdraws and deposits are different. With withdraws, you need to tell the anchor your financial account information so they can send you funds off-chain. But with deposits, the anchor actually needs to tell you their financial account information so you can send the funds off-chain. Thats why the deposit info objects don't have a types map today -- there is typically no extra non-KYC information that is needed, and that is why we have an instructions field in the deposit endpoints response.

Location selection seems like the one exception to that rule. The anchor can't tell the user which location to go to, the user needs to pick. I can't think of another type of deposit that would require extra arguments from the client.

Because of that, I think I still prefer adding location_id as a first-class parameter to deposit request endpoints, rather than it being a custom field defined in GET /info. If you can point out other potential uses of a types array for deposits, I'm willing to change my mind.

EDIT: actually, I know its not a requirement for MGI since you can pick up cash at any location, but should we consider adding location_id to withdraw request endpoints as well?

JakeUrban · 2024-01-31T00:42:40Z

ecosystem/sep-0006.md

+      "types": {
+        "location": {
+          "fields": {
+            "id": {
+              "description": "ID of location user must visit to pick up cash"
+            }
+          }


I think the example should be something like:

Suggested change

"types": {

"location": {

"fields": {

"id": {

"description": "ID of location user must visit to pick up cash"

}

}

"types": {

"cash": {

"fields": {

"location_id": {

"description": "ID of location user must visit to pick up cash"

}

}

Same for the other examples

JakeUrban · 2024-01-31T00:47:50Z

ecosystem/sep-0006.md

-        "bank_account": {
-          "fields": {
-              "dest": {"description": "your bank account number" },
-              "dest_extra": { "description": "your routing number" },
-              "bank_branch": { "description": "address of your bank branch" },
-              "phone_number": { "description": "your phone number in case there's an issue" },
-              "country_code": {
-                "description": "The ISO 3166-1 alpha-3 code of the user's current address",
-                "choices": ["USA", "PRI"]
-              }
-          }
-        },


To confirm, we're removing this because the bank_account type is requesting the user's KYC info like phone number and bank account number right? And we want them to do this via SEP-12?

Don't we still need the client to provide the type=bank_account query parameter though, so the anchor knows to request bank account fields in the GET /customer response? If that is the case, then we need to do something like this:

"bank_account": {}

This signals that the type should be passed but there are no extra fields that need to passed in the transaction creation request.

If we do it like described above, that would mean that we accept field named bank_account with empty body though, am I wrong?
We still have type in the request, it should be used as before

If we do it like described above, that would mean that we accept field named bank_account with empty body though, am I wrong?

On what endpoint would anchors accept a "bank_account": {} key-value pair from wallet requests? I'm referring to the fact that GET /info responses should have all possible values of the type query parameter as keys. So we shouldn't remove the entire bank_account object, just the items within the object, since no additional parameters should be sent to the anchor other than the type parameter in that case.

types in info describes type of on object, not of a string. type of type parameter from the request is string. With current protocol we can't describe acceptable values of type`, because it's not an object.
That being said, I agree with just adding location_id

JakeUrban · 2024-01-31T00:56:55Z

ecosystem/sep-0006.md

+  type of deposit. In the case that the Anchor requires additional fields for a deposit, it should set the
+  transaction status to `pending_customer_info_update`. The wallet can query the `/transaction` endpoint to get the
+  fields needed to complete the transaction in `required_customer_info_updates` and then use


This stops mid-sentence. I also think you mean pending_transaction_info_update and required_info_updates.

Ifropc · 2024-01-31T22:02:17Z

I don't mind just adding location_id directly, but I think it's more flexible if we allow anchors to define their own fields, so we don't need to modify protocol for any small deviation from SEP-6 protocol

JakeUrban · 2024-01-31T22:17:46Z

I agree it is valuable that your solution provides anchors a way to request their own custom fields, and if we were starting from scratch I think this is the right approach. The reason I think we should try to avoid making this change is simply because it is breaking, and it is unclear how common the need for custom fields will be.

I also think that a location_id parameter makes sense to include in an on/off ramp standard API. Right now MGI is the only cash-agent network that supports a stellar on/off ramp standard, but any future cash-agent network anchor will need this. And if we don't standardize the location_id parameter, they'll define their own parameter in GET /info which could use a different name.

JakeUrban · 2024-02-05T17:57:10Z

Ok, it sounds like we're on the same page of adding location_id to request parameters rather than the approach defined here, so I'll close this PR.

Ifropc · 2024-02-06T20:04:08Z

Reopened PR to preserve discussion history

JakeUrban

Approved with edits

JakeUrban · 2024-02-09T17:19:47Z

ecosystem/sep-0006.md

@@ -828,6 +829,7 @@ Request Parameters:
 | `country_code`                | string                  | (optional) The [ISO 3166-1 alpha-3](https://en.wikipedia.org/wiki/ISO_3166-1_alpha-3) code of the user's current address. This field may be necessary for the anchor to determine what KYC information is necessary to collect.                                                                                                                                                                                                                                       |
 | `claimable_balance_supported` | string                  | (optional) `true` if the client supports receiving deposit transactions as a claimable balance, `false` otherwise.                                                                                                                                                                                                                                                                                                                                                    |
 | `customer_id`                 | string                  | (optional) id of an off-chain account (managed by the anchor) associated with this user's Stellar account (identified by the JWT's `sub` field). If the anchor supports [SEP-12], the `customer_id` field should match the [SEP-12] customer's id. `customer_id` should be passed only when the off-chain id is know to the client, but the relationship between this id and the user's Stellar account is not known to the Anchor.                                   |
+| `location_id`                 | string                  | (optional) id of the chosen location for cash pick up cash                                                                                                                                                                                                                                                                                                                                                                                                            |


JakeUrban · 2024-02-09T17:20:04Z

ecosystem/sep-0006.md

@@ -726,6 +726,7 @@ Request parameters:
 | `refund_memo`        | string                  | (optional) The memo the anchor must use when sending refund payments back to the user. If not specified, the anchor should use the same memo used by the user to send the original payment. If specified, `refund_memo_type` must also be specified.                                                                                                                                                                                                                  |
 | `refund_memo_type`   | string                  | (optional) The type of the `refund_memo`. Can be `id`, `text`, or `hash`. See the [memos](https://developers.stellar.org/docs/encyclopedia/memos) documentation for more information. If specified, `refund_memo` must also be specified.                                                                                                                                                                                                                             |
 | `customer_id`        | string                  | (optional) id of an off-chain account (managed by the anchor) associated with this user's Stellar account (identified by the JWT's `sub` field). If the anchor supports [SEP-12], the `customer_id` field should match the [SEP-12] customer's id. `customer_id` should be passed only when the off-chain id is know to the client, but the relationship between this id and the user's Stellar account is not known to the Anchor.                                   |
+| `location_id`        | string                  | (optional) id of the chosen location for cash pick up cash                                                                                                                                                                                                                                                                                                                                                                                                            |


JakeUrban · 2024-02-09T17:20:49Z

Also, don't forget to update the updated-at and version fields.

# Conflicts: # ecosystem/sep-0006.md

Ifropc requested review from JakeUrban and philipliu January 27, 2024 00:57

philipliu reviewed Jan 28, 2024

View reviewed changes

Ifropc marked this pull request as draft January 29, 2024 20:34

Ifropc changed the title ~~[SEP-6] Add extra field~~ [SEP-6] Add types to /info deposits, update description for withdrawal Jan 30, 2024

Ifropc marked this pull request as ready for review January 30, 2024 20:51

JakeUrban reviewed Jan 31, 2024

View reviewed changes

JakeUrban closed this Feb 5, 2024

Ifropc reopened this Feb 6, 2024

Ifropc changed the title ~~[SEP-6] Add types to /info deposits, update description for withdrawal~~ [SEP-6] Add location_id to withdrawal Feb 6, 2024

Add location_id

30d7862

Ifropc force-pushed the SEP-6-extra branch from 75a9a89 to 30d7862 Compare February 6, 2024 20:03

philipliu previously approved these changes Feb 8, 2024

View reviewed changes

JakeUrban previously approved these changes Feb 9, 2024

View reviewed changes

Updates

2ba38f0

Ifropc dismissed stale reviews from JakeUrban and philipliu via 2ba38f0 February 9, 2024 20:37

Ifropc added 3 commits February 9, 2024 12:40

Merge branch 'master' into SEP-6-extra

b37ac9f

Merge remote-tracking branch 'origin/master' into SEP-6-extra

c7369e4

# Conflicts: # ecosystem/sep-0006.md

Merge branch 'master' into SEP-6-extra

270c164

JakeUrban approved these changes Feb 21, 2024

View reviewed changes

Merge branch 'master' into SEP-6-extra

5210de9

Ifropc enabled auto-merge (squash) February 21, 2024 19:05

Ifropc merged commit c521ed4 into master Feb 21, 2024
2 checks passed

Ifropc deleted the SEP-6-extra branch February 21, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEP-6] Add location_id to withdrawal #1433

[SEP-6] Add location_id to withdrawal #1433

Ifropc commented Jan 27, 2024 •

edited

Loading

philipliu Jan 28, 2024

JakeUrban commented Jan 30, 2024 •

edited

Loading

JakeUrban left a comment •

edited

Loading

JakeUrban Jan 31, 2024

JakeUrban Jan 31, 2024

JakeUrban Jan 31, 2024

Ifropc Jan 31, 2024

JakeUrban Jan 31, 2024

Ifropc Feb 1, 2024

JakeUrban Jan 31, 2024

Ifropc commented Jan 31, 2024

JakeUrban commented Jan 31, 2024 •

edited

Loading

JakeUrban commented Feb 5, 2024

Ifropc commented Feb 6, 2024

JakeUrban left a comment

JakeUrban Feb 9, 2024

JakeUrban Feb 9, 2024

JakeUrban commented Feb 9, 2024

	\| `location_id` \| string \| (optional) id of the chosen location for cash pick up cash \|
	\| `location_id` \| string \| (optional) id of the chosen location to drop off cash \|

[SEP-6] Add location_id to withdrawal #1433

[SEP-6] Add location_id to withdrawal #1433

Conversation

Ifropc commented Jan 27, 2024 • edited Loading

Background

Proposed change

Choose a reason for hiding this comment

JakeUrban commented Jan 30, 2024 • edited Loading

JakeUrban left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ifropc commented Jan 31, 2024

JakeUrban commented Jan 31, 2024 • edited Loading

JakeUrban commented Feb 5, 2024

Ifropc commented Feb 6, 2024

JakeUrban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeUrban commented Feb 9, 2024

Ifropc commented Jan 27, 2024 •

edited

Loading

JakeUrban commented Jan 30, 2024 •

edited

Loading

JakeUrban left a comment •

edited

Loading

JakeUrban commented Jan 31, 2024 •

edited

Loading