-
Notifications
You must be signed in to change notification settings - Fork 146
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
fix: add schema validation for stamps #5348
Conversation
b67f97d
to
23e1dbf
Compare
I've used fp-ts and io-ts a lot before. It is a more opinionated version of a zod. It was very convenient to easily:
anyway, 2 hands up for zod 🙌. But can we open this PR in monorepo? This query is already moved to there in this PR leather-io/mono#113 |
@@ -68,7 +77,12 @@ async function fetchStampsByAddress(address: string): Promise<StampsByAddressQue | |||
const resp = await axios.get<StampsByAddressQueryResponse>( | |||
`https://stampchain.io/api/v2/balance/${address}` | |||
); | |||
return resp.data; | |||
try { |
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 this is a pattern we can apply to a generic query wrapper?
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, thx for doing. 👍
@edgarkhanzadian I will push a commit to your PR |
Merging in for now, but to refactor once Edgar's PR is merged into mono |
fyi this is the current schema at stampchain:
|
Following the outage last night, caused by a schema change on the Stamps API, it's important we add thorough validation to API responses to prevent this happening again.
Here, I've added them for Stamps. On validation fail, we trigger an analytics event, which we could set alerts for.
I've used Zod, which is a library change we haven't discussed, but it's better at inferring types, more widely adopted for React apps, and we can use in mobile app, rather than continuing with Yup. LMK if anyone disagrees with this move to replace Yup.
I'll open issues for us to validate responses, and infer types, from other APIs as well.