runtime: simplify account saver filter #2992
Conversation
|
honestly i thought it was a me problem for a bit that i needed to write out in words what this did every time i read it, but then i remembered a conversation a couple weeks ago where myself and the other person discussed this function for several minutes only to realize we both had it backwards |
runtime/src/account_saver.rs
Outdated
| } | ||
| }) | ||
| { | ||
| for (i, (address, account)) in transaction_accounts.iter().enumerate() { |
There was a problem hiding this comment.
You need to limit the iteration to transaction.account_keys().len() because transaction_accounts includes some appended program accounts.
There was a problem hiding this comment.
youre right, thank you for catching that
unrelated to this pr, but do you know a reason i might have missed re: why we would need to append them? the extra loaders added in load_transaction_accounts(), which i believe is the only place accounts are added. i forgot we do it because i havent been doing it in my new account loader, and it doesnt seem to cause any problems in testing. ProgramCacheForTxBatch is populated with all builtins and as best i can tell invoke context always gets loaders from there
(to be clear i still count them against data size limits)
There was a problem hiding this comment.
I'm not exactly sure at this point whether it's necessary to append them or not. The program cache came later so it's very possible it alleviates the need to do this unintuitive appending behavior.
Problem
negation inside a bracketed OR makes the filter predicate used to select what accounts need to be saved unusually difficult to understand for what is essentially one line of code
Summary of Changes
rewrite in a more straightforward imperative style. the logic itself is unchanged