Skip to content

Add generics for contract type-safety #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

PHCitizen
Copy link
Contributor

@PHCitizen PHCitizen commented May 30, 2024

this is the implementation of the snippets I post on telegram. Types are a bit different because I make a fallback if "any" is passed. this is needed if the user are using javascript or artifact are imported somewhere, and we cant infer the types

latest ts playground

btw i remove infer using string. i think if they will use it, much better to use interface and define type there.

Copy link

vercel bot commented May 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cashscript ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 11:06am

@PHCitizen PHCitizen marked this pull request as draft May 30, 2024 05:32
@PHCitizen PHCitizen marked this pull request as ready for review June 1, 2024 13:09
@rkalis
Copy link
Member

rkalis commented Jun 4, 2024

Hey, this is really cool! Thanks for creating this PR. This functionality would be really nice to have, we saw that there's some TODOs / refactors still needed. If we want to integrate this, then the inferring would also need to work for the unlock parts of the contract for sure.

We will have more bandwidth to get this implemented / integrated once the debugging tooling upgrade (v0.10) is released, so we'll get back to this issue after that.

@PHCitizen
Copy link
Contributor Author

on commit 90191fb i use ignore that error, because we know that it must be indexed and we are setting the right value
if we don't use ignore, we need to cast type manually

const name = f.name as keyof TFunctions;
this.functions[name] = this.createFunction(f) as TFunctions[keyof TFunctions];

@rkalis
Copy link
Member

rkalis commented Sep 24, 2024

Hey @PHCitizen, just want to write to let you know we haven't forgotten about this PR. We've finally released the debugging functionality with version 0.10.0 earlier this month. We will finally be able to pick this PR up in November. Thanks once again for the effort thusfar, and we'll work to get this integrated in November. 🙌

@rkalis rkalis added this to the v0.10.3 milestone Oct 22, 2024
@mr-zwets
Copy link
Member

mr-zwets commented Nov 2, 2024

Regarding completing the TypeMap as documented in

// TODO: use proper value for types below

a back and forth with chatGPT suggested the following to include byte and bytesX types

type GenerateBytes<N extends number, Result extends unknown[] = []> = 
  Result['length'] extends N
    ? never
    : `bytes${Result['length'] | 1}` | GenerateBytes<N, [...Result, unknown]>;

// TypeMap with dynamic fixed-length byte types
type TypeMap = {
  bool: boolean;
  int: bigint;
  string: string;
  byte: Uint8Array;
  bytes: Uint8Array;
  [K in GenerateBytes<100>]: Uint8Array;  // Generates `bytes1` to `bytes100`
  pubkey: Uint8Array;
  sig: Uint8Array;
  datasig: Uint8Array;
};

@mr-zwets
Copy link
Member

mr-zwets commented Nov 2, 2024

ChatGPT also suggested a possible solution to the nesting in GetTypeAsTuple and suggested renaming for clarity

/**
 * Converts an array of artifact parameters into a tuple type representing each parameter's type.
 *
 * Example:
 * If T = [{ type: 'int' }, { type: 'bool' }], GetParameterTuple<T> resolves to [bigint, boolean].
 */
type GetParameterTuple<T> = 
  T extends readonly [{ type: infer ParamType }, ...infer Rest] 
    ? [ParamType extends keyof TypeMap ? TypeMap[ParamType] : any, ...GetParameterTuple<Rest>]
    : [];

@mr-zwets
Copy link
Member

mr-zwets commented Nov 2, 2024

while further trying to improve the types ChatGPT suggested sometthing similar for InferContractFunction<T> which solves the deep nesting and combines the Prettify<T> and _InferContractFunction<T> helper types

It needs careful review and testing! but solving the nesting looks promising for maintainability... 👀

