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

Implement GetBalance client call and server endpoint #217

Merged
merged 26 commits into from
Mar 31, 2024
Merged

Conversation

jiyoontbd
Copy link
Contributor

@jiyoontbd jiyoontbd commented Mar 29, 2024

  • Implemented GetBalance client method (with auth token)
  • Implemented GetBalance server endpoint and request handler (with auth token verification)
  • Wrote bare bones BalancesApi interface with 1 method, GetBalances(requesterDid: String) and implemented FakeBalancesApi
  • Wrote tests for the new client and server methods

@jiyoontbd jiyoontbd changed the base branch from main to 207-protocol-spec-changes March 29, 2024 05:37
@jiyoontbd jiyoontbd linked an issue Mar 29, 2024 that may be closed by this pull request
Base automatically changed from 207-protocol-spec-changes to main March 30, 2024 05:10
@jiyoontbd jiyoontbd changed the title 133 get balance Implement GetBalance client call and server endpoint Mar 31, 2024
@jiyoontbd jiyoontbd marked this pull request as ready for review March 31, 2024 04:13
* @param pfiDid The decentralized identifier of the PFI.
* @param requesterDid The decentralized identifier of the entity requesting the balances.
* @return A list of [Balance] matching the request.
* @throws TbdexResponseException for request or response errors.
Copy link

Choose a reason for hiding this comment

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

Thinking out loud: in tbdex-js, we have both TbdexRequestError and TbdexResponseError, representing errors that happen before and after the client call respectively. It's a slight misnomer to throw a RequestException for both request and response errors, so I'm making a mental note that in my next tidy-up PR, I may explore introducing a TbdexRequestException class. In any case, it's relatively low priority and outside the scope of this PR.

val jsonNode = jsonMapper.readTree(responseString)
return jsonNode.get("data").elements()
.asSequence()
.map { Balance.parse(it.toString()) }
Copy link

Choose a reason for hiding this comment

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

Nit/suggestion: Wrap this Balance.parse() in a try/catch that catches and re-throws as a TbdexResponseException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Comment on lines +145 to +146
coVerify(exactly = 1) { callback.invoke(applicationCall) }
coVerify { applicationCall.respond(HttpStatusCode.OK, any<GetBalancesResponse>()) }
Copy link

Choose a reason for hiding this comment

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

niceeee. thanks for setting this up. it'll be easy now to copy this pattern for other callback tests in this library.

Copy link

Choose a reason for hiding this comment

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

i love to see such thorough testing

Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

a few nits, but nothing blocking. great work.

* @return A list of [Balance] matching the request.
* @throws TbdexResponseException for request or response errors.
*/
@Suppress("SwallowedException")
Copy link
Member

Choose a reason for hiding this comment

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

what exception gets swallowed?

Copy link
Contributor Author

@jiyoontbd jiyoontbd Mar 31, 2024

Choose a reason for hiding this comment

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

the exception thrown if Balance.parse(it.toString()) fails in the try/catch, line 123. it gets re-thrown as TbdexResponseException as implemented per this suggestion

.get()
.build()

val response: Response = client.newCall(request).execute()
Copy link
Member

Choose a reason for hiding this comment

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

ah does this throw an exception if the request fails? e.g. timeout, EHANGUP etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. from the docs:

Throws:
IOException - if the request could not be executed due to cancellation, 
a connectivity problem or timeout. Because networks can fail during an exchange,
it is possible that the remote server accepted the request before the failure.
IllegalStateException - when the call has already been executed.

Comment on lines +38 to +83
val authzHeader = call.request.headers[HttpHeaders.Authorization]
if (authzHeader == null) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Authorization header required"
)
)
)
)
return
}

val arr = authzHeader.split("Bearer ")
if (arr.size != 2) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Malformed Authorization header. Expected: Bearer TOKEN_HERE"
)
)
)
)
return
}

