-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix #21397 #21535
Fix #21397 #21535
Conversation
…struction_account keyed_accounts in CPI.
automerge label removed due to a CI failure |
Codecov Report
@@ Coverage Diff @@
## master #21535 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 505 505
Lines 141409 141416 +7
=======================================
+ Hits 115506 115515 +9
+ Misses 25903 25901 -2 |
.ok_or(InstructionError::CallDepth)?; | ||
let first_instruction_account = frame | ||
.number_of_program_accounts | ||
.checked_sub(1) |
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.
@Lichtso why do you subtract 1 here?
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.
Because the number_of_program_accounts - 1
is first_instruction_account
.
Or in other words number_of_program_accounts
is first_instruction_account + 1
.
first_instruction_account
is actually the index of the program_id
of that instruction, see:
solana/program-runtime/src/invoke_context.rs
Line 132 in c9bfc99
.get(self.number_of_program_accounts.saturating_sub(1)) |
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 change is not working for me while developing a native program. I'm calling get_instruction_keyed_accounts
but the first account is the program account.
@@ -277,6 +277,8 @@ pub trait InvokeContext { | |||
fn remove_first_keyed_account(&mut self) -> Result<(), InstructionError>; | |||
/// Get the list of keyed accounts | |||
fn get_keyed_accounts(&self) -> Result<&[KeyedAccount], InstructionError>; | |||
/// Get the list of keyed accounts skipping `first_instruction_account` many entries |
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.
If the intent is to also include the instruction program keyed account in this list, can you update the comment to say so? The comment right now indicates that this will return a list of instruction account inputs (excluding the program account)
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.
Yes, it includes the program_id
(at index 0).
And good point, I will update the 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.
Thanks! It would be really helpful if the comment made it clearer what the breakdown of invoke context keyed accounts is and when it's appropriate to use get_instruction_keyed_accounts
over get_keyed_accounts
.
Problem
#20448 changed how the program
keyed_accounts
are popped from the front from usingInvokeContext::remove_first_keyed_account()
to instead using a parameter calledfirst_instruction_account
.However, while the change was applied to most of the program runtime, CPI was left out.
Luckily,
InvokeContextStackFrame::number_of_program_accounts
is set toprogram_indices.len()
(which is the same value asfirst_instruction_account + 1
) inInvokeContext::push()
so we can use that in CPI without routingfirst_instruction_account
explicitly as a parameter.Summary of Changes
Adds
InvokeContext::get_instruction_keyed_accounts()
to skipfirst_instruction_account
keyed_accounts
in CPI.Fixes #21397