search

stellar / stellar-protocol

Developer discussion about possible changes to the protocol.
515 stars 302 forks source link

SEP-6: /deposit and /withdraw IDs should map to list of transactions rather than a single transaction #1341

Closed marcinx closed 3 months ago

marcinx commented 1 year ago

Hey.

Based on our experience as SEP-6 anchor and conversations we had with wallet service providers, we'd like to propose an approach to better consider the fact that SEP-6 deposits and withdrawals can contain more than a single fund transfer transaction.

Happy to hear everyone's feedback, discuss, and proceed with a draft pull request.

Problem

Deposits and withdrawals can be composed of many individual transactions. In its original design, however, SEP-6 assumes that a request to /deposit or /withdraw will always result in exactly one transaction. Situations where numerous deposits/withdrawals are sent to individual instructions are unaccounted for.

In the real world users cannot be prevented from sending additional payments to instructions received by /deposit or /withdraw and there are some use cases where this is actually required. We see people underpaying their invoices literally every day and then sending additional transactions to cover the difference. It's a very real case.

Solution

We think that the way to account for these transactions is to map the deposit and withdraw ids to a list of transaction objects rather than a single transaction object.

Therefore, we'd like to propose the following (backwards compatible) approach:

/transaction endpoint

Example: 

/transaction?id=085a2e4aca82edd52d12b37388da

{
   "transaction":{
      "id":"085a2e4aca82edd52d12b37388da-814f2fe117-0",
      "status":"completed",
      "message":"Transaction fully completed.",
      "kind":"deposit",
      "amount_in":"0.0001000",
      "amount_in_asset":"iso-24165:BTC",
      "amount_out":"0.0001000",
      "amount_out_asset":"stellar:BTC:GCQVEST7KIWV3KOSNDDUJKEPZLBFWKM7DUS4TCLW2VNVPCBGTDRVTEIT",
      "amount_fee":"0.0000000",
      "from":"bc1qj633nx575jm28smgcp3mx6n3gh0zg6ndr0ew23",
      "started_at":"2022-10-04T20:50:18.307Z",
      "completed_at":"2022-10-04T20:51:20.032Z",
      "stellar_transaction_id":"a426a057aaba5a5811d5ee243f76c767a6756df89f417610a4ebd114e162596a",
      "external_transaction_id":"814f2fe1175101d5d1b3fc75c053cc090590bb0e251b938c255598c828742c43"
   },
   "related_transactions":[
      {
         "transaction":{
            "id":"085a2e4aca82edd52d12b37388da-e1175101d5-0",
            "status":"completed",
            "message":"Transaction fully completed.",
            "kind":"deposit",
            "amount_in":"0.0001000",
            "amount_in_asset":"iso-24165:BTC",
            "amount_out":"0.0001000",
            "amount_out_asset":"stellar:BTC:GCQVEST7KIWV3KOSNDDUJKEPZLBFWKM7DUS4TCLW2VNVPCBGTDRVTEIT",
            "amount_fee":"0.0000000",
            "from":"bc1qj633nx575jm28smgcp3mx6n3gh0zg6ndr0ew23",
            "started_at":"2022-10-04T20:49:18.307Z",
            "completed_at":"2022-10-04T20:50:20.032Z",
            "stellar_transaction_id":"aaba5a5811d5ee243f76c767a6756df89f417610a4ebd114e162596aa426a057",
            "external_transaction_id":"e1175101d5d1b3fc75c053cc090590bb0e251b938c255598c828742c43814f2f"
         }
      }
   ]
}

/transactions endpoint

Conclusion

This approach is what we landed on in production after conversation with wallet service providers and based on our own observations in our product requirements. It is a backwards compatible change and takes into account the fact that a deposit or withdrawal does not always result in exactly one transaction.

Happy to hear everyone's feedback, discuss, and proceed with a draft pull request.

@mikestabronik @JakeUrban @sui77 @anexgo @antb123 @ivanmudryj @thomazbt

JakeUrban commented 1 year ago

Hey @marcinx, thanks for sharing this! I agree that this is a gap worth addressing in SEP-6 and SEP-24, although probably not SEP-31 since it involves a B2B transfer where the two businesses have a commercial agreement.

I think what we should discuss first is whether or not these additional payments should be defined as first-order transactions. Do we really need these payments to be query-able by ID on the endpoint? Do they need to have their own status, amount_in/amount_out?

What if instead we added a payments list? This is actually somewhat similar to the refunds object.

{
  "transaction": {
    "id":"085a2e4aca82edd52d12b37388da-814f2fe117-0",
    "status":"completed",
    "message":"Transaction fully completed.",
    "kind":"deposit",
    "amount_in":"100",
    "amount_in_asset":"iso-24165:BTC",
    "amount_out":"100",
    "amount_out_asset":"stellar:BTC:GCQVEST7KIWV3KOSNDDUJKEPZLBFWKM7DUS4TCLW2VNVPCBGTDRVTEIT",
    "amount_fee":"0",
    "from":"bc1qj633nx575jm28smgcp3mx6n3gh0zg6ndr0ew23",
    "started_at":"2022-10-04T20:50:18.307Z",
    "completed_at":"2022-10-04T20:51:20.032Z",
    "stellar_transaction_id":"a426a057aaba5a5811d5ee243f76c767a6756df89f417610a4ebd114e162596a",
    "external_transaction_id":"814f2fe1175101d5d1b3fc75c053cc090590bb0e251b938c255598c828742c43"
    "payments": [
      {
        "id": "814f2fe1175101d5d1b3fc75c053cc090590bb0e251b938c255598c828742c43",
        "amount": "100",
        "asset": "iso-24165:BTC"
      }
    ]
  }
}

