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-809] Add sep31 methods to asset config and info response #1537

Open
wants to merge 4 commits into
base: feature/v3
Choose a base branch
from

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Oct 2, 2024

Description

Add methods to sep31.receive in asset config per discussion.

Also add methods to Sep31InfoResponse. To keep consistency of the sep info response, new SEP-31 response will be look like this:

  {
    "receive": {
      "JPYC": {
        "enabled": true,
        "quotes_supported": true,
        "quotes_required": false,
        "min_amount": 0,
        "max_amount": 1000000,
        "fields": {
          "type": {
            "description": "methods supported by the anchor for asset deposits",
            "choices": ["SEPA","SWIFT"],
            "optional": false
          }
        }
      },
      "USDC": {
        "enabled": true,
        "quotes_supported": true,
        "quotes_required": false,
        "min_amount": 0,
        "max_amount": 10, 
        "fields": {
          "type": {
            "description": "methods supported by the anchor for asset deposits",
            "choices": ["SEPA","SWIFT"],
            "optional": false
          }
        }
      }
    }
  }

Testing

  • ./gradlew test

Documentation

Will update protocol and anchor tests to reflect the change once we reached agreement on the new response format

AssetResponse assetResponse = new AssetResponse();
assetResponse.setQuotesSupported(isQuotesSupported);
assetResponse.setQuotesRequired(isQuotesRequired);
assetResponse.setMinAmount(assetInfo.getSep31().getReceive().getMinAmount());
assetResponse.setMaxAmount(assetInfo.getSep31().getReceive().getMaxAmount());
assetResponse.setFields(ImmutableMap.of("type", type));
Copy link
Contributor Author

@JiahuiWho JiahuiWho Oct 2, 2024

Choose a reason for hiding this comment

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

In my opinion type is too general (i.e in SEP-12 type is used to refer customer type), we should use something more descriptive, maybe Payment Methods etc.

Also curious if we can get rid of the the nested fields: { types: { ... } } structure (This is also used in SEP-6 info response)? fields is mostly used to list needed KYC info, but now SEP-6 and SEP-31 both has delegate it to SEP-12

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion type is too general (i.e in SEP-12 type is used to refer customer type), we should use something more descriptive, maybe Payment Methods etc.

I think it's fine to call it type here because SEP-6 also calls the methods, type. Ideally, we rename both to something like method but I'm not sure it's making a protocol change so we can rename a field.

Also curious if we can get rid of the the nested fields: { types: { ... } } structure (This is also used in SEP-6 info response)? fields is mostly used to list needed KYC info, but now SEP-6 and SEP-31 both has delegate it to SEP-12

Makes sense to me. SEP-6 does this already and calls the config property, methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to call it type here because SEP-6 also calls the methods, type. Ideally, we rename both to something like method but I'm not sure it's making a protocol change so we can rename a field.

Right I also talked to Jamie about this and let's stick with type for now

@JiahuiWho JiahuiWho marked this pull request as ready for review October 2, 2024 18:00
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.

2 participants