feat: add pooled-model exploration (PRD + FundingBeacon + StabilityPool skeleton)#30
feat: add pooled-model exploration (PRD + FundingBeacon + StabilityPool skeleton)#30tiero wants to merge 2 commits into
Conversation
…ol skeleton) Captures the move from the isolated StabilityVault/StabilityOffer model to a pooled model in response to quant feedback (positions non-fungible, Seeker redemptions act as enforced margin calls). Variant B is chosen: Seekers transfer-only, no on-chain redeem to BTC; Provider exit gated by leverage. Index-based funding accrual via a standalone FundingBeacon. - docs/stability-pool-prd.md: full design doc (state model, leverage gates, contract surface, phased build, open questions). - examples/stability/funding_beacon.ark (Phase 1): monotone cumulative yield-index oracle; mirrors PriceBeacon shape. - examples/stability/stability_pool.ark (Phase 2 skeleton): provider-only flows (deposit, leverage-gated withdraw) with index-based accrual. - examples/stability/provider_share.ark (Phase 2 skeleton): per-Provider share UTXO; share-equity dilution math marked TODO for Phase 2 completion. Compiles cleanly; full test suite passes; cargo fmt clean. Isolated-model contracts and tests are intentionally left in place until the pooled model is feature-complete.
Playground PreviewA live preview of this PR's playground is available at:
|
WalkthroughThis PR introduces a complete product requirements document and contract skeleton implementation for a perpetual-bond pooled stability pool system (Variant B). It includes an index-based funding oracle (FundingBeacon), provider capital representation (ProviderShare), and pool management (StabilityPool) with provider-only entry and leverage-gated exit flows. ChangesPooled StabilityPool System
Sequence DiagramsequenceDiagram
participant Provider
participant DepositTx as Deposit Tx
participant StabilityPool
participant FundingBeacon
participant ProviderShare
Provider->>DepositTx: initiate providerDeposit(depositSats, providerPk)
DepositTx->>StabilityPool: validate dust, beacon freshness
StabilityPool->>FundingBeacon: read current yieldIndex/yieldClock
StabilityPool->>StabilityPool: refresh poolYieldIndex
StabilityPool->>FundingBeacon: passthrough beacon assets
DepositTx->>ProviderShare: create output with depositedSats, entryIndex
Provider->>DepositTx: initiate providerWithdraw(withdrawSats, providerPk)
DepositTx->>StabilityPool: validate dust, beacon freshness
StabilityPool->>StabilityPool: compute seekerCapital = aggregateSeekerUSD + accrued
StabilityPool->>StabilityPool: check leverage (3×seekerCapital ≤ totalAfter)
StabilityPool->>FundingBeacon: passthrough beacon assets
DepositTx->>Provider: require SingleSig payout ≥ withdrawSats
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Aggressive Code Review — #30
Reviewer: Arkana (AI)
Verdict: Request changes — protocol-critical code requires human review
This PR adds pooled-model contracts (FundingBeacon, StabilityPool, ProviderShare) plus a comprehensive PRD. The FundingBeacon is solid and mirrors PriceBeacon well. The StabilityPool skeleton has structural issues that need resolution before this should be considered even a reference skeleton.
🔴 CRITICAL — Exit variant on singleton pool UTXO
Files: stability_pool.ark (all functions), compiled stability_pool.json
The options { server = server; exit = exit; } pattern generates exit variants for providerDeposit and providerWithdraw where the gate key is providerPk — a function argument (witness-supplied), NOT a constructor parameter.
Compare with existing contracts:
- PriceBeacon exit: gated by
oraclePk(constructor param) ✅ - StabilityVault exit: gated by
seekerPk+providerPk(constructor params) ✅ - StabilityPool exit: gated by
providerPk(witness arg)⚠️
For per-user VTXOs this pattern is safe — the user exits their own funds. For a singleton pool covenant holding everyone's capital, the exit path may allow an arbitrary party to drain the pool after the CSV timelock, since the gate key isn't baked into the script at creation time.
The compiled exit variant ASM (stability_pool.json lines ~1593-1600):
"<providerPk>", "<providerPkSig>", "OP_CHECKSIG",
"<exit>", "OP_CHECKSEQUENCEVERIFY", "OP_DROP"
Action needed: Clarify how the compiler resolves <providerPk> in exit variants when it's a function arg, not a constructor param. If it's witness-supplied, this is a fund-drain vector. The pool may need a dedicated poolAuthority constructor param as the exit-path gate, or a different options configuration.
🔴 HIGH — No covenant-level verification of ProviderShare input
File: stability_pool.ark:227-296 (providerWithdraw)
The pool's providerWithdraw function assumes input[3] is a legitimate ProviderShare UTXO but never introspects tx.inputs[3].scriptPubKey to verify it. The pool trusts the ProviderShare contract to self-enforce, but there's no binding between the two.
In the cooperative (server) path, the server gates this. In the exit path, there's no gate at all.
Risk: A crafted UTXO at input[3] that is NOT a ProviderShare could be used to satisfy the providerWithdraw spending path. The pool would decrement its value and pay out to the "provider" without a legitimate share being burned.
Fix: Add require(tx.inputs[3].scriptPubKey == new ProviderShare(providerPk, ..., exit), "input[3] not a valid ProviderShare"). This requires either passing depositedSats/entryIndex as function args or reading them from the input.
🟡 MEDIUM — No on-chain rate cap on FundingBeacon
File: funding_beacon.ark:38-59 (update)
PRD §5 says: "Christian flagged death-spiral risk if rate is unbounded in distress. Decide whether to cap the on-chain index growth rate. Strongly recommend yes for v0."
But the update() function only enforces monotonicity (newIndex >= currentIndex). There is no cap on the per-block index growth rate. A compromised or malicious oracle can set an arbitrarily high index in a single update, instantly shifting all provider capital to seekers.
Suggestion: Enforce (newIndex - currentIndex) / (newBlockHeight - currentHeight) <= MAX_RATE_PER_BLOCK on-chain, even if conservative. The oracle trust surface is already the biggest risk vector; bounding it on-chain is cheap defense-in-depth.
🟡 MEDIUM — No tests for any new contracts
Files: No new files in tests/
The PR description says "full test suite passes" but there are zero new tests for FundingBeacon, StabilityPool, or ProviderShare. The existing tests/beacon_test.rs only covers PriceBeacon.
At minimum, Phase 1 (FundingBeacon) should ship with:
- Compilation test (parses, produces valid artifact)
- update: monotonicity enforcement (index regression rejected, height regression rejected)
- passthrough: assets preserved
- migrate: oracle key rotated, index/height preserved
🟡 MEDIUM — Compiler type-mixing warnings
Files: funding_beacon.json, stability_pool.json
Both compiled artifacts emit warnings about uint64le/scriptnum mixing:
warning[type]: fn update: comparison '>=' mixes uint64le ('int') with scriptnum ('uint64le')
These implicit conversions can cause subtle comparison bugs at boundary values (e.g., large indices near 2^63). The code should use explicit le64ToScriptNum() conversions as the compiler suggests.
🟢 LOW — Redundant undercollateralisation check
File: stability_pool.ark:249-253
require(totalAfter > seekerCapital, "withdraw would undercollateralise");
// ...
int leverageBound = seekerCapital * 3;
require(leverageBound <= totalAfter, "leverage gate: provider withdraw blocked");
The leverage gate (3 × seekerCapital ≤ totalAfter) is strictly tighter than the undercollateralisation check (totalAfter > seekerCapital). The first require is always satisfied if the second passes. Not harmful, but unnecessary gas in script execution.
🟢 LOW — Overflow risk at scale
File: stability_pool.ark:243-245
int seekerCapitalNominal = aggregateSeekerUSD * 100000000 / currentPrice;
int fundingAccrued = aggregateSeekerUSD * deltaIndex / 100000000;
With 64-bit signed ints, aggregateSeekerUSD * 100000000 overflows at ~$920M aggregate seeker exposure (9.2×10^10 cents × 10^8 = 9.2×10^18, near 2^63). The PRD §6 flags this. Not blocking for v0 exploration, but the production version needs checked arithmetic or a wider intermediate.
ℹ️ INFO — Design observations
-
FundingBeacon is clean. Mirrors PriceBeacon shape correctly. update/passthrough/migrate all enforce monotonicity and script survival. The migrate function correctly requires both old and new oracle signatures in the exit path (2-of-2). Well done.
-
ProviderShare
redeem()is a bare signature check. The TODO acknowledging missing equity math is fine for a skeleton, but the comment atprovider_share.ark:13("v0 trusts the pool to enforce the leverage gate") creates a circular trust dependency — the pool also doesn't verify the share. One of them needs to do the binding. -
PRD is thorough. §5 open questions are well-identified. The phased approach is sensible. Leaving isolated-model contracts in place until pooled is complete is the right call.
Verdict
Request changes. The critical exit-variant concern and the missing ProviderShare input verification need resolution before merge. The FundingBeacon rate-cap and test coverage gaps should be addressed in this PR or tracked as immediate follow-ups.
🤖 Generated with Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@docs/stability-pool-prd.md`:
- Around line 241-243: The overflow estimate is incorrect: with $1B = 1e11
cents, multiplying aggregateSeekerUSD by INDEX_SCALE = 1e8 yields 1e19 which
exceeds signed 64-bit; update the doc and the StabilityPool math by either
lowering INDEX_SCALE or switching the affected arithmetic (e.g., in functions
that use aggregateSeekerUSD × INDEX_SCALE) to a wider integer type/bignum or
rebase units to avoid multiplying two large scales; specifically, correct the
numeric example in the text and add a note in the StabilityPool implementation
referencing INDEX_SCALE and aggregateSeekerUSD about required bitwidth or
mitigation.
In `@examples/stability/funding_beacon.ark`:
- Around line 80-96: The passthrough() path currently uses >= checks which allow
bumping yieldTicker/yieldClock assets without oracleSig; change the two asset
checks in passthrough() so tx.outputs[0].assets.lookup(yieldTicker) ==
tx.inputs[0].assets.lookup(yieldTicker) and
tx.outputs[0].assets.lookup(yieldClock) ==
tx.inputs[0].assets.lookup(yieldClock) to enforce immutability of those oracle
values (leave FundingBeacon script equality as-is and ensure only update() can
modify these assets).
In `@examples/stability/stability_pool.ark`:
- Around line 102-141: The deposit/withdrawal checks assume the FundingBeacon
appears at tx.outputs[2] while FundingBeacon.update()/passthrough()/migrate()
recreate the beacon at tx.outputs[0], so change the output indexing or the
contracts to agree: either update the StabilityPool checks to expect the funding
beacon at tx.outputs[0] (adjust the price/funding passthrough requires to use
tx.outputs[0].assets.lookup for yieldTicker/yieldClock) or modify
FundingBeacon.* (update, passthrough, migrate) to reconstruct the beacon at the
index used by StabilityPool (tx.outputs[2]); pick one consistent convention and
update all references (StabilityPool, FundingBeacon.update,
FundingBeacon.passthrough, FundingBeacon.migrate and the output index checks
around the ProviderShare) so a single transaction can satisfy both contracts.
- Around line 175-177: The math for seekerCapitalNominal and fundingAccrued can
overflow 64-bit: avoid multiplying aggregateSeekerUSD by 100000000 or deltaIndex
directly. Change calculations in the seekerCapitalNominal, deltaIndex, and
fundingAccrued logic to use a wider integer or safe multiply/divide routine
(e.g., 128-bit intermediate or a checked mulDiv) and/or reorder to divide first
(compute aggregateSeekerUSD / currentPrice then * scale, or use
mulDiv(aggregateSeekerUSD, scale, currentPrice)) and validate bounds on
deltaIndex (currentIndex - poolYieldIndex) before use; ensure all references to
seekerCapitalNominal and fundingAccrued use the overflow-safe implementation and
add unit checks to prevent wrapped values from passing collateral/leverage
checks.
- Around line 159-227: providerWithdraw currently uses providerPk from the
witness without proving a matching ProviderShare is being burned; require that
tx.inputs[3] is the ProviderShare being redeemed and that its on-chain
entitlement binds to providerPk and withdrawSats. Concretely, validate
tx.inputs[3].scriptPubKey == new ProviderShare(...expected params...) or
otherwise read the ProviderShare fields from tx.inputs[3] and require its owner
pubkey equals providerPk and its recorded share/entitlement covers withdrawSats
(and consume the share by requiring it as an input); keep the existing
pool-level checks (aggregateSeekerUSD, poolYieldIndex, exit) but block the
fallback path unless the above ProviderShare ownership/entitlement checks pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7bfa807-a619-4428-9522-4e002509f62d
📒 Files selected for processing (7)
docs/stability-pool-prd.mdexamples/stability/funding_beacon.arkexamples/stability/funding_beacon.jsonexamples/stability/provider_share.arkexamples/stability/provider_share.jsonexamples/stability/stability_pool.arkexamples/stability/stability_pool.json
| 6. **Index unit.** `INDEX_SCALE = 1e8` gives sat-precision per cent of USD. | ||
| Worth running the numbers at $1B aggregateSeekerUSD to confirm no | ||
| overflow risk (Arkade ints are 64-bit signed → `1e10 × 1e8 = 1e18`, fits). |
There was a problem hiding this comment.
The overflow estimate here is off by an order of magnitude.
$1B is 1e11 cents, so aggregateSeekerUSD × INDEX_SCALE is 1e19, not 1e18. That exceeds signed 64-bit and matches the overflow risk in the current StabilityPool math.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~241-~241: In American English, “percent” is the recommended spelling.
Context: ...INDEX_SCALE = 1e8 gives sat-precision per cent of USD. Worth running the numbers at...
(PER_CENT)
🤖 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/stability-pool-prd.md` around lines 241 - 243, The overflow estimate is
incorrect: with $1B = 1e11 cents, multiplying aggregateSeekerUSD by INDEX_SCALE
= 1e8 yields 1e19 which exceeds signed 64-bit; update the doc and the
StabilityPool math by either lowering INDEX_SCALE or switching the affected
arithmetic (e.g., in functions that use aggregateSeekerUSD × INDEX_SCALE) to a
wider integer type/bignum or rebase units to avoid multiplying two large scales;
specifically, correct the numeric example in the text and add a note in the
StabilityPool implementation referencing INDEX_SCALE and aggregateSeekerUSD
about required bitwidth or mitigation.
| function passthrough() { | ||
| require( | ||
| tx.outputs[0].scriptPubKey == new FundingBeacon(yieldTicker, yieldClock, oraclePk, exit), | ||
| "beacon script must survive" | ||
| ); | ||
|
|
||
| int currentIndex = tx.inputs[0].assets.lookup(yieldTicker); | ||
| require( | ||
| tx.outputs[0].assets.lookup(yieldTicker) >= currentIndex, | ||
| "index asset must survive" | ||
| ); | ||
|
|
||
| int currentHeight = tx.inputs[0].assets.lookup(yieldClock); | ||
| require( | ||
| tx.outputs[0].assets.lookup(yieldClock) >= currentHeight, | ||
| "clock asset must survive" | ||
| ); |
There was a problem hiding this comment.
passthrough() must preserve the oracle values exactly.
This path is callable without oracleSig, so the >= checks let any reader bump yieldIndex or yieldClock. That can spoof funding accrual and make stale data appear fresh. These assets need to be immutable on passthrough(); only update() should move them.
Suggested fix
function passthrough() {
require(
tx.outputs[0].scriptPubKey == new FundingBeacon(yieldTicker, yieldClock, oraclePk, exit),
"beacon script must survive"
);
int currentIndex = tx.inputs[0].assets.lookup(yieldTicker);
require(
- tx.outputs[0].assets.lookup(yieldTicker) >= currentIndex,
+ tx.outputs[0].assets.lookup(yieldTicker) == currentIndex,
"index asset must survive"
);
int currentHeight = tx.inputs[0].assets.lookup(yieldClock);
require(
- tx.outputs[0].assets.lookup(yieldClock) >= currentHeight,
+ tx.outputs[0].assets.lookup(yieldClock) == currentHeight,
"clock asset must survive"
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function passthrough() { | |
| require( | |
| tx.outputs[0].scriptPubKey == new FundingBeacon(yieldTicker, yieldClock, oraclePk, exit), | |
| "beacon script must survive" | |
| ); | |
| int currentIndex = tx.inputs[0].assets.lookup(yieldTicker); | |
| require( | |
| tx.outputs[0].assets.lookup(yieldTicker) >= currentIndex, | |
| "index asset must survive" | |
| ); | |
| int currentHeight = tx.inputs[0].assets.lookup(yieldClock); | |
| require( | |
| tx.outputs[0].assets.lookup(yieldClock) >= currentHeight, | |
| "clock asset must survive" | |
| ); | |
| function passthrough() { | |
| require( | |
| tx.outputs[0].scriptPubKey == new FundingBeacon(yieldTicker, yieldClock, oraclePk, exit), | |
| "beacon script must survive" | |
| ); | |
| int currentIndex = tx.inputs[0].assets.lookup(yieldTicker); | |
| require( | |
| tx.outputs[0].assets.lookup(yieldTicker) == currentIndex, | |
| "index asset must survive" | |
| ); | |
| int currentHeight = tx.inputs[0].assets.lookup(yieldClock); | |
| require( | |
| tx.outputs[0].assets.lookup(yieldClock) == currentHeight, | |
| "clock asset must survive" | |
| ); |
🤖 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 `@examples/stability/funding_beacon.ark` around lines 80 - 96, The
passthrough() path currently uses >= checks which allow bumping
yieldTicker/yieldClock assets without oracleSig; change the two asset checks in
passthrough() so tx.outputs[0].assets.lookup(yieldTicker) ==
tx.inputs[0].assets.lookup(yieldTicker) and
tx.outputs[0].assets.lookup(yieldClock) ==
tx.inputs[0].assets.lookup(yieldClock) to enforce immutability of those oracle
values (leave FundingBeacon script equality as-is and ensure only update() can
modify these assets).
| require( | ||
| tx.outputs[0].scriptPubKey == new StabilityPool( | ||
| priceTicker, priceClock, yieldTicker, yieldClock, | ||
| aggregateSeekerUSD, currentIndex, exit | ||
| ), | ||
| "invalid pool output" | ||
| ); | ||
| int poolValueAfter = tx.inputs[0].value + depositSats; | ||
| require( | ||
| tx.outputs[0].value >= poolValueAfter, | ||
| "pool deposit not credited" | ||
| ); | ||
|
|
||
| // PriceBeacon passthrough | ||
| require( | ||
| tx.outputs[1].assets.lookup(priceTicker) >= currentPrice, | ||
| "price beacon must survive" | ||
| ); | ||
| require( | ||
| tx.outputs[1].assets.lookup(priceClock) >= priceHeight, | ||
| "price clock must survive" | ||
| ); | ||
|
|
||
| // FundingBeacon passthrough | ||
| require( | ||
| tx.outputs[2].assets.lookup(yieldTicker) >= currentIndex, | ||
| "funding beacon must survive" | ||
| ); | ||
| require( | ||
| tx.outputs[2].assets.lookup(yieldClock) >= yieldHeight, | ||
| "funding clock must survive" | ||
| ); | ||
|
|
||
| // ProviderShare output for the depositor | ||
| require( | ||
| tx.outputs[3].scriptPubKey == new ProviderShare( | ||
| providerPk, depositSats, currentIndex, exit | ||
| ), | ||
| "invalid provider share output" | ||
| ); |
There was a problem hiding this comment.
The output ordering here is incompatible with FundingBeacon.
These paths require the funding beacon to reappear at tx.outputs[2], but FundingBeacon.update()/passthrough()/migrate() all reconstruct the beacon at tx.outputs[0]. A deposit/withdraw tx cannot satisfy both contracts as written.
Also applies to: 193-220
🤖 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 `@examples/stability/stability_pool.ark` around lines 102 - 141, The
deposit/withdrawal checks assume the FundingBeacon appears at tx.outputs[2]
while FundingBeacon.update()/passthrough()/migrate() recreate the beacon at
tx.outputs[0], so change the output indexing or the contracts to agree: either
update the StabilityPool checks to expect the funding beacon at tx.outputs[0]
(adjust the price/funding passthrough requires to use
tx.outputs[0].assets.lookup for yieldTicker/yieldClock) or modify
FundingBeacon.* (update, passthrough, migrate) to reconstruct the beacon at the
index used by StabilityPool (tx.outputs[2]); pick one consistent convention and
update all references (StabilityPool, FundingBeacon.update,
FundingBeacon.passthrough, FundingBeacon.migrate and the output index checks
around the ProviderShare) so a single transaction can satisfy both contracts.
| function providerWithdraw(int withdrawSats, pubkey providerPk) { | ||
| require(withdrawSats >= 330, "withdraw below dust"); | ||
|
|
||
| int currentPrice = tx.inputs[1].assets.lookup(priceTicker); | ||
| require(currentPrice > 0, "invalid price beacon"); | ||
| int priceHeight = tx.inputs[1].assets.lookup(priceClock); | ||
| int priceAge = tx.time - priceHeight; | ||
| require(priceAge <= 144, "stale price oracle"); | ||
|
|
||
| int currentIndex = tx.inputs[2].assets.lookup(yieldTicker); | ||
| int yieldHeight = tx.inputs[2].assets.lookup(yieldClock); | ||
| int yieldAge = tx.time - yieldHeight; | ||
| require(yieldAge <= 144, "stale funding oracle"); | ||
| require(currentIndex >= poolYieldIndex, "yield index regressed"); | ||
|
|
||
| // Compute current seeker capital claim including accrued funding. | ||
| int seekerCapitalNominal = aggregateSeekerUSD * 100000000 / currentPrice; | ||
| int deltaIndex = currentIndex - poolYieldIndex; | ||
| int fundingAccrued = aggregateSeekerUSD * deltaIndex / 100000000; | ||
| int seekerCapital = seekerCapitalNominal + fundingAccrued; | ||
|
|
||
| int totalAfter = tx.inputs[0].value - withdrawSats; | ||
| require(totalAfter > seekerCapital, "withdraw would undercollateralise"); | ||
|
|
||
| // Leverage gate: 3 × seekerCapital <= totalAfter ⇔ leverage_after <= 1.5 | ||
| int leverageBound = seekerCapital * 3; | ||
| require(leverageBound <= totalAfter, "leverage gate: provider withdraw blocked"); | ||
|
|
||
| // The ProviderShare being burned is at input[3]; the share contract | ||
| // enforces its own redemption rules (signature check, equity math). | ||
| // The pool covenant here only enforces pool-level invariants. | ||
|
|
||
| // Pool output: refreshed yield index, unchanged aggregateSeekerUSD, | ||
| // value reduced by withdrawSats. | ||
| require( | ||
| tx.outputs[0].scriptPubKey == new StabilityPool( | ||
| priceTicker, priceClock, yieldTicker, yieldClock, | ||
| aggregateSeekerUSD, currentIndex, exit | ||
| ), | ||
| "invalid pool output" | ||
| ); | ||
| require(tx.outputs[0].value >= totalAfter, "pool value mismatch"); | ||
|
|
||
| // PriceBeacon passthrough | ||
| require( | ||
| tx.outputs[1].assets.lookup(priceTicker) >= currentPrice, | ||
| "price beacon must survive" | ||
| ); | ||
| require( | ||
| tx.outputs[1].assets.lookup(priceClock) >= priceHeight, | ||
| "price clock must survive" | ||
| ); | ||
|
|
||
| // FundingBeacon passthrough | ||
| require( | ||
| tx.outputs[2].assets.lookup(yieldTicker) >= currentIndex, | ||
| "funding beacon must survive" | ||
| ); | ||
| require( | ||
| tx.outputs[2].assets.lookup(yieldClock) >= yieldHeight, | ||
| "funding clock must survive" | ||
| ); | ||
|
|
||
| // Provider payout | ||
| require( | ||
| tx.outputs[3].scriptPubKey == new SingleSig(providerPk), | ||
| "output 3 not provider" | ||
| ); | ||
| require(tx.outputs[3].value >= withdrawSats, "provider underpaid"); |
There was a problem hiding this comment.
providerWithdraw() never proves ownership of a ProviderShare.
providerPk is just witness data for the payout script here, and the function never inspects tx.inputs[3] despite the comment saying a share is being burned there. In the generated fallback path, any signer can pick their own providerPk after exit and withdraw up to the leverage limit. This flow needs to stay disabled until the withdraw is cryptographically bound to a matching burned share and its entitlement.
🤖 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 `@examples/stability/stability_pool.ark` around lines 159 - 227,
providerWithdraw currently uses providerPk from the witness without proving a
matching ProviderShare is being burned; require that tx.inputs[3] is the
ProviderShare being redeemed and that its on-chain entitlement binds to
providerPk and withdrawSats. Concretely, validate tx.inputs[3].scriptPubKey ==
new ProviderShare(...expected params...) or otherwise read the ProviderShare
fields from tx.inputs[3] and require its owner pubkey equals providerPk and its
recorded share/entitlement covers withdrawSats (and consume the share by
requiring it as an input); keep the existing pool-level checks
(aggregateSeekerUSD, poolYieldIndex, exit) but block the fallback path unless
the above ProviderShare ownership/entitlement checks pass.
| int seekerCapitalNominal = aggregateSeekerUSD * 100000000 / currentPrice; | ||
| int deltaIndex = currentIndex - poolYieldIndex; | ||
| int fundingAccrued = aggregateSeekerUSD * deltaIndex / 100000000; |
There was a problem hiding this comment.
This leverage math overflows before the scale the PRD calls out.
aggregateSeekerUSD * 100000000 already exceeds signed 64-bit above roughly $922M, and aggregateSeekerUSD * deltaIndex has no bound at all. Once either multiply wraps, the collateral and leverage checks are evaluating garbage.
🤖 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 `@examples/stability/stability_pool.ark` around lines 175 - 177, The math for
seekerCapitalNominal and fundingAccrued can overflow 64-bit: avoid multiplying
aggregateSeekerUSD by 100000000 or deltaIndex directly. Change calculations in
the seekerCapitalNominal, deltaIndex, and fundingAccrued logic to use a wider
integer or safe multiply/divide routine (e.g., 128-bit intermediate or a checked
mulDiv) and/or reorder to divide first (compute aggregateSeekerUSD /
currentPrice then * scale, or use mulDiv(aggregateSeekerUSD, scale,
currentPrice)) and validate bounds on deltaIndex (currentIndex - poolYieldIndex)
before use; ensure all references to seekerCapitalNominal and fundingAccrued use
the overflow-safe implementation and add unit checks to prevent wrapped values
from passing collateral/leverage checks.
…into claude/pooled-model-exploration-ocTK8
There was a problem hiding this comment.
🔍 Follow-up Review — #30 (post-merge of master)
Reviewer: Arkana (AI)
Scope: Only the new merge commit 9c2c8c4 (brings in #25: validation layer + tests from master). The stability pool contracts are unchanged — my previous review findings still apply.
Previous findings status
All critical/high findings from my first review remain unresolved — the stability pool contract files were not modified in this update:
- 🔴 Exit variant on singleton pool UTXO — still open
- 🔴 No covenant-level verification of ProviderShare input — still open
- 🟡 No on-chain rate cap on FundingBeacon — still open
- 🟡 No tests for new contracts — partially addressed (see below)
- 🟡 Compiler type-mixing warnings — still open
New findings from the merge
🟡 MEDIUM — Missing named roundtrip tests for protocol-critical contracts
The merge brings in tests/compilation_roundtrip_test.rs with named tests for price_beacon, stability_vault, and stability_offer — but not for the three new contracts in this PR: funding_beacon.ark, stability_pool.ark, provider_share.ark.
The generic sweep test (all_examples_compile_and_satisfy_invariants) catches them, but named tests provide better failure diagnostics for protocol-critical code. Add:
#[test]
fn roundtrip_funding_beacon() { ... }
#[test]
fn roundtrip_stability_pool() { ... }
#[test]
fn roundtrip_provider_share() { ... }ℹ️ INFO — New validator correctly handles these contracts
Verified that the new AST validator (src/validator/mod.rs) correctly passes all three stability pool contracts:
- Non-empty names, unique function names, unique parameters ✅
options.serverwith correspondingoptions.exit✅- All non-internal functions contain
require()statements ✅
No false positives or negatives for this PR's contracts.
ℹ️ INFO — Output invariant errors don't halt compilation
src/compiler/mod.rs:266-278: The new output validator surfaces detected compiler bugs as warnings but still returns Ok(json). For a Bitcoin L2 compiler, a detected compiler bug should halt compilation. This is a concern for the compiler generally (not specific to this PR), but worth noting since this PR's contracts will now run through this pipeline.
Verdict
Request changes — previous critical findings remain unresolved. The merge itself is clean and the validator integration is correct. Please address the exit-variant and ProviderShare verification concerns from the first review before merge.
🤖 Generated with Claude Code
2 participants
Captures the move from the isolated StabilityVault/StabilityOffer model to a
pooled model in response to quant feedback (positions non-fungible, Seeker
redemptions act as enforced margin calls). Variant B is chosen: Seekers
transfer-only, no on-chain redeem to BTC; Provider exit gated by leverage.
Index-based funding accrual via a standalone FundingBeacon.
contract surface, phased build, open questions).
yield-index oracle; mirrors PriceBeacon shape.
flows (deposit, leverage-gated withdraw) with index-based accrual.
share UTXO; share-equity dilution math marked TODO for Phase 2 completion.
Compiles cleanly; full test suite passes; cargo fmt clean. Isolated-model
contracts and tests are intentionally left in place until the pooled model
is feature-complete.
Summary by CodeRabbit
New Features
Documentation