I think this may be a good middle ground. It fills the gap on how to handle underpayments and expose that data, but it avoids anchors having to implement two pieces of your solution that I think are nontrivial:

  1. Returning the most recent payment as the top level transaction for the same ID. I think this actually could be problematic for wallets that haven't updated, since they would show the the top-level transaction with all its amounts & timestamps changed.
  2. Making individual payments for a transaction query-able, which has impacts on their data model.

Question for you: are you sending users their Stellar BTC on each payment, even if they underpay? And in the same way, are you sending users native BTC if they underpay on the Stellar side? What if they over pay?

*Also, thanks for mentioning ISO 24165, I didn't know that was a thing. I'll add it to the asset identification format.

marcinx commented 1 year ago

Hey Jake,

Thanks for your follow up.

we should discuss first is whether or not these additional payments should be defined as first-order transactions.

I think they have to be. If SEP-6 gives me a BTC deposit address or SEPA bank account information I can send as many payments as I want. The anchor can't stop me and these payments should be accounted for.

Apart from underpaid invoices and other user errors, there are actually good use cases where an anchor wants to receive multiple transfers to the same deposit/withdrawal instructions given by SEP-6. In our product, for example, clients receive a dedicated and sticky BTC/LTC deposit address and can send arbitrary amounts and numbers of transactions. In this type of situation all transactions are of equal priority and need to have their own status, amount_in/amount_out, stellar_tx_id, external_tx_id, and so on. It would be good if we had a way to make sure that all payments made to a single deposit/withdraw instruction are equally queryable and accounted for in the transaction history.

Generally I don't think this is an issue isolated to us as an anchor but a fundamental design issue in SEP-6. The model currently assumes that there will only ever be a single transfer sent to a given deposit/withdraw instruction, however in reality this is not the case and the way things stand payments can be missing or unaccounted for. The solution seems to be to map deposit/withdraw ids to a list of payments rather than a single payment. Happy with any approach but I think the real challenge here is maintaining backwards compatibility.

Do we really need these payments to be query-able by ID on the endpoint?

I'd definitely want to be able to query each of my deposits/withdrawals individually but in it's current form it is unclear what /transaction?id=xxx should do when more than a single transfer was made to the deposit/withdraw instructions related to id. We'd be on the right track with any approach that identifies all payments as "first-order transactions" and makes them individually queryable. We use the related_transactions list at the moment but there are probably more elegant ways to solve all this. The payments approach probably won't work because it prohibts individual querying of arbitrary transfers.

Our original thought was that wallets could use stellar_tx_id or external_tx_id to query individual transfers belonging to a single deposit or withdrawal id. However, we've been told by wallet operators that they use the /transactions endpoint to display recent transfer activity and use the id contained therein to load and refresh from /transaction?id=xxx. Apparently this is the only mandatory query param on the /transaction endpoint and we were told that some anchors don't support querying by stellar or external tx id.

Do they need to have their own status, amount_in/amount_out?

Can't go without it. See above.

Returning the most recent payment as the top level transaction for the same ID. I think this actually could be problematic for wallets that haven't updated, since they would show the the top-level transaction with all its amounts & timestamps changed.

This is why we need a unique id in /transactions that resolves to the relevant tx directly. In cases where wallets query by deposit/withdraw id (and not the unique transaction id) the lowest hanging fruit seems to be to return the latest tx on top and the rest in an additional list. Wallets that query /transaction by deposit/withdraw id only and would have their txids and timestamps etc. mixed up, have much bigger problems at the moment anyway: payments are simply missing and unaccounted for because they cannot be queried. Additional payments might appear in /transactions but the way it currently stands they would have the same id if they were sent to a single deposit/withdraw instruction (and that causes some wallet operator problems -- check with Mike S.).

Making individual payments for a transaction query-able, which has impacts on their data model.

I agree that it's complicated (partly because of the way the SEP-6 model was originally thought up) but they have to be somehow query-able and accounted for. Currently they are not.

Question for you: are you sending users their Stellar BTC on each payment, even if they underpay? And in the same way, are you sending users native BTC if they underpay on the Stellar side? What if they over pay?

Every confirmed deposit tx on Bitcoin or Litecoin results in a mint on Stellar. Every confirmed withdraw tx on Stellar results in a burn and redemption on Bitcoin or Litecoin.

Happy to chat more and looking forward to ideas. Hope we can sort this all out slowly but steady.

marcinx commented 1 year ago

Hey.

I think the following examples illustrate the issue pretty well:

Current SEP-6 way:

