-
Notifications
You must be signed in to change notification settings - Fork 0
Tw 138 clean up js doc pt2 #171
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
3b89a23 to
0fa06ad
Compare
caf7e6b to
88cb40f
Compare
| /** | ||
| * Check AWM & MBE Version | ||
| * | ||
| * Check your version of the connection between the Advanced Wallet Manager (AWM) and the Master Bitgo Express (MBE) servers. | ||
| * | ||
| * @tag Advanced Wallets | ||
| * @operationId v1.advancedwalletmanager.version | ||
| */ |
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 honestly don't know what version this is actually checking, since we already have a version check endpoint for the AWM and another for the MBE. I just formatted this JS Doc based on the ping check between the servers, which makes sense to me. Please confirm the accuracy of this.
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.
Technically AWM is not reachable publicly. The setup is to have the AWM hosted in a private server that only MBE can reach it. That way you can only "ping"/"version" AWM via MBE.
AWM needs an endpoint for MBE (and only MBE) to ping it, MBE needs two endpoints. One for us to ping MBE itself, one for us to ping AWM via MBE.
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.
That makes sense for pinging*. With regards to versioning though, we have 3 endpoints, but only 2 servers. Is this endpoint just used as a sort of bridge to grab the version number? Do you suggest any changes to the endpoint summary and description I proposed above?
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.
Is this endpoint just used as a sort of bridge ro grab the version number?
Precisely. Say a client already deploy AWM and MBE with AWM being private. Then there is no way to ping or to get the version number from AWM directly. So they will have to call this MBE endpoint to indirectly ping/version AWM.
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.
my preferred syntax for the op ID will be something like v1.advancedwallet.xxxxx for all MBE endpoints, and v1.advancedwallet.awm.xxxx for AWM endpoints (or even v1.advancedwallet.internal.xxx, whatever make sense). Ideally the operation ID should closely match the actual API url. (this one is on me, I should have raised this in a previous PR 🙏 ).
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.
For these one in particular, we can have v1.advancedwallet.ping/version.mbe/awm for the ones exposed on MBE (i.e. pinging MBE itself and pinging AWM via MBE), and have v1.advancedwallet.awm.ping/version
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 don't have any issue with the summary itself tho
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.
Cool, thanks for the explanation. Let me know if I got them all correct now.
88cb40f to
8ea5e14
Compare
|
Also now that I am thinking about it, maybe it is worth changing the keys in the router so that it matches with the operation ID as well. We do that for all other BitGo repo that uses api-ts and that makes it easier for developers to debug through certain issue. But that is not an change that is within the scope of this PR. |
6822ce9 to
f210dd6
Compare
834cdd0 to
100339f
Compare
|
sorry of the back and forth, but the API itself is a tad confusing so I want to get the documentation as clear as possible to cut down confusion later down the line |
763e05a to
d15f869
Compare
f3fcd3d to
19910bd
Compare
Ticket: TW-138
1d7eec6 to
9a50212
Compare
Final cleanup efforts for the JS Doc.
Resolves TW-138.