-
Notifications
You must be signed in to change notification settings - Fork 60
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
Migrate Balance
validation to zod
#1257
Conversation
@@ -125,7 +125,7 @@ export class SafeBalancesApi implements IBalancesApi { | |||
): Promise<Balance[]> { | |||
const tokenAddresses = balances | |||
.map((balance) => balance.tokenAddress) | |||
.filter((address): address is string => address !== null); | |||
.filter((address): address is `0x${string}` => address !== null); |
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.
This is a requirement as the schema checksums the address.
@@ -230,7 +231,7 @@ export class ZerionBalancesApi implements IBalancesApi { | |||
): Erc20Balance { | |||
const { fungible_info, quantity } = zerionBalanceAttributes; | |||
return { | |||
tokenAddress, | |||
tokenAddress: getAddress(tokenAddress), |
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.
As before regarding the checksumming of addresses. I will not highlight the other instances of this, but everywhere an address is wrapped in a getAddress
call, it is the same.
Pull Request Test Coverage Report for Build 8230510014Details
💛 - Coveralls |
address: transactionApiBalancesResponse[1].tokenAddress | ||
? getAddress(transactionApiBalancesResponse[1].tokenAddress) | ||
: transactionApiBalancesResponse[1].tokenAddress, |
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.
Type-wise, tokenAddress
may be 0x${string}
or null
, hence the ternary requirement.
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.
👏🏻👏🏻👏🏻
{ | ||
code: 'invalid_union', | ||
unionErrors: [ | ||
// @ts-expect-error - inferred types don't allow optional fields |
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.
Is this comment accurate? The error I get when removing the @ts-expect-error
annotation is:
Type '{ issues: { code: "invalid_type"; expected: "string"; received: "undefined"; path: string[]; message: string; }[]; name: string; }' is missing the following properties from type 'ZodError<any>': errors, format, message, isEmpty, and 4 more.
(this is also applicable to other lines: 193, 253, 266)
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.
True - it looks like the error is not on our end. I updated the comment in 67f7390.
Summary
This migrates the validation of
Balance
(and associated sub-types) tozod
.Changes
BalancesValidator
and associated schemaBalanceSchema
and infers type(s) from it