Refactor: move ix parsing to interface#41
Conversation
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 |
kaze-cow
left a comment
There was a problem hiding this comment.
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.
a365a63 to
e31ceb7
Compare
kaze-cow
left a comment
There was a problem hiding this comment.
upon re-review I didn't observe anything new/out of the ordinary.
This PR moves the on-chain input-parsing layer (the
InstructionInputParsingtrait and the per-instruction*Inputstructs) out of thesettlementprogram and intosettlement-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
test-fixturesof the interface. This required some reorganization but overall not much.Cargo.lockdoesn't add any new dependencies, just references.solana-account-viewis taken with thecopyfeature soAccountView(andAddress) areCopy, 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.settlement-clientalso gets aproptestmoved to it, now that we can export the parsing.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 theanza-xyz/solana-sdkmonorepo for verification.solana-account-viewsolana-addressHow 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:
The output has more nuanced colors. Green and red are as usual, but other colors represent moved comment.
You can split your reviews into:
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 (likecrate::interface::whatever->whatever) and the fixture changes, which need to actually be reviewed.