replace low-hanging fruit dependencies on solana-program#6351
replace low-hanging fruit dependencies on solana-program#6351t-nelson merged 1 commit intoanza-xyz:masterfrom
solana-program#6351Conversation
|
The Firedancer team maintains a line-for-line reimplementation of the |
steviez
left a comment
steviez
left a comment
There was a problem hiding this comment.
Just a couple comments, nothing actionable so LGTM
| solana_program::instruction::get_stack_height, | ||
| solana_program::{instruction::get_stack_height, program::invoke}, |
There was a problem hiding this comment.
I think I saw you asking about this in Discord - confirming that reintroducing solana_program::program::invoke() is intentional as solana_cpi::invoke() is not 1-for-1 ?
There was a problem hiding this comment.
yeah native target code path is just a noop
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall, mostly small things!
All of the solana_program::program usage in programs/sbf (invoke, invoke_signed, set_return_data, get_return_data) should be able to use solana_cpi instead, but that can be done in a follow-up.
| solana_transaction_context::BorrowedAccount, | ||
| std::{mem, ptr}, | ||
| }; | ||
| // consts inlined to avoid solana-program dep |
There was a problem hiding this comment.
This is pretty glorious. I do wonder if we'll need to expose these consts somewhere else, but we can let people complain first
There was a problem hiding this comment.
i was surprised that they weren't in solana-cpi tbh
| solana_account_info::AccountInfo, | ||
| solana_msg::msg, | ||
| solana_program::{ | ||
| epoch_stake::{get_epoch_stake_for_vote_account, get_epoch_total_stake}, |
There was a problem hiding this comment.
Eep, these need to get migrated somewhere: anza-xyz/solana-sdk#178
t-nelson
force-pushed
the
dplf
branch
2 times, most recently
from
9767598 to
6912775
Compare
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
ok. addressed all comments. ci should go this time
| solana_program::instruction::get_stack_height, | ||
| solana_program::{instruction::get_stack_height, program::invoke}, |
There was a problem hiding this comment.
yeah native target code path is just a noop
| solana_transaction_context::BorrowedAccount, | ||
| std::{mem, ptr}, | ||
| }; | ||
| // consts inlined to avoid solana-program dep |
There was a problem hiding this comment.
i was surprised that they weren't in solana-cpi tbh
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6351 +/- ##
=======================================
Coverage 82.7% 82.8%
=======================================
Files 849 849
Lines 379076 379076
=======================================
+ Hits 313862 313910 +48
+ Misses 65214 65166 -48 🚀 New features to boost your workflow:
|
steviez
left a comment
steviez
left a comment
There was a problem hiding this comment.
This LGTM, but it'd be good to get a ship-it from Jon too
|
ok ready for real this time |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks great! Thanks for fighting through it
6 participants
Problem
solana-programhas received similar treatment to that ofsolana-sdk, so we now have individual crates for most of the symbols that this omnicrate used to provide. continuing to use the omnicrate symbols unnecessarily bloats the dependency graph and compile timesSummary of Changes
replace the low-hanging fruit dependencies upon
solana-program