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

RFC: Determine how to handle type assertions at both compile- and run-time #458

Closed
jessepinho opened this issue Feb 5, 2024 · 2 comments · Fixed by #484
Closed

RFC: Determine how to handle type assertions at both compile- and run-time #458

jessepinho opened this issue Feb 5, 2024 · 2 comments · Fixed by #484
Assignees
Labels
concept refactor Improving existing system with new design

Comments

@jessepinho
Copy link
Contributor

jessepinho commented Feb 5, 2024

Per our web check-in on Feb 5, 2024, we want a clean and expressive way to assert that particular properties exist on objects that come over the wire and are deserialized from Protobufs.

The problem

We are currently using the classes generated by bufbuild to represent data types, even though those classes are designed for in-transit data. This means that, even if we know that some data should have certain properties in certain places, we're still treating all properties as optional, since that's how bufbuild generates classes.

Thus, throughout the codebase, we have to use ugly type guards everywhere (example). For example, let's say we have an <AddressViewComponent /> that renders an AddressView. AddressView may or may not have a case of visible; if it does, it may or may not have an index.account property, an address property, and/or a walletId property. Inside <AddressViewComponent />, we'll need to assert that addressView.case === 'visible' && !!addressView.value.address && !!addressView.value.index.account before we can e.g., render the text Account #1 here:
image

This problem gets even worse if you have multiple levels of nesting of Protobuf types. A TransactionView, for instance, contains underneath it ActionViews (which themselves contain SpendViews, OutputViews, etc.), MemoViews, etc. There is a huge number of possible branches that one has to deal with based on whether certain properties are present vs. missing at each level of the object structure.

And the worst part is that, in many cases, we actually know via our business logic that certain properties must already exist — i.e., that it isn't actually possible for certain properties to be missing. For example, when the approval dialog opens for a "Send" transaction, we know that the current user is the one creating the transaction, and thus that the memoView must have a case of visible. Any other possibility wouldn't make sense. And yet we still have to act as though all properties are optional, like we don't know what the case is, etc.

Requirements for solving this problem

  • A minimum of repeated code. We don't want to have to repeat imperative type guards at various levels of the component tree.
  • A minimum of imperative code. An expressive, declarative option would be preferable.
  • A marriage of compile- and run-time type safety. We don't want to have to e.g., use ! to tell the compiler, "Trust us, we know this property exists." Rather, we should have a way of run-time checking our objects to make sure they're in the shape we expect; and the run-time checking should result in compile-time type assertions so that we can fully trust our types.

Proposed solutions

Utilize PartialMessage + type predicate functions

PartialMessage is a type provided by bufbuild that represents an all-optional version of a Protobuf message. That is, all fields are optional, recursively to the deepest-nested object.

We could create TypeScript utility interfaces that assert that some deeply nested properties do exist, and then create type predicate functions with those, like so:

interface WithAccountIndex {
  addressView: {
    case: 'decoded';
    value: {
      index: {
        account: number;
      };
    };
  };
}

const hasAccountIndex = (
  addressView: PartialMessage<AddressView>,
): addressView is PartialMessage<AddressView> & WithAccountIndex => {
  return (
    addressView.addressView?.case === 'decoded' &&
    typeof addressView.addressView.value.index?.account !== 'undefined'
  );
};

Then, in our components, instead of needing to check the case and the existence of the account index, we'd just:

<div>
  {hasAccountIndex(addressView) && <span>{addressView.addressView.value.index.account /* <-- no need for `?`s or `!`s */}</span>}
</div>

(I'd suggest we neatly organize where all these utility interfaces and type predicate functions live, of course.)

Use Zod for compile- and run-time type validation

This is a pretty similar solution to the one above, but using Zod to more expressively combine the run-time and compile-time checking:

export const hasAccountIndex = isType(
  z.object({
    addressView: z.object({
      case: z.literal('decoded'),
      value: z.object({
        index: z.object({
          account: z.number(),
        }),
      }),
    }),
  }),
);

(isType is defined here.)

Same as above, in our components, we'd use this helper like so:

<div>
  {hasAccountIndex(addressView) && <span>{addressView.addressView.value.index.account /* <-- no need for `?`s or `!`s */}</span>}
</div>

Create a parallel set of types or classes, with required properties

The shortcoming of both of the above solutions is that they require using type guards at all. One could argue that there should never be, e.g., an AddressView with a defined index.account but an undefined address. If that's true, then perhaps we should define classes for specific use cases, such as CurrentUserAddressView which always has both an index.account and an address, since we know that we'll always have access to that for addresses belonging to the current user.

On the other hand, that solution isn't really that much more elegant than the ones above, and could get extremely messy when it comes to deeply nested objects. A TransactionView, for example, might have some CurrentUserAddressViews nested underneath it, but also some AddressViews belonging to other users. Would we have to define a type of TransactionView that indicates that it contains a CurrentUserAddressView as well as some opaque AddressViews, and possibly other combinations of object types underneath it? What would the name of such a class be? TransactionViewWithCurrentUserAddressInMemoButOtherUserInOutputAction or something crazy like that? I don't think is really a feasible option, unless I'm thinking incorrectly about how such an object would be defined to enable access to nested properties without tons of type guards and safe navigation operators.

Outcome

Create an ADRs directory in the web repo to store our ADRs for future reference, and include a summary of the outcome of this RFC.

Commenters

Please feel free to flesh out the solutions I listed above if I'm scant on detail, or add anything else you think is relevant to this issue.

Notes to self

  • If we end up wanting to use named functions, this should come in handy.
@jessepinho jessepinho changed the title DRAFT: ADR: Determine how to handle type assertions at both compile- and run-time DRAFT: RFC: Determine how to handle type assertions at both compile- and run-time Feb 5, 2024
@jessepinho jessepinho changed the title DRAFT: RFC: Determine how to handle type assertions at both compile- and run-time RFC: Determine how to handle type assertions at both compile- and run-time Feb 5, 2024
@grod220
Copy link
Collaborator

grod220 commented Feb 6, 2024

We are trying to take a deeply nested, optional-laden object and do type narrowing so Typescript recognizes it as an object with specific fields present.

My first inclination is to add type guards everywhere. But then we quickly find this becomes repetitive and burdensome. But pulling out to a helper guard does not work:

const assertHasIndex = (av: AddressView) => {
  if (av.addressView.case !== 'decoded') throw new Error('addressView is not decoded');
  if (!av.addressView.value.index) throw new Error('no index in address view');
  return av
};

const getIndexFromAV = (av: AddressView) => {
  assertHasIndex(av)
  return av.addressView.value.index.account; // ❌ not recognized
};

We'd have to have the type predicate or assertion function (: asserts av is PartialMessage<AddressView> & WithAccountIndex ). Either way, we can't get away from defining types of some kind (zod or otherwise).

Another thought. We are trying to assert this optional object has fields so that we can retrieve fields from that object to display. What if we just make a bunch of helpers to get stuff for us?

function getIndex(av: AddressView) {
  if (av.addressView.case === 'decoded' && av.addressView.value.index) {
    return av.addressView.value.index.account;
  }
  throw new Error('Address view does not have an index');
}

<div>
   <span>{getIndex(addressView)}</span>
</div>

We kind of give up trying to define a new type and just get fields we are expecting with error guards if the object isn't what we are expecting.

@grod220
Copy link
Collaborator

grod220 commented Feb 7, 2024

Possibly related: Bufbuild has a library called protovalidate. They seem to be actively working on a typescript version of this.

@grod220 grod220 added concept refactor Improving existing system with new design labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept refactor Improving existing system with new design
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants