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

ts: Add missing IDL types #2688

Closed
wants to merge 1 commit into from

Conversation

vadorovsky
Copy link
Contributor

The following new IDL types were introduced in #2011:

  • GenericLenArray
  • Generic
  • DefinedWithTypeArgs

Usage of these types was leading to incompatibility of the IDL with the TypeScript IdlType.

Fixes: #2687

@vercel
Copy link

vercel bot commented Oct 28, 2023

@vadorovsky is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

The following new IDL types were introduced in coral-xyz#2011:

* `GenericLenArray`
* `Generic`
* `DefinedWithTypeArgs`

Usage of these types was leading to incompatibility of the IDL with
the TypeScript `IdlType`.

Fixes: coral-xyz#2687
@acheroncrypto acheroncrypto added ts idl related to the IDL, either program or client side labels Oct 28, 2023
Comment on lines +142 to +145
| IdlTypeArray
| IdlTypeGenericLenArray
| IdlTypeGeneric
| IdlTypeDefinedWithTypeArgs;
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

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.

@acheroncrypto acheroncrypto mentioned this pull request Feb 25, 2024
@acheroncrypto
Copy link
Collaborator

Superseded by #2824.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idl related to the IDL, either program or client side ts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IDL: Types with generics are not assignable to parameter of type Idl
2 participants