-
Notifications
You must be signed in to change notification settings - Fork 15
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 balances.ts to use protobuf types #453
Conversation
apps/webapp/src/state/send.ts
Outdated
if (selection?.value.valueView.case !== 'knownDenom') throw new Error('unknown denom selected'); | ||
if (!selection.value.valueView.value.denom?.penumbraAssetId) | ||
throw new Error('no denom in valueView'); | ||
if (selection.address.addressView.case !== 'visible') | ||
throw new Error('address in view is not visible'); | ||
if (!selection.address.addressView.value.index) throw new Error('no address in addressView'); | ||
if (!selection.address.addressView.value.address) throw new Error('no address in addressView'); |
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 guard hell lol
cdb708c
to
c4d777d
Compare
@@ -16,7 +16,6 @@ | |||
"strictNullChecks": true, | |||
"allowUnreachableCode": false, | |||
"allowUnusedLabels": false, | |||
"exactOptionalPropertyTypes": true, |
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.
Not trying to sneak this in without discussion 😆 I removed this to unblock my work, since it appears that Zod doesn't support this option when using .optional()
. The workaround for this would be ridiculously painful: I'd have to modify a schema way down into a deeply nested property, just to assert that a property is allowed to be present-but-undefined instead of not present. If I can't figure out a workaround by review time, let's discuss how to move forward.
OK, I think I'm at a good place to check in. Wanted to talk about my approach to using Zod. @grod220 commented about type-guard hell above. I wanted to find an elegant solution to that problem, so I played with lenses again, ad-hoc selector functions, and now Zod. Zod seems like the most expressive/declarative way of both A) making sure our types are safe at runtime, but also B) giving us compile-time type assertions. That latter point is quite cool, as it means we can make type predicates that assert that multiple conditions exist at the same time — e.g., that not only does the This is all a bit abstract, so I'll continue Gabe's example from Discord: Before: {a.address && <AddressIcon address={a.address} size={20} />}
{a.index && <h2 className='font-bold md:text-base xl:text-xl'>Account #{index}</h2>} After (without Zod): {a.address.addressView.value?.address && (
<AddressIcon address={a.address.addressView.value.address} size={20} />
)}
{a.address.addressView.case === 'visible' &&
a.address.addressView.value.index?.account && (
<h2 className='font-bold md:text-base xl:text-xl'>
Account #{a.address.addressView.value.index.account}
</h2>
)} After (with Zod): {hasAddress(assetBalance.address) && (
<AddressIcon address={assetBalance.address.addressView.value.address} size={20} />
)}
{hasAccountIndex(assetBalance.address) && (
<h2 className='font-bold md:text-base xl:text-xl'>
Account #{assetBalance.address.addressView.value.index.account}
</h2>
)} Those |
f44f88e
to
7d4e776
Compare
7d4e776
to
90f7f47
Compare
@hdevalence the whole premise of this PR is using two new data structures:
How does that look to you? |
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.
Whew, OK. Mostly LGTM, although I have some suggestions that are mostly small but, I think, important, related to naming and organization. Feel free to merge after addressing those.
})(); | ||
// getAccount updates the address every block | ||
// getAddrByIndex updates the address every block |
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.
Outside the scope of this PR, but there is never a good reason to disable the exhaustive-deps
ESLint rule. We should investigate why getAddrByIndex
changes on every render, rather than disabling this rule.
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.
I think it's because the zustand selector isn't memoized for some reason 🤔
|
||
interface SelectAccountProps { | ||
getAccount: (index: number, ephemeral: boolean) => Promise<Account> | Account | undefined; | ||
getAddrByIndex: (index: number, ephemeral: boolean) => Promise<Address> | Address; |
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.
Since this is being renamed, I would also rename this variable to getAddrByIndex
.
if (assetIn.address.addressView.case !== 'decoded') | ||
throw new Error('address in view is not decoded'); | ||
if (!assetIn.address.addressView.value.index) throw new Error('No index for assetIn address'); | ||
if (assetOut?.penumbraAssetId === undefined) throw new Error('assetOut has no asset id'); |
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 do you think of using a Zod schema here like I did in the send slice? Or do you prefer to leave this as-is until we've made a decision on Zod vs. something else?
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 case is sadly tricky because getDisplayDenomExponent()
below requires the actual Metadata
type. After zod narrowing, it becomes something different. Let's save this for Thursday's discussion.
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.
Another point on validateAndReturnOrThrow
. This is the equivalent of Schema.parse(obj)
. And the reason why we aren't using this everywhere is because validateSchema()
was supposed to be a bit more graceful for our users (only throws in dev, just logs in prod). For invalid schemas, should we throw for everyone though as the default? If so, we can use the standard zod parse.
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.
Ahh, gotcha — didn't know that re: Schema.parse
@@ -11,7 +11,7 @@ | |||
"clean": "turbo clean", | |||
"format": "prettier --write .", | |||
"format-check": "prettier --check .", | |||
"all-check": "pnpm install && pnpm format && pnpm format-check && pnpm lint && pnpm test && pnpm build" |
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.
I just commented on something similar in another PR — if we were going to remove one or the other, shouldn't we remove pnpm format
and keep pnpm format-check
? pnpm format
fixes whatever Prettier errors it can, but leaves the ones it can't. pnpm format-check
simply checks if there are any Prettier errors (fixable or otherwise), and exits with an error code if there are any errors.
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.
I initially swapped it because I found it strange to do checking if the step before we did the actual formatting.
Not sure how folks are using this, but I am typically running this to ensure everything will pass in the CI/CD later. By just doing format checking, if there are formatting errors, the dev will have to run pnpm format
manually and re-run pnpm all-check
. Given the formatting takes as long as checking, I figured we should just do the fixes. But maybe that isn't best 🤷
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.
Ah, maybe you're right. For some reason, I thought prettier --write
(which is what pnpm format
calls) only fixed what it could, but might not be able to fix everything. But now I'm realizing I was incorrect.
return 'unknown'; | ||
} | ||
|
||
throw new Error(`unexpected case ${view.valueView.case}`); |
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 you're intending this, but keep in mind that view.valueView.case
can be undefined
if there isn't any value to display. Should this really throw in that case?
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.
Yeah, that's fair. Going to return unknown
.
@@ -7,15 +7,16 @@ import { getDisplayDenomExponent } from '@penumbra-zone/types/src/denom-metadata | |||
|
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.
import { getDisplayDenomExponent } from '@penumbra-zone/types';
instead of :
import { getDisplayDenomExponent } from '@penumbra-zone/types/src/denom-metadata';
(since you've added it as an export from the types package)
</div> | ||
); | ||
} | ||
|
||
return <></>; | ||
}; | ||
|
||
export const getDisplayDenomFromView = (view: ValueView) => { |
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.
I would strongly suggest moving this helper into the types
package, especially since you're importing it from inside another component. IMHO, component files should only export components.
throwIfExtNotInstalled(); | ||
const balancesByAccount = await getBalancesByAccount(); | ||
const balancesByAccount = await getAssetBalances(); |
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.
const balancesByAccount = await getAssetBalances(); | |
const assetBalances = await getAssetBalances(); |
(Took me a while to figure out how this was being used due to the old name.)
}); | ||
} | ||
|
||
return balancesByAccount; | ||
}; | ||
|
||
export const SendForm = () => { | ||
const accountBalances = useLoaderData() as AccountBalance[]; | ||
const accountBalances = useLoaderData() as AssetBalance[]; |
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.
const accountBalances = useLoaderData() as AssetBalance[]; | |
const assetBalances = useLoaderData() as AssetBalance[]; |
Closes: #427, #429
This one is a bit of a doozie. The custom types we were relying on were pretty simplified. However, in the spirit of using the protobuf types (and therefore becoming more flexible with spec changes) we are removing the old stuff.
It touches nearly every aspect of the UI 😅 and has large effects on how we render this data.