-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Draft] Don't include PhantomData fields in IdlBuild impl #3433
base: master
Are you sure you want to change the base?
Conversation
@ssadler is attempting to deploy a commit to the coral-xyz Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to work on this!
I think we also need to remove the generics that are only used by the PhantomData
to make the definition valid. Otherwise, using declare_program!
with this IDL would result in a compile error because Rust doesn't allow unused generic parameters.
Would like to know if this PR is acceptable as is, or what other changes need to be made also?
It's helpful and has minimal changes, so yes, it's certainly acceptable. However, it would be great to have some tests for this in here. Tests should cover compilation of a definition with:
- Existing generic parameters (other than the one used by the
PhantomData
field) - No existing generic parameters
The generated IDL should also be checked to make sure:
- The
PhantomData
fields don't exist - Generic parameter definitions are correctly omitted.
syn::Type::Path(path) if the_only_segment_is(path, "PhantomData") => { | ||
Ok((TokenStream::new(), vec![])) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: match
arms more or less follow the order of IdlType
, so it would be great if you could move this logic either inside the Defined
impl:
anchor/lang/syn/src/idl/defined.rs
Lines 484 to 485 in 6df05aa
// Defined | |
syn::Type::Path(path) => { |
or just above it.
How would I produce this error? The change is Works On My Machine(TM), because though the account struct has a generic param, it's only the impl that omits encoding the PhantomData field and rust allows that. Updated title. |
Hi, I made this small change which addresses #3241 in order to be able to use a PhantomData field in an account.
It gets the IDL to build by filtering out PhantomData fields, however i have not tested it the other way, i.e., what happens if you want to encode a struct that includes a PhantomData field from a JS input.
Would like to know if this PR is acceptable as is, or what other changes need to be made also? For example:
Best