robosats
robosats copied to clipboard
Add OpenAPI v3.0 Docs for API
This is the first PR for fix of #75
This PR aims to document all the routes, responses and request objects as OpenAPI spec. You can browse the Redoc Documentation UI at /api/
.
As suggestd by @ShatteredBunny, went with drf-spectacular
. Easy to use and get's the job done. It's ability to use seriallizers already defined to generate the api spec is spectacular indeed.
After merging of the PR, we should have complete OpenAPI v3 documentation UI available for the current API at /api
or /api/v0/
on production.
Some more work in remaining, which needs more understanding and reading of what the /order
route does carefully.
TODO:
- [x] Going through code and logic and carefully looking at all possibilities of responses and possible and documenting them.
- [ ]
/order
route (Needs more reading of the code) - [x] Write intro and include Robosats logo
- [ ] Write how to do Auth (How to include
Authorization
header properly)
TODO from issue #75
Backend
- [x] Expose OpenAPI definition endpoint (JSON). If it is possible to do everything using Django restframework, that's best; otherwise e.g. drf-spectacular could be used
- [x] Add UI replacing this issue, e.g. SwaggerUI
- [ ] Make sure the API definition is complete, in particular that all request and response types and parameters are available and correct. If possible this is auto-generated, but maybe metadata in form of annotations is necessary. Also add the descriptions from this issue
Just checked around on /api/
and this is very very top! Super useful explanations of all the endpoints and very useful examples. I am feeling the urge to build something over this API :yum:
I realized all of the Django decimalFields
(...numerical) are strings
, unexpected to me.
I am feeling the urge to build something over this API :yum:
:smile:
I realized all of the Django decimalFields (...numerical) are strings, unexpected to me.
Yes, that was weird to me as well. But my guess is because of the constraints max_digits
, decimal_places
it makes it into strings so that it can show those constraints using regex in the docs? Not super convinced myself that this is the reason though (who even understands regex? :laughing:) and it does seem like a bug maybe. Didn't play around changing that model type and seeing what happens when I remove those constraints, but this should be easily fixable using serializers.
@Reckless-Satoshi Update on the Decimal Field - I just recently realised that the decimal fields are converted to and from strings. This is a DRF/Django thing as far as I can tell, as if you check the current behavior on production, all decimal fields are actually sent as strings in JSON.
@Reckless-Satoshi It's taken longer than expected to complete this. Reading through all the logic flows and figuring out when some field is sent in the response was really time consuming. I tried my best to document it, but I may have missed some. In the API refactor we should consider making dedicated serializers for each type of response we expect - If that is too much (and there is only one way to know for sure, i.e by doing it) we can use jsonschema to it's fullest capabilities to express the models (eg Status.WFB model will be different from say Status.PAY model). Nonetheless, that's a task of it's own. As mentioned earlier, the new serializers or only for documenting purpose and are not used to do actual serialization.
I took the liberty of publishing the api docs from github pages so that it's easier to review - https://redphix.github.io/robosats/api/
PS. will get to POST /order soon. Hopefully should not take as long as it took for GET /order Also, lmk if required, I will reduce the scope of this PR and open another one, if you wish
@Reckless-Satoshi It's taken longer than expected to complete this. Reading through all the logic flows and figuring out when some field is sent in the response was really time consuming.
Hey @redphix , to be honest the work you are doing on this PR goes far and beyond what was expected for the task. You have had to dive very deep into the models and logics to understand and disentangle the full Order endpoint (I can only image how much work that took!). Amazing contribution!
The API documentation page is looking gorgeous, only reviewing it is a task of its own :) I will be checking over it during next days and leave comments if necessary. All in all, the work you are doing here opens up a much more clear picture for devs and the ways to go for improving the API. A major revision of it is due.
Also, lmk if required, I will reduce the scope of this PR and open another one, if you wish
I think we can keep stacking commits on this PR.
Amazing contribution!
😄 Thank you!
All in all, the work you are doing here opens up a much more clear picture for devs and the ways to go for improving the API. A major revision of it is due.
Yup. Gives a clearer picture right away on what has to be improved. I have a list of things, that I shall share soon.
Wew! That was a nice ride down the codebase! I think I have got a good understanding of the trade pipeline and it's inner workings in much detail now. I should be able to use this knowledge to write some docs as well soon!
One thing is clear. The API needs re-writing! hahaha. As mentioned in DMs @Reckless-Satoshi, I should have a issue out soon in which I lay out some problems and give, in my opinion, how we should solve it and we can discuss there.
All the routes should be complete with openapi spec down to each field that the api sends, except for the POST /order route. I left it because I think it's going to be more wasted work because it's better to first move to v1 API and then do the second part of #75 (i.e using openapi to generate code that the FE can use). I have made sure to do the least wasteful work possible. All the explanations of the various fields can be carried over to v1 later on as well, and some serializers defined here can also be re-used in v1
@Reckless-Satoshi This is a lot to review, I understand, so take your time :)
Also, the openapi yaml/json files can be obtained by: YAML format:
docker exec -it django-dev python3 manage.py spectacular --file openapi.yaml
JSON format:
docker exec -it django-dev python3 manage.py spectacular --format openapi-json --file openapi.json
Can you take care of solving the merge conflict too?
So let's double the planned 300K rewards.
Awesome! Much appreciated! :D
lnbc6m1p3nnjycpp53zxaupmfaxpzjlgzeewvj9m3wr7cvv3sg02lfs5hdsxmtugsjlksdqqcqzpgxqyd9uqsp5e5l5h2hgfkkfaf0lgeq33h2rghdr0xgr6v838k89gwgtsltkpjts9qyyssqp0afyn9cf5kysfwh0nj99nzf6etd6sl53h4run78pjwg5hmpnj44hf4r0yyd8rf79zflde708fxelk43lyatek42g2z9zcqquupeqaqpevvgzr
Would be great if we come up with a concrete action plan that can be split into several sub-tasks. :rocket:
:+1: Will open an issue soon.
lnbc6m1p3nnjycpp53zxaupmfaxpzjlgzeewvj9m3wr7cvv3sg02lfs5hdsxmtugsjlksdqqcqzpgxqyd9uqsp5e5l5h2hgfkkfaf0lgeq33h2rghdr0xgr6v838k89gwgtsltkpjts9qyyssqp0afyn9cf5kysfwh0nj99nzf6etd6sl53h4run78pjwg5hmpnj44hf4r0yyd8rf79zflde708fxelk43lyatek42g2z9zcqquupeqaqpevvgzr
888dde0769e982297d02ce5cc9177170fd86323043d5f4c2976c0db5f11097ed