-
Notifications
You must be signed in to change notification settings - Fork 143
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
refactor: apply mono package updates #5944
Conversation
@alter-eggo I added this PR to knock out the |
@alexp3y yeah, will add commit here |
2c2a47c
to
631fce6
Compare
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 LGTM, nice work and some good cleanups too 👍
Tested this build, looks good, BNSv2 names loading as intended. |
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 @alexp3y
All good to merge here as to not get out of sync with mono. But going forward, I do want to be strict on introducing changes that bypass the typescript compiler. There's no guarantee at all that these values actually are Stx20CryptoAssetInfo
, which introduces scope for bugs. (The issue with the decimals earlier was pretty much caused by a misused as any
).
function castFullTokenInfo(rawToken: Partial<Stx20CryptoAssetInfo>) { | ||
return { | ||
chain: rawToken.chain!, | ||
category: rawToken.category!, | ||
protocol: rawToken.protocol!, | ||
symbol: rawToken.symbol!, | ||
decimals: rawToken.decimals!, | ||
hasMemo: rawToken.hasMemo!, | ||
}; | ||
} |
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.
What are your thoughts on using a zod validator here to ensure the type?
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.
A schema validator could for example, filter the array for those that do not match, proving runtime safety
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.
We do control data access here via our own library call useStx20Tokens
, so we shouldn't really need any runtime type validation.
The reason this was typed as a Partial in @leather.io/query
was to accommodate the WIP model refactor that temporarily adds two unused fields to Stx20CryptoAssetInfo
.
To support that change we either needed to assert once like this or change every downstream file/component that handles Stx20CryptoAssetInfo
to handle partials, which would have been a big increase in scope.
cd54bff
to
1b7f874
Compare
Rebasing as we need these changes |
1b7f874
to
9e35417
Compare
9e35417
to
ceb3b40
Compare
Applies recently upgraded
leather.io
packages and adapts code to use updated types.leather-io/mono#601