fix(minter): reject deposit cycle parameters that cannot cover the RPC cost and fee - #212
Draft
gregorydemay wants to merge 2 commits into
Draft
gregorydemay wants to merge 2 commits into
gregorydemay wants to merge 2 commits into
Conversation
…C cost and fee process_deposit checks that the caller attached process_deposit_required_cycles and later charges the getTransaction RPC cost plus the consolidation fee. If the configured required cycles were below that sum, the charge would trap in the reply callback and leave the per-account guard held until the next upgrade. Validate the constraint on init and upgrade like the other parameter constraints and document it in the design. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…undary Rename the error field to get_transaction_cycles since it reports the cycles budget attached to the getTransaction call, not its actual cost. Reword the design constraint so operators size the parameter from that budget and know the constraint is re-checked on event replay. Add a test that a caller attaching exactly the required cycles with no refund is charged exactly that amount. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Adds cycle-parameter validation to prevent underfunded deposit processing, with boundary tests and documentation.
Changes:
- Validates the required cycle invariant during initialization and upgrades.
- Adds state-validation and exact-charge tests.
- Documents configuration and deployment-history considerations.
| File | Description |
|---|---|
minter/src/state/tests.rs |
Tests invalid and boundary configurations. |
minter/src/state/mod.rs |
Adds cycle-configuration validation and error reporting. |
minter/src/deposit/manual/tests.rs |
Tests exact cycle charging without refunds. |
docs/design.md |
Documents the parameter constraint. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
process_depositchecks that the caller attached at leastprocess_deposit_required_cycles, then charges the cycles attached to thegetTransactioncall minus the refund plus, on success, the consolidation fee. If the two parameters are ever configured so that the required amount is below that sum, the charge asserts inside the reply callback; a trap there skips destructors, so the per-account guard is never released and the account stays blocked until the next upgrade.This adds the constraint
process_deposit_required_cycles >= GET_TRANSACTION_CYCLES + deposit_consolidation_feeto the state validation run on init and upgrade, documents it in section 3.3.4 of the design, and pins the boundary with a test that charges exactly the required amount with no refund. The current defaults and the staging minter's history satisfy it with a wide margin; since validation also runs when the event log is replayed, the design note says that raising the cycles budget constant requires checking deployment histories first.Found during the review of #211, where
deposit_solwould have added a second consumer of the same parameters.🤖 Generated with Claude Code