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

[SEP-6][SEP-31] Deprecating fields and adding funding_method #1567

Merged
merged 9 commits into from
Nov 8, 2024

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Oct 30, 2024

What's Changed

  • SEP-6
    • GET info: Add funding_methods to each asset
    • GET deposit and deposit-exchange: deprecate type, add required param funding_method to replace
    • GET withdraw and deposit-exchange: deprecate type, add required param funding_method to replace
  • SEP-31
    • GET info: Clean up deprecated fields and sep12 in the code example, adding funding_methods to each asset
    • POST transaction: add required param funding_method
  • SEP-38
    • GET info: Add note that buy_delivery_methods and sell_delivery_methods should be in line with the funding_methods in the protocol where the quote will be applied

Context
SEP-31 and SEP-6 have both delegate KYC to SEP-12, there’s no need for these SEPs to include KYC-specific details that are not directly related to transaction creation in their info responses.

The only exception is the funding_method which may be needed to decide what KYC info required.

Also change the name from offchain_delivery_method to funding_method, as withdrawals may not necessarily involve cash.

More details:
https://docs.google.com/document/d/15FJ4p1Iw6Gkv-6vuRor49ENKlImGwbgeGY74tHjeQyk/edit?usp=sharing

Copy link
Contributor

@lijamie98 lijamie98 left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

SEP-6 doc is missing version update

ecosystem/sep-0031.md Show resolved Hide resolved
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.

Overall I think this change looks good but I think we need a better explanation for why we're introducing funding_method and deprecating type, because their parameter descriptions in the deposit and withdrawal are almost identical.

My understanding is that we're deprecating GET /info's types object, because we don't want KYC sent in transaction initiation requests. However, it is still necessary to indicate what your funding/delivery method is, so the anchor can request necessary info via SEP-12, so we're introducing a new field that serves the same purpose as the original type parameter but removing the connection to the related object in GET info.

Another thing I think is missing is the connection to SEP-38. We should make it clear that the funding_methods defined in SEP-6 should match the delivery methods defined in SEP-38, if SEP-38 is supported.

I'd also argue that we should reuse the delivery_methods name in order to stay consistent with SEP-38.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0031.md Outdated Show resolved Hide resolved
@JiahuiWho
Copy link
Contributor Author

JiahuiWho commented Nov 5, 2024

Regarding funding_method

  • For the identical description, I intentionally kept it the same because it serves the same purpose across deposit, withdrawal, and receive requests, with consistent behavior. A uniform explanation improves clarity and avoids redundancy. I can add a note or example tailored to each request to highlight the different context.
  • For the naming, I still prefer funding_method over delivery_method because the latter lacks intuitiveness without prefixes like offchain, buy, sell, or specific SEP-38 context.

Another thing I think is missing is the connection to SEP-38. We should make it clear that the funding_methods defined in SEP-6 should match the delivery methods defined in SEP-38, if SEP-38 is supported.

I would argue that it's actually the other way around: the buy/sell_delivery_method defined in SEP-38 should match the funding_method specified in SEP-6 or SEP-31 (and this is missing in our current doc). This serves as the definitive guide for both wallets and anchors to determine which SEP should be the authoritative source in the event of any conflicts.

@JakeUrban
Copy link
Contributor

I think its more important to be consistent rather than accurate with our naming. Its confusing that SEP-38's "delivery methods" are the same thing as SEP-6 "funding methods".

It also doesn't matter which SEP is the authoritative source because they should always match. If they don't, its a bug, and wallets should report it to the anchor rather than assume one is correct and the other isn't. Can we have SEP-6/24/31 reference SEP-38 and have SEP-38 reference SEP-6/24/31?

@lijamie98
Copy link
Contributor

delivery_method feels a little confusing to me because it is like the non-stellar-asset-funding-method which is too long.

I think it would be good if we also rename SEP-38 to funding_method. The AP can support both names for backward compatibility for SEP-6, 31, 38.

@JiahuiWho
Copy link
Contributor Author

How about we also make change to SEP-38 so they are consistent?

@JakeUrban
Copy link
Contributor

I don't think its worth making a breaking change or supporting both parameters in SEP-38 because "funding" is more clear than "delivery". I recognize the parameter name could be better, but I think we need to prioritize the developer's experience in this situation.

  • Re-using "delivery method" makes it clear that SEP-38 and SEP-6 are referring to the same set of values, even if the word "delivery" isn't the most intuitive.
  • If we were to use different words ("funding" in SEP-6 and "delivery" in SEP-38), it might be confusing at first but we can make it clear that they're the same thing using the descriptions. So this would be my 2nd pick.
  • Supporting both parameters or replacing delivery_method would be unnecessarily confusing -- we shouldn't deprecate or remove parameters purely because we identified a better name.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
@JiahuiWho JiahuiWho requested a review from JakeUrban November 7, 2024 21:15
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!

@JiahuiWho JiahuiWho merged commit 21d30cd into master Nov 8, 2024
3 checks passed
JiahuiWho added a commit to stellar/anchor-platform that referenced this pull request Nov 21, 2024
### Description

- Add `funding_methods` to `/info` response. This field is mandatory for
the anchor.
- Add `funding_method` to `POST /transaction` param. This field is
mandatory for wallet.

### Context

SEP-31 changes scoped in
stellar/stellar-protocol#1567

### Testing

- `./gradlew test`

### Next
The SEP-6 change will be handled in a separate PR.
JiahuiWho added a commit to stellar/anchor-platform that referenced this pull request Dec 2, 2024
### Description

- In `/info` response, replace `fields`(in deposit) and `types`(in
withdraw) with `funding_methods`
- In all 4 transaction creation request, replace `type` with
`funding_method`, and this param will be mandatory

### Context

SEP-31 changes scoped in
stellar/stellar-protocol#1567

### Testing

- `./gradlew test`
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.

4 participants