[Suggestion] Switch Lumos validation and serialization to Duck Typing

[phroi]

As per title, I have encountered the following issue: Lumos internally uses a deprecated types validator that doesn't validate ImmutableJS types.

In the current version of Lumos Utils I have extensively used ImmutableJS's Records for defining types implementing the Interfaces defines by Lumos such a Scripts, OutPoint... but then when I try to use them, often I encounter deprecated types validators flagging them as not valid.

For example, let's say I implement the Script interface with:

export type I8Script = Record<Script> & Readonly<Script>;
export const I8ScriptFrom = Record<Script>({
    codeHash: "0x0000000000000000000000000000000000000000000000000000000000000000",
    hashType: "data",
    args: "0x"
});

 encodeToAddress(I8ScriptFrom())

Lumos errors out with:

/home/user/ickb/lumos-utils/node_modules/@ckb-lumos/toolkit/src/validators.js:31 throw new Error(errorMessage); ^ Error: script does not have correct keys! Required keys: [args, codeHash, hashType], optional keys: [], actual keys: [__ownerID, _values] at assertObjectWithKeys (/home/user/ickb/lumos-utils/node_modules/@ckb-lumos/toolkit/src/validators.js:31:11) at Object.ValidateScript (/home/user/ickb/lumos-utils/node_modules/@ckb-lumos/toolkit/src/validators.js:70:3) at encodeToAddress (/home/user/ickb/lumos-utils/node_modules/@ckb-lumos/helpers/src/index.ts:239:14) at I8Secp256k1AccountFrom (/home/user/ickb/lumos-utils/src/secp256k1.ts:69:36) at main (/home/user/ickb/v1-core/src/deploy.ts:33:43) at processTicksAndRejections (node:internal/process/task_queues:95:5)

This is due to Lumos Toolkit's ValidateScript that, while being marked as deprecate, it's still used to validate the Script parameter:

/**
 * @deprecated please follow the {@link https://lumos-website.vercel.app/migrations/migrate-to-v0.19 migration-guide} 
 */
export function ValidateScript(script: any, { nestedValidation, debugPath }?: {
  nestedValidation?: boolean;
  debugPath?: string;
}): script is api.Script;

So while I8Script correctly implements the Script interface (as it correctly exporse the fields codeHash, hashType and args), it's flagged as invalid by this deprecated validator.

Should I report all the errors of this kind that I encounter and ask them to be fixed?

As usual I'm asking here since GitHub issues are SEO friendly and very likely in the future there will be other L1 developers wondering the same :wink:

Phroi

[homura]

Hi @phroi, the function encodeToAddress can only accept a plain JavaScript object as its argument while Record is an ImmutableJS object with extra keys, such as __ownerID, values, etc. If you want to use encodeToAddress with your unique pattern, adding a custom layer between Lumos and your application may be a good idea

import * as helpers from '@ckb-lumos/helpers'

function compatibilize<Fn extends (arg: any) => any>(originalFn: Fn): Fn {
  return function (immutableOrPlain: any) {
    const plain = immutableOrPlain.toJS
      ? immutableOrPlain.toJS()
      : immutableOrPlain;
    return originalFn(plain);
  } as Fn;
}

export const encodeToAddress = compatibilize(helpers.encodeToAddress)

[phroi]

On one side there are many other methods where the Lumos API makes the assumption that input objects are exclusively a type implementation of the interface. encodeToAddress is just one example. This simplifies Lumos implementation at the cost of generality.

On the other side there are other methods that exclusively check if the correct fields are correctly defined on the object. See for example validateConfig. This is Duck Typing and in my opinion it should be the correct validation and serialization.

Keeping both forms of validation across Lumos is inconsistent and confusing for the final DApp developer as it's not documented in any way in the methods signature nor documentation...

Thank you @homura for your suggestion, I appreciate you spending time to look for a good solution, but Lumos Utils is a small library itself, not the final DApp. I cannot possibly ask the final DApp developer to call compatibilize every time he intends to use a Lumos-Utils's ImmutableJS objects with Lumos. I'll just rewrite them in way that is compatible with Lumos.

I'll leave this issue open as in my opinion this issue should be gradually addressed

[homura]

