feat: IDL support for custom vector length discriminators#4148
feat: IDL support for custom vector length discriminators#4148Otter-0x4ka5h wants to merge 10 commits into
Conversation
|
Someone is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
acheroncrypto
left a comment
There was a problem hiding this comment.
Have you seen #4076 (comment) ?
|
@acheroncrypto sorry for the confusion here, will make changes. I think I pushed the wrong commit. was implemented with impl IdlSerialization {
/// Returns the number of bytes used for Vec length prefix in this serialization format.
pub fn vec_length_bytes(&self) -> usize {
match self {
IdlSerialization::Borsh => 4,
IdlSerialization::BorshU8 => 1,
IdlSerialization::BorshU16 => 2,
IdlSerialization::Bytemuck | IdlSerialization::BytemuckUnsafe => {
8
}
IdlSerialization::Custom(_) => {
4
}
}
}
} |
re-pushed the correct changes |
acheroncrypto
left a comment
There was a problem hiding this comment.
Overall, I think the approach is correct, but there are a few more things that need to get addressed:
-
The serialization should apply to all unsized types; otherwise, it would be very unintuitive (on-chain programs can't handle more than 2 bytes length
borshde/serialization anyway) -
How to use the new serialization specs?
- In programs, how to use them without having to manually implement the de/serialization?
- How to include this information (the
serializationfield) in the IDL conveniently?
Perhaps something like a generic
#[serialization]attribute or a specific#[custom_borsh]similar to the#[zero_copy]attribute? -
declare_program!also needs to support these new serialization methods, e.g. in:Depending on the resolution of the last point, something like:
IdlSerialization::BorshU8 => quote!(#[custom_borsh(u8)]), IdlSerialization::BorshU16 => quote!(#[custom_borsh(u16)]),
| - run: cd tests/bpf-upgradeable-state | ||
| - run: cd tests/bpf-upgradeable-state && anchor build --skip-lint --ignore-keys | ||
| - run: cd tests/bpf-upgradeable-state && solana program deploy --program-id program_with_different_programdata.json target/deploy/bpf_upgradeable_state.so | ||
| - run: cd tests/bpf-upgradeable-state && solana program deploy --program-id program_with_different_programdata.json --use-rpc target/deploy/bpf_upgradeable_state.so |
There was a problem hiding this comment.
Need rebase since this is fixed in master (#4164)
| .try_into() | ||
| .unwrap(); | ||
| // Use serialization format from parent type definition, or default to Borsh (u32) | ||
| let serialization = serialization.unwrap_or(&IdlSerialization::Borsh); |
There was a problem hiding this comment.
It's better to use the actual default rather than manually specifying the default.
| let serialization = serialization.unwrap_or(&IdlSerialization::Borsh); | |
| let serialization = serialization.unwrap_or_default(); |
| [dependencies] | ||
| anyhow = "1" | ||
| serde = { version = "1", features = ["derive"] } | ||
| serde_json = "1.0.146" No newline at end of file |
There was a problem hiding this comment.
Why do we need this? The IDL spec itself doesn't care about JSON at all.
| serde_json = "1.0.146" |
|
|
||
| impl IdlSerialization { | ||
| /// Returns the number of bytes used for Vec length prefix in this serialization format. | ||
| pub fn vec_length_bytes(&self) -> usize { |
There was a problem hiding this comment.
It will be unintuitive and inconsistent if other unsized types still use 4 bytes when they specify BorshU8 or BorshU16 serialization. Therefore, it would be better if this was a generalized method like length_prefix_bytes.
The return type should be Option<usize> instead of usize because not all serialization specs support unsized types.
| pub fn vec_length_bytes(&self) -> usize { | |
| pub fn length_prefix_bytes(&self) -> Option<usize> { |
| IdlSerialization::Borsh => 4, | ||
| IdlSerialization::BorshU8 => 1, | ||
| IdlSerialization::BorshU16 => 2, |
There was a problem hiding this comment.
In line with the previous comment:
| IdlSerialization::Borsh => 4, | |
| IdlSerialization::BorshU8 => 1, | |
| IdlSerialization::BorshU16 => 2, | |
| IdlSerialization::Borsh => Some(4), | |
| IdlSerialization::BorshU8 => Some(1), | |
| IdlSerialization::BorshU16 => Some(2), |
| { ...field, type: genericArg.type }, | ||
| types | ||
| types, | ||
| undefined, |
There was a problem hiding this comment.
Nit: it's better to use null when you need to pass a value that you can't omit.
| undefined, | |
| null, |
| return IdlCoder.fieldLayout( | ||
| { type: typeDef.type.alias, name }, | ||
| types, | ||
| undefined, |
There was a problem hiding this comment.
Same here:
| undefined, | |
| null, |
| const vecValue = type.vec; | ||
| const innerType = | ||
| typeof vecValue === "string" || !("type" in vecValue) | ||
| ? typeof vecValue === "string" | ||
| ? vecValue | ||
| : (vecValue as any) | ||
| : vecValue.type; |
There was a problem hiding this comment.
What does this change achieve? It looks wrong. Serialization should not affect generic argument resolution.
| const vecValue = type.vec; | |
| const innerType = | |
| typeof vecValue === "string" || !("type" in vecValue) | |
| ? typeof vecValue === "string" | |
| ? vecValue | |
| : (vecValue as any) | |
| : vecValue.type; |
| : vecValue.type; | ||
| const args = IdlCoder.resolveGenericArgs({ | ||
| type: type.vec, | ||
| type: innerType, |
There was a problem hiding this comment.
In line with the previous comment:
| type: innerType, | |
| type: type.vec, |
| const innerType = idlType.vec; | ||
| return `Vec<${this.formatIdlType(innerType)}>`; |
There was a problem hiding this comment.
This change doesn't seem to be doing anything.
| const innerType = idlType.vec; | |
| return `Vec<${this.formatIdlType(innerType)}>`; | |
| return `Vec<${this.formatIdlType(idlType.vec)}>`; |
|
Deployment failed with the following error: |
51767f8 to
7256289
Compare
Co-authored-by: acheron <98934430+acheroncrypto@users.noreply.github.com>
6b09543 to
1059a19
Compare
Anchor IDLs now support vectors with custom length discriminators (u8, u16, or u32), not just the default u32. This helps optimize account size for programs that use compact vectors.
Fixes #4076