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

PARTNER-146: Add external_account_id to SEP-9 #1410

Merged
merged 8 commits into from
Oct 24, 2023
Merged

PARTNER-146: Add external_account_id to SEP-9 #1410

merged 8 commits into from
Oct 24, 2023

Conversation

Ifropc
Copy link
Contributor

@Ifropc Ifropc commented Oct 19, 2023

No description provided.

@Ifropc Ifropc requested a review from JakeUrban October 19, 2023 23:07
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

What is the difference between the id sent in SEP-12 requests vs. the external_account_id parameter? They seem like the same thing to me.

@CassioMG
Copy link
Contributor

What is the difference between the id sent in SEP-12 requests vs. the external_account_id parameter? They seem like the same thing to me.

Very good point. I guess it would depend on how the partner is planning on handling things on their side, but yes I believe they could potentially be the same thing as Sep-12's id field refers to the account id on the Anchor(partner) side.

@Ifropc
Copy link
Contributor Author

Ifropc commented Oct 20, 2023

What is the difference between the id sent in SEP-12 requests vs. the external_account_id parameter? They seem like the same thing to me.

Ideally, it should be the same (I don't see why they should be different, actually). This field is to be used in other SEPs that may not use SEP-12, to pass the customer's ID along the KYC (mainly, it's for SEP-24)
Do you think that it makes sense to just add it there instead?

@JakeUrban
Copy link
Contributor

I think so. Adding something like customer_id to the SEP-24 (and SEP-6) transaction initiation endpoint, noting that this field should match the value returned from the anchor's SEP-12 PUT /customer response, seems to make sense.

@JakeUrban
Copy link
Contributor

This matches SEP-31's design of having sender_id and receiver_id fields in its transaction initiation request.

@Ifropc
Copy link
Contributor Author

Ifropc commented Oct 20, 2023

@JakeUrban do you think we need to add customer_id to SEP-6, considering client first calls SEP-12 in the flow?

@JakeUrban
Copy link
Contributor

JakeUrban commented Oct 20, 2023

I think adding the customer_id parameter to SEP-6 actually makes sense in that case, where the client already knows the customer ID. We're changing the SEP-6 flow to also allow clients to initiate transactions before passing KYC though, so in that case the parameter wouldn't make sense. But if we make it optional, I don't see a problem with adding it.

@Ifropc
Copy link
Contributor Author

Ifropc commented Oct 20, 2023

I think it actually makes sense to pass customer_id before KYC.
If it's passed after, the business should already have tied JWT to their internal account.
However, if it's done before, business can do the assignment of Stellar account to the internal account at this step.

@Ifropc Ifropc requested a review from JakeUrban October 20, 2023 19:12
ecosystem/sep-0024.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
@JakeUrban
Copy link
Contributor

JakeUrban commented Oct 20, 2023

Ah you're right. The customer_id attribute only needs to be passed if

  • the customer exists in the anchor's records
  • the client somehow knows the ID
  • the anchor doesn't know the customer's stellar account
    • and therefore wasn't registered via SEP-12 yet, if they need to be at all

@@ -331,6 +331,7 @@ Name | Type | Description
`amount` | string | (optional) The amount of the asset the user would like to deposit with the anchor. This field may be necessary for the anchor to determine what KYC information is necessary to collect.
`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 account associated with this user (identified by JWT) used by the anchor. If the anchor supports [SEP-12], the `customer_id` field should match [SEP-12] Customer's 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 don't quite understand the definition.

ID of an account associated with this user (identified by JWT) used by the anchor.

Is the account supposed to be some kind of off-chain record, like a github account? Or are you referring to a Stellar account here?

When you say (identified by JWT), are you referring to the user or their account mentioned above?

Unrelatedly, lets make sure we're being consistent with how we use "id" vs. "IDand remove the capital C inCustomer's`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the account supposed to be some kind of off-chain record, like a github account

Yes, off-chain account (internal anchor's account)

When you say (identified by JWT), are you referring to the user or their account mentioned above?

Account + memo combination

I will rephrase description so it's more clear

JakeUrban
JakeUrban previously approved these changes Oct 20, 2023
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

LGTM with the grammar edits made in my comment.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
Copy link
Contributor

@CassioMG CassioMG left a comment

Choose a reason for hiding this comment

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

Lgtm

@Ifropc Ifropc merged commit 1abf481 into master Oct 24, 2023
2 checks passed
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.

3 participants