Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughSPEC.md now documents ST0xOrchestrator as a second custody, mint, and burn path alongside vault-direct mode, including mode-scoped aggregate state, event wiring, service verification, status output, and TOML-based configuration. ChangesOrchestrator Mode Documentation
Related Issues: Not specified in the provided change data. 🐰 Two custody paths now share one spec line, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
JuaniRios
changed the title
JuaniRios
changed the title
348050d to
cc19963
Compare
82ce67f to
bb80be2
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
7ca826e to
cc91de4
Compare
![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)
SPEC.md (1)
2341-2353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShow the orchestrator recovery branch in the state machine.
OrchestratorMintRecoveredis documented in the mapping table, but the mint state diagram never shows that recovery success path. Add the transition here so the state machine matches the new orchestrator behavior.🤖 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 `@SPEC.md` around lines 2341 - 2353, The mint state machine diagram is missing the orchestrator recovery success path referenced by OrchestratorMintRecovered, so update the state transitions in SPEC.md to show how the recovery branch returns to the normal flow. Add the appropriate recovery transition alongside the existing MintingFailed recovery paths so the diagram matches the behavior documented in the mapping table and the OrchestratorMintRecovered flow.
🤖 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 `@SPEC.md`:
- Around line 2341-2353: The mint state machine diagram is missing the
orchestrator recovery success path referenced by OrchestratorMintRecovered, so
update the state transitions in SPEC.md to show how the recovery branch returns
to the normal flow. Add the appropriate recovery transition alongside the
existing MintingFailed recovery paths so the diagram matches the behavior
documented in the mapping table and the OrchestratorMintRecovered flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8250777d-9a17-4f3d-b14e-7ebc5cae8a63
📒 Files selected for processing (1)
SPEC.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
SPEC.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
SPEC.md
🔇 Additional comments (2)
SPEC.md (2)
1962-1969: 🗄️ Data Integrity & IntegrationDrop this comment —
ORCHESTRATOR_ASSETSis already the single source of truth forvault_mode.> Likely an incorrect or invalid review comment.
377-386: 🎯 Functional CorrectnessNo issue: mint mode is already anchored at
Initiated, andRecoveralready transitions back toCallbackPending.> Likely an incorrect or invalid review comment.
a51ce44 to
d5a7e7f
Compare
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)
SPEC.md (1)
2351-2386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
Closedto the persisted mint state.The state machine now introduces
Closed, butStoredMintRequest::MintStatusstill has no way to represent it. That leaves the newCloseMintterminal path without a backing storage state, and the schema below still reads as if nothing changed.Proposed doc update
enum MintStatus { PendingJournal, JournalCompleted, Minting, CallbackPending, Completed, + Closed, Failed(String), }🤖 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 `@SPEC.md` around lines 2351 - 2386, Add persisted support for the new Closed mint state by updating the StoredMintRequest/MintStatus documentation and schema so it explicitly includes Closed alongside the existing terminal states. Locate the MintStatus enum in the StoredMintRequest section and adjust the surrounding state model text to reflect that CloseMint transitions can now persist as Closed, ensuring the documented storage representation matches the updated state machine.
🤖 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 `@SPEC.md`:
- Around line 2351-2386: Add persisted support for the new Closed mint state by
updating the StoredMintRequest/MintStatus documentation and schema so it
explicitly includes Closed alongside the existing terminal states. Locate the
MintStatus enum in the StoredMintRequest section and adjust the surrounding
state model text to reflect that CloseMint transitions can now persist as
Closed, ensuring the documented storage representation matches the updated state
machine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa958107-2aa9-47cb-9a8a-9a1d143d4292
📒 Files selected for processing (1)
SPEC.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: static
- GitHub Check: test
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
SPEC.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
SPEC.md
🔇 Additional comments (1)
SPEC.md (1)
1484-1561: 🗄️ Data Integrity & IntegrationVerify the burn-verification API update is landed everywhere.
This section documents a 4-argument
verify_burn_tx(..., expected_proof)contract, but the suppliedVaultService/BurnManagersnippets still show the 3-argument form. Please confirm the implementation ticket updates the trait and every caller atomically; otherwise the mode-scoping guarantee here will drift from the code.
d5a7e7f to
16dc218
Compare
4849f1c to
9ed1fa8
Compare
ddbb097 to
2956f05
Compare
2956f05 to
8e89f09
Compare
8e89f09 to
79ad55a
Compare
Merge activity
|
github-actions
Bot
added
the
externally-merged
3 participants
Motivation
Today the issuance bot mints and burns tokens by talking to the vault contracts directly, and it does all the receipt bookkeeping itself in its own database. A new contract, the ST0x Orchestrator, is going to take that job over. It becomes the single place every mint and burn goes through, it holds the receipts, and it does the bookkeeping on-chain instead of in the bot.
That makes the bot simpler in the long run, but a lot of moving parts have to change to point at the orchestrator instead of the vault. Our rule in this repo is that we write the plan in SPEC.md before writing any code. This PR is only that plan. The actual code changes are separate tickets, starting at RAI-1217.
Solution
Docs only. This edits SPEC.md and changes no code. It writes down how minting and burning work once they go through the orchestrator, and it settles seven design decisions so the follow-up tickets have concrete answers instead of open questions. The decisions now live in the spec itself (Orchestrator Migration -> Design Decisions), so you do not have to go dig them out of Linear.
The seven decisions, in short:
The reasoning behind each decision, and the rules for every failure case, are in the spec's Design Decisions and Failure States sections. The migration ticket breakdown, meaning who does what and in what order, stays in Linear on RAI-1215.
Anything else
This spec was hardened with several automated review passes, but the last round of edits was not independently re-reviewed, so it is worth a careful human read before the follow-up tickets build on it. The trickiest part to check is the mint recovery rule in decision 2: when a retry reports the mint was already done, we must confirm it matches the exact token and amount we meant, not just the wallet and a reused number, or we could mark the wrong mint as complete.
Sequencing note: this whole orchestrator change is meant to ship after the issuer wallet moves from Fireblocks to Turnkey (RAI-1123), so it lands once Turnkey is live and Fireblocks is gone. The plan is signer-agnostic, so the parts that mention Fireblocks just mean whatever signer is active at the time, which will be Turnkey.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit