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-746] Add SEP-12 customer callback support #1444

Closed
Ifropc opened this issue Aug 1, 2024 · 10 comments
Closed

[ANCHOR-746] Add SEP-12 customer callback support #1444

Ifropc opened this issue Aug 1, 2024 · 10 comments
Assignees

Comments

@Ifropc
Copy link
Contributor

Ifropc commented Aug 1, 2024

Background:

Following SEP-6 flow update, the KYC gathering logic is now the following:

Once SEP-6 transaction is in the pending_customer_info_update , wallet should call SEP-12 GET /customer?transaction_id=<id> to gather all KYC fields that is expected to be provided.

Wallet then calls PUT /customer?transaction_id=<id> to update KYC. GET/PUT are called in sequence until KYC reaches ACCEPTED/REJECTED status. This works well until PROCESSING is added to the flow. In that case wallet needs to continuously pull GET /customer to check if status has been updated. This is not ideal considering that this call would need to be made from the user device for non-custodial wallets.

Instead, we should add SEP-12 customer callback that notifies wallet backend about necessary change. We should update notify_customer_info_updated RPC. When called, it should fetch latest KYC using callback to business server (get /customer) and:

Update transaction status to pending_customer_info_update (NEEDS_INFO); pending_anchor (PROCESSING); pending_user_transfer_start (ACCEPTED); error (REJECTED)

We should add a new callback to send customer information. We can use /sep12 path in v2 and organize it better in v3

Slack discussion thread
Internal issue: ANCHOR-746

@philipliu
Copy link
Contributor

Rather than calling the customer callback in NotifyCustomerInfoUpdated. What do you think of introducing a new parameter called status (SEP-12 status)? The business server should have access to the customer status and we can avoid new failure mode in the RPC this way.

@Ifropc
Copy link
Contributor Author

Ifropc commented Aug 1, 2024

We could do that, but the idea is that in a callback we need to pass the the formed KYC response in the callback, back to the wallet. So we need to make a call to BS either way

Ref:
https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0012.md#customer-callback-put

Whenever the user's status field changes, the anchor will issue a POST request to the callback URL. The payload of the POST request will be the same as the response of GET /customer. Anchors will submit POST requests until the user's status changes to ACCEPTED or REJECTED. If a wallet needs to watch a user's KYC status after that, it will need to set a callback again.

@philipliu
Copy link
Contributor

So we need to make a call to BS either way

Right. The new failure mode I was trying to avoid was more along the lines of calling a callback from an RPC. We don't do this today. The customer callback will be made in the callback event processor.

@philipliu
Copy link
Contributor

Also, the ACCEPTED status probably should not move the transaction into pending_user_transfer_start as this is handled by the request funds RPCs.

@JakeUrban
Copy link
Contributor

Right. The new failure mode I was trying to avoid was more along the lines of calling a callback from an RPC. We don't do this today. The customer callback will be made in the callback event processor.

I think we need to make the request from RPC while processing the notify_customer_info_updated call. If we don't, the customer status could change again before we make the callback from the event processor.

@philipliu
Copy link
Contributor

This is how it works at the moment:

  1. Business server calls NotifyCustomerInfoUpdated(status = status, tx = TX_ID)
  2. RPC service updates transaction TX_ID with new transaction status
  3. Event processor makes a customer request to get latest customer status
  4. Event processor sends a customer event to to client servers

If I understand correctly, @JakeUrban, you are suggesting we make a customer request at 2?

@JakeUrban
Copy link
Contributor

Yes thats right 👍

@philipliu
Copy link
Contributor

There is always a risk that the customer's status may change before the event processor makes the callback. The issue arises from the latency between steps 2 and 4, during which these status changes can occur. It's unclear if making another request at step 2 would reduce this latency.

I think this situation is acceptable anyway. A change in the customer's status should trigger another NotifyCustomerInfoUpdated request so the wallet would eventually be informed about the updated status.

@JakeUrban
Copy link
Contributor

JakeUrban commented Aug 7, 2024

Lets say I'm an business and my customers go from NEEDS_INFO to PROCESSING to ACCEPTED very quickly.

If we go with the current approach, the business will make a notify_customer_info_updated request with a status parameter of PROCESSING. The anchor platform will update the transaction status, queue the customer event, and return.

Then the business could finish processing and make another notify_customer_info_updated with a status ACCEPTED. The anchor platform will do the same thing.

Now there are two customer callback status change events, but when the event processor calls the business' GET /customer callback for each event, the responses will be identical, and the client will never see a customer record with a PROCESSING status.

@philipliu
Copy link
Contributor

philipliu commented Aug 8, 2024

I understand your point.

Does the client need to see the PROCESSING status? If the customer quickly moves between statuses, they might only care about the final status. There is no action required from the customer for intermediate statuses like PROCESSING.

If we want this behavior, your approach works. However, I'm not sure about sending events from the RPC handler. Maybe there are other options we can consider. We can explore the implementation in the PR rather than discussing it here.

philipliu added a commit that referenced this issue Aug 13, 2024
### Description

Addresses
#1444.

This change:

- Updates the `NotifyCustomerInfoUpdated` RPC so that it updates the
transaction based on the requested Customer's status and sends a
Customer updated event. `customerId` and `type` are new parameters
needed to distinguish whether a customer is a SEP-31 receiver, sender,
or a SEP-6 customer.
- Updates SEP-12 service such that it passes the full SEP-12 customer
into the `AnchorEvent`. Previously only the customer ID was present in
the event. This is not a breaking change since the SEP-12 customer
object is serializer backward compatible since it also contains an ID.
- Sends a SEP-12 status callback to all clients.

Callback configuration and end-to-end tests will be added in another PR.

### Context

N/A

### Testing

- `./gradlew test`

### Documentation

stellar-docs PR will be created

### Known limitations

N/A
@Ifropc Ifropc closed this as completed Aug 26, 2024
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

No branches or pull requests

3 participants