val token = arr[1]
val requesterDid: String
try {
requesterDid = RequestToken.verify(token, pfiDid)
} catch (e: Exception) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Could not verify Authorization header."
)
)
)
)
return
Copy link
Member

Choose a reason for hiding this comment

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

I haven't used ktor before but if it has the concept of middleware, maybe it'd be helpful to abstract this logic out into a verifyAuthzHeader middleware? and if ktor doesn't have the middleware concept, could just tuck this into a method and use it for all protected endpoints. (note for future PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. there is a ton of repeat here for all protected endpoints' request handler. Issue #227

Copy link
Contributor

Choose a reason for hiding this comment

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

nice. had a similar issue in swift TBD54566975/tbdex-swift#85

js could use some drying up also. will create an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@mistermoe mistermoe left a comment

Choose a reason for hiding this comment

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

great work @jiyoontbd !

Copy link
Contributor

@kirahsapong kirahsapong left a comment

Choose a reason for hiding this comment

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

nice! couple comments, mostly just that balance endpoint is optional. once resolved lgtm

)
}

get("/balances") {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an optional endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, i added the suggested change you posted earlier, but actually instead o that, i'd like to wrap this in a condition that checks for a new config value balancesEnabled

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 will be good for testing to still have balancesApi = config.balancesApi ?: FakeBalancesApi()

Copy link
Contributor

@kirahsapong kirahsapong Mar 31, 2024

Choose a reason for hiding this comment

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

i thought it might be helpful to share how the optional endpoint is implemented in js right now: https://github.com/TBD54566975/tbdex-js/blob/main/packages/http-server/src/http-server.ts#L45

and also this comment thread on the PR for it: TBD54566975/tbdex-js#212 (comment)

@KendallWeihe's proposal was simplifying it by keeping consistent with the other fields, and having the consumer explicitly pass in FakeBalancesApi() when instantiating the server to indicate they wanted to opt in / enable it

but either approach is cool, prob just good to make sure its aligned. i can open an issue in JS to add a balancesEnabled config value if we want to go with it, lmk!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okey, i've aligned on essentially using balancesApi null check essentially the config on whether to enable balances. i like balancesEnabled config as it's slightly more explicit? but can see how things can go awry there too. let's revisit it if there's feedback from sdk consumers to change this

Comment on lines +38 to +83
val authzHeader = call.request.headers[HttpHeaders.Authorization]
if (authzHeader == null) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Authorization header required"
)
)
)
)
return
}

val arr = authzHeader.split("Bearer ")
if (arr.size != 2) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Malformed Authorization header. Expected: Bearer TOKEN_HERE"
)
)
)
)
return
}

val token = arr[1]
val requesterDid: String
try {
requesterDid = RequestToken.verify(token, pfiDid)
} catch (e: Exception) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Could not verify Authorization header."
)
)
)
)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

nice. had a similar issue in swift TBD54566975/tbdex-swift#85

js could use some drying up also. will create an issue

Comment on lines +38 to +83
val authzHeader = call.request.headers[HttpHeaders.Authorization]
if (authzHeader == null) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Authorization header required"
)
)
)
)
return
}

val arr = authzHeader.split("Bearer ")
if (arr.size != 2) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Malformed Authorization header. Expected: Bearer TOKEN_HERE"
)
)
)
)
return
}

val token = arr[1]
val requesterDid: String
try {
requesterDid = RequestToken.verify(token, pfiDid)
} catch (e: Exception) {
call.respond(
HttpStatusCode.Unauthorized,
ErrorResponse(
errors = listOf(
ErrorDetail(
detail = "Could not verify Authorization header."
)
)
)
)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

@jiyoontbd jiyoontbd merged commit d87adb2 into main Mar 31, 2024
4 of 6 checks passed
@jiyoontbd jiyoontbd deleted the 133-get-balance branch March 31, 2024 21:19
@diehuxx diehuxx mentioned this pull request Mar 31, 2024
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.

Implement GET balance endpoint and client method
4 participants