solana
solana copied to clipboard
bank.rs too large for github diff view syntax highlighting
Problem
$ wc -l runtime/src/bank.rs
20160 runtime/src/bank.rs
bank.rs, containing some of the most runtime-critical code in the repo, is too large for syntax highlighting in diff view on Github. this makes code review, especially of large change sets, tedious, if not error prone. inviting bugs like this is not The Way :tm:
Proposed Solution
we already have a module dir for bank, let's put it to better use!
- ~~move
tests
submodule to its own file~~ (https://github.com/solana-labs/solana/pull/29912) - move ancillary types and impls to their own files
- encapsulate low-hanging fruit to own type/impls in their own files
- more risky stuff depending on where the above gets us
is too large for syntax highlighting in diff view
And too large to load /blame
😢
I'm in favor of the proposed solution, even though we previously declined a contributor's Bank refactor as too dangerous.
move tests submodule to its own file
This will get us 12k+(ish) (20k - 8k) out
Is the code coverage high enough to at least reduce the risk of such change?
I think we can close this issue now that Bank.rs
is well under the 20k syntax highlighting threshold. Any objections?
I think we can close this issue now that
Bank.rs
is well under the 20k syntax highlighting threshold. Any objections?
I think it would still be nice to explore bullets 2 and 3 in the proposed solution.
description updated
I'd still like to see more progress here. Reopening.
Ha, you beat me to it Tyera, but 100% agreed. bank.rs
is more manageable, but we've somewhat just shifted the problem as /runtime/src/bank/tests.rs
is now approaching 14k lines.
I'm not sure of the line-counts for each of these across source and tests, but I think two good candidates for their own file under /runtime/src/bank/
would be rewards
and rent
. I can take a pass at that in today or tomorrow
in /runtime/src/bank/tests.rs
file, there are 8 impls
that should be moved to their structs. Also, 61 non-test functions can be defined in a separate file (e.g. /runtime/rc/bank/test_utils.rs
). That will remove 2000-2500 LOC.
Also, I suggest creating a folder runtime/src/bank/tests/
, and separating tests into files there (or maybe into /runtime/tests/
).