Add orders to BeginSettle#33
Merged
Merged
Conversation
kaze-cow
requested changes
Jun 9, 2026
kaze-cow
left a comment
kaze-cow
left a comment
Contributor
There was a problem hiding this comment.
first half of review, starting from programs/settlement/tests/common/mod.rs later.
I wonder if we can get away without including the bump data in the BeginSettle input data, and instead put it in the order's own data
This has a couple benefits:
- order provenance verification becomes self-contained in a
OrderAccountloading function - we don't need to include
bumpin the call data (reduces solver integration complexity--especially for those of us coming from EVM land) for those not using the client library - a 0 bump can be treated as a cancelled/invalid order (field can have a double use)
…ers-in-settlement
…ers-in-settlement
tilacog
approved these changes
Jun 11, 2026
tilacog
left a comment
tilacog
left a comment
Contributor
There was a problem hiding this comment.
LGTM. All my comments are nits/non-blocking stuff.
kaze-cow
approved these changes
Jun 15, 2026
7 tasks
fedgiac
added a commit
that referenced
this pull request
Jun 30, 2026
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](#33 (comment)), 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](#33 (comment)), `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](https://github.com/anza-xyz/solana-sdk/blob/95db1f7419e1fbf7cfc5ba498dd0851e291122f0/account-view/Cargo.toml#L4) | | `solana-address` | [address/Cargo.toml#L4](https://github.com/anza-xyz/solana-sdk/blob/14a725d6e9180e6cfbd98054473d61ef3aabde57/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](https://github.com/git/git/blob/883a47ef6496c96a5d6132ed8c87fcd44ebf8d1a/diff.c#L81-L105) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the list of settled orders to
BeginSettleand validate each one.BeginSettlepreviously took only the instructions sysvar and checked the begin/finalize pairing. It now also carries the orders being settled and validates them. No economic work yet, this PR is just about establishing and validating the list.The validation is also missing two steps: that the order isn't cancelled or expired. This will be part of another PR. I only included the validation steps that are more relevant to the parsing of the order.
The format of the instruction building function in the interface is kind of arbitrary: it takes three arrays with bumps, owners, and sell token accounts. I'm not sure whether this is a good format or I should take them in a different format, suggestions are welcome. I'm quite happy of the client's
begin_settlethough.Tests
All introduced failure cases should have been included in one way or another. Please confirm that, I'll add new tests as needed.
CI should be passing.