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.

Sign up for GitHub

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

ts: Add missing IDL types #2688

Closed
wants to merge 1 commit into from
Closed

ts: Add missing IDL types #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions tests/idl/tests/generics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import * as anchor from "@coral-xyz/anchor";
import { Idl } from "@coral-xyz/anchor";
import { expect } from "chai";

import { Generics, IDL } from "../target/types/generics";

describe("Generics", () => {
anchor.setProvider(anchor.AnchorProvider.env());

/**
* Checks if the IDL produced by Anchor CLI is compatible with the TypeScript
* `Idl` type. Detects a potential mismatch between Rust and TypeScript IDL
* definitions.
*/
it("Raw IDL", () => {
const idl: Idl = IDL;
expect(idl).to.not.be.undefined;
});
});
34 changes: 33 additions & 1 deletion ts/packages/anchor/src/idl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ export type IdlType =
| IdlTypeOption
| IdlTypeCOption
| IdlTypeVec
| IdlTypeArray;
| IdlTypeArray
| IdlTypeGenericLenArray
| IdlTypeGeneric
| IdlTypeDefinedWithTypeArgs;
Comment on lines +142 to +145
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't hurt to merge but the reason why these types were not added was because they are not yet stable, and not supported with the default IDL generation method(idl-parse).

Copy link
Contributor Author

@vadorovsky vadorovsky Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, IDL produced by idl-parse method is still going to satisfy the Idl type, so I don't think that keeping unstable types there is going to break anything or be backwards-incompatible. I could add some comments.

On the other hand, not having them there makes idl-build not fully usable, as soon as you have any generics in your struct, which I think is a bug.

To be fair - I know that previous versions of Anchor didn't support generics - we are maintaining a fork with ugly patches to make that work in Anchor 0.28. 😢 That, and we also have a hacky way for supporting fields with types from external dependencies and for supporting Accounts structs wrapped by your own macros (we have a macro adding accounts which are mandatory in Light Protocol, so you don't have to copy them over all the time). They are ugly enough that I've never tried to upstream them and I've also seen the idl-build effort going on, so decided to wait. Supporting such use-cases with idl-parse method is horrible. I sincerely hope that one day idl-build is just going to fully replace idl-parse.

I'm looking forward to ditch our fork, because the idl-build method seems to work for us with this patch. Idl-parse solves all the problems that drove us towards patching 0.28 (it works absolutely fine with fields with types from external crates, supports generics and custom macros wrapping Accounts).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, IDL produced by idl-parse method is still going to satisfy the Idl type, so I don't think that keeping unstable types there is going to break anything or be backwards-incompatible. I could add some comments.

This was more to discourage the use of the unimplemented types as those types could change in a backwards-incompatible manner.

On the other hand, not having them there makes idl-build not fully usable, as soon as you have any generics in your struct, which I think is a bug.

Adding these types will only remove the type error which doesn't even exist in the runtime code. With these changes, if you were to use those types in any way, you'll get runtime errors instead of a compile-time error because they are not implemented.

To be fair - I know that previous versions of Anchor didn't support generics - we are maintaining a fork with ugly patches to make that work in Anchor 0.28. 😢 That, and we also have a hacky way for supporting fields with types from external dependencies and for supporting Accounts structs wrapped by your own macros (we have a macro adding accounts which are mandatory in Light Protocol, so you don't have to copy them over all the time). They are ugly enough that I've never tried to upstream them and I've also seen the idl-build effort going on, so decided to wait. Supporting such use-cases with idl-parse method is horrible. I sincerely hope that one day idl-build is just going to fully replace idl-parse.

I'm looking forward to ditch our fork, because the idl-build method seems to work for us with this patch. Idl-parse solves all the problems that drove us towards patching 0.28 (it works absolutely fine with fields with types from external crates, supports generics and custom macros wrapping Accounts).

Those patches sound painful and I'm glad idl-build is useful for you guys. As for replacing the default generation method, I think the best way forward will be to combine the generation methods in order to get the best of both worlds as there are some parts where it makes more sense to use idl-parse and some parts with idl-build. Furthermore, keeping 2 different generation methods is not maintainable as some features can only be implemented in one of the generation methods which results in discrepancy between the generated IDLs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed in #2824, along with many other things with the IDL. It would greatly help if you could try it out and give feedback before it's included in a release, as it becomes harder to make changes after people start using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome! I will give it a try later today.


// User defined type.
export type IdlTypeDefined = {
Expand All @@ -162,6 +165,35 @@ export type IdlTypeArray = {
array: [idlType: IdlType, size: number];
};

export type IdlTypeGenericLenArray = {
genericLenArray: [idlType: IdlType, generic: string];
};

export type IdlTypeGeneric = {
generic: string;
};

export type IdlTypeDefinedWithTypeArgs = {
definedWithTypeArgs: { name: string; args: IdlTypeDefinedTypeArg[] };
};

export type IdlTypeDefinedTypeArg =
| IdlTypeDefinedTypeArgGeneric
| IdlTypeDefinedTypeArgValue
| IdlTypeDefinedTypeArgType;

export type IdlTypeDefinedTypeArgGeneric = {
generic: string;
};

export type IdlTypeDefinedTypeArgValue = {
value: string;
};

export type IdlTypeDefinedTypeArgType = {
type: IdlType;
};

export type IdlEnumVariant = {
name: string;
fields?: IdlEnumFields;
Expand Down