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

SEPs: use Accept-Language header instead of lang request attribute #1157

Closed
JakeUrban opened this issue Mar 24, 2022 · 18 comments
Closed

SEPs: use Accept-Language header instead of lang request attribute #1157

JakeUrban opened this issue Mar 24, 2022 · 18 comments
Assignees

Comments

@JakeUrban
Copy link
Contributor

What problem does your feature solve?

Many of our SEPs have a lang parameter or attribute in requests, which is meant to communicate to the anchor which language any human readable content should be returned in.

However, there are two reasons why this is not ideal.

Language is not as specific as locale

The current lang parameter uses ISO 639-1 values, but Accept-Language values often include the client's country in addition to the language. This additional information can have a significant impact on the content returned.

Accept-Language is widely used

Many client applications may send the Accept-Language header in all requests, including those sent to anchor services. This creates the possibility of sending conflicting header and lang parameter values.

It is also very likely that many client applications have not implemented the inclusion of the lang parameter, but by default send the Accept-Language header, which means anchors are missing information that could help them return better content.

What would you like to see?

We should deprecate all usages of the lang parameter and instead instruct clients and server-side applications to use the Accept-Language header.

@JakeUrban JakeUrban self-assigned this Mar 24, 2022
@vniranjankumar
Copy link

Agree - Language locale is more appropriate to use and I vote for header - "Accept-Language"
Side Note - IOS Supports Language Locale while selecting in the device settings.

@leighmcculloch
Copy link
Member

I'm on the fence about this. I agree with the points in the description. Also:

While Accept-Language exists, modifying a web page automatically based on the header can be at times confusing for a user. This might not apply to the pages served by SEP servers, but in typical web pages it is preferable to have a unique URL for a specific locale. Why: search engines index pages based on URLS, users share specific URLs expecting content to be consistent, using Accept-Language alone doesn't allow a user to change what page they're viewing and for multi-user devices not all users might want the same language.

For a mobile app wallet that is communicating with a SEP server, it would also break the flow if the app is configured with lang X, but the phones default browser Accept-Language is configured with Y, X, and so then Y lang is used in pages loaded by the app.

@CassioMG
Copy link
Contributor

I was leaning towards preferring the "Accept-Language" header, but based on what @leighmcculloch stated maybe we could instead allowing the Sep-24 lang parameter to also accept locale? Like wallets/anchors could choose between using en or en-US formats. This would also be backwards compatible with any current implementation.

@vniranjankumar @JakeUrban how does that sound to you?

@JakeUrban
Copy link
Contributor Author

Mike from Lobstr provided some input via Slack:

In the current implementation we have, we feel it would be better using whatever is set in the Settings on user’s LOBSTR account for consistency as changing the language requires a direct action from a user. And since we already have this info stored in the app, this will be much less of a tech lift versus additionally checking for local system Settings.

The idea is if someone sets the app language to something other than English (default), they for sure had the intend to do so and would probably like to see the same language when going through the interactive flow since it is a part of the in-app experience.

If we stick with the lang parameter, we should be able to add this both for interactive deposit/withdrawal requests, as well as a GET parameter to the more info URL. Since this is currently rarely used, could we alter the SEPs to reflect that it supports both ISO 639-1 and the locale formats like en-US?

I’m told that at least for the web app we don’t influence the settings, headers etc when opening the More Info URL, and changing something here would require a research and probably a workaround solution. Based on our research, mobile apps do not specify headers when opening the link via respective webview solutions we are using. It should be possible to set it on Android, while iOS would likely require changing to alternative webview solution to implement this.

@leighmcculloch
Copy link
Member

leighmcculloch commented Apr 12, 2022

ISO639 is the first two-to-three letters in locale formats like en-US, so if we support the latter we support the former. Ideally implementers follow the typical fallback schemes, i.e. if user requests en-us, the implementer falls back to en, or en-gb, etc.

So, I think what we would reference in SEPs is:

  • RFC4646 that defines the locale format, such as en-us, and references ISO639.

  • RFC4647 that defines how a server matches a client provided tag onto a server list of possible tags, but I think this is out-of-scope of SEPs and is up to implementers how sophisticated they wish to be.

@JakeUrban
Copy link
Contributor Author

cc @mike-lobstr

@vniranjankumar
Copy link

vniranjankumar commented Apr 29, 2022

Based on all the conversation, I guess below is what we are leaning towards ? Please confirm.

  1. lang parameter in POST SEP24 /interactive deposit/withdrawal request.
  2. Also lang parameter in GET SEP24 /transactions endpoint. So that Anchors can append the same in the MoreInfo URL when it returns in the response of each transaction object.

@CassioMG
Copy link
Contributor

CassioMG commented Apr 29, 2022

Looking at the Sep-24 docs, there are currently only 2 endpoints which support the lang parameter:

  1. lang parameter in SEP24 - /interactive POST request body (as mentioned by @vniranjankumar)
  2. lang parameter in SEP24 - /info GET request (not mentioned)

Thoughts on adding lang to the SEP24 - /transactions GET request as well?

@lijamie98
Copy link
Contributor

If we are deprecating lang, we probably can process Accept-Language at all end-points and forward the language information to subsequent calls, such as interactive url and more_info_url.

@leighmcculloch
Copy link
Member

Thoughts on adding lang to the SEP24 - /transactions GET request as well?

I don't see a problem with this. It makes sense that anchors be able to offer localization of messages in the response that are intended for passing through to the user. I think it is important to note that for any API response any localization should not impact the format of any fields intended for parsing by the requesting client, such as date-time fields, amount fields, etc.

@leighmcculloch
Copy link
Member

leighmcculloch commented Apr 29, 2022

@lijamie98 @JakeUrban I think we should stick with lang, because there doesn't seem to be a compelling return on this breaking change, and as @mike-lobstr pointed out, lang provides more control to wallet apps that embed the interactive flow and API response messages.

We can pursue more explicit best practices with RFC4646 (ref) around how to use the lang value separately to this issue if folks think it is compelling.

@CassioMG
Copy link
Contributor

@lijamie98 @JakeUrban are you OK on keeping the lang parameter and update it to also accept the en-US format?

@JakeUrban
Copy link
Contributor Author

That works for me 👍

@lijamie98
Copy link
Contributor

Yes, that works. 👍

@CassioMG
Copy link
Contributor

@lijamie98 @JakeUrban Great! Another proposal here was to also add the lang parameter to the GET /transactions request so that it would reflect on the more_info_url being served in the correct language - as suggested by @vniranjankumar.

Let us know in case you have any objections to it, otherwise next week I'm planning on opening a PR to update the SEP with the lang updates - thanks!

@lijamie98
Copy link
Contributor

@CassioMG That sounds good. 👍

@JakeUrban
Copy link
Contributor Author

@CassioMG @lijamie98 have we reached a consensus to use lang? Do we need to provide an update in this issue here before we close?

@CassioMG
Copy link
Contributor

Yes, we've reached a consensus on using the lang parameter. It has been merged already on this PR: #1191

This issue is safe to be closed

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

No branches or pull requests

5 participants