feat(stablecoin): rebuild withdraw_collateral with the collateralization check - #348
Conversation
3a92d45 to
b13e825
Compare
d6108e0 to
17d1634
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are still doc/test inconsistencies in the modified areas (outdated panic/docs and a test that claims to assert four echoed globals but only checks one), which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR rebuilds the stablecoin program’s withdraw_collateral instruction to follow the spec’s fee-aware, post-decrement collateralization invariant, expanding the ABI to include additional read-only global/state accounts and updating the guest entrypoint, IDL, unit tests, and the zkVM integration test fixtures accordingly.
Changes:
- Replaces the scaffolded “no debt allowed” guard with a spec-aligned collateralization check using projected accumulator/redemption-price values and adds the protocol frozen gate.
- Expands
WithdrawCollateralaccount list from 4 → 8 (adds accumulator, redemption price state, protocol parameters, and clock) and regenerates IDL/guest wiring. - Updates stablecoin unit tests and the integration test harness to seed/pass the new global accounts.
File summaries
| File | Description |
|---|---|
| programs/stablecoin/src/withdraw_collateral.rs | Implements freeze check + post-decrement collateralization enforcement; adds global PDA validation helper; updates destination naming. |
| programs/stablecoin/src/tests.rs | Adds new withdraw-collateral tests and updates existing ones for the 8-account ABI. |
| programs/stablecoin/methods/guest/src/bin/stablecoin.rs | Updates guest instruction signature to accept the four new accounts. |
| programs/stablecoin/core/src/lib.rs | Updates Instruction::WithdrawCollateral docs to reflect the new account list and semantics. |
| programs/integration_tests/tests/stablecoin.rs | Seeds and passes the new global accounts in the e2e test state and call site. |
| artifacts/stablecoin-idl.json | Regenerates IDL to match the new withdraw-collateral ABI (account names/order). |
Review details
Suppressed comments (3)
programs/stablecoin/src/withdraw_collateral.rs:37
- The panic list still claims non-zero debt is rejected, but debt is now allowed as long as the post-withdrawal position remains collateralized. Update this bullet to reflect the actual failure mode.
/// - `Position.normalized_debt_amount` is non-zero.
programs/stablecoin/src/tests.rs:1310
- Test name still uses the old
destinationterminology, but the account is nowuser_collateral_holding. Renaming the test improves clarity and keeps terminology consistent.
STABLECOIN_PROGRAM_ID,
100,
);
}
programs/stablecoin/src/tests.rs:1333
- Test name still uses the old
destinationterminology, but the account is nowuser_collateral_holding. Renaming the test improves clarity and keeps terminology consistent.
protocol_parameters_account(false),
clock_account(NOW),
STABLECOIN_PROGRAM_ID,
100,
);
}
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b13e825 to
a552497
Compare
9f7cba9 to
a2aa9a4
Compare
3fe6237 to
46885a3
Compare
60317da to
0305cf0
Compare
858c0b6 to
af9a944
Compare
16d2c8f to
de82977
Compare
Carries #348's fix to the other §6.2 call site. The accumulator stays narrow — it divides `amount` to size the debt delta, so a value `u128` cannot hold is not a mint we can price.
af9a944 to
5ecc658
Compare
…on in withdraw_collateral
…o-op- and overflow-safe `withdraw_collateral(0)` is a no-op per spec §11, so it now skips both projections and the §6.2 check — nothing about the position changed. An indebted position previously still projected, and a decaying rate that reaches zero failed the call. The projections and the check move to saturating `U512`. A rate at the controller's permitted limit projects past `u128` about 45 minutes out, which panicked before the comparison could run and blocked withdrawals that were comfortably collateralized. Saturation is the safe direction: the check's left-hand side tops out near 10^119, so a saturated requirement always loses. 8 new tests.
de82977 to
426811b
Compare
Carries #348's fix to the other §6.2 call site. The accumulator stays narrow — it divides `amount` to size the debt delta, so a value `u128` cannot hold is not a mint we can price.
Carries #348's fix to the other §6.2 call site. The accumulator stays narrow — it divides `amount` to size the debt delta, so a value `u128` cannot hold is not a mint we can price.
Rebuilds
withdraw_collateralto spec §10.6, replacing the scaffold'snormalized_debt_amount == 0placeholder with the real §6.2 check.Accounts go 4 → 8, adding the three read-only globals — each pinned to its
canonical PDA — and
clock. The check runs after the decrement against debt andredemption price projected to
now(§5.3). Adds the frozen gate the scaffoldlacked.
Guards the check needs:
amount == 0is a no-op (§11) and skips the projections entirely — nothingabout the position changed, so there is nothing to re-check.
collateral and let an indebted position be drained completely.
The projections and the check itself run in saturating
U512. A rate at thecontroller's permitted limit projects past
u128about 45 minutes out, whichpanicked before the comparison could run. Saturation is the safe direction: the
check's left-hand side tops out near
10^119, so a saturated requirement alwaysloses.
32 unit tests.
closes #177