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-31] Add methods to info response and update fields desc #1560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Oct 7, 2024

  1. This is part of the ticket https://stellarorg.atlassian.net/browse/ANCHOR-809, where methods were introduced to assets config to represents the deposit methods supported by anchor. To align the config and info response across SEPs, adding methods to SEP-31 info response

  2. Also adjust the desc of fields to reflect the change.

  3. Minor fix of broken pending-transaction-info-update reference

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.

SEP-6 uses the fields object provided in GET /info responses to inform clients what the supported values are for the type request parameter in GET /deposit-exchange and GET /withdraw-exchange.

There is no type parameter in SEP-31's POST /transactions endpoint, so the fields object currently doesn't serve a purpose. I think in order for this to be valuable, we need to add a type parameter to this endpoint.

@JiahuiWho
Copy link
Contributor Author

Can we use the fields object just for display purposes? Even without a type parameter in SEP-31, it's meaningful to inform clients about the deposit types supported by the anchor. Otherwise the methods is not being used anywhere in the anchor platform either.

I remember that fee_fixed and fee_percent used to be included in the info response, and they are not being used at all (stellar/java-stellar-anchor-sdk#1483). I wonder if those two are for informational use as well?

@JakeUrban
Copy link
Contributor

Clients can already use SEP-38 to see what an anchor's supported buy and sell methods are, so providing that information in the SEPs /info endpoints is already a bit redundant. Thats also the reason why we don't support fee_fixed and fee_percent in the anchor platform -- SEP-38 provides better support for communicating fee details.

@philipliu
Copy link
Contributor

Can we use the fields object just for display purposes? Even without a type parameter in SEP-31, it's meaningful to inform clients about the deposit types supported by the anchor. Otherwise the methods is not being used anywhere in the anchor platform either.

I think we need the client to provide the type, otherwise the anchor would not know what KYC fields to request.

@@ -376,7 +383,7 @@ the corresponding off-chain asset, after fees have been applied.
| `fee_percent` | number | (optional) A percentage fee in percentage points. Leave blank if there is no fee or fee calculation cannot be modeled using a fixed and percentage fee. |
| `sender_sep12_type` | string | (**deprecated**, optional) The value of the `type` parameter the Sending Anchor should use for a `SEP-12 GET /customer` request. This field can be omitted if no KYC is necessary. Use a value from `sep12.sender.types` instead if any are present. |
| `receiver_sep12_type` | string | (**deprecated**, optional) The value of the `type` parameter the Sending Anchor should use for a `SEP-12 GET /customer` request. This field can be omitted if no KYC is necessary. Use a values from `sep12.receiver.types` instead if any are present. |
| `fields` | object | (**deprecated**, optional) An object containing the per-transaction parameters required in `POST /transactions` requests. Pass [SEP-9] fields via [SEP-12 PUT /customer](sep-0012.md#customer-put) instead. |
| `fields` | object | (**deprecated**, optional) An object that contains multiple key-value pairs, where each key represents a field related to the transaction, and the value is an `transaction` object describing the field's details. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are adding type to fields, I think we should remove deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these descriptions quite make sense. The original version of the fields object schema looked like this:

{
  "fields": {
    "transaction": {}
  }
}

We deprecated fields because its only purpose was for transaction to specify request parameters required to be sent in POST /transactions requests.

Now that we're adding type, I think we have a couple options:

  • Deprecate fields.transaction specifically, and un-deprecated fields. This way we can add the type key to the fields object:
{
  "fields": {
    "type": {
      "description": "",
      "choices": [],
      "optional"
    }
  }
}
  • Un-deprecate both fields and field.transaction, and add type as a subfield of field.transactions:
{
  "fields": {
    "transaction": {
      "type": {
        "description": "",
        "choices": [],
        "optional"
      }
    }
  }
}

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