/**
 * Infers function argument types from an artifact ABI, producing a map of function names to argument tuples.
 *
 * Example:
 * If T = [{ name: 'func', inputs: [{ type: 'int' }] }, { name: 'otherFunc', inputs: [{ type: 'bool' }] }],
 * then InferContractFunction<T> resolves to { func: [bigint], otherFunc: [boolean] }
 */
type InferContractFunction<T> = 
  T extends readonly [{ name: infer Name, inputs: infer Inputs }, ...infer Rest]
    ? { [K in Name & string]: GetParameterTuple<Inputs> } & InferContractFunction<Rest>
    : {};

@PHCitizen
Copy link
Contributor Author

type GenerateBytes<N extends number, Result extends unknown[] = []> = 
  Result['length'] extends N
    ? never
    : `bytes${Result['length'] | 1}` | GenerateBytes<N, [...Result, unknown]>;

// TypeMap with dynamic fixed-length byte types
type TypeMap = {
  bool: boolean;
  int: bigint;
  string: string;
  byte: Uint8Array;
  bytes: Uint8Array;
  [K in GenerateBytes<100>]: Uint8Array;  // Generates `bytes1` to `bytes100`
  pubkey: Uint8Array;
  sig: Uint8Array;
  datasig: Uint8Array;
};

does this one compile? i cant compile it. anyway i expected there that the number are fixed and did not allow arbitrary number

if any number can be use i think we can use this one

type TypeMap = {
  [k: `bytes${number}`]: Uint8Array;
} & {
  "bool": boolean;
  "int": bigint;
  .....
}

@PHCitizen
Copy link
Contributor Author

PHCitizen commented Nov 2, 2024

Regarding to other improvement listed, i think i encounter that before and that is my first solution/attempt.

Then i came to place where "any" messed all up, so i try to push it futher and ended it that code😆

But maybe im wrong? who will feed "any" on that function. Anyway i think most of the user that will ended in "any" are those who uses JS, maybe they do not need type support or im thinking if the edge case error will show/not on them

will look futher soon

@mr-zwets
Copy link
Member

does this one compile? i cant compile it. anyway i expected there that the number are fixed and did not allow arbitrary number

if any number can be use i think we can use this one

type TypeMap = {
  [k: `bytes${number}`]: Uint8Array;
} & {
  "bool": boolean;
  "int": bigint;
  .....
}

You are correct, my attempt did not 😅 thanks for the correct way!

Regarding to other improvement listed, i think i encounter that before and that is my first solution/attempt.

Then i came to place where "any" messed all up, so i try to push it futher and ended it that code😆

But maybe im wrong? who will feed "any" on that function. Anyway i think most of the user that will ended in "any" are those who uses JS, maybe they do not need type support or im thinking if the edge case error will show/not on them

will look futher soon

@PHCitizen it would be super helpful to also have that code if you have the typescript playground link!
I think indeed any handling might not be worth the additional complexity 🤔

@mr-zwets
Copy link
Member

mr-zwets commented Nov 21, 2024

I did another attempt to simplify the typings to increase maintainability & simplicity of the typescript code. I named the generics to make it more clear what is going on. Here is the main part:

class Contract<
  Artifact extends { constructorInputs: readonly any[]; abi: readonly any[] } = {
    constructorInputs: any[];
    abi: any[];
  },
  ResolvedTypes extends {
    constructorInputs: any[];
    functions: Record<string, any>;
  } = {
    constructorInputs: ParamsToTuple<Artifact["constructorInputs"]>;
    functions: AbiToFunctionMap<Artifact["abi"]>;
  }
> {
  functions!: ResolvedTypes["functions"];

  constructor(artifact: Artifact, params: ResolvedTypes["constructorInputs"]) {}
}

48 lines compared to the original nested 78 but it also removes the InferFn & InferCompiledFn helpers which might be helpful for manual typings

typscript playground link

@PHCitizen
Copy link
Contributor Author

PHCitizen commented Nov 24, 2024

I did another attempt to simplify the typings to increase maintainability & simplicity of the typescript code. I named the generics to make it more clear what is going on. Here is the main part:

