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

fix: add schema validation for stamps #5348

Merged
merged 1 commit into from
May 7, 2024
Merged

fix: add schema validation for stamps #5348

merged 1 commit into from
May 7, 2024

Conversation

kyranjamie
Copy link
Collaborator

@kyranjamie kyranjamie commented May 7, 2024

Try out Leather build 23e1dbfExtension build, Test report, Storybook, Chromatic

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.

@edgarkhanzadian
Copy link
Contributor

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:

  1. handle errors of mismatches coming from the backend side
  2. pinpoint a bug and rollout a quick fix for it.

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 {
Copy link
Contributor

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?

Copy link
Contributor

@fbwoolf fbwoolf left a 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. 👍

@kyranjamie
Copy link
Collaborator Author

@edgarkhanzadian I will push a commit to your PR

@kyranjamie
Copy link
Collaborator Author

Merging in for now, but to refactor once Edgar's PR is merged into mono

@kyranjamie kyranjamie added this pull request to the merge queue May 7, 2024
Merged via the queue into dev with commit 7a89337 May 7, 2024
28 checks passed
@kyranjamie kyranjamie deleted the fix/validate-api branch May 7, 2024 11:55
@reinamora137
Copy link

fyi this is the current schema at stampchain:

interface StampRow {
  stamp: number | null;
  block_index: number;
  cpid: string;
  creator: string;
  divisible: number;
  keyburn: number | null;
  locked: number;
  stamp_base64: string;
  stamp_mimetype: string;
  stamp_url: string;
  supply: number;
  timestamp: Date;
  tx_hash: string;
  tx_index: number;
  ident: SUBPROTOCOLS;
  creator_name: string | null;
  stamp_hash: string;
  is_btc_stamp: number;
  file_hash: string;
}

type SUBPROTOCOLS = "STAMP" | "SRC-20" | "SRC-721";

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