Refactor: reorganize file and helper structure - #140
Conversation
kaze-cow
left a comment
There was a problem hiding this comment.
Looks great!
Maybe the client lib split was a bit excessive (the original instruction.rs file seemed fine as it was) but I don't mind it much either way.
I did notice that instructions.rs was renamed through folder structure to instruction (without the s) which creates a bit of an unnecessary breaking change. I know its good for consistency, but just something to highlight.
In test-cli/src/cmd/create_order.rs I noticed that there were some absolute resolves of functions that should probably be transformed into some sort of a use, as the path is becomin quite long. for example
- let sell = crate::token::resolve(&ctx.rpc, &ctx.payer.pubkey(), sell_tok)?;
- let buy = crate::token::resolve(&ctx.rpc, &ctx.payer.pubkey(), buy_tok)?;
+ let sell = crate::utils::token::resolve(&ctx.rpc, &ctx.payer.pubkey(), sell_tok)?;
+ let buy = crate::utils::token::resolve(&ctx.rpc, &ctx.payer.pubkey(), buy_tok)?;
The command suggested to be used in the instruction
git diff --find-renames --color-moved HEAD
Since I was on a branch with the changes already checked in I needed to add the branch name to compare with (instead of HEAD):
git diff --find-renames --color-moved main
Implemented by using
Description fixed! |
Our current module/file structure is a bit of a mess because it mixes up per-purpose files (e.g., one per instruction/PDA) with helper files (e.g., files with shared validation functions).
Also, these helpers are scattered around different files and
lib.rs/mod.rs.The logic of this refactor is:
utilsfolder under a descriptive file name. Thisutilsfolder can be in a subfolder.mod.rs.This change was inspired by a review of another refactoring PR.
Detailed changes
programs/settlement/processor/(src/create_order.rs→src/processor/create_order.rs), moved the dispatchmatchfromlib.rsintoprocessor/mod.rs, and leftlib.rsentrypoint-only, like solana-program/token'spinocchio/program/src/processor/.processor.rs(a hodgepodge ofCanonicalPda, state-PDA auth, and CPI checks) into concern-named files:processor/utils/pda.rs,processor/utils/auth.rs,processor/utils/cpi.rs, analogous top-token's processor/shared/. Now
processormeans the dispatcher.processor/settle/begin.rs+processor/settle/finalize.rsintoprocessor/begin_settle.rs+processor/finalize_settle.rs, named after theSettlementInstruction::BeginSettle/FinalizeSettlevariantslike every other handler.
validate_token_program_accountwas duplicated inline inprocessor/create_buffer.rsandprocessor/reclaim_buffer.rs(and lived in the settle module); extracted it to a singleprocessor/utils/token.rsreused byall four callers, and moved
validate_counterparttoprocessor/utils/settle.rs.interface/lib.rsheldSettlementError,SettlementInstruction,Role,SettlementAccount, andrecover_discriminator(plus all their tests) in one file; gave each its own module, re-exported from the crate root:error.rs,instruction/mod.rs(SettlementInstruction+recover_discriminator),role.rs,pda/mod.rs(SettlementAccount). Each type's unit tests moved with it. Somewhat close tosolana-program/token's interface (
instruction.rs,error.rs,state/).lib.rsis now justdeclare_id!+ crate-root re-exports + thefixturesmodule.tests/idl/) parses interface source by path; repointed itsSourceconsts (ERROR_RS,INSTRUCTION_MOD_RS,ROLE_RS,PDA_MOD_RSintests/idl/parse_rust.rs) and dropped the now-unusedINTERFACE_LIB_RS.client/instruction.rsinto aninstruction/folder, one file per builder (instruction/create_order.rs,instruction/begin_settle.rs,instruction/finalize_settle.rs,instruction/add_solver.rs, ...), keeping the flat public API (cow_settlement_client::instruction::BeginSettle) viapub usere-exports ininstruction/mod.rs.test-cli/utils/:helpers.rs→utils/output.rs, the SPL instruction builders (wrap_sol,approve) →utils/spl_instructions.rs, andtoken.rs→utils/token.rs; same "shared helpersin
utils/" idea as marginfi/squads. The CLI has noinstructionmodule (it isn't a peer of the interface builders).How to review
This PR is best reviewed in a checked-in local copy.
I suggest ignoring the diff.
First, take a look at the folder structure and determine you like it. Then, take a look at the functions that are stored in the file and decide whether it makes sense for it to be located there and for the file to have that name.
If you're satisfied, check out the diff with:
You should see red/green only for the changes you can expect: imports, file-level docs, extending the use of the
validate_token_program_accounthelper, some extramod ..., and fixing the docs link in some comments.This means that the rest of the content has just been moved without changes and that there are no logic changes introduced in this PR.