-
Notifications
You must be signed in to change notification settings - Fork 76
Fix: include UnwrapLamports (45) in TokenInstruction::try_from #118
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
base: main
Are you sure you want to change the base?
Fix: include UnwrapLamports (45) in TokenInstruction::try_from #118
Conversation
joncinque
left a comment
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.
Looks good to me, but let's get an approval from @febo
|
@victorchukwuemeka Looks good – I think we need to remove the extra blank spaces that were added so CI passes. |
| use {crate::error::TokenError, pinocchio::program_error::ProgramError}; | ||
|
|
||
|
|
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: Not needed.
|
|
||
|
|
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: Not needed.
| AccountOwner, | ||
| /// Authority to close a token account | ||
| CloseAccount, | ||
|
|
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: Not needed.
| assert_eq!( | ||
| TokenInstruction::from_repr(variant_u8), | ||
| Some(TokenInstruction::try_from(variant_u8).unwrap()) | ||
|
|
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: Not needed.
|
|
||
|
|
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: Not needed.
|
okay i;m clearing the spaces now |
2600e50 to
ead864c
Compare
|
@febo i have cleaned the spaces |
There are a couple ones left. |
| } | ||
| } | ||
|
|
||
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: Remove the extra spaces.
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.
done
ead864c to
851ffe1
Compare
| match value { | ||
| // SAFETY: `value` is guaranteed to be in the range of the enum variants. | ||
| 0..=24 | 38 | 255 => Ok(unsafe { core::mem::transmute::<u8, TokenInstruction>(value) }), | ||
| 0..=24 | 38 | 45 | 255 => Ok(unsafe { core::mem::transmute::<u8, TokenInstruction>(value) }), |
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: Last nit for CI.
| 0..=24 | 38 | 45 | 255 => Ok(unsafe { core::mem::transmute::<u8, TokenInstruction>(value) }), | |
| 0..=24 | 38 | 45 | 255 => { | |
| Ok(unsafe { core::mem::transmute::<u8, TokenInstruction>(value) }) | |
| } |
The TryFrom implementation for TokenInstruction was missing the
UnwrapLamports = 45 variant, causing
test_token_instruction_from_u8_exhaustive to fail.
This PR adds the missing variant to keep the conversion logic in sync with
the enum definition.