feat(wpi-token): cap mint size per transaction - #59
Merged
Junman140 merged 4 commits intoJul 30, 2026
Conversation
`mint` and `mint_from_deposit` were bounded only by the rolling-window volume limit, so a single call from a compromised admin key or a buggy relayer could mint an unbounded amount in one transaction as long as the window had capacity. Add a `max_mint_per_tx` ceiling checked on every mint path, owned by the same independent volume-limit admin as the window limits so the bridge admin cannot raise its own ceiling. Mints fail closed with `MintTxCapNotConfigured` until it is set. An over-cap mint returns `false` without changing balances, consuming window capacity, or marking its deposit processed, and publishes `MintTxCapExceeded` — the call succeeds at the transaction layer so the alert is committed rather than rolled back with the invocation. It does not pause the bridge: the per-transaction ceiling is an input check, while sustained over-minting still trips the rolling-window breaker. Stored under its own instance key so an upgraded deployment's existing `VolumeLimitConfig` keeps decoding. Also fixes a test that did not compile on main (`volume_limit_config().unwrap()`), unused bindings that fail `clippy -D warnings`, and rustfmt drift. Closes Pi-Defi-world#19
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe WPI token contract adds configurable per-transaction mint-cap enforcement, redemption replay protection, storage TTL handling, and related tests. README, architecture documentation, changelog, and relayer messaging describe the new cap behavior and committed rejection outcomes. ChangesToken controls and operational integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Relayer
participant WpiToken
participant MintTxCap
participant DepositState
Relayer->>WpiToken: mint_from_deposit
WpiToken->>MintTxCap: enforce_mint_tx_cap
MintTxCap-->>WpiToken: reject oversized mint and publish MintTxCapExceeded
WpiToken-->>Relayer: committed cap rejection
WpiToken->>DepositState: process deposit only after accepted mint
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
relayer/src/stellar/mintSubmitter.ts (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the warning cover both rejection causes.
This branch handles both the per-transaction ceiling and the rolling-window circuit breaker, but the log says only “a bridge mint cap.” Use wording such as “per-transaction or rolling-window mint cap” so operators are not given an inaccurate diagnosis.
🤖 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 `@relayer/src/stellar/mintSubmitter.ts` at line 47, Update the warning message in the mint-blocking branch of the mint submitter to identify both possible rejection causes: the per-transaction mint cap and the rolling-window mint cap/circuit breaker. Keep the existing warning metadata unchanged.Stellar-contracts-v1/wpi-token/src/test.rs (1)
186-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider narrowing the bare
#[should_panic]attributes.Both authorization tests pass on any panic, including one from a mistyped
fn_nameor an unrelated future failure — which would silently void the "only the volume-limit admin owns the cap" guarantee these tests exist to protect. Addingexpected = "..."with a substring of the auth-failure panic ties them to the intended cause.🤖 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 `@Stellar-contracts-v1/wpi-token/src/test.rs` around lines 186 - 227, Narrow the #[should_panic] attributes on non_admin_signer_cannot_authenticate_configure_max_mint_per_tx and bridge_admin_cannot_raise_the_tx_cap_after_rotation by adding expected substrings matching the authorization-failure panic. Use the actual panic text emitted by configure_max_mint_per_tx so both tests fail for mistyped calls or unrelated panics.
🤖 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.
Nitpick comments:
In `@relayer/src/stellar/mintSubmitter.ts`:
- Line 47: Update the warning message in the mint-blocking branch of the mint
submitter to identify both possible rejection causes: the per-transaction mint
cap and the rolling-window mint cap/circuit breaker. Keep the existing warning
metadata unchanged.
In `@Stellar-contracts-v1/wpi-token/src/test.rs`:
- Around line 186-227: Narrow the #[should_panic] attributes on
non_admin_signer_cannot_authenticate_configure_max_mint_per_tx and
bridge_admin_cannot_raise_the_tx_cap_after_rotation by adding expected
substrings matching the authorization-failure panic. Use the actual panic text
emitted by configure_max_mint_per_tx so both tests fail for mistyped calls or
unrelated panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eb290de-5dea-43b5-a99e-bf54ae9a84c8
📒 Files selected for processing (5)
Stellar-contracts-v1/README.mdStellar-contracts-v1/wpi-token/src/lib.rsStellar-contracts-v1/wpi-token/src/test.rsrelayer/src/stellar/mintSubmitter.tsrelayer/src/stellar/wpiContractClient.ts
Name both rejection causes in the relayer warn log, and pin the two new auth tests to the auth panic so an unrelated panic cannot satisfy them.
Upstream split the single admin key into admin/minter/pauser (Pi-Defi-world#5), so `mint` and `mint_from_deposit` are now minter-gated. The per-transaction cap sits above that gate unchanged and is still owned by the volume-limit admin, which is now a stronger separation: the hot minter key cannot raise its own ceiling. - Kept upstream's `require_minter` gates and merged the `mint` doc comment. - Dropped this branch's drive-by CI fixes in favour of upstream's eef6676, which fixed the same fmt/clippy/compile breakage on main. - Took upstream's rewrite of `test_existing_admin_retains_privileges_until_acceptance`, which now asserts on `propose_admin` since pausing moved to the pauser role. - `every_role_can_be_a_contract_address_not_only_an_eoa` configures a cap: it mints, and mints now fail closed until the ceiling is set. - Renamed the cap authorization test to rotate the minter as well, so it covers the post-split threat model, and renamed the bypass test to match the minter role. - Recorded `configure_max_mint_per_tx` in the role tables in the contract README and the role-separation design doc, and added a CHANGELOG entry.
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)
docs/design/admin-role-separation.md (1)
74-77: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDocument the required cap configuration before resuming minting.
The upgraded contract fails closed with
MintTxCapNotConfiguredwhen the new storage key is absent, but this plan says no migration step is required and that the relayer continues working. Add a mandatoryconfigure_max_mint_per_txstep, performed by theVolumeLimitAdmin, before routing bridge traffic; otherwise existing deployments will stop all mints after upgrade.Also applies to: 100-102
🤖 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 `@docs/design/admin-role-separation.md` around lines 74 - 77, Update the upgrade plan in the admin role separation document to include a mandatory configure_max_mint_per_tx call by VolumeLimitAdmin after the WASM upgrade and before resuming bridge traffic or minting. Remove the claim that no migration step is required, and state that relaying must remain paused until the cap is configured.
🧹 Nitpick comments (1)
Stellar-contracts-v1/wpi-token/src/test.rs (1)
90-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRetry the same rejected deposit in the state-preservation test.
The test confirms
deposit_id(1)remains unprocessed, but the successful call usesdeposit_id(2). After raising the cap, retrydeposit_id(1)and assert it succeeds to cover the documented replay/state-preservation guarantee directly.🤖 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 `@Stellar-contracts-v1/wpi-token/src/test.rs` around lines 90 - 93, Update the state-preservation test to retry deposit_id(1) after governance raises the minting ceiling, rather than using deposit_id(2). Assert that this retry succeeds while preserving the existing checks that the rejected attempt consumed no window capacity and did not mark deposit_id(1) as processed.
🤖 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 `@docs/design/admin-role-separation.md`:
- Around line 74-77: Update the upgrade plan in the admin role separation
document to include a mandatory configure_max_mint_per_tx call by
VolumeLimitAdmin after the WASM upgrade and before resuming bridge traffic or
minting. Remove the claim that no migration step is required, and state that
relaying must remain paused until the cap is configured.
---
Nitpick comments:
In `@Stellar-contracts-v1/wpi-token/src/test.rs`:
- Around line 90-93: Update the state-preservation test to retry deposit_id(1)
after governance raises the minting ceiling, rather than using deposit_id(2).
Assert that this retry succeeds while preserving the existing checks that the
rejected attempt consumed no window capacity and did not mark deposit_id(1) as
processed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39ec529e-836a-486c-abab-9652ebe3e68c
📒 Files selected for processing (6)
CHANGELOG.mdStellar-contracts-v1/README.mdStellar-contracts-v1/wpi-token/src/lib.rsStellar-contracts-v1/wpi-token/src/test.rsdocs/design/admin-role-separation.mdrelayer/src/stellar/mintSubmitter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- Stellar-contracts-v1/README.md
- relayer/src/stellar/mintSubmitter.ts
- Stellar-contracts-v1/wpi-token/src/lib.rs
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
2 participants
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.
Closes #19
Problem
The rolling per-window cap already existed, but nothing bounded a single call. mint and mint_from_deposit could mint an unbounded amount in one transaction as long as the window had capacity.
Change
Adds max_mint_per_tx, checked on both mint paths, so no privileged path bypasses it.
configure_max_mint_per_tx / max_mint_per_tx are gated on the volume-limit admin (the delegated multisig role from #5), not the bridge admin — a compromised minting key cannot raise its own ceiling. Mints fail closed with MintTxCapNotConfigured until it is set.
An over-cap mint returns false and publishes MintTxCapExceeded { to, amount, max_mint_per_tx }. It returns successfully at the transaction layer on purpose: an Err would roll the event back, and the alert must be visible off-chain. No balances change, no window capacity is consumed, and the deposit id stays unprocessed, so it is re-submittable if governance raises the cap.
Design notes for review
An over-cap mint does not pause the bridge. The per-tx cap is an input ceiling; the window limit stays the circuit breaker. Pausing on one oversized deposit would give any depositor a bridge-wide DoS. Splitting a large mint into cap-sized mints still trips the window breaker (tested).
Stored under its own instance key rather than added to VolumeLimitConfig as adding a field would make read_volume_config panic with ConversionError on any instance upgraded via upgrade().
Testing
31 contract tests (10 new) and 55 relayer tests pass. Full CI-equivalent run is green: fmt --check, clippy --locked --all-targets -D warnings, test --locked --all, wasm release build, dependency provenance.
New coverage: over-cap mint rejected with the exact event asserted and all state untouched; mint(i128::MAX) rejected; exactly-at-cap accepted; under-cap mints still trip the window breaker; cap enforced behind the pause and replay gates; invalid caps rejected; non-admin and post-rotation bridge admin cannot configure; delegated guardian raises the cap and the rejected deposit then mints.
Note for maintainers
main is currently red: test_existing_admin_retains_privileges_until_acceptance (from #57) calls .unwrap() on a non-Result, so cargo test does not compile, plus 14 unused-binding warnings and rustfmt drift that fail CI. Fixed here so the suite runs; happy to split into a separate PR.
Summary by CodeRabbit
redemption_id, emitting theredemption_idin the burn event.