Skip to content

Refactor: reorganize file and helper structure - #140

Open
fedgiac wants to merge 3 commits into
mainfrom
revisit-helper-structure
Open

Refactor: reorganize file and helper structure#140
fedgiac wants to merge 3 commits into
mainfrom
revisit-helper-structure

Conversation

@fedgiac

@fedgiac fedgiac commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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:

  • A folder should have a single purpose.
  • Shared helpers should go to a utils folder under a descriptive file name. This utils folder can be in a subfolder.
  • The shared core helpers and types (the central types and the helpers its own files build on) for a specific folder can stay in the relevant file or mod.rs.
  • Don't have large files with a lot of things going on at the same time.

This change was inspired by a review of another refactoring PR.

Detailed changes

  • programs/settlement/
    • Moved the instruction handlers under processor/ (src/create_order.rssrc/processor/create_order.rs), moved the dispatch match from lib.rs into processor/mod.rs, and left lib.rs entrypoint-only, like solana-program/token's pinocchio/program/src/processor/.
    • Split the misnamed processor.rs (a hodgepodge of CanonicalPda, state-PDA auth, and CPI checks) into concern-named files: processor/utils/pda.rs, processor/utils/auth.rs, processor/utils/cpi.rs, analogous to
      p-token's processor/shared/. Now processor means the dispatcher.
    • Flattened processor/settle/begin.rs + processor/settle/finalize.rs into processor/begin_settle.rs + processor/finalize_settle.rs, named after the SettlementInstruction::BeginSettle/FinalizeSettle variants
      like every other handler.
    • validate_token_program_account was duplicated inline in processor/create_buffer.rs and processor/reclaim_buffer.rs (and lived in the settle module); extracted it to a single processor/utils/token.rs reused by
      all four callers, and moved validate_counterpart to processor/utils/settle.rs.
  • interface/
    • lib.rs held SettlementError, SettlementInstruction, Role, SettlementAccount, and recover_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 to
      solana-program/token's interface (instruction.rs, error.rs, state/).
    • lib.rs is now just declare_id! + crate-root re-exports + the fixtures module.
    • The IDL generator (tests/idl/) parses interface source by path; repointed its Source consts (ERROR_RS, INSTRUCTION_MOD_RS, ROLE_RS, PDA_MOD_RS in tests/idl/parse_rust.rs) and dropped the now-unused
      INTERFACE_LIB_RS.
  • client/
    • Split the single ~400-line instruction.rs into an instruction/ 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) via pub use re-exports in instruction/mod.rs.
  • test-cli/
    • Consolidated all CLI helpers under utils/: helpers.rsutils/output.rs, the SPL instruction builders (wrap_sol, approve) → utils/spl_instructions.rs, and token.rsutils/token.rs; same "shared helpers
      in utils/" idea as marginfi/squads. The CLI has no
      instruction module (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:

git diff --find-renames --color-moved main

You should see red/green only for the changes you can expect: imports, file-level docs, extending the use of the validate_token_program_account helper, some extra mod ..., 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.

@fedgiac
fedgiac requested a review from a team as a code owner September 1, 2026 22:36

@kaze-cow kaze-cow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fedgiac

fedgiac commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

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)?;

Implemented by using utils as the base: 243dd4f

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):

Description fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants