Best practices and recommendations in o1js

[kantp]

In the following locations, the auditors identified opportunities to improve code quality and increase adherence to TypeScript best practices:

• o1js/src/lib/provable/merkle-list.ts:

  • MerkleList.forEach(): Consider putting the isDummy argument first in the callback. In TypeScript, (element: T) => void will type-check as an instance of (element: T, isDummy: Bool, i: numbere) => void, but this almost certainly signals an error by the user.

  • MerkleList.popIf(): this.data is supposed to restore the state from before pop() if the condition evaluates to false. However, if the list was empty, and the condition was false, then the data beforehand was []. Restoring the state should keep it as [], rather than changing it to [{previousHash: emptyHash, element: provable.empty()}].
    Luckily, this new list is functionally the same for the purposes of a MerkleList. Nonetheless, restoring the original data may prevent prover errors in the future.

  • MerkleListIterator.previous(): An inline comment mentions that “it returns a dummy element if we're at the start of the array". Note that a MerkleListIterator has two array-like structures:

    • The underlying JavaScript array, which is in the reverse order of the list.
    • The list, represented by the sequence of hashes.

    We recommend clarifying in this comment that previous() returns a dummy element if at the start of the list, which corresponds to the end of the array.

  • MerkleList.next(): Unlike MerkleList.previous(), MerkleList.next() does not prove membership in the list. Instead, it only computes the “next hash” if this element were appended to the current hash.

  This adds a very important caller requirement: users must remember to assert the iterator is at the end of the list! Otherwise, the circuit will be under-constrained.

  We recommend adding documentation to next() warning of this fact, and moving it into the Unsafe module. If a “safe” implementation is required, we recommend requiring users to provide a function implementing their desired invocation of next(), which can be executed within a context that is guaranteed to call assertAtEnd().