builder-stake: move the instruction builders into the crate - #4303
Merged
Conversation
The harness said to do this when something outside the tests needed them, and something does. A caller in another repository has to post a bond to test what it does with the account, and hand-encoding the accounts there would put the processor's account order in two places, in two repositories, with nothing to keep them in step. A move, not a rewrite. The harness re-exports them, so every test that used them reads unchanged, and all 21 still pass.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The newly public set_admin builder hard-codes the upgrade authority signer and the new admin value to the same key, which does not match the processor’s API shape and limits external callers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR moves the builder-stake instruction builder helpers out of the test harness and into the onchain program crate under instruction::builders, so other crates can reuse the canonical account ordering when constructing transactions.
Changes:
- Added
solana/programs/builder-stake/src/instruction/builders.rswith public instruction builder functions. - Updated the test harness to re-export the crate builders so existing tests keep the same call sites.
- Documented the move in
CHANGELOG.md.
File summaries
| File | Description |
|---|---|
| solana/programs/builder-stake/tests/common/mod.rs | Removes in-test builders and re-exports instruction::builders from the crate. |
| solana/programs/builder-stake/src/instruction/mod.rs | Exposes the new builders module. |
| solana/programs/builder-stake/src/instruction/builders.rs | New shared instruction builders that centralize processor account ordering. |
| CHANGELOG.md | Adds an entry noting the new crate-level builders. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The processor reads two keys and the builder took one. Account 1 is the upgrade authority, checked against the program data account, and the instruction data is the value written to the config. Collapsing them meant the builder could only ever say "make the upgrade authority the admin", which is not what a deployment wants. Every existing call site set them to the same key, which is what hid it. A new assertion covers the case that was inexpressible: the upgrade authority naming a different key, so an upgrade key can stay in cold storage while a warmer one runs the program. The other seven builders were checked for the same conflation. Each takes its signer and its data separately already, or reuses one key because the processor derives an address from it.
packethog
approved these changes
Sep 9, 2026
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
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.
A move, not a rewrite. 155 lines out of the test harness and into
instruction::builders.Why now
The harness carried a note saying to do this when something outside the tests needed them:
Something does now. A caller in another crate has to post a real bond to test what it then does with the account, and hand-encoding those accounts there would put the processor's account order in two places with nothing keeping them in step. A reordered account would break one copy silently.
What moved
initialize_program,set_admin,set_paused,set_tier_parameters,initialize_builder_stake,post_bond,withdraw,set_hold_expiry. Every dependency they need was already a normal dependency of the crate, so nothing new is pulled in.The harness re-exports them, so every test that used them reads unchanged.
Testing Verification
All 21
builder-staketests pass with the builders coming from the crate rather than the harness. That is the assertion that matters: the tests exercise the same account orders through the moved code.One local note that cost me a wrong answer before I checked it. The
ProgramTesttests fail on my machine withProgram processor not availableunlessBPF_OUT_DIRpoints at the built.so. That failure reproduces identically on an unmodifiedmain, so it is my environment and not this change. WithBPF_OUT_DIRset, all 21 pass on both.cargo clippy --all-targets -p doublezero-builder-stake -- -Dwarningsandcargo fmt --all --checkpass in thesolana/workspace.