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-591] Add clients config to sep10 auth configuration #347

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Mar 1, 2024

  • remove the unused attributes
    SEP10_CLIENT_ATTRIBUTION_ALLOWLIST
    SEP10_REQUIRE_KNOWN_OMNIBUS_ACCOUNT
    SEP10_OMNIBUS_ACCOUNT_LIST
  • add clients config and example
  • preview
Screenshot 2024-03-04 at 11 08 45 AM Screenshot 2024-03-04 at 11 09 20 AM Screenshot 2024-03-04 at 11 09 27 AM

@stellar-jenkins
Copy link

1 similar comment
@stellar-jenkins
Copy link

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.

Please also check documentation in other places (I think it's somewhere in FAQ) and explain how to work with clients config on business server side (e.g. they will receive client name as part of JWT)

# Optional The URL of the client's callback API endpoint
callback_url: https://callback.client0.com/api/v1/anchor/callback
# Optional. Default to false. If set to true, allows any destination for deposits.
allow_any_destination: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you note that this 2 fields are ONLY for custodial

SEP10_REQUIRE_KNOWN_OMNIBUS_ACCOUNT=true
SEP10_OMNIBUS_ACCOUNT_LIST=GBIBMZNXMD3P7HXVQCYIWWT5NG43NEIIY7VYBQ5SADV6UULUKCAJTGPG
```yaml
clients:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you have 2 examples of custodial and non custodial separated a bit and add more comments to it (e.g. list fields that are required and optional for custodial/noncustodial)

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@@ -32,7 +32,8 @@ You should use the `sub` field of the JWT token. For custodial wallets, this val

### How to identify the wallet?

You should use a combination of `sub` and `data.client_domain` fields of the JWT token. For custodial wallets, the `sub` value will be in the format `account:memo`. Use the account to identify the wallet. For noncustodial wallets, use the `data.client_domain` field. Note that the wallet must provide the `client_domain` beforehand as a part of SEP-10 authentication.
giYou should use a combination of `sub` and `data.client_domain` fields of the JWT token. For custodial wallets, the `sub` value will be in the format `account:memo`. Use the account to identify the wallet. For noncustodial wallets, use the `data.client_domain` field. Note that the wallet must provide the `client_domain` beforehand as a part of SEP-10 authentication. If [clients] configuration is present, the `data.client_name` field will also be included in the JWT token, which can be used to identify the wallet.
Copy link
Contributor

@Ifropc Ifropc Mar 4, 2024

Choose a reason for hiding this comment

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

This documentation seems to be outdated. I think our current recommendation should be to just use clients

@Ifropc
Copy link
Contributor

Ifropc commented Mar 4, 2024

LGTM except one small comment

@stellar-jenkins
Copy link

@JiahuiWho JiahuiWho merged commit 3a65957 into main Mar 5, 2024
2 checks passed
@JiahuiWho JiahuiWho deleted the anchor-591-update-clients-config-in-sep10 branch March 5, 2024 00:11
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.

3 participants