Where is the second tx? Current SEP-6 design does not account for this type of (very real) usage. Payments are literally missing. Also, the payload of the /transaction?id=xxx endpoint changes each time a new tx is sent to the deposit address (newest one replaces all previous ones), i.e. the situation that you described above (with changing tx ids etc.) and that we would like to avoid actually exists in the current SEP-6 version already. Unless we go the radical way and enforce a list in /transaction, this behavior will be probably be hard to evade.

Proprietary solution by us:

Happy to move things around to make it more elegant and with any solution that addresses this within SEP-6 natively.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

marcinx commented 7 months ago

What happened to this issue? Are we just going to let it go stale? It's been a year. SEP-6 still does not understand how to handle multiple txs per /deposit or /withdraw.

JakeUrban commented 7 months ago

Hey @marcinx sorry for not responding to this earlier. I have my own reasons for why I don't think it makes sense to update the specification as you've proposed so far, and I can describe them below, but I think we need more ecosystem input here. If you know other businesses that would be interested in discussing this change it would be helpful to bring them into the conversation, and I'll do the same.

As you pointed out, SEP-6 (and SEP-24 / SEP-31) have the assumption that clients will explicitly create transaction records using the anchor's API before making payments to the anchor's accounts. I wasn't around when these SEPs were originally authored but I assume it was designed this way to ensure that anchors were able to signal their approval/disapproval of moving forward with each transaction before the client sent funds.

There are many reasons why the anchor would want to do this, such as enforcing limits on transaction volumes and wanting to avoid the operational or financial cost of refunding when that limit is reached. Or maybe the anchor no longer has or wants to use an account to that was previously used to receive payments, so they want to be able to provide payment instructions for each transaction. There are also reasons why clients themselves would want to engage with the business on a per-transaction basis prior to sending funds, such as wanting to get a firm quote of the exchange rate from the anchor.

The above leads me to believe that skipping this transaction initiation step is actually more like an edge case, where the anchor and client don't want or need to exchange information prior to transferring funds. And if it is an edge case, then we need to be careful not to break any assumptions made by those who aren't concerned about it.

Adding an optional payments list to each transaction record seems like a low-impact non-breaking way to surface this information. But I understand that your primary motivation for proposing these changes is to make each funds transfer queryable. I don't understand how this would work since clients can't be given an ID unique to the transfer if they don't create the record via API prior, unless there is some shared assumption on how IDs are generated. For example, maybe the client and anchor use the ID provided by the payment rail, but that seems like it would be error prone and dependent on the specific payment rail. Any shared assumption on how IDs are generated would be specific to that anchor, complicating logic for the client.

I will reopen this issue and share it in our common ecosystem channels to see if we can get some more input.

marcinx commented 7 months ago

Hey @JakeUrban,

Thanks for your follow up and taking the time to write down your thoughts on this.

There are many obvious reasons why listing all payments is necessary, for example it is important so that third parties can verify the entire SEP-6 tx history and wallet addresses for proof of reserve purposes.

If you know other businesses that would be interested in discussing this change it would be helpful to bring them into the conversation, and I'll do the same.

It was actually the team at LOBSTR that pointed out that they cannot find and query some payments listed in /transactions. We include all payments that were sent to instructions given by /deposit or /withdraw in the /transactions endpoint. Originally multiple payments would simply carry the same request_id when listed in /transactions but we were told that's a problem for them.

Our solution to help them was:

As you pointed out, SEP-6 (and SEP-24 / SEP-31) have the assumption that clients will explicitly create transaction records using the anchor's API before making payments to the anchor's accounts.

Exactly. It's designed on false assumptions. We see every day that users in the wild don't comply with that. Sometimes they split payments across different wallets, sometimes some surprise withdrawal fee on their CEX results in the payment amount being too small, sometimes they fat-finger amounts, and so on and so forth. Also, it's a feature to have sticky deposit/withdrawal instructions on a per-user bases. When you send a bank payment you don't query a new set of instructions every time before making the payment.

I assume it was designed this way to ensure that anchors were able to signal their approval/disapproval of moving forward with each transaction before the client sent funds.

That might very well be but it does not align with reality. You cannot stop a user from sending multiple times and sometimes it is desirable that the user does have this option. If a user does send multiple times then payment tx must be accounted for (see proof of reserve purposes above, consistency in data, and so on). The way SEP-6 is designed payments can simply disappear and I have given you examples where this is the case.

There are many reasons why the anchor would want to do this, such as enforcing limits on transaction volumes and wanting to avoid the operational or financial cost of refunding when that limit is reached. Or maybe the anchor no longer has or wants to use an account to that was previously used to receive payments, so they want to be able to provide payment instructions for each transaction. There are also reasons why clients themselves would want to engage with the business on a per-transaction basis prior to sending funds, such as wanting to get a firm quote of the exchange rate from the anchor.

It's actually irrelevant if some anchors might wish or desire exactly one payment per /deposit or /withdraw request because in reality they cannot stop the user from sending multiple times. In fact, they should not (falsely) assume that users will comply with always sending exactly one payment to a set of instructions but instead have routines to handle multiple payments correctly and have them accounted for. SEP-6 should guide them in that, whereas at the moment it misleads them into building on flawed assumptions.

The above leads me to believe that skipping this transaction initiation step is actually more like an edge case, where the anchor and client don't want or need to exchange information prior to transferring funds.

