Repository navigation
feat(yield): draft yield-bearing saving circles via crowdstake sDAI - #190
BreadrichEngels wants to merge 1 commit into
Conversation
…189) First draft of the sDAI-yield integration for saving circles. NOT yet compiled (agent sandbox was out of disk, no Foundry) — needs CI build, tests, and an accounting/security review. - IYieldModule.sol: crowdstake.fun yield-module interface (mint/burn/ claimYield/yieldAccrued). - SavingCircles.sol: mark _deposit/_withdraw/decommission `virtual` so the yield variant can hook them (storage layout unchanged). - YieldSavingCircles.sol: routes deposits into the yield module; per-member time-weighted accrual via a MasterChef-style accumulator (earlier deposits earn more); claimant may take the pot or leave it staked to keep compounding; modular per-circle claim-timing (Anytime vs EndOnly); BREAD wrapping isolated behind _toUnderlying/_fromUnderlying seams. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
ss251
left a comment
ss251
left a comment
There was a problem hiding this comment.
Requesting changes before this contract/API becomes something the Stacks UI depends on.
I reviewed PR #190 against issue #189, the current SavingCircles state machine, AutomaticSavingCircles, and the current CrowdStake token/pool implementations. There are several blockers that need to be resolved at the contract-design level, not just patched around in the frontend.
Inline comments below map to each numbered point. I only attached GitHub suggested-changes where a small, correct edit exists. The rest are design blockers, not one-line patches.
1. CrowdStake interface drift currently makes deposits incompatible with the concrete implementations.
IYieldModule declares:
mint(uint256 amount, address receiver)and this PR calls:
yieldModule.mint(_underlyingAmount, address(this));but the current CrowdStake token/pool surfaces implement:
mint(address receiver, uint256 amount)These are different ABI selectors (0x94bf804d vs 0x40c10f19). The interface in CrowdStake itself appears stale, so this is not solely a saving-circles issue, but #190 cannot integrate with the concrete implementations until the canonical interface is reconciled.
2. claimYield() is using CrowdStake's donated-yield path as though it were the SavingCircles holder's yield.
Current CrowdStake distinguishes:
yieldAccrued()— donated/distributable yieldtotalYieldAccrued()— total vault surpluskeptYieldOf(account)/claimKeptYield()— yield retained by an individual holdersetYieldSplit()— controls how much of a holder's yield is kept vs donated
#190 reads the module-wide yieldAccrued(), allocates that locally to SavingCircles members, and calls claimYield().
That conflicts with CrowdStake's current economic model. If SavingCircles is meant to own the yield generated by its CrowdStake position, the integration should explicitly establish that ownership — e.g. via the holder/kept-yield path — rather than treating the donated pool as SavingCircles yield.
This matters especially if the CrowdStake module is shared with other holders: their donated yield can enter the numerator of _harvestGlobal() while their principal is absent from _totalPrincipal.
3. The current claimYield() call is not authorized on a normally wired CrowdStake instance.
CrowdStake's concrete token and pool implementations restrict claimYield() to the configured yieldClaimer.
The standard deployment wiring sets the DistributionManager as that claimer.
#190 instead has YieldSavingCircles call:
yieldModule.claimYield(...)There is no deployment/configuration work in this PR that makes YieldSavingCircles the authorized claimer. This needs an explicit integration decision rather than assuming direct access to the donated-yield claim path.
4. Principal/yield asset types do not line up with the abstraction in #190.
The current sDAI implementations redeem principal through:
sDAI -> WXDAI -> native xDAI
and send native xDAI to the burn() receiver.
YieldSavingCircles calls:
yieldModule.burn(_principal, address(this));but has no payable receiver and then proceeds as if it received an ERC-20 that _fromUnderlying() can convert and _payoutToken().safeTransfer() can send.
Pool-mode yield claims return WXDAI, while token-mode SexyDaiYield.claimYield() mints the CrowdStake token. These are three different asset flows:
- principal redemption: native xDAI
- pool-mode yield: WXDAI
- token-mode yield: CrowdStake token
They cannot currently be represented by a single _fromUnderlying(uint256) seam.
5. The BREAD adapter needs to be redesigned, not just filled in.
Actual BREAD conversion is stateful:
BREAD burn -> native xDAI
native xDAI / WXDAI -> BREAD mint
But _toUnderlying(uint256) is called both when a real deposit is made and again later when historical roundDeposits are processed during withdrawal.
If _toUnderlying() performs the actual BREAD burn, the withdrawal path attempts to convert the historical deposit a second time.
We need to separate:
- asset movement/conversion, and
- amount/accounting conversion.
The current hooks conflate them.
6. _withdraw() clears historical roundDeposits, which breaks the base SavingCircles state machine.
#190 does:
roundDeposits[_id][_round][_depositor] = 0;when a pot is claimed.
The inherited _deposit() and _isDecommissionable() logic rely on those historical values to determine whether the previous round was fully funded.
After round 0 is claimed, its deposit records become zero. When round 1 begins, the inherited logic sees round 0 as incomplete and reverts new deposits with CircleTimedOut() / makes the circle decommissionable.
roundDeposits is protocol history and cannot be consumed as the yield-principal ledger. Yield redemption state needs separate storage.
7. withdrawStakedPrincipal() withdraws all of a user's principal, not the claimant pot they elected to keep staked.
Every normal deposit increments:
_principalOf[_member]and withdrawStakedPrincipal() does:
uint256 _principal = _principalOf[msg.sender];
_principalOf[msg.sender] = 0;
yieldModule.burn(_principal, ...);There is no circle id and no separate keptPotPrincipal.
A member can therefore withdraw ordinary contributions that are still backing active rounds. roundDeposits continues to say those contributions exist, and the later pot claim attempts to subtract that principal again.
This needs separate per-circle balances for mandatory deposits versus a claimant's voluntarily retained pot.
8. The claimant does not currently control the advertised keep-staked decision.
The base contract intentionally allows anybody to call:
withdrawFor(id, member)for a valid claimant.
The new keep-staked behavior is controlled by a temporary _keepStakedFlag that is only set by:
claim(id, keepStaked)So another caller — including the existing automation path — can call withdrawFor(id, claimant) with _keepStakedFlag == false and force the claimant's pot out of the yield position.
The recipient still receives their funds, but their advertised "take vs keep staked" choice has been made by somebody else.
AutomaticSavingCircles currently still uses withdrawFor().
9. The global reward index can be valid, but member state cannot be global if claim policy is per circle.
A shared _accYieldPerPrincipal is reasonable if all circles intentionally share one yield source.
The problematic part is:
_principalOf[address]
_rewardDebt[address]
claimableYield[address]A member can participate in multiple circles, while claim timing is configured per circle.
Once yield is collapsed into:
claimableYield[Alice]the contract no longer knows which portion belongs to which circle.
That directly breaks ClaimTiming. _yieldUnlocked() currently blocks all of a member's yield whenever any active circle they belong to is EndOnly, including yield earned in an Anytime circle.
At minimum, economic ownership needs circle-scoped state, e.g.:
principal[circleId][member]
rewardDebt[circleId][member]
claimableYield[circleId][member]
keptPotPrincipal[circleId][member]The reward source/index may remain shared if that is the intended architecture.
10. decommission() is incompatible with yield-bearing custody.
The inherited implementation returns the ERC-20 amounts recorded in roundDeposits directly from SavingCircles.
After #190's deposit path, that principal has been moved into CrowdStake.
After the intended BREAD conversion is added, the deposited BREAD will have been burned entirely.
Decommission therefore needs a yield-aware unwind that:
- settles accounting,
- redeems the correct principal,
- realizes/assigns owed yield,
- converts assets back to BREAD,
- returns the correct amounts to members.
The current inherited implementation cannot do that.
11. Important configuration invariants exist only in comments.
setYieldModule() says the module must be set before deposits, but the contract does not enforce that. The owner can change the module while _totalPrincipal, _principalOf, _rewardDebt, and _lastYieldSeen still refer to the old position.
Likewise setSoleToken() can be changed while circles exist, while inherited create() still accepts any token in allowedTokens. There is no enforced relationship between circle.token and _soleToken.
These should be immutable after initialization or guarded by explicit no-active/no-principal invariants.
12. The UI API is not ready to depend on claimableYield(address) as currently defined.
claimableYield is stored/settled state. Fresh sDAI yield is incorporated only when _harvestGlobal() / _settle() execute.
The UI currently reads the stored mapping and treats zero as "nothing to claim", disabling the claim action. That can report zero while unsettled yield exists.
The contract should expose a current view such as pendingYield(circleId, member) / claimableYield(circleId, member) that simulates the accumulator without requiring a state-changing transaction.
Required direction before this API stabilizes
Before we extend the UI around this ABI, I think the contract work needs to establish these invariants explicitly:
- Decide whether SavingCircles gets a dedicated CrowdStake instance or uses holder-level isolation inside a shared one.
- Use the canonical CrowdStake deposit interface and asset semantics.
- Define how SavingCircles legally/economically owns and claims its yield (
keptYieldpath vs a dedicated claimer architecture). - Separate BREAD conversion from accounting-only amount conversion.
- Preserve
roundDepositsas immutable historical ROSCA bookkeeping. - Track principal, reward debt, claimable yield, and retained claimant pots by circle/member.
- Make
withdrawStakedPrincipalcircle-scoped and limited to retained pot principal. - Resolve
withdrawFor/ automation semantics for the claimant's keep-staked choice. - Implement yield-aware decommissioning.
- Lock or validate the yield module/token configuration once funds exist.
- Add a current read-only yield calculation for the UI.
- Add tests covering at least:
- multiple members depositing at different times,
- round 0 claim followed by successful round 1 deposits,
- one member participating in multiple circles,
- Anytime + EndOnly circles simultaneously,
- keep-staked pot followed by later mandatory deposits,
- unauthorized principal withdrawal attempts,
- decommission with principal + accrued yield,
- BREAD -> xDAI/WXDAI -> sDAI -> BREAD round trip,
- CrowdStake claim authorization / holder yield ownership,
- AutomaticSavingCircles behavior,
- proxy/storage upgrade safety.
One CI note: the current GitHub Build & Test failure did not reach Solidity compilation. Dependency installation failed while fetching forge-std, so forge build and tests were skipped. Compilation therefore remains unverified rather than known-failing.
Given the state-machine issues and the unresolved CrowdStake/BREAD ownership model, I'm requesting changes rather than treating these as follow-up cleanup. The UI PR should remain decoupled from this ABI until these contract invariants are settled and covered by integration tests.
| * Deposit model: `mint(amount, receiver)` pulls `amount` of the module's | ||
| * UNDERLYING asset from the caller and credits `receiver` with principal. | ||
| * `burn(amount, receiver)` redeems `amount` of principal and sends the | ||
| * underlying to `receiver`. Yield accrues on top of principal and is read | ||
| * via `yieldAccrued()` and pulled via `claimYield(amount, receiver)`. | ||
| */ | ||
| interface IYieldModule { | ||
| /// @notice Stake `amount` of the underlying (pulled from caller) crediting `receiver`. | ||
| function mint(uint256 amount, address receiver) external; |
There was a problem hiding this comment.
1. Interface drift. This mint(uint256,address) selector (0x94bf804d) is not what the concrete CrowdStake token/pool implements.
The current concrete CrowdStake token/pool implementations expose mint(address receiver, uint256 amount) (0x40c10f19):
- https://github.com/BreadchainCoop/crowdstake.fun/blob/main/contracts/src/abstract/AbstractToken.sol#L197
- https://github.com/BreadchainCoop/crowdstake.fun/blob/main/contracts/src/implementation/pool/AbstractStakePool.sol#L194
- https://github.com/BreadchainCoop/crowdstake.fun/blob/main/contracts/src/interfaces/IToken.sol#L8
- https://github.com/BreadchainCoop/crowdstake.fun/blob/main/contracts/src/interfaces/IStakePool.sol#L30
The selector mismatch proves this local IYieldModule is not the canonical deposit interface. Rather than changing IYieldModule.mint(...), use CrowdStake’s actual deposit surface (IStakePool / IToken) for principal operations, and keep IYieldModule limited to yield-related reads/claims if that’s its intended role.
deposit / withdraw principal
→ IStakePool or IToken
yield inspection / claiming
→ IYieldModule
|
|
||
| uint256 _underlyingAmount = _toUnderlying(_value); | ||
| _underlyingToken().forceApprove(address(yieldModule), _underlyingAmount); | ||
| yieldModule.mint(_underlyingAmount, address(this)); |
There was a problem hiding this comment.
1. Wrong mint ABI. This encodes mint(uint256,address). The current concrete CrowdStake token/pool implementations expose mint(address,uint256) only, so deposits against the current concrete CrowdStake token/pool implementations revert.
Do not keep this as IYieldModule.mint. Principal deposit/withdraw should go through IStakePool / IToken (mint(address,uint256) / burn(uint256,address)). Leave IYieldModule for yield inspection / claiming.
| /// @dev Book newly accrued module yield into the global per-principal accumulator. | ||
| function _harvestGlobal() internal { | ||
| if (_totalPrincipal == 0) return; | ||
| uint256 _accruedNow = yieldModule.yieldAccrued(); | ||
| if (_accruedNow > _lastYieldSeen) { | ||
| uint256 _delta = _accruedNow - _lastYieldSeen; | ||
| _accYieldPerPrincipal += (_delta * _ACC_PRECISION) / _totalPrincipal; | ||
| _lastYieldSeen = _accruedNow; | ||
| } |
There was a problem hiding this comment.
2. Wrong yield source. yieldAccrued() is CrowdStake's donated/distributable pool, not this contract's holder yield.
CrowdStake splits:
- yieldAccrued() — donated/distributable yield
- claimYield(...) — consumes donated yield; restricted to yieldClaimer
- totalYieldAccrued() — full vault surplus
- keptYieldOf(account) / claimKeptYield() — holder-owned
yieldAccrued() itself is a public view. The claimer restriction is on claimYield(), which reverts unless msg.sender == yieldClaimer.
If this module is shared, other holders' donated yield enters _delta while their principal is not in _totalPrincipal. SavingCircles needs an explicit ownership path (dedicated instance + claimer, or 100% kept-yield as the holder) before this accumulator is meaningful.
| // Pull yield (underlying) from the module, then hand it to the member. | ||
| yieldModule.claimYield(_amount, address(this)); | ||
| _lastYieldSeen -= _amount; // module's yieldAccrued() dropped by _amount | ||
| uint256 _out = _fromUnderlying(_amount); | ||
| _payoutToken().safeTransfer(_receiver, _out); |
There was a problem hiding this comment.
3 + 4. Unauthorized claimer, and the wrong asset comes back even if it were authorized.
AbstractToken.claimYield / pool claimYield revert unless msg.sender == yieldClaimer. Standard CrowdStake wiring sets DistributionManager as that claimer. This PR never makes YieldSavingCircles the claimer.
If it did succeed:
- token-mode
SexyDaiYield.claimYieldmints the CrowdStake token to the receiver - pool-mode pays WXDAI
Neither is "underlying ERC-20 that _fromUnderlying + _payoutToken().safeTransfer can handle" in the general case. This cannot assume the donated-yield claim path.
| } else { | ||
| yieldModule.burn(_pot, address(this)); | ||
| uint256 _out = _fromUnderlying(_pot); | ||
| _payoutToken().safeTransfer(_member, _out); |
There was a problem hiding this comment.
4. burn remits native xDAI, not an ERC-20. Both SexyDaiYield._remit and PoolNativeYield._remit do sDAI -> WXDAI -> native xDAI and _nativeTransfer(receiver_, amount_).
This contract has no receive() / fallback(), so the native transfer reverts. Even if it accepted ETH, _fromUnderlying + _payoutToken().safeTransfer would still treat a native credit as if an ERC-20 arrived.
Same bug on withdrawStakedPrincipal (burn at L133). Principal vs yield vs token-mode claim are three different asset flows; a single _fromUnderlying(uint256) seam cannot represent them.
| // --- MasterChef-style global yield accounting (single shared pool) --- | ||
| uint256 private _accYieldPerPrincipal; // scaled by _ACC_PRECISION | ||
| uint256 private _totalPrincipal; // total principal currently staked across all circles | ||
| uint256 private _lastYieldSeen; // module yieldAccrued() already booked into the accumulator | ||
| mapping(address member => uint256 principal) private _principalOf; | ||
| mapping(address member => uint256 debt) private _rewardDebt; | ||
| mapping(address member => uint256 amount) public claimableYield; // credited, awaiting claim |
There was a problem hiding this comment.
9 + 12. Member state cannot be global if claim policy is per circle; and this mapping is not a UI-safe read.
A shared _accYieldPerPrincipal can be fine if every circle shares one yield source. _principalOf / _rewardDebt / claimableYield cannot: a member in an Anytime circle and an EndOnly circle gets one collapsed claimableYield[alice]. _yieldUnlocked then blocks all of their yield if any active circle is EndOnly.
Need at least:
principal[circleId][member]
rewardDebt[circleId][member]
claimableYield[circleId][member]
keptPotPrincipal[circleId][member]claimableYield is also settled storage. Fresh sDAI yield only appears after _harvestGlobal + _settle. The UI reading this mapping and treating 0 as "nothing to claim" will disable claim while unsettled yield exists. Expose a view pendingYield(circleId, member) that simulates the accumulator.
| /// @dev A member's yield is locked while any of their circles is EndOnly and still active. | ||
| function _yieldUnlocked(address _member) internal view returns (bool) { | ||
| uint256[] memory _ids = memberCircles[_member]; | ||
| for (uint256 i = 0; i < _ids.length; i++) { | ||
| if (isActive[_ids[i]] && claimTiming[_ids[i]] == ClaimTiming.EndOnly) return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
9. ClaimTiming is broken by global member state. Any active EndOnly circle locks the member's entire claimableYield, including yield earned in an Anytime circle. This only becomes correct once yield is circle-scoped.
|
|
||
| /// @inheritdoc ISavingCircles | ||
| function decommission(uint256 _id) external override nonReentrant onlyActive(_id) onlyMember(_id, msg.sender) { | ||
| function decommission(uint256 _id) external virtual override nonReentrant onlyActive(_id) onlyMember(_id, msg.sender) { |
There was a problem hiding this comment.
10. Making decommission virtual is not enough. YieldSavingCircles never overrides it.
The inherited body safeTransfers circle.token amounts from roundDeposits out of this contract. After _deposit those tokens are in CrowdStake (and after the BREAD adapter they have been burned). Members get nothing / the call reverts.
Override must: settle accounting, redeem the correct principal from the module, realize owed yield, convert back to BREAD, then pay members. Also broken further if _withdraw zeros roundDeposits (point 6).
There was a problem hiding this comment.
Amendment (author DMs): decommission is not "return each member their yield." At-fault members' unclaimed yield is meant to be spread to not-at-fault members as a reward for staying current.
That is a scope change vs #189 / this PR's "no forfeiture" language, and it cannot be implemented on the inherited body or on global claimableYield[address]. It also depends on roundDeposits remaining intact so "at fault" is computable.
Open: exact at-fault definition, yield-only vs principal, no clawback of Anytime claims, and what happens to keep-staked pots. Full writeup in the conversation amendment.
| /// @notice Set the yield module. Owner only. Must be set before any deposits. | ||
| function setYieldModule(address _yieldModule) external onlyOwner { | ||
| if (_yieldModule == address(0)) revert YieldModuleNotSet(); | ||
| yieldModule = IYieldModule(_yieldModule); | ||
| emit YieldModuleSet(_yieldModule); | ||
| } |
There was a problem hiding this comment.
11. "Must be set before any deposits" is only a comment. Owner can setYieldModule while _totalPrincipal, _principalOf, _rewardDebt, and _lastYieldSeen still describe the old position.
Configure yieldModule once during initialization / make it immutable for the lifetime of the deployment. If module migration is required, implement it as an explicit migration/unwind operation with its own invariants.
_totalPrincipal == 0 is not sufficient to prove rotation is safe — outstanding claimableYield, stale _lastYieldSeen, and other economic state can still be tied to the old module.
Same class of bug on setSoleToken (L274): no relationship to circle.token / allowedTokens, and it can change while circles exist.
| /// @notice Owner sets the single supported ERC20 for circles + payouts (v1 constraint). | ||
| function setSoleToken(address _token) external onlyOwner { | ||
| _soleToken = _token; | ||
| } |
There was a problem hiding this comment.
11. setSoleToken has no invariant. Inherited create() still accepts any allowedTokens entry. Nothing requires circle.token == _soleToken. Owner can retarget payouts while circles are live. Make this immutable at initialize, or require no active circles / no principal, and reject create when token != _soleToken.
3 participants
Draft implementation for #189 — routing saving-circle deposits into crowdstake.fun's sDAI yield module.
What's here
src/interfaces/IYieldModule.sol— crowdstake.fun yield-module interface (mint/burn/claimYield/yieldAccrued).src/contracts/SavingCircles.sol— marked_deposit/_withdraw/decommissionasvirtualso the yield variant can hook them. Storage layout unchanged.src/contracts/YieldSavingCircles.sol— the yield layer (inheritsSavingCircles).Model implemented (per #189)
claim(id, keepStaked)— take the pot to your wallet, or leave it staked so it keeps compounding for you (withdrawStakedPrincipallater).ClaimTiming.AnytimevsEndOnly) viasetClaimTiming.Known open items (need a compiler + review)
keptYieldOf) instead of a local accumulator is an open design call._toUnderlying/_fromUnderlyingseams and still needs implementing.AutomaticSavingCircleslockstep / decommission yield-unwind — see Epic: Yield-bearing savings circles (sDAI yield via crowdstake.fun) #189 gaps.Closes none yet; tracks #189.