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

[ANCHOR-719] Update SEP31 to replace SEP12 with async kyc flow #1502

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Jul 12, 2024

What's Changed:

SEP-6, 8, 30
Nits and prettify

SEP-12
Nits

SEP-31

  1. KYC completion is no longer required before creating a SEP-31 transaction. Sending anchor can optionally pre-register customers and use the obtained sender_id or receiver_id to create transaction, or leave these fields empty.

  2. After the transaction is created, the Sending anchor will call GET /customer?transaction_id=<sep31-txn-id>&type= to determine the required fields for each type of client, and then make a PUT call to the same endpoint to update the information

Copy link
Contributor

@philipliu philipliu left a comment

Choose a reason for hiding this comment

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

This change makes a lot of sense and brings SEP-31 closer to the new KYC standard set by SEP-6, although the current approach would break existing clients. I left a recommendation for how we can avoid this.

ecosystem/sep-0012.md Outdated Show resolved Hide resolved
ecosystem/sep-0012.md Outdated Show resolved Hide resolved
ecosystem/sep-0031.md Outdated Show resolved Hide resolved
}
}
}
"max_amount": 1000
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 it's possible to use the new KYC flow and keep the sep12 block right? The new flow works if the SEP-12 request is type=sep31-large-sender and transaction_id=.... The special types sep31-sender and sep31-receiver can be hardcoded values we use in the Anchor Platform but we don't need to restrict them at the protocol level. Otherwise, this would be a breaking change for SEP-31 clients.

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's possible, but I don't think it's necessary because the sending anchor no longer needs to classify customers.

In the new flow, sending anchors can pre-register the sender and receiver with some basic information. Then, it’s up to the receiving anchor to determine what additional information is needed (such as whether the customer is foreign or a large sender) based on the transaction details and return the required fields to the sending anchor. This way, the receiving anchor can manage its own classified KYC customer decision-making process.

What are your thoughts?

Additionally, since SEP-31 is not currently being adopted by any clients, it's a good opportunity to make these improvements.

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 mind getting rid of custom-define types entirely, but we would need to deprecate it, rather than straight removing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also worry about some gaps in this flow with customized customer type.

If the sending anchor create a SEP-31 transaction without pre-register the customers, and is doing kyc now, how would they know what customer type they should use in the GET /cutomer?transaction_id=<>&type=<> call?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's up for the anchor to decide, generally. Client is only supposed to either use sep-6 or sep-31 when interacting with business that uses AP

ecosystem/sep-0031.md Outdated Show resolved Hide resolved
@Ifropc Ifropc self-requested a review July 16, 2024 20:32
@@ -86,11 +86,11 @@ sequenceDiagram
Anchor-->>-Wallet: transaction id

note right of Wallet: If User has already <br> been accepted, <br> KYC is unnecessary.
Wallet->>+Anchor: GET [SEP-12]/customer?transaction_id=<id>&type=
Copy link
Contributor

Choose a reason for hiding this comment

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

Type should still be passed. Theoretically anchor can use numerical ids for transactions (by type), therefore it'd need to distinguish between sep-6 transaction id N and SEP-31.
In addition, it makes implementation on AP more straightforward (otherwise we need to query both tables to find transaction with this ID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can add it back. Just curious, what should the customer type be for SEP-6? Currently, there is no configuration to define a SEP-6 customer type

Copy link
Contributor

Choose a reason for hiding this comment

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

Should just pass types as sep-6 and sep-31. We can make recommendation in the standard to follow this rule, but that's generally how AP should be implemented to use the correct table

ecosystem/sep-0012.md Outdated Show resolved Hide resolved
ecosystem/sep-0012.md Outdated Show resolved Hide resolved
the Sending Client to make the payment to the Receiving Anchor.
1. The Sending Anchor makes [SEP-12 PUT /customer](sep-0012.md#customer-put) requests containing the collected
information for each client.
1. Optionally the Sending Anchor can pre-register clients using [SEP-12 PUT /customer](sep-0012.md#customer-put) and
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 it'd be nice to have a bit more detailed description of how it works (basically explaining flow chart above)

}
}
}
"max_amount": 1000
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 mind getting rid of custom-define types entirely, but we would need to deprecate it, rather than straight removing

Copy link
Contributor

@philipliu philipliu left a comment

Choose a reason for hiding this comment

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

The changes look good. We should also update the SEP-31 version and changelog.

Copy link
Contributor

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

LGTM.
Could you update versions, run formatting (./bin/prettier.sh) and write a comment that for integrations using Anchor Platform type should be sep6 and sep31? This would be a guideline for wallets who sometimes look at this spec.

@JiahuiWho
Copy link
Contributor Author

JiahuiWho commented Jul 23, 2024

LGTM. Could you update versions, run formatting (./bin/prettier.sh) and write a comment that for integrations using Anchor Platform type should be sep6 and sep31? This would be a guideline for wallets who sometimes look at this spec.

How to distinguish sender from receiver if we are just using sep31? Or is this something up for anchor to decide?
The prettier does't give me anything on local, not sure what is going wrong...

@Ifropc
Copy link
Contributor

Ifropc commented Jul 23, 2024

The anchor that receives KYC should know if it's receiving or sending anchor based on transaction id I think

@JiahuiWho JiahuiWho merged commit 33447eb into ap-v3 Jul 24, 2024
2 of 3 checks passed
@JiahuiWho JiahuiWho deleted the anchor-719-update-sep31 branch July 24, 2024 15:36
JiahuiWho added a commit that referenced this pull request Aug 6, 2024
* [ANCHOR-719] Update SEP31 to replace SEP12 with async kyc flow (#1502)

* update sep31 to leverage sep12 with txn_id

* more prettify

* address comments

* more comments

* fix sep31 diagram (#1513)

* address comments

* address comments

* prettier
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