I don't think it is. It's a very real case that happens every day. I have given you examples from other anchors. If you query through our tx history, for example, you can see that it happens several times a day.

And if it is an edge case, then we need to be careful not to break any assumptions made by those who aren't concerned about it.

Yeah, the challenge is to implement this in a backwards-compatible way, for example the way we did it in collaboration with LOBSTR. Those are non-breaking additions and if they work for us it will work for all other anchors as well.

Adding an optional payments list to each transaction record seems like a low-impact non-breaking way to surface this information.

Agree. See the output of: https://sep6.whalestack.com/transactions?asset_code=BTC and https://sep6.whalestack.com/transaction?id=61892cab2e9fea0f2329d6193f19

But I understand that your primary motivation for proposing these changes is to make each funds transfer queryable.

That was actually LOBSTR's concern/requirement. We were (and still would be) happy with the request_id appearing multiple times in /transactions and the /transaction endpoint carrying an additional "related_transactions" list. Their app wasn't compatible with that approach, which is why we added the unique hash in /transactions. When /transaction is queried with the hash it returns only that specific payment directly, see: https://sep6.whalestack.com/transaction?id=61892cab2e9fea0f2329d6193f19-c3b3183968

I don't understand how this would work since clients can't be given an ID unique to the transfer if they don't create the record via API prior, unless there is some shared assumption on how IDs are generated.

We did it like this: /deposit and /withdraw return a request_id with a set of payment instructions as per SEP-6. Multiple payments to the same instructions result in a hash suffix on the transaction id listed in /transactions, e.g. 61892cab2e9fea0f2329d6193f19-c3b3183968.

When you query https://sep6.whalestack.com/transaction?id=61892cab2e9fea0f2329d6193f19 you receive the latest tx the normal way in transaction, all previous ones are listed in related_transactions.

When you query https://sep6.whalestack.com/transaction?id=61892cab2e9fea0f2329d6193f19-c3b3183968 you get only that specific payment transaction. (This was LOBSTR's requirement)

For example, maybe the client and anchor use the ID provided by the payment rail, but that seems like it would be error prone and dependent on the specific payment rail. Any shared assumption on how IDs are generated would be specific to that anchor, complicating logic for the client.

Not sure why. It seems really simple. They can still generate whatever id they want. The suffix for multi-payments is non breaking.

I will reopen this issue and share it in our common ecosystem channels to see if we can get some more input.

Alright.

marcinx commented 7 months ago

PS: In our case the format for the list in /transactions is request_id-hash where hash consists of 10 alnum chars followed by another dash and a vout index. Other anchors can pick an arbitrary value after request_id. It's irrelevant what the value looks like as long as it uniquely identifies a payment sent to a specific set of instructions given by /deposit or /withdraw.

JakeUrban commented 7 months ago

I think we are on the same page with the problem statement, which is that users don't always adhere to the one-payment-per-transaction assumption that was incorporated into SEP-6's design.

I think we differ on how businesses should / do handle this case, and the level of impact the proposed change has on existing implementations.

I'd like to have other on/off ramps provide input on how they handle this case, rather than speak from second-hand experience, so I won't address that in detail other than to say that I believe most on/off ramps will not accept or process unexpected payments or payment amounts that differ from the intent provided earlier. If the payment rail is push, so the business receives an incorrect or unexpected payment, then they either automatically return funds or require customers to contact them. The exception to this seems to be exchanges or other businesses that are licensed to hold funds for their customers, rather than only licensed as a money transmitter. If the payment rail is pull, then this is a non-issue since the business can debit the customer's account with the appropriate amount and they don't receive unexpected payments.

Regarding the solution you've implemented with Lobstr, there are several ways this is not backwards-compatible and would have a large impact on the implementation of both clients and anchors if we asked them to update.

  1. Unexpected deposits to the anchor's off-chain account would result in new transaction records in the API. Client logic may break when they encounter a transaction record they did not initiate.
  2. We would need to standardize how IDs are generated when the transaction is not initiated by the client, otherwise clients would have to generate IDs using bespoke conventions agreed upon with each anchor, potentially varying per payment rail. It may be easy for crypto-to-crypto transactions since you can use the on-chain hash, but we'd have to use a different scheme for bank-to-crypto or cash-to-crypto transactions. I don't think it is realistic to define conventions for each possibility here.
  3. Changing the transaction that is returned as a result of using the same ID is a sure-fire way to break client implementations. Clients would not be able to index any transaction data in association with the anchor-provided ID.
  4. Anchors do not create transaction records in their API without initiation from the client. With the proposed change, we'd be asking them to build this functionality.
  5. Anchors would have to complicate their data model and endpoint implementation logic to support fetching a different transaction object using the same ID. They would also need to build a mapping of ID to related transactions, a concept that doesn't exist today.

All-in-all I highly doubt that the current anchor network and set of compatible wallets would be in favor of this change due to the effort it would require to update their implementations.

maxxb95 commented 7 months ago

This issue was just brought to my attention by @JakeUrban - happy to add my perspective as a SEP24 anchor integrator at Kado, in case it's helpful:

I think the problem makes sense, but it’s not something we’ve really run into (that I can remember) for SEP24 or non-SEP24 deposits, because of the way that we try to facilitate transactions with wallet providers, and the explicit language we use in our on/off ramp application to make sure that users deposit exact amounts for off-ramps.

