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

Add type arithmetic to improve event type-safety #705

Merged
merged 2 commits into from
May 4, 2020
Merged

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Apr 30, 2020

This PR just adds some utility types that improve type-safety on event types. They are currently not used, but I figured that it was worth including it in a separate PR since it is rather dense code that takes some time to get used to.

Test Plan

Example of it in action:

const eventData: AnyEvent<BatchExchange> = {} as any
switch (eventData.event) {
case "Token":                              // ERR
  break
case "OrderPlacement":
  eventData.returnValues.buyToken = "asdf" // OK
  break
case "Withdraw":
  eventData.returnValues.buyToken = "asdf" // ERR
  break
}

Produces the folling tsc errors:

src/streamed/events.ts(50,8): error TS2678: Type '"Token"' is not comparable to type '"OrderPlacement" | "TokenListing" | "OrderCancellation" | "OrderDeletion" | "Trade" | "TradeReversion" |
 "SolutionSubmission" | "Deposit" | "WithdrawRequest" | "Withdraw"'.
src/streamed/events.ts(56,28): error TS2339: Property 'buyToken' does not exist on type '{ user: string; token: string; amount: string; 0: string; 1: string; 2: string; }'.

Copy link

@Velenir Velenir left a comment

Choose a reason for hiding this comment

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

We did the same at contract level in dex-js 😁
I wonder if it makes sense to centralize the types somewhere

@nlordell
Copy link
Contributor Author

@Velenir I actually stole some of it from your implementation, but I didn't get a discriminated union for the AllEvents type from type AllValues<T extends object> = T[keyof T] But this could be because we have different generated contract types.

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Very cool. Is this something that could be included upstream in the package that creates types for the contracts/web3 ?

@nlordell
Copy link
Contributor Author

Very cool. Is this something that could be included upstream in the package that creates types for the contracts/web3 ?

Probably! I created #706 so we don't forget.

@nlordell nlordell changed the base branch from ev_ob_1 to master May 4, 2020 11:02
@nlordell nlordell merged commit 1213bd5 into master May 4, 2020
@nlordell nlordell deleted the ev_ob_2 branch May 4, 2020 11:21
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.

5 participants