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

refactor: apply mono package updates #5944

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

alexp3y
Copy link
Contributor

@alexp3y alexp3y commented Nov 8, 2024

Try out Leather build ceb3b40Extension build, Test report, Storybook, Chromatic

Applies recently upgraded leather.io packages and adapts code to use updated types.

leather-io/mono#601

@alexp3y
Copy link
Contributor Author

alexp3y commented Nov 8, 2024

@alter-eggo I added this PR to knock out the CryptoAssetInfo adjustments needed after the MarketData changes. Would be great if you can check those out. Also noticed there's some other type check failures relating to the BNS V2 stuff. We'll likely need to do those simultaneously. Do you want to just take this branch over to make those changes here?

@alter-eggo
Copy link
Contributor

@alexp3y yeah, will add commit here

@alter-eggo alter-eggo force-pushed the refactor/apply-mono-package-updates branch from 2c2a47c to 631fce6 Compare November 11, 2024 08:51
@alter-eggo alter-eggo marked this pull request as ready for review November 11, 2024 10:07
Copy link
Contributor

@pete-watters pete-watters left a 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 👍

@314159265359879
Copy link
Contributor

Tested this build, looks good, BNSv2 names loading as intended.

Copy link
Collaborator

@kyranjamie kyranjamie left a 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).

Comment on lines +27 to +36
function castFullTokenInfo(rawToken: Partial<Stx20CryptoAssetInfo>) {
return {
chain: rawToken.chain!,
category: rawToken.category!,
protocol: rawToken.protocol!,
symbol: rawToken.symbol!,
decimals: rawToken.decimals!,
hasMemo: rawToken.hasMemo!,
};
}
Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Contributor Author

@alexp3y alexp3y Nov 11, 2024

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.

@kyranjamie kyranjamie force-pushed the refactor/apply-mono-package-updates branch from cd54bff to 1b7f874 Compare November 11, 2024 18:55
@kyranjamie
Copy link
Collaborator

Rebasing as we need these changes

@kyranjamie kyranjamie force-pushed the refactor/apply-mono-package-updates branch from 1b7f874 to 9e35417 Compare November 11, 2024 18:58
@kyranjamie kyranjamie force-pushed the refactor/apply-mono-package-updates branch from 9e35417 to ceb3b40 Compare November 11, 2024 19:11
@kyranjamie kyranjamie added this pull request to the merge queue Nov 11, 2024
Merged via the queue into dev with commit 7ef1703 Nov 11, 2024
32 checks passed
@kyranjamie kyranjamie deleted the refactor/apply-mono-package-updates branch November 11, 2024 19:24
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.

6 participants