Skip to content
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

Add default type values to parse functions in js-experimental #164

Closed

Conversation

mcintyre94
Copy link
Contributor

Currently when we generate a ParsedXInstruction type it has sensible default type params:

export type ParsedTransferSolInstruction<
  TProgram extends string = "11111111111111111111111111111111",
  TAccountMetas extends readonly IAccountMeta[] = readonly IAccountMeta[],
> = {

But the parseXInstruction doesn't have default type params:

export declare function parseTransferSolInstruction<
  TProgram extends string,
  TAccountMetas extends readonly IAccountMeta[],
>(instruction: <snip>
): ParsedTransferSolInstruction<TProgram, TAccountMetas>;

This means that the ParsedXInstruction type returned by parseXInstruction(instruction) returned ParsedXInstruction<string, readonly IAccountMeta[]>

This causes a problem because the definition of ParsedSplSystemProgramInstruction includes ParsedTransferSolInstruction without specifying type params, ie using the default of the program ID

Therefore we get a type error if we try to do:

const parsedInstruction = parseTransferSolInstruction(instruction)
const x: ParsedSplSystemInstruction = {
  instructionType: SplSystemInstruction.TransferSol,
  ...parsedInstruction // program address incompatible, got string, expected programID 
}

I think the best fix is to just add the default type params to the parseXInstruction function to match the ParsedXInstruction type, which is what this PR does

Most changes are a result of the generated test being updated, since this changes the parse function in each instruction. Non-generated changes are to src/renderers/js-experimental/fragments/instructionParseFunction.njk + https://github.com/metaplex-foundation/kinobi/compare/main...mcintyre94:cm-default-parse-types?expand=1#diff-7924ecc400cbdbf956997331e2f3f3736f6405fd3649c66d314961724d8d89ac

Copy link

changeset-bot bot commented Feb 29, 2024

🦋 Changeset detected

Latest commit: b4a188a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@metaplex-foundation/kinobi Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcintyre94 mcintyre94 changed the title Add default type params to parseXInstruction functions Add default type values to parse functions in js-experimental Feb 29, 2024
@lorisleiva
Copy link
Collaborator

I think a better fix would be to parameterise the generated union type like so:

export type ParsedSplSystemInstruction<TProgram extends string = "..."> =
  | ({
      instructionType: SplSystemInstruction.CreateAccount;
    } & ParsedCreateAccountInstruction<TProgram>)
  | ({
      instructionType: SplSystemInstruction.TransferSol;
    } & ParsedTransferSolInstruction<TProgram>);

I'm not a big fan of providing default values to function type parameters because IIRC there are some cases where it affects the type inferences.

@mcintyre94
Copy link
Contributor Author

mcintyre94 commented Feb 29, 2024

Do you mean make it export type ParsedSplSystemInstruction<TProgram extends string = string> =? That should work, because we'd loosen the type of the ParsedTransferSolInstruction to any string. But putting the program ID in there is the same as what we already have, because ParsedTransferSolInstruction is defaulting to its program ID.

FWIW we can also not change this, and pass the responsibility to the caller: const parsedInstruction = parseTransferSolInstruction<typeof SPL_SYSTEM_PROGRAM_ADDRESS, readonly IAccountMeta[]>(instruction);. And you'll only need that when you do care about the TProgram on ParsedTransferSolInstruction. So it's not critical

@mcintyre94
Copy link
Contributor Author

Closing - I'll open a separate PR to parameterise the ParsedXInstruction type but fix this specific issue with better instruction narrowing in web3js.

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.

2 participants