Create TransactionFrame for ABIv2#9800
Conversation
|
The Firedancer team maintains a line-for-line reimplementation of the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9800 +/- ##
========================================
Coverage 82.5% 82.5%
========================================
Files 844 844
Lines 316364 316472 +108
========================================
+ Hits 261228 261320 +92
- Misses 55136 55152 +16 🚀 New features to boost your workflow:
|
nagisa
left a comment
nagisa
left a comment
There was a problem hiding this comment.
LGTM, but maybe worth getting a stamp from somebody else as well.
|
transaction-context/src/lib.rs
Outdated
| self.get_instruction_context_at_index_in_trace(index_in_trace) | ||
| } | ||
|
|
||
| pub fn configure_cpi_instruction( |
There was a problem hiding this comment.
Is there a reason to factor this out of configure_next_instruction() without parent_index being the optional?
There was a problem hiding this comment.
We only increment the number of instructions during a CPI, so I thought I could split it out. If you wish, I can put back everything together.
There was a problem hiding this comment.
Yep, I would like it to stay one function.
transaction-context/src/lib.rs
Outdated
| .transaction_frame | ||
| .number_of_instructions | ||
| .saturating_add(1); | ||
| self.transaction_frame.cpi_scratchpad = VmSlice::new( |
There was a problem hiding this comment.
How is this not necessary at the top-level instructions too? Or rather how can you have any CPI if only already CPI-ed instructions can do more CPI?
There was a problem hiding this comment.
It should be done for top-level instructions too. I did it wrong.
| ), | ||
| current_executing_instruction: 0, | ||
| number_of_instructions: number_of_instructions as u16, | ||
| number_of_executed_cpis: 0, |
There was a problem hiding this comment.
Wouldn't it be more useful to have the total_number_of_top_level_instructions here and if somebody really needs the number_of_executed_cpis they calculate number_of_instructions - total_number_of_top_level_instructions?
There was a problem hiding this comment.
I don't think it makes much of a difference, and, at the same time, I can't really foresee which parameter users will need more: number of executed CPIs or number of top level instructions.
I followed one suggestion given in this comment: solana-foundation/solana-improvement-documents#177 (comment)
PS: total_number_of_top_level_instructions doesn't need to have the total_ prefix, since this number never changes.
There was a problem hiding this comment.
Well, without the prefix it could also mean number_of_top_level_instructions_executed_so_far, which is the top_level_instruction_index
4 participants
Problem
In order for ABIv2 to work, we need a
TransactionFramethat is going to share transaction information with programs. The frame follows the design for transaction metadata described in SIMD-0177.Summary of Changes
configure_cpi_instructionto facilitate the management of some inner fields.