-
Notifications
You must be signed in to change notification settings - Fork 1
Add coin ticker available check #403
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
Conversation
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.
So I generally am leaning toward just trying to fetch the ticker in question and checking for a 404 to indicate that it's available. I suppose maybe the concern is that the coin endpoint returns a decent-sized payload and maybe that's a perf issue?
Would like some input from @raymondjacobson as well.
|
||
// Test with empty ticker parameter (should return 400) | ||
{ | ||
status, body := testGet(t, app, "/v1/coins/ticker//available") |
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 am not certain route matching works like this? Worth double checking that it actually calls the handler for the new endpoint if it can't match a route parameter for the ticker id
// If count is 0, ticker is available (return 404 like handle validation) | ||
if count == 0 { | ||
return c.Status(fiber.StatusNotFound).JSON(fiber.Map{ | ||
"available": true, |
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.
The pattern of this endpoint feels a little wonky (maybe from trying to match the handle check behavior?)
404 itself indicates the thing you called doesn't exist. If you're looking for a particular record by id, we don't know it, etc. In this case it could mean that the route you called doesn't exist (and we don't really have a way to distinguish between those two cases). Also a little strange to return a response body with a 404 status.
I would suggest either:
- Return 200 in all cases other than bad params/error and use your boolean to indicate available or not.
- Don't implement this endpoint at all and just call
GET /v1/coins/ticker/:tickerId
and treat a 404 from that endpoint as the ticker being available.
Second concern: Curious if client handles this correctly because SDK definitely has special logic for requests that return >= 400 status codes.
type: boolean | ||
example: true | ||
'400': | ||
description: Bad request - ticker parameter is required |
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.
nit: true now but maybe too specific generally for a 400, which I usually treat as "something was invalid about your request; more details in the response message"
- name: ticker | ||
in: path | ||
description: The ticker to check for availability | ||
required: true |
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.
Can we add some min/max length props to this?
Adds get endpoint to check if coin ticker is available for use, which is needed during the create artist coin flow