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

feat(apps/hermes): new client package for hermes #1653

Merged
merged 40 commits into from
Jun 7, 2024
Merged

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented Jun 4, 2024

  • created a new hermes client package
  • used zod for schema validation
  • moved hermes server to a new server folder in apps/hermes
  • removed oracle-swap and send-usd example since they are moved to https://github.com/pyth-network/pyth-examples

will deprecate price-service-client once this package is published

Copy link

vercel bot commented Jun 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
xc-admin-frontend ⬜️ Ignored (Inspect) Visit Preview Jun 7, 2024 0:06am

Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Nice! I left some comments. I think it would be good if it gets more eyes.

apps/hermes/server/Dockerfile Outdated Show resolved Hide resolved
apps/hermes/client/js/.eslintrc.js Outdated Show resolved Hide resolved
apps/hermes/client/js/package.json Show resolved Hide resolved
apps/hermes/client/js/package.json Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@cprussin cprussin left a comment

Choose a reason for hiding this comment

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

Most of my comments are just little suggestions and not super important.

IMO the most important thing is I strongly, strongly recommend using a runtime schema validator rather than relying on generated types and unsafe casting.

Feel free to disregard the other comments as you feel it makes sense. Happy to expand on anything if it helps, a lot of this is somewhat esoteric knowledge I've picked up over the years and have become opinionated on but of course other opinions are valid too.

.github/workflows/ci-hermes-server.yml Show resolved Hide resolved
apps/hermes/client/js/.eslintrc.js Show resolved Hide resolved
apps/hermes/client/js/Dockerfile Outdated Show resolved Hide resolved
apps/hermes/client/js/README.md Show resolved Hide resolved
apps/hermes/client/js/jest.config.js Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesConnection.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/index.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/src/types.ts Outdated Show resolved Hide resolved
apps/hermes/client/js/tsconfig.json Show resolved Hide resolved
Copy link
Collaborator

@cprussin cprussin left a comment

Choose a reason for hiding this comment

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

LGTM

apps/hermes/client/js/package.json Outdated Show resolved Hide resolved
apps/hermes/client/js/src/HermesClient.ts Outdated Show resolved Hide resolved
@cctdaniel cctdaniel merged commit f3249d9 into main Jun 7, 2024
7 checks passed
@cctdaniel cctdaniel deleted the hermes-client branch June 7, 2024 12:13
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.

4 participants