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-295] Implement Kotlin reference server #1060

Merged

Conversation

philipliu
Copy link
Contributor

@philipliu philipliu commented Aug 22, 2023

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    paymentservice.stellar, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.

What

This PR implements the callback endpoints in the Kotlin reference server. It moves all the tests to use the sep24 service runner profile, which points to the Kotlin reference server. Another PR will delete the anchor-reference-server project.

Why

The Java reference server is deprecated.

Known limitations

N/A

@philipliu philipliu force-pushed the feature/anchor-295-kotlin-reference branch from 29188cc to 2d6db06 Compare August 23, 2023 18:32
@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@philipliu philipliu marked this pull request as ready for review August 23, 2023 18:54
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
@stellar stellar deleted a comment from stellar-jenkins Aug 23, 2023
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.

🙌 I left a few questions but this looks great. I'm interested to hear your perspective on what its like to integrate with the anchor platform's interfaces generally.

when (cfg.authSettings.type) {
AuthSettings.Type.JWT ->
authentication {
jwt("integration-auth") {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Should we name this something different considering we'd use the same auth type in our persistent deployment eventually?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to endpoint-auth.

.countryCode(call.parameters["country_code"])
.expireAfter(call.parameters["expire_after"])
.clientId(call.parameters["client_id"])
.id(call.parameters["id"])
Copy link
Contributor

Choose a reason for hiding this comment

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

id is not a parameter to our GET /rate callback, is this used by our integration tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The id is used to store quote at the reference server. I think we need more time to discuss whether to store the quote at AP or at the business server.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what Jake is trying to say is that id should not be passed in a first place, as it's generated on business server side, according to our current documentation

Comment on lines 71 to 72
route("/invalidate_clabe") {
get("{id}") {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ I'm assuming this is used to setup an integration test? Any chance we can make the endpoint available only when deployed in that context? We could do this later when we add a persistent deployment too, so I'll leave it to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I put a comment in the code to consider enabling this endpoint only when testing.

val rate =
when {
request.id != null -> getRate(request.id)
request.sellAmount != null || request.buyAmount != null -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ I'm not sure I understand why we have this condition, won't validate_request() catch the case where this condition is not true?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is to simplify the validate_request(). The validate request does not check the condition where both amounts are null. So, we need this condition outside validate_request() function. I think this is legit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than have one validation check outside the request, why can't this check be the first within the validate_request() function?

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@@ -23,7 +23,7 @@
<option name="METHOD_NAME" value="" />
<option name="TEST_OBJECT" value="class" />
<envs>
<env name="TEST_PROFILE_NAME" value="default" />
<env name="TEST_PROFILE_NAME" value="sep24" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to keep the sep24 test profile after we merge the Java and Kotlin reference servers.

@@ -23,7 +23,7 @@
<option name="METHOD_NAME" value="" />
<option name="TEST_OBJECT" value="class" />
<envs>
<env name="TEST_PROFILE_NAME" value="default" />
<env name="TEST_PROFILE_NAME" value="sep24" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to keep the sep24 test profile after we merge the Java and Kotlin reference servers.

when (cfg.authSettings.type) {
AuthSettings.Type.JWT ->
authentication {
jwt("integration-auth") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to endpoint-auth.

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

1 similar comment
@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

.type(call.parameters["type"])
.lang(call.parameters["lang"])
.build()
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a helper function for try/catch errors -> respond with status code?

}
}
}
route("/invalidate_clabe") {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a config flag that marks it's only for test. You can do if (flag) route { }

.countryCode(call.parameters["country_code"])
.expireAfter(call.parameters["expire_after"])
.clientId(call.parameters["client_id"])
.id(call.parameters["id"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what Jake is trying to say is that id should not be passed in a first place, as it's generated on business server side, according to our current documentation

@stellar-jenkins
Copy link

Reference Server Preview is available here:
https://anchor-ref-pr1060.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1060.previews.kube001.services.stellar-ops.com/

@lijamie98 lijamie98 merged commit bdf0799 into stellar:develop Aug 29, 2023
6 checks passed
@philipliu philipliu deleted the feature/anchor-295-kotlin-reference branch August 30, 2023 01:28
philipliu added a commit that referenced this pull request Aug 30, 2023
### Description

The changes include:
1. Introduces a new plugin to catch and map exceptions to a HTTP error
code.
2. Put the `/invalidate_clabe` endpoint behind a configuration.
3. Re-introduces the SEP-12 validation from stellar-anchor-tests.
Previously, when a GET /customer request does not include an
account/memo, the query does not assert that they are null. This caused
a race when updating an account that maps to many customers.

### Context

Address the comments from the previous PR
#1060.

### Testing

`./gradlew test`

### Known limitations

N/A
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.

5 participants