We have basic monitoring in place to alert our support team if we detect a deposit that doesn’t have a corresponding “order” associated with it, or doesn’t receive the correct amount and is therefore unhandled. In that case we try to reach out to the customer to figure out how they wish to proceed, or allow them to contact our support.

Ultimately though, we operate under the assumption that each off-ramp order has a single transaction associated with it, and that hasn’t been a real problem for our particular flow up to this point.

marcinx commented 7 months ago

Hey @JakeUrban,

Thanks for your follow up.

I think we are on the same page with the problem statement, which is that users don't always adhere to the one-payment-per-transaction assumption that was incorporated into SEP-6's design.

Yes, because SEP-6 wasn't orginially designed modeling reality correctly and was based on a flawed assumption. We already discussed some the consequences of using 1:1 instead of 1:n, namely inconsistent, incomplete, and disappearing accounting and transaction records, vastly limited utility of SEP-6, engineering gotchas, UX blockers, and potential customer support nightmares.

I think we differ on how businesses should (...) handle this case

There is only two ways how this can be handled. Either we continue with 1:1 and accept all its limitations, drawbacks, and inconsistencies and pretend that this is how it should be and accept all the consequences that come with that, or we somehow migrate to 1:n and enable SEP-6 implementations to account for otherwise missing/disappearing/error/unhandled transactions (that oftentimes cannot be automatically mitigated and require manual intervention).

I think we differ on how businesses (...) do handle this case

We have worked with at least a dozen of SEP-6 implementations that came and went in the last few years. I know from first-hand experience that the ones we worked with did accept multiple payments to a single transaction set given by /deposit or /withdraw. This is the intuitive and normal way to do it. Unfortunately SEP-6 does not specify how to handle this situation at all and so developers perform proprietary acrobatics to account for it.

In our experience, the usual way to handle 1:n is this:

  1. SEP-6 returned deposit address bc1qnppjqj7s9a3rdht279pfgyqk0jztpeckhxavgxdkveyan72dzz0sfp3mep and issued request_id 6fd27b34-8e48-46a3-ad80-b5d629ff8eae
  2. User made first deposit to bc1qnppjqj7s9a3rdht279pfgyqk0jztpeckhxavgxdkveyan72dzz0sfp3mep
  3. The /transaction endpoint correctly reflects the deposit https://dead.apay.io/api/transaction?id=6fd27b34-8e48-46a3-ad80-b5d629ff8eae
  4. User made a second deposit to bc1qnppjqj7s9a3rdht279pfgyqk0jztpeckhxavgxdkveyan72dzz0sfp3mep
  5. The /transaction endpoint now reflects the second payment only and the first one is suddenly missing entirely, as if it never happened.
  6. The /transactions endpoint now contains two records with the same id 6fd27b34-8e48-46a3-ad80-b5d629ff8eae

It is most likely not possible to simply "return the funds" for the second transaction just because SEP-6 never thought about this case, neither in crypto nor in banking. To the contrary, it is often actually desirable, required, or a feature to allow for multiple payments.

We have also seen cases where wallets take huge detours to make their implementation fit into the current constraints of SEP-6 by enforcing a (normally optional) amount parameter and then adding random decimals to the amount as a "sort-of id" so they can map inbound payments into the 1:1 schema (to some extent). Of course, this exercise did not actually solve their problem of trying to make inbound payments unique per call to /deposit and /withdraw. It's almost comical because this type of user experience and engineering effort would be completely unnecessary if SEP-6 would correctly handle 1:n.

I believe most on/off ramps will not accept or process unexpected payments or payment amounts that differ from the intent provided earlier.

I believe they do (see above) and if some of them don't then the reason for that might very well be that SEP-6 constraints them in such way, they maybe haven't thought about it yet, and would actually accept and process them if SEP-6 would account for 1:n in the first place. Nobody wants to deal with orphaned payments, manual intervention, or other support and accounting nightmares. Maybe they are no big deal for some wallets, as the commenter above suggests, but they become expensive and unsustainable quickly when traffic grows.

We don't use SEP-24 but in the context of SEP-6 I think we shouldn't consider calls to /deposit or /withdraw as a "payment intent" for exactly one payment. I guess this approach is a symptom of the fundamental 1:1 design flaw that I'm trying to address.

Regarding the solution you've implemented with Lobstr, there are several ways this is not backwards-compatible and would have a large impact on the implementation of both clients and anchors if we asked them to update.

I am all ears to hear alternative approaches. We're not married to this proposal and only came up with it out of necessity. What we suggest and have been running seems to be the lowest hanging fruit though.

In any case, I understand that surfacing this topic is inopportune because we've been doing it the 1:1 way in SEP-6 for so long. This doesn't mean that we have been doing it right though and have to keep doing it that way forever. The 1:1 design was thought up in version 0 of SEP-6 and I can't imagine it's supposed to be set in stone for all time.

I brought up the topic a year ago and re-surfaced it this week to see if there is room to normalize 1:n in SEP-6 but I understand if it's too complicated to deal with it for now. If that's the case then maybe there will be an opportunity to revisit this in the future.

