-
Notifications
You must be signed in to change notification settings - Fork 164
Update advance_nonce_account.rs #271
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
Conversation
sonicfromnewyoke
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.
good catch, system program uses u32 le as discriminator
also, would you mind updating upgrade_nonce_account.rs and making the same change?
| program_id: &crate::ID, | ||
| accounts: &account_metas, | ||
| data: &[4], | ||
| data: &DATA, |
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.
maybe this way will be simpler?
| data: &DATA, | |
| data: &[4, 0, 0, 0], |
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.
true haha, just made a new commit with the change and also updated upgrade_nonce_account.rs
|
|
||
| // instruction | ||
| // instruction data uses 4-byte LE discriminator expected by the system program | ||
| const DATA: [u8; 4] = [4, 0, 0, 0]; |
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.
use this array directly
| const DATA: [u8; 4] = [4, 0, 0, 0]; |
sonicfromnewyoke
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.
LGTM
| ]; | ||
|
|
||
| // instruction | ||
| // instruction data uses 4-byte LE discriminator expected by the system program |
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: Could we keep the existing comment? I think it is a bit redundant to have it on every instruction and other don't have it.
| let account_metas: [AccountMeta; 1] = [AccountMeta::writable(self.account.key())]; | ||
|
|
||
| // instruction | ||
| // instruction data uses 4-byte LE discriminator expected by the system program |
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: Same here, could we keep the existing 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.
Yup, just pushed the new commit to keep the existing comment
febo
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.
This is great, thank you! I will fix the CI issue so we can get this merged.
|
@dedeleono If you rebase the PR, CI should be able to run. |
- Using array slices directly
6991914 to
e892ebe
Compare
Nice, just did! @febo |
febo
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 great, thanks!
This update builds the instruction with a const [4, 0, 0, 0] payload, ensuring the System Program receives the 4-byte little-endian discriminator it expects. This prevents the invalid instruction data errors previously occurring.
Ref: Issue #270