o1-labs / o1js

TypeScript framework for zk-SNARKs and zkApps
https://docs.minaprotocol.com/en/zkapps/how-to-write-a-zkapp
Apache License 2.0
473 stars 105 forks source link

Types function inconsistency #1663

Open aii23 opened 1 month ago

aii23 commented 1 month ago

o1js version: 1.2.0

Faced several inconsistency in types function. Nothing critical, but sometimes inconvenient to use.

assertEquals incosistency For some types assertEquals have an optional parameter for error message, others don't.

Types that have error message:

Types that don't:

UInt .from inconsistency

UInt8 has this .from function signature: static from(x: UInt8 | UInt64 | UInt32 | Field | number | bigint): UInt8; UInt32: static from(x: UInt32 | number | string | bigint): UInt32; UInt64: static from(x: UInt64 | UInt32 | number | string | bigint): UInt64;

So I can create UInt8 from pretty much everything, but I can't create UInt32 from UInt8, UInt64 and Field. And can't create UInt64 from UInt8 and Field.

It is quite frustrating when dealing with different types.

dfstio commented 4 weeks ago

There is also a minor inconsistency: UInt64.toBigInt() but UInt32.toBigint()

mitschabaude commented 4 weeks ago

assertEquals inconsistency

this should be fixed and can be done in a non-breaking way, since the message argument is optional

UInt .from inconsistency

some of this is intentional. the general idea is that from() does simple conversions which don't add extra constraints.

However, it seems that for UInt8 we went for a different design and just add the necessary constraints inside from(). So there using from(UInt64 | UInt32 | Field) is fine.

Maybe, for consistency, we should just make UInt32/64.from() do the same constrained conversion 🤔

In any case, we can ad UInt8 inputs to UInt32.from() and UInt64.from() for free.

mitschabaude commented 4 weeks ago

There is also a minor inconsistency: UInt64.toBigInt() but UInt32.toBigint()

that's annoying. I'd like us to deprecate all toBigInt() and add toBigint() throughout o1js

mitschabaude commented 4 weeks ago

In general, I don't like the combinatorial explosion of dealing with conversions between all different integer types. Same as I don't like the duplication in our code base for implementing different uints, and the fact that some of them are missing.

The best way forward might be a general UInt type which keeps track of its number of bits internally

dfstio commented 4 weeks ago

The best way forward might be a general UInt type which keeps track of its number of bits internally

Please keep in mind that it is convenient sometimes to specify the bits size explicitly to pack several integers into one Field

export class NFTparams extends Struct({
  price: UInt64,
  version: UInt32,
}) {
  pack(): Field {
    const price = this.price.value.toBits(64);
    const version = this.version.value.toBits(32);
    return Field.fromBits([...price, ...version]);
  }
  static unpack(packed: Field) {
    const bits = packed.toBits(64 + 32);
    const price = UInt64.from(0);
    price.value = Field.fromBits(bits.slice(0, 64));
    const version = UInt32.from(0);
    version.value = Field.fromBits(bits.slice(64, 64 + 32));
    return new NFTparams({ price, version });
  }
}

export class NFTContractV2 extends SmartContract {
  @state(Field) name = State<Field>();
  @state(MetadataParams) metadataParams = State<MetadataParams>();
  @state(PublicKey) owner = State<PublicKey>();
  @state(Field) data = State<Field>(); // NFTparams packed
...