class Contract<
  Artifact extends { constructorInputs: readonly any[]; abi: readonly any[] } = {
    constructorInputs: any[];
    abi: any[];
  },
  ResolvedTypes extends {
    constructorInputs: any[];
    functions: Record<string, any>;
  } = {
    constructorInputs: ParamsToTuple<Artifact["constructorInputs"]>;
    functions: AbiToFunctionMap<Artifact["abi"]>;
  }
> {
  functions!: ResolvedTypes["functions"];

  constructor(artifact: Artifact, params: ResolvedTypes["constructorInputs"]) {}
}

48 lines compared to the original nested 78 but it also removes the InferFn & InferCompiledFn helpers which might be helpful for manual typings

typscript playground link

On ParamsToTuple, if we pass type that dont match on the typemap, it defaults to UintArray

Solution: add extends to infer instead of using TypeMap[Type & keyof TypeMap]

type ParamsToTuple<Params> = Params extends readonly [infer Head, ...infer Tail]
  ? Head extends { type: infer Type extends keyof TypeMap}
      ? [TypeMap[Type], ...ParamsToTuple<Tail>]
    : [any, ...ParamsToTuple<Tail>]
  : [];

and also adding ReturnType generic args to AbiToFunctionMap, instead of infering the return type as any. But im not really sure about this one

i think the best way is not to return function type when using AbiToFunctionMap, instead just return the result of ParamsToTuple

To make it simple, what i really intend is to simplify return type typing specifically for Contract.functions and Contract.unlock

@PHCitizen it would be super helpful to also have that code if you have the typescript playground link!

AFAIK I send the initial copy somewhere, i cant remember where, i assume it's on telegram, but i forgot what channel

@mr-zwets
Copy link
Member

I found the original on telegram! 😄 👍 https://t.me/bch_compilers/7456

hello just want to share type-safe "infer" for cashscript contract
but currently its for contract functions only (not for contract constructor)

but this need to change how the js function are called, thats why im not sure if you will like it😅

i mean of this
contract.functions.some_method(param1, param2)

it will become
contract.functions.some_method({ param1: value, param2: value})

but the advantage are autocomplete on available functions method, and error for params, (eg if we have class for every >paramType, ts-server can error if invalid class are passed)

TypeScript playground

@rkalis
Copy link
Member

rkalis commented Nov 26, 2024

We got started on implementing the fixes discussed by @PHCitizen and @mr-zwets above, and integrating the required changes into this PR. We also started adding tests for this type inference to the github repo, based on the test cases from the TS playground link(s) above.

One difficulty we noticed is that it is impossible to import a JSON file as const in TypeScript, which would be required to have proper type inference when importing artifacts from JSON files. The workaround for this is to store the artifacts in a .ts files instead of JSON, but this does reduce their portability. See microsoft/TypeScript#32063 for more info about this issue.

We'll continue the work on this on Thursday, and I think we should be able to finish most of the test cases and refactors by then. And then we'll just need to think if there are better ways to mitigate the "const json" issue.

@rkalis
Copy link
Member

rkalis commented Nov 28, 2024

We finished the refactors + tests, so this should be good to go. The only drawback is still that we cannot infer proper types from JSON files, so we want to add an option to the cashc CLI to export a .ts file instead of a .json file, so that types can be inferred.

Next Tuesday we will add this option, update documentation and release this functionality. 🫡

@rkalis
Copy link
Member

rkalis commented Dec 3, 2024

Thanks @PHCitizen for this contribution. We added the --format ts flag that we talked about in the previous comment. Hopefully TypeScript makes it possible some time in the future to import JSON files as const so we don't need that workaround. But for now it is nice to give our users the option of proper typings if they use that flag.

@rkalis rkalis changed the title added generics for contract type-safety Add generics for contract type-safety Dec 3, 2024
@rkalis rkalis merged commit 1dec0f6 into CashScript:master Dec 3, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants