-
Notifications
You must be signed in to change notification settings - Fork 2
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
[READY] Migrate signer-service to Typescript #352
[READY] Migrate signer-service to Typescript #352
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for going through this tedious process @Sharqiewicz 👌 Looks good overall, I just found a few things we might want to improve.
Some remarks:
- IIRC our deployment currently assumes that
yarn start
is enough to run the signer-service. If we now split it up intoyarn build
andyarn start
, we'll have to touch the deployment instructions of the service or it won't start. This is an important consideration before we merge this. Either we a) combine building and running inyarn start
or b) change the deployment steps. b) is favorable IMO. - The linting seems to be broken.
yarn lint
finds lots of issues that doesn't seem like issues to me. Maybe we need to change the config files.
signer-service/src/api/controllers/googleSpreadSheet.controller.ts
Outdated
Show resolved
Hide resolved
…om:pendulum-chain/vortex into 281-switch-to-typescript-in-signer-service
@ebma
so |
} | ||
}; | ||
|
||
export const sendStatusWithPkController = async (_req: Request, res: Response, _next: NextFunction) => { |
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 think we're not using this fn anymore.
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.
Good catch @gianfra-t
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.
Thanks for addressing the comments @Sharqiewicz !
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.
Looks good to me, just two minor things. Let's address that, resolve the merge conflict and finally get this on staging ✅
export const SUPPORTED_FIAT_CURRENCIES = ['eur', 'ars'] as const; | ||
export type FiatCurrency = (typeof SUPPORTED_FIAT_CURRENCIES)[number]; | ||
|
||
export interface QuoteRequest { |
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: This type is unused and a duplicate of QuoteQuery
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.
Maybe it's just me, but don't you think the comments we had were helpful @pendulum-chain/devs? I suggest we re-add them because the logic is not obvious.
@ebma ✅ |
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.
Let's go 🚀
PR 🦈🟢
signer-service
to Typescriptyarn build
Compiles TypeScript code into JavaScript using the fast SWC compileryarn start
Runs the compiled applicationyarn build:start
Compiles and then immediately runs the application (When you want to test production)yarn dev
Runs the application in development mode with automatic reloading on code changesHow to review
There are only two new files
.swcrc
andtsconfig.json
they are short and consistent.The challenge with the review is going through every migrated file. I tried to keep the logic the same, but sometimes there were a lot of TypeScript errors, so I had to make changes to the files.