stellar / java-stellar-anchor-sdk

Java SDK for the Stellar network anchor development.
Apache License 2.0
35 stars 33 forks source link

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

Closed Ifropc closed 2 weeks ago

Ifropc commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Yes thats right 👍

philipliu commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.