Add plan rendering to tmplrmgr writes (ENG-506)#523
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughSummary
Validation
Human review focus
WalkthroughThe manager CLI now supports print-only transaction planning with JSON or Sputnik output, typed secret-key parsing, mode-aware signer credentials, lazy signer public-key resolution, and validation for incompatible print and execution options. ChangesManager write planning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SignerArgs
participant CliContext
participant OperationPlan
participant Printer
CLI->>SignerArgs: parse print mode and optional public key
SignerArgs-->>CliContext: expose print format
CliContext->>OperationPlan: create unsigned write plan
OperationPlan-->>CliContext: return planned transaction
CliContext->>Printer: render JSON or Sputnik proposal
Printer-->>CLI: output plan
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/manager/src/commands/signer.rs (1)
1-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid plan/execution credential split; consider modeling the exclusivity as a type instead of scattered runtime checks.
secret(),public_key(), and (indirectly)resolve()each re-checkself.print.is_some()to enforce that plan mode and execution credentials are mutually exclusive. Clap'sconflicts_with/requiresalready guarantee this at the CLI boundary, so the runtime checks are redundant safety nets scattered across three methods. ACredentialsenum (Execution(SecretKey) | Plan { format: PrintFormat, public_key: Option<PublicKey> }) constructed once after clap parsing would encode the invariant in the type itself and remove the duplicatedif self.print.is_some()branches.As per path instructions: "Prefer designs where protocol states, request/response payloads, persisted records, state-machine transitions, permissions, identifiers, and bounded values are represented with concrete Rust types rather than loosely constrained primitives, maps, strings, booleans, or serde_json::Value... Flag code that permits invalid combinations and relies on scattered runtime checks when the invariant could reasonably be encoded with: enums for mutually exclusive states or variants..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/manager/src/commands/signer.rs` around lines 1 - 104, Refactor SignerArgs to construct a Credentials enum after Clap parsing, with distinct Execution and Plan variants carrying their valid data. Update secret(), public_key(), and resolve() to match on this typed state instead of independently checking print.is_some(), while preserving the existing CLI validation and error behavior. Remove the redundant scattered runtime exclusivity checks once the invariant is encoded by Credentials.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/manager/src/commands/signer.rs`:
- Around line 1-104: Refactor SignerArgs to construct a Credentials enum after
Clap parsing, with distinct Execution and Plan variants carrying their valid
data. Update secret(), public_key(), and resolve() to match on this typed state
instead of independently checking print.is_some(), while preserving the existing
CLI validation and error behavior. Remove the redundant scattered runtime
exclusivity checks once the invariant is encoded by Credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b0b5116-42c1-48ab-81f3-798d9a528c93
📒 Files selected for processing (15)
tools/manager/src/commands/deploy_common.rstools/manager/src/commands/full_access_key.rstools/manager/src/commands/market/create.rstools/manager/src/commands/proxy_oracle/create.rstools/manager/src/commands/proxy_oracle/governance/create.rstools/manager/src/commands/proxy_oracle/mod.rstools/manager/src/commands/redstone/create.rstools/manager/src/commands/registry/deploy.rstools/manager/src/commands/signer.rstools/manager/src/context.rstools/manager/src/dispatch/proposals.rstools/manager/src/dispatch/teardown.rstools/manager/src/tests.rstools/manager/src/tests/proxy_oracle.rstools/manager/src/tests/registry.rs
1 participant
Summary
--print json|sputniksupport to everytmplrmgrwrite carryingSignerArgsFunctionCallproposal kinds without submitting transactionsVerification
cargo test -p templar-manager— 141 passedcargo fmt --all --checkcargo clippy -p templar-manager --all-targets -- -D warningsLinear: https://linear.app/templar-protocol/issue/ENG-506/add-print-jsonsputnik-to-render-a-tmplrmgr-write-as-a-plan
This change is