-
Notifications
You must be signed in to change notification settings - Fork 34
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-555] Add client_domain
and client_name
to Platform transactions
#1210
[ANCHOR-555] Add client_domain
and client_name
to Platform transactions
#1210
Conversation
0687950
to
6bf0327
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* | ||
* @return the client domain. | ||
*/ | ||
String getClientDomain(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a part of the standard, isn't it?
https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0006.md#transaction-history
Why do we want to add client_domain ?
Should we use client_name instead? Derived from the configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a part of the standard, isn't it?
It will only be visible by querying the Platform API, it is not being mapped on the SEP side. https://github.com/stellar/java-stellar-anchor-sdk/blob/7e42b5ccc7cf82a19a29a84716a6141208b0cb7c/core/src/main/java/org/stellar/anchor/sep6/Sep6TransactionUtils.java#L37-L63
Why do we want to add client_domain ?
Should we use client_name instead? Derived from the configuration?
I can add client_name
, but should we keep client_domain
as well? In SEP-24 both are included in the interactive JWT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general it's easier to work with client_name including both sounds like a good idea. But won't it require to add client_name to other SEPs too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But won't it require to add client_name to other SEPs too?
Yeah, this PR already adds the client_domain
to SEP-24 and SEP-31. I will do the same for client_name
.
client_domain
and client_name
to Platform transactions
Description
This adds the
client_domain
andclient_name
to the Platform transaction objectsContext
client_name
andclient_domain
are inaccessible to the business servers for SEP-6 and SEP-31 at the moment.Testing
./gradlew test
Documentation
The AP docs need to be updated with this new field.
Known limitations
N/A