Typescript definitions for REST response #77

cwdx · 2022-05-12T15:41:29Z

Done:

Typescript definitions for REST endpoints

Fixes #75

nilswx

This is a great start of what this library should be. Thank you, @cwdx!

What do you think, @tiagosiebler?

nilswx · 2022-05-26T08:08:10Z

package.json

@@ -1,6 +1,6 @@
 {
-  "name": "ftx-api",
-  "version": "1.1.8",
+  "name": "ftx-api-typed",


This PR shouldn't change the package name.

Btw @cwdx if this was to just make testing on your end easier, you can also get your package json to point to either a local folder (where you have your dev copy of the package) or your git fork, example:

"ftx-api": "github:cwdx/ftx-api#master"

You can also do this via npm install, even for a folder: npm install ../path/to/ftx-api

In case it makes testing easier.

nilswx · 2022-05-26T08:08:27Z

package.json

-  "name": "ftx-api",
-  "version": "1.1.8",
+  "name": "ftx-api-typed",
+  "version": "1.2.3",


Why 1.2.3?

+1. Should be same version or 1.1.9 (assuming no breaking changes). If potentially risky (type conflicts), you can go for 1.2.0

nilswx · 2022-05-26T08:13:09Z

src/util/requestUtils.ts

-export type GenericAPIResponse = Promise<any>;
+export type GenericAPIResponse<T> = Promise<{
+  success: boolean;
+  result: T;


GenericApiResponse should be a union type that models both the Success and Failure case.

When success is false, there is no result.

We could also strip result from GenericApiResponse and instead define it as Promise<T> that throws an FtxError when success is false. I think this is more intuitive for library consumers.

Agree. GenericAPIResponse was primarily intended as a placeholder that all endpoints return Promise<semiStructuredSomething> without being as frustrating as returning unknown (frustrating for those less typescript exposed). I like the suggestion of the union between both cases

Also, while definitely a nitpick I was planning on Promise<APIResponse<T>> becoming the successor to GenericAPIResponse<T>. I'm probably overthinking but I wanted it more obvious that all API calls return promises that contain API responses with T. GenericAPIResponse was just the very open placeholder until I had a chance to work on stronger types.

Agree. Promise shouldn't be part of the response type.

nilswx · 2022-05-26T09:20:50Z

src/types/rest/response/futures.ts

+export interface FuturesPosition {
+  future: string;
+  size: number;
+  side: FuturesCoinType;


This should be OrderSide.

tiagosiebler

Thank you for spending time on this, this is a huge contribution that will benefit everyone using my connector. Some minor requests in some of the finer details but I love the direction this is going.

tiagosiebler · 2022-05-26T09:50:53Z

src/rest-client.ts

+  getRebateHistory(): GenericAPIResponse<unknown> {
    return this.requestWrapper.get('referral_rebate_history');
  }

-  getAnnouncements(language: string = 'en') {
+  getAnnouncements(language: string = 'en'): GenericAPIResponse<unknown> {
    return this.requestWrapper.get(
      'notifications/get_announcements?language=' + language
    );
  }

-  requestDust(toCoin: string): GenericAPIResponse {
+  requestDust(toCoin: string): GenericAPIResponse<unknown> {
    return this.requestWrapper.post(`dust/quotes`, { toCoin });
  }

-  getDustStatus(quoteId: string): GenericAPIResponse {
+  getDustStatus(quoteId: string): GenericAPIResponse<unknown> {
    return this.requestWrapper.get(`dust/quotes/${quoteId}`);
  }

-  acceptDust(quoteId: string): GenericAPIResponse {
+  acceptDust(quoteId: string): GenericAPIResponse<unknown> {


Since these are still "to do", I would prefer Promise<APIResponse<any>> so that the change doesn't make any existing implementations angry about the lack of strict type checks once they upgrade.

tiagosiebler · 2022-05-26T09:53:47Z

package.json

@@ -1,6 +1,6 @@
 {
-  "name": "ftx-api",
-  "version": "1.1.8",
+  "name": "ftx-api-typed",


Btw @cwdx if this was to just make testing on your end easier, you can also get your package json to point to either a local folder (where you have your dev copy of the package) or your git fork, example:

"ftx-api": "github:cwdx/ftx-api#master"

You can also do this via npm install, even for a folder: npm install ../path/to/ftx-api

In case it makes testing easier.

tiagosiebler · 2022-05-26T09:57:29Z

src/util/requestUtils.ts

-export type GenericAPIResponse = Promise<any>;
+export type GenericAPIResponse<T> = Promise<{
+  success: boolean;
+  result: T;


Also, while definitely a nitpick I was planning on Promise<APIResponse<T>> becoming the successor to GenericAPIResponse<T>. I'm probably overthinking but I wanted it more obvious that all API calls return promises that contain API responses with T. GenericAPIResponse was just the very open placeholder until I had a chance to work on stronger types.

tiagosiebler · 2022-05-26T09:58:41Z

yarn.lock

@@ -0,0 +1,3669 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


I've been using npm for my connectors, please remove the yarn.lock

tiagosiebler · 2022-05-26T10:00:17Z

src/rest-client.ts

@@ -75,25 +153,25 @@ export class RestClient {
   * Misc possible undocumented endpoints - these may not always work
   **/

-  getRebateHistory() {
+  getRebateHistory(): GenericAPIResponse<unknown> {


Suggested change

getRebateHistory(): GenericAPIResponse<unknown> {

getRebateHistory(): Promise<APIResponse<any>> {

tiagosiebler · 2022-05-26T10:04:41Z

src/types/rest/response/account.ts

+  price: number;
+}
+
+export type HistoricalBalances = Array<HistoricalBalance>


Same as above, would prefer if you just used HistoricalBalance[] instead of creating a duplicate type

Suggested change

export type HistoricalBalances = Array<HistoricalBalance>

tiagosiebler · 2022-05-26T10:05:30Z

src/types/rest/response/fills.ts

+  type: "orders" | string; // other types undocumented
+}
+
+export type Fills = Fill[];


Same again, and anywhere else with a "plural" type. Just use Fill[] directly where you have an array of the Fill type.

tiagosiebler · 2022-05-26T10:06:10Z

src/types/rest/response/fills.ts

+  id: string;
+  liquidity: "maker" | "taker";
+  market: string;
+  baseCurrency?: string | null;


I've seen a few like this, just to check - are they really undefined or string or null? all 3 possibilities?

tiagosiebler · 2022-05-26T10:06:49Z

src/types/rest/response/futures.ts

+  /** @description: USD volume in the last 24 hours */
+  volumeUsd24h: number;
+  /** @description quantity traded in the last 24 hours */
+  volume: number; 24390.24


Suggested change

volume: number; 24390.24

volume: number;

Good catch. This number was also showing up in IntelliSense results in VS Code.

tiagosiebler · 2022-05-26T10:07:40Z

src/types/rest/response/futures.ts

+  type: FuturesCoinType
+}
+
+export type Futures = Array<FutureCoin>;


Suggested change

export type Futures = Array<FutureCoin>;

Just use FutureCoin[] instead of creating Futures to represent the array of future coin.

cwdx added 13 commits May 12, 2022 15:22

response types

bd90d72

bugfixes

304a68c

move response types to folder

ac70a1b

linting fixes

f463fe5

ws event type fix

2e2edcb

type fix2

79bd9d7

version and package name

2e3667c

withdrawal fee endpoint

a60a7ce

version bump

03b508a

otc history

8085d2c

otcHistory params

54b3c5c

low level api

9027d20

getOtcHistory endpoint

82df900

nilswx approved these changes May 26, 2022

View reviewed changes

nilswx reviewed May 26, 2022

View reviewed changes

tiagosiebler requested changes May 26, 2022

View reviewed changes

cwdx added 3 commits August 30, 2022 09:38

option to pass custom axios client

0df550d

pass ftx requests through event q

6864f40

remove logs

ca7fd9b

tiagosiebler marked this pull request as draft October 22, 2022 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript definitions for REST response #77

Typescript definitions for REST response #77

cwdx commented May 12, 2022 •

edited

nilswx left a comment •

edited

nilswx May 26, 2022

tiagosiebler May 26, 2022

nilswx May 26, 2022

tiagosiebler May 26, 2022 •

edited

nilswx May 26, 2022 •

edited

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

nilswx May 30, 2022

nilswx May 26, 2022

tiagosiebler left a comment

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

tiagosiebler May 26, 2022

nilswx May 30, 2022 •

edited

tiagosiebler May 26, 2022

		@@ -0,0 +1,3669 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

	getRebateHistory(): GenericAPIResponse<unknown> {
	getRebateHistory(): Promise<APIResponse<any>> {

Typescript definitions for REST response #77

Are you sure you want to change the base?

Typescript definitions for REST response #77

Conversation

cwdx commented May 12, 2022 • edited

nilswx left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagosiebler May 26, 2022 • edited

Choose a reason for hiding this comment

nilswx May 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagosiebler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilswx May 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwdx commented May 12, 2022 •

edited

nilswx left a comment •

edited

tiagosiebler May 26, 2022 •

edited

nilswx May 26, 2022 •

edited

nilswx May 30, 2022 •

edited