Ultimately, we're actually all set from our side and got it to work the proprietary way. It would be good to see SEP-6 understand, model, and mitigate real world 1:n situations in a standardized fashion though.

marcinx commented 7 months ago

Hey @JakeUrban (and everyone),

I would like to introduce some ideas, if that's ok. Let's just consider them a simple thought experiment for a more flexible but imaginary SEP-6 transaction flow.

Let's imagine this:

Following this model it's not possible to encounter a transaction that was not initiated via /deposit or /withdraw because that's not where it happens. Instead, a transaction object is created when it was actually received, and only then is it a assigned a transaction_id.

By decoupling the transaction creation from /deposit and /withdraw and introducing this "payment instructions concept" to the model, we solve the current problem of how to handle multiple transactions sent to the same set of instructions. Currently these transactions must either be ignored, creatively mitigated, or equipped with a non-unique transaction_id. All this causes inconsistencies and a whole host of other problems.

Using the new model we'd also be opening up SEP-6 for a whole lot of beautiful things that are not easily possible currently:

Would love to hear your thoughts on that. Obviously there are too many breaking changes to simply push this into SEP-6 but maybe we can consider this type of model for some potential future major version or so. Happy to briefly jump on video call some time to chat about SEP-6 and maybe demo you how we currently use it on our side and maybe talk about each other's perspectives some more.

PS: All this is input specifically for SEP-6 and not SEP-24. As far as I understand SEP-24 is a type of popup window and browser UI and I guess the 1:1 flow makes some sense in that type of integration. A single transaction per session probably works there. I feel like SEP-6 as a backend integration is much more flexible and powerful than that and therefore should not follow the exact same and strict flow envisioned for SEP-24. It would probably be good to decouple the concepts and flows of SEP-6 and SEP-24 as we evolve.

IvanMudryj commented 6 months ago

Hi all, I've gone through the proposal but I'm still not entirely convinced about the pressing need for it.

I actually believe that either I'm not understanding the issue or, it's too specific for a crypto-to-crypto scenario and I'm missing something, or it is caused when trying to force instructions.

As you're aware, we operate fiat<>crypto anchors, and it's important to note that fiat rails vary significantly from country to country. Despite this diversity, the "1 payment : 1 request" model applies well across all scenarios.

In our day-to-day operations, especially with extensive transaction processing experience, I believe I've seen it all. Users seldom follow instructions. It's as if they just avoid following them.... Nevertheless, the 1:1 model remains applicable, with each anchor's implementation tailored to the network it interfaces with.

We (our anchor system) dont care about how much the user requested to deposit or withdraw, we just process as soon as we receive the payment. We consider they payments (no matter the amount) as a new request when there is no match on our records. If we didn't do it that way, I'm certain that the number of manual resolution cases would be so high that providing support would be very costly, and at the same time, the user experience wouldn't be the best.

User impersonation example: If the anchor charges 1% fee for deposit, and I request a deposit of 100 LTC but I send only 80 LTC; will you not send me the LTC until we discuss and define how to resolve it? Why not simply send me my 79.2 LTC to my address asap and that's it?

Here in Argentina, people often interpret our instructions as, 'Hey, if I transfer to this account, I'll see the balance on Beans... lets just send money.' What should I do? Contact users and say, "Hey, I've received your money, but you didn't request it. Go to Beans, request the deposit, and I'll match your request with your payment..."? Should I send the money back? I cant, there is no from account info for me to be able to know where to send the money, and... who will pay the banking fees and taxes, the user? They would hate me. My solution: simplicity, one payment one request.

In all honesty, I fail to see the necessity. Even if we were to implement this workflow, I believe our transactions would never have more than one payment in any country, for any form of deposit or withdrawal. What would be the closest use case where it could apply? Withdraw Bill-Payments; however, I'm confident that the list of payments associated with a request does the job.

marcinx commented 6 months ago

Hey @IvanMudryj. Thanks for weighing in. Appreciate it. It sounds like you have the same problems as us.

We (our anchor system) don't care about how much the user requested to deposit or withdraw, we just process as soon as we receive the payment.

That's exactly how we do it too.

We consider they payments (no matter the amount) as a new request when there is no match on our records.

Exactly the same here.

If the anchor charges 1% fee for deposit, and I request a deposit of 100 LTC but I send only 80 LTC; will you not send me the LTC until we discuss and define how to resolve it? Why not simply send me my 79.2 LTC to my address asap and that's it?

Totally agree. That's how we do it too.

If we didn't do it that way, I'm certain that the number of manual resolution cases would be so high that providing support would be very costly, and at the same time, the user experience wouldn't be the best.

100%

All of the above is what SEP-6 does not cover in its current form. Since you can and do process multiple payments to a /deposit or /withdraw "payment intent", how do you handle calls to /transaction or /transactions? SEP-6 does not tell you how to do it and in fact it cannot actually do it.

The problem, as it stands with SEP-6, is that the transaction_id is created on the call to /deposit or /withdraw and this breaks everything. It should be created when the payment is received instead and a set of deposit/withdraw instructions should be able to contain many payment transactions. There should be no "payment intent", only payment instructions and actual payment transactions, as outlined in my previous comment above.

