Enforce JSON serializability of snap state

[metamask]

Enforce JSON serializability of snap state

[FrederikBolding]

@rekmarks I implemented this initially by simply re-using the existing isValidJson utility function. This function does exactly what you suggested above.

I do however think it might be worth it to consider replacing it with something a bit more performant.

We could do something that looks more like this.

const VALID_JSON_TYPES = ['number', 'boolean', 'string'];

/**
 * {@link Json} type guard.
 *
 * @param value - The value to check.
 * @returns Whether the value is valid JSON.
 */
export function isValidJson(value: unknown): value is Json {
  if (
    value === null ||
    value === undefined ||
    VALID_JSON_TYPES.includes(typeof value)
  ) {
    return true;
  } else if (!isPlainObject(value) && !Array.isArray(value)) {
    return false;
  }

  try {
    const values = Object.values(value);

    for (const nestedValue of values) {
      if (!isValidJson(nestedValue)) {
        return false;
      }
    }
  } catch (_) {
    return false;
  }

  return true;
}

We would then also need to replace current isPlainObject utility function, which also currently seems to accept classes, weirdly?

For that we could do something like this.

export function isPlainObject(value: unknown): value is PlainObject {
  if (typeof value !== 'object' || value === null) return false;

  try {
    let proto = value;
    while (Object.getPrototypeOf(proto) !== null) {
      proto = Object.getPrototypeOf(proto);
    }

    return Object.getPrototypeOf(value) === proto;
  } catch (_) {
    return false;
  }
}

This approach is inspired by the way redux-toolkit does their detection of non-serializable objects.

See the following links:
https://github.com/reduxjs/redux-toolkit/blob/2ff1fa4364ce0dfa82dbbd3187aa0c63e57d5582/packages/toolkit/src/serializableStateInvariantMiddleware.ts#L35

https://github.com/reduxjs/redux-toolkit/blob/2ff1fa4364ce0dfa82dbbd3187aa0c63e57d5582/packages/toolkit/src/isPlainObject.ts#L11

[rekmarks]

That's a great suggestion @FrederikBolding.

Before we pursue a particular implementation, @shanejonas, have you or the OpenRPC folks by chance built anything that could be of help here?

[FrederikBolding]

My bad - didn't see you already approved the PR. Will merge now.

[FrederikBolding]

@rekmarks Based on our convo about possibly wanting to limit the size allowed for the JSON object I re-wrote my implementation to consider that too.

It's a lot less readable than the way we currently do things, but it is speedier. I compared to a simplified version here: https://jsben.ch/3VY6B - which it beats. And I assume with the deepEqual check we currently do, this would be much faster.

export function getJsonSizing(value: unknown): [boolean, number] {
  if (value === undefined) {
    return [true, 0];
  } else if (value === null) {
    return [true, 4];
  }

  // eslint-disable-next-line default-case
  switch (typeof value) {
    case 'string':
      // +2 for quotes
      return [true, value.length + 2];
    case 'boolean':
      return [true, value ? 4 : 5];
    case 'number':
      return [true, value === 0 ? 1 : Math.floor(Math.log10(value) + 1)];
  }

  if (!isPlainObject(value) && !Array.isArray(value)) {
    return [false, 0];
  }

  try {
    // Starts at 2 because the string will minimally contain {}/[]
    return [
      true,
      Object.entries(value).reduce((sum, [key, nestedValue], idx, arr) => {
        const [valid, size] = getJsonSizing(nestedValue);
        if (!valid) {
          throw new Error();
        }
        // +3 "key":
        const keySize = Array.isArray(value) ? 0 : key.length + 3;
        const seperator = idx < arr.length - 1 ? 1 : 0;
        return sum + keySize + size + seperator;
      }, 2),
    ];
  } catch (_) {
    return [false, 0];
  }
}

[rekmarks]

Suggested implementation (absent a try/catch):

if (!deepEqual(JSON.parse(JSON.stringify(snapState)), snapState)) throw Error('Invalid state');

On a related note, we may want to enforce some kind of maximum size for snap state, given by the length of the string. The above operation is less than cheap for large objects.

[rekmarks]

Also @FrederikBolding, if we end up pursuing your implementation, you might find this TypeScript Json type test suite a helpful reference for the unit tests.

[FrederikBolding]

@rekmarks Definitely. Do you want to treat this suggestion as another PR and/or issue? Since the actual issue here should be solved by my already open PR.

[rekmarks]

That looks awesome. We should thoroughly document what this is doing in its docstring, but this looks pretty damn good.

[FrederikBolding]

@rekmarks I've assumed 1 byte per character for now, but we can make sure of that and/or check byte length of certain characters 👍 But yeah, we should add great documentation for this as well as a bunch of tests 😄