ssz icon indicating copy to clipboard operation
ssz copied to clipboard

Add strict mode > throw on non-existing value

Open dapplion opened this issue 5 years ago • 6 comments

Add a strict mode that applies to Eth2.0 data structures. It should make accessing a value in a list that does not contain a value that should throw an exception.

Rationale

While accessing non-existing keys in javascript objects returns undefined, this behavior is very problematic with Typescript. As of now, Typescript has no way to tell if list[I] is defined but assumes it is defined (which may be false). This can make large parts of the codebase not type-safe. See https://github.com/ChainSafe/lodestar/issues/1092 as an example of this non-type-safety in action.

Making SSZ data structures ensure that list[I] is defined by throwing an error otherwise, would fix that problem making Typescript types true.

Implementation

Revert https://github.com/ChainSafe/ssz/commit/f94ad9db979d4b9b5895b8b7721f98d92d4f5be9#

dapplion avatar Jul 04 '20 17:07 dapplion

I think this is a good idea. I think ideally we can/should support both behaviors. I think this could look like extending asTreeBacked with a second parameter, something like a:

export interface ITreeBackedOptions {
  strict: boolean;
}

eg: asTreeBacked(target: Tree, options: Partial<ITreeBackedOptions> = {}): TreeBacked<T>

We can propagate this options param to all constructor-ish methods: defaultValue, createValue, clone, deserialize, etc.

wemeetagain avatar Jul 04 '20 17:07 wemeetagain

An issue with the above approach: Each CompositeType and TreeHandler (or subclasses of it) are only created once per 'type' -- one TreeHandler object per type. Any values of that type reuse the same CompositeType+TreeHandler objects. The TreeHandler is also the proxy handler for a "tree-backed" object, the proxy-wrapped tree. Because of this, there is no current location to place options on a per-tree or per-proxy level (as we're wanting to do with these options).

Potential solutions:

  • Still use one handler object per type, use a WeakMap to map Tree to ITreeBackedOptions, use mapping to lookup options when needed.
  • Separate the tree-navigation logic from the proxy handler logic, create a new handler object per proxy, store options on the handler.

wemeetagain avatar Jul 04 '20 18:07 wemeetagain

Another solution could be to sidestep the strict-mode altogether. Instead of using proxies that mimic Object and Array, to create proxies with an interfaces that mimic Map. Eg: Lists/vectors can be type ArrayMap<T> = Map<number, T>, containers can be type ContainerMap<T> = Map<keyof T, T[keyof T]>

wemeetagain avatar Jul 06 '20 01:07 wemeetagain

In that case what would be the syntax to access a value at an index? cont[index] or cont.get(index)?

dapplion avatar Jul 06 '20 11:07 dapplion

Thats the idea, yea. Implement the full Map interface: get, set, delete, has, forEach, entries, values, iterator, clear. There are some annoying byproducts of doing that tho. Mainly big code changes throughout lodestar.

I'm just brainstorming. Was hoping this was an easy fix, turns out to not be so easy. So just hunting for the right solution before diving in.

While Map interfaces might be useful in their own right, I think there's still a lot of value to having a strict-mode Array and Object. I'm hoping to go in the strict-mode direction, as it will be useful/necessary(?) for consuming proofs, when we get there.

wemeetagain avatar Jul 06 '20 16:07 wemeetagain

A valuable point of not using Map is that the code looks and feels more like the official spec. I'm not sure if that's important but it's a plus to understand the codebase more easily

dapplion avatar Jul 06 '20 17:07 dapplion