In the current model, one call to /deposit returns a unique transaction_id which can be used in calls to /transaction but a transaction is really supposed to be only one single payment made in the real world. How do you represent the multiple payments you received to the instructions given by /deposit when the user queries /transaction with the transaction id? You can't because the model mandates only exactly one payment transaction in there. SEP-6 forces you to pick a random/latest/oldest one in there and omit the other ones entirely.

Same problem in /transactions. You can either list all the real payments you received with the exact same transaction_id (this can break wallets we were told), come up with some other creative workaround (like we did out of necessity), or you can just make the additional real payments disappear entirely because SEP-6 never thought about it actually being possible for them to exist.

I would really love to hear how you represent those additional payments in /transaction and /transactions. It must be a non-standardized way like us, because there is no standard for it.

I think SEP-6 is great but it reached a dead end because of how its schema is modeled. At first it seems like just a detail but the whole idea of calls to /deposit and /withdraw always being followed by exactly one tx and one tx only is fundamentally breaking it (and hampering innovation, blocking cool use cases, denying normalized exception mitigation etc. etc.).

At this point I am leaning towards drafting up a new lightweight "SEP-0042 Cross-Blockchain Interface", that is loosely based on SEP-6 but improves its current limitations and removes some convolution that was added over the last years. It would be focused on normalizing custodial and/or smart-contract blockchain bridging in and out of Stellar only, not fiat.

CassioMG commented 6 months ago

I’d like to add some inputs on how @marcinx's initial proposal would impact things on the Vibrant wallet.

Regarding Vibrant’s current Frontend implementation it looks like the new proposal would partially work for us. We currently rely on horizon’s history + /transaction + stellar_transaction_id to fetch transaction details on the Vibrant History screen, which means we are not relying on the transaction.id parameter for that. So as long as the Anchor is able to return a SEP transaction object for every stellar_transaction_id we would be fine for past completed on-chain transactions.

On the other hand we rely on /transactions + transaction.id for displaying the pending transactions, so if those “arbitrary extra payments” were made using off-chain rails and were to create new pending transactions then Vibrant wouldn’t see them because it was not initiated on the client. In our use case right now I believe this would be the most problematic. Whenever the user initiates a transaction, we track the transaction.id in our DB. If we get a callback matching that ID we process it, otherwise it is ignored. There would definitely be some work needed to adjust to a new format with multiple possible transactions like that: probably a fundamental change in our data model as well as in the processing of callbacks.

Generally speaking, I think marcinx's proposal has its pros and cons. I think the good part is adding another degree of flexibility to the SEP which means more possibilities and potentially less UX friction. E.g. users could have a single payment instructions and send payments without having to create a new intent every time. Another pro would be covering for those edge cases where we have multiple payments for the same transaction. The cons would be the consequences of it, as it would result in a much more complex structure to implement and it would also reduce control of things (which means more risk) so I believe some Anchors/Wallets would be willing to follow it and some don’t depending on how they manage things internally or yet on how much risk they would be willing to take.

This discussion reminded me of how common it is in Brazil to pay for things through installments (multiple payments) - is that a thing in the US? For that to work we have a well defined contract stating how many installments it would be and the value of each installment. Have we considered this case? E.g.: allowing multiple payments but as long as it has been defined during the deposit/withdraw intent creation so we have a well defined contract.

I would really love to hear how you represent those additional payments in /transaction and /transactions. It must be a non-standardized way like us, because there is no standard for it.

I'm also curious to understand how @IvanMudryj is handling those additional payments on their side

At this point I am leaning towards drafting up a new "SEP-0042 Cross-Blockchain Interface", that is loosely based on SEP-6 but improves its current limitations and removes some convolution that was added over the last years. It would be focused on normalizing custodial and/or smart-contract blockchain bridging in and out of Stellar only, not fiat.

I think proposing a new SEP would be a good call, we wouldn't need to worry about backwards compatibility and Wallets and Anchors could choose themselves which path to take.

marcinx commented 6 months ago

Hey @CassioMG,

Thanks for this input. It's very insightful.

We currently rely on horizon’s history + /transaction + stellar_transaction_id to fetch transaction details on the Vibrant History screen (...) so as long as the Anchor is able to return a SEP transaction object for every stellar_transaction_id we would be fine for past completed on-chain transactions.

That makes a lot of sense. One thing to keep in mind though: However unlikely it may seem, it is possible that some custodial service sends several withdrawal operations on Stellar within the same transaction. I haven't seen this happen before but it is not impossible. In this unlikely event, the stellar_transaction_id would be ambivalent and the anchor would not be able to extract a single SEP-6 transaction object. I guess a better choice for a cross-network interface like SEP-6/X would actually be the 100% unambiguous stellar_operation_id.

It's worth pointing out that the "external_transaction_id" is actually also ambiguous (in the context of blockchain bridging at least). This is a real case that we encountered: Two users made payment towards separate checkouts from a large CEX at somewhat the same time. The large CEX bundles many Bitcoin UTXOs into single transactions and they ended up bundling both independent user payments into the same transaction. This resulted in things blowing up on our end because now there were two deposits with the same external_transaction_id.

As it therefore turns out, not only transaction.id is ambiguous in the SEP-6 model but stellar_transaction_id and external_transaction_id as well.