Thank you for pointing out the design inconsistency in Lumos between CKB-related and non-CKB-related objects. This inconsistency can be confusing for developers who want to use Duck Typing. We will review whether the validation is too strict for the objects related to CKB types

[homura]

@phroi Hi, PR #598 makes Lumos work with the Duck Typing by replacing the previous validators with codecs as validators, I hope the refactor helps.

[phroi]

Brilliant!! That's only one side of the story tho, the other is serialization. Going by memory codecs handle serialization perfectly, but there are other times in which these structures are serialized as JSON.

This happens for example when I call get_cells and I pass my script which also happens to have additional fields. These additional fields are used to manage all required information for a cell. For example my ImmutableJS Script implementation is actually something like this:

export interface I8Scriptable extends Script {
    cellDeps: List<I8CellDep>;
    headerDeps: List<I8Header>;
    witness?: HexString;
    since?: PackedSince;
    //Extra information is an escape hatch for future uses
    extra?: unknown;
}
export type I8Script = Record<I8Scriptable> & Readonly<I8Scriptable>;
export const I8ScriptFrom = Record<I8Scriptable>({
    codeHash: defaultHex,
    hashType: "data",
    args: "0x",
    cellDeps: List(),
    headerDeps: List(),
    witness: undefined,
    since: undefined,
    extra: undefined
});

[phroi]

Hey @homura, I wanted to let you know that I have dropped ImmutableJS and re-implemented the data structures as Typescript classes (frozen at construction time) and the additional properties as Symbols (hackish, but bypasses both validation and serialization so far).

For example, now I implement a Script with:

export const immutable = Symbol("immutable");
export const cellDeps = Symbol("cellDeps");
export const headerDeps = Symbol("headerDeps");
export const witness = Symbol("witness");
export const since = Symbol("since");

export class I8Script implements Script {
    readonly [immutable] = true
    readonly codeHash: Hash;
    readonly hashType: HashType;
    readonly args: HexString;

    readonly [cellDeps]: readonly I8CellDep[];
    readonly [headerDeps]: readonly I8Header[];
    readonly [witness]?: HexString;
    readonly [since]?: PackedSince;
    private constructor(i: Script & Partial<I8Script>) {
        this.codeHash = i.codeHash;
        this.hashType = i.hashType;
        this.args = i.args;

        this[cellDeps] = Object.freeze(i[cellDeps] ?? []);
        this[headerDeps] = Object.freeze(i[headerDeps] ?? []);
        this[witness] = i[witness];
        this[since] = i[since];
    }
    static from(i: Script & Partial<I8Script>) { return Object.freeze(i instanceof I8Script ? i : new I8Script(i)); }
}

With #598 I could probably move away from Symbols, but I don't really mind keeping them like this 🤔

What's your opinion?

[homura]

I wanted to let you know that I have dropped ImmutableJS and re-implemented the data structures as Typescript classes (frozen at construction time)

@phroi Thx for the update. Could you explain why all ckb-related objects are brozen in ckb-utils instead of using plain JS objects directly?

[phroi]

Thank you for asking @homura :pray:

An unfrozen plain JS objects in TransactionSkeleton is a security issue and an invitation to bad practices. The issue is that anybody who has a reference to this object can modify it, even without the user explicit agreement (which would otherwise happen by return a new TransactionSkeleton). For example, it would allow stuff like this to happen:

function myInnocentLookingFunction(tx : TransactionSkeletonType) : void {
  // some code, that when some conditions are met, let the following statement be executed ...
  tx.outputs.get(0).cellOutput.lock = myMaliciousLock;
  // ...
}

[homura]

anybody who has a reference to this object can modify it, even without the user explicit agreement

Exactly, however, it's important to prevent malicious overriding behavior. If someone is able to modify the object, they can also do other kinds of overriding. For example, they could override the Uint8Array constructor to execute their own code, like this:

window.Uint8Array = class extends Uint8Array {
  constructor(bytes) {
    if (isOverridingTarget(bytes)) {
      override(bytes)
    }
  }
}

Therefore, it's crucial to take measures to prevent such kinds of malicious behavior.

To avoid these things, we'd better work with trustable environment, dependencies, etc.

[phroi]

To avoid these things, we'd better work with trustable environment, dependencies, etc.

You are right, that's also why I try to minimize dependencies on external libraries and Lumos makes this easy, thanks again @homura :pray: