Skip to content

Refactor: move ix parsing to interface#41

Merged
fedgiac merged 1 commit into
mainfrom
move-input-parsing-to-interface
Jun 30, 2026
Merged

Refactor: move ix parsing to interface#41
fedgiac merged 1 commit into
mainfrom
move-input-parsing-to-interface

Conversation

@fedgiac

@fedgiac fedgiac commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

This PR moves the on-chain input-parsing layer (the InstructionInputParsing trait and the per-instruction *Input structs) out of the settlement program and into settlement-interface, beside the off-chain builders that already define each instruction's wire format.
The interface was already the source of truth for encoding and for the byte decoders; the parsers are the decode half of that same format, so this puts both halves of each instruction in one module. This is a pure move: no behavior changes.

Before this PR, the parsing logic belonged to the program and could not be used outside of it. This was commented on before, and it makes sense in general to make input parsing available in the interface.

What isn't just a move

  • Now, there are test helpers that are shared between program and interface. What I did is exporting them under the feature gate test-fixtures of the interface. This required some reorganization but overall not much.
  • I didn't import Pinocchio on the interface: I used the underlying libraries it uses. You can see that there are no new actual dependencies simply by seeing that Cargo.lock doesn't add any new dependencies, just references.
  • solana-account-view is taken with the copy feature so AccountView (and Address) are Copy, exactly how the program gets them. Technically not needed, but it decreases the diff and it makes working with them simpler. Maybe we can try to drop this when working on CU efficiency later.
  • As discussed in the comment above, settlement-client also gets a proptest moved to it, now that we can export the parsing.
  • While relocating the doc comments I fixed two small typos in them: "Validate a singe order" → "single", and a redundant "its sell token account of the order" → "the order's sell token account". These are the only non-move red/green outside the pub/import/fixture changes noted below

New dependencies

Both are standard Anza/Solana SDK crates (the same ones pinocchio re-exports), added to settlement-interface. The link in the table points at each crate's package declaration in the anza-xyz/solana-sdk monorepo for verification.

Crate Solana source
solana-account-view account-view/Cargo.toml#L4
solana-address address/Cargo.toml#L4

How to test

This is a large PR but there are very few actual changes.
The fastest way to confirm there's no hidden logic change is to let git match the relocated blocks:

git diff --find-renames --find-copies --color-moved --ignore-all-space main move-input-parsing-to-interface

The output has more nuanced colors. Green and red are as usual, but other colors represent moved comment.
You can split your reviews into:

  1. Make sure you like the high-level structure of the code after the changes.
  2. Check only the red and green output of the diff. All the rest is moved, so if you like the structure in the previous step then there are no other relevant changes.
    A few changes to expect: structs and their fields have become pub. Imports have changed (but no need to check since they are handled by CI). So what remains is mostly minor changes (like crate::interface::whatever -> whatever) and the fixture changes, which need to actually be reviewed.

@fedgiac fedgiac requested a review from a team as a code owner June 16, 2026 12:09
@kaze-cow

Copy link
Copy Markdown
Contributor

git diff --find-renames --find-copies --color-moved --ignore-all-space pull-user-funds-process move-input-parsing-to-interface

I tried running this command, but it appears it only works when you have all the branches initialized locally. Otherwise, you want to make sure to add origin/ (or whatever you call the upstream in your git client) prefix to pull-user-funds-process to prevent an issue here.

@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.

With the provided command there are some places where moved code was not able to be found, but it seemed to all be for trivial name changing/indentation. So it wasn't a silver bullet.

Prior to this change it looked like most of the code in settlement was very self contained with both the parsing and the processing happening mostly in the same file, which makes it easy to track what is going on, and we only really used the interface as a place to store pda/data structure information. The interface also focused very much towards things necessary for use on the client. I don't see any real issues with moving unnecessary logic outside of the settlement program though, and as you mentioned, this improves the test structuring as well! I can also see cases where ex. our backend would want to be able to parse the instruction data without actually running the processing.

Overall very happy about this change. Lets get er done without too much fanfare.

Base automatically changed from pull-user-funds-process to main June 26, 2026 15:50
@fedgiac fedgiac force-pushed the move-input-parsing-to-interface branch from a365a63 to e31ceb7 Compare June 26, 2026 17:57
@fedgiac fedgiac requested a review from kaze-cow June 30, 2026 06:48

@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.

upon re-review I didn't observe anything new/out of the ordinary.

@fedgiac fedgiac merged commit 7110823 into main Jun 30, 2026
12 checks passed
@fedgiac fedgiac deleted the move-input-parsing-to-interface branch June 30, 2026 07:20
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