On the other hand we rely on /transactions + transaction.id for displaying the pending transactions, so if those “arbitrary extra payments” were made using off-chain rails and were to create new pending transactions then Vibrant wouldn’t see them because it "was not initiated on the client".

I guess this is the fundamental problem I am trying to address. The transaction.id should never be created by SEP-6 in the call to /deposit or /withdraw. The "payment intent" step is unnecessary (more on that below) and creating the initial transaction object in this step causes serious issues in the SEP-6 data model and subsequent payment flow. Instead, transaction objects should be created by anchors when they actually receive real payment transactions to a set of instructions (which are a whole other object that these transactions link to).

Whenever the user initiates a transaction, we track the transaction.id in our DB. If we get a callback matching that ID we process it, otherwise it is ignored.

What would happen when you receive a callback for some transaction.id and then later receive a callback with the same transaction.id but with a different payment. I guess things would somehow blow up?

There would definitely be some work needed to adjust to a new format with multiple possible transactions like that: probably a fundamental change in our data model as well as in the processing of callbacks.

Yes. It looks like it will not be feasible to change the data model for anchors with existing SEP-6 history. Therefore I think it's a dead-end. The solution we implemented on our side and originally proposed in this post is really just patchwork to somehow work around the current limitations and not the best and elegant way to deal with it. In the context of this discussion it also turned out that the ambiguous transaction.id is not the only problem and it looks more and more like we need a clean cut.

I can only guess the reasons for why things were modeled around a "payment_intent" in SEP-6. My suspicion is that this is somehow influenced by payment flows used on credit card, PayPal, etc. and the original authors did not anticipate that this flow is not compatible with how bridging works.

Traditional payment methods (like card or PayPal) implement a pull model. It makes perfect sense to create an intent and a pending transaction object when the server controls the actual payment execution. The server knows that there can only ever be exactly one payment transaction for the intent, because the server controls the entire process and pulls the payment from the card. It's therefore a fine assumption to only ever expect one tx.

In the context of Stellar bridging or blockchain payments in general we operate with a push model, however. The user must actively make payment and push transactions instead of the server pulling the funds from their card. Declaring a payment intent with exactly one possible transaction makes no sense in the push model because the server has no control over what the user is going to do. Therefore we should create transactions once they are actually received.

I had some conversations with wallet operators in the last few days and was told that it somehow does not make sense for them to create a payment intent and results in bad UX. Why should the user have to go into his wallet app and click some buttons to declare some artificial payment intent and receive the exact the same payment instructions he already has to make payment? It does not make sense. Instead, I was told and agree, we need something that "resembles bank payments". You only ever receive one bank account and now you can deposit as much as you want, as many times as you want. This is how we should ideally model a bridging SEP. In it's current form SEP-6 is prohibitive of this approach (unless you invent proprietary hacks to make it work).

This discussion reminded me of how common it is in Brazil to pay for things through installments (multiple payments) - is that a thing in the US?

Perfectly good use case, showcasing another limitation of the current model.

For that to work we have a well defined contract stating how many installments it would be and the value of each installment. Have we considered this case? E.g.: allowing multiple payments but as long as it has been defined during the deposit/withdraw intent creation so we have a well defined contract.

See above. We should get rid of the "payment intent" approach entirely and then this isn't a problem anymore.

I think proposing a new SEP would be a good call, we wouldn't need to worry about backwards compatibility and Wallets and Anchors could choose themselves which path to take.

I agree. It seems impossible to refactor SEP-6 at this point. Too many stakeholder, components, and inevitable breaking changes. We have experience being a SEP-6 client as well as being a SEP-6 anchor so we somewhat understand the pain on both sides.

We have some ideas for a new blockchain-bridging SEP but need more time to think about it, discuss it with other stakeholders, and to decide whether it is worth the effort. Currently, things already work with our proprietary approach and we were able to implement workarounds to cover all our use cases. It works for all the wallet apps that consume our SEP-6 endpoints already but it looks like some other wallet integrations could break unless they do manual changes in their code and data model.

Idealistically speaking we're leaning towards drafting a new SEP that addresses the shortcomings we experience as both an anchor and client, hopefully getting it active, and letting it peacefully coexist with SEP-6 and let every consumer in the ecosystem figure out what suits them best. Our SEP would be much more lightweight than SEP-6, and focused on blockchain bridging exclusively.

With the upcoming launch of smart contracts it might be a good time to propose a normalized lightweight bridging interface like this to encourage Stellar as a Layer-2 for every other blockchain but there is the risk of this as well: https://xkcd.com/927/ ...

JakeUrban commented 6 months ago

I would support a new SEP with different core assumptions about the way on/off ramps are consumed. Through hearing from others in the space it is clear to me that there are two equally viable models.

I'm not convinced that this new standard should be positioned as a purely crypto-to-crypto anchor / bridge specification though. I think the core difference between SEP-6/24/31 & the design @marcinx is advocating for lies in whether payment intents should be a necessary step in the facilitation of transactions. I don't see a reason why fiat-to-crypto couldn't use a standard that didn't utilize payment intents.

Requiring payment intents definitely has its advantages.

At the same time, a solution that doesn't require payment intents has advantages as well.

github-actions[bot] commented 4 months ago

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.