feat: add member-level withdrawability and fix viewer canWithdraw for late claims

[franrolotti]

We need to change this in order for members to claim late because it fixes the canWithdraw from the viewer.

Today, late claims are valid at the contract level (_claimable allows a member to claim after their round has passed, as long as their round was fully funded and they have not already claimed), but the viewer only exposed withdrawability for the current round withdrawer. That makes canWithdraw incorrect for members who are still eligible to claim from an earlier round.

What changed

• Added isMemberWithdrawable(uint256 id, address member) to ISavingCircles and SavingCircles
• Updated SavingCirclesViewer to compute UserCircleData.canWithdraw using isMemberWithdrawable(circleId, user) instead of isCurrentWithdrawer && isWithdrawable(circleId)
• Updated isWithdrawable(uint256 id) to return true if any member in the circle is claimable (aggregate circle-level status).
• Updated fuzz test (SavingCirclesMultiRound) to use member-specific withdrawability when deciding whether the selected recipient should withdraw

Now:

• viewer.canWithdraw reflects whether the user can claim, including late claims
• isWithdrawable remains useful as a circle-level “someone can withdraw” signal

Functions behavior

• isWithdrawable(id)

  • Circle-level check
  • Returns true if the circle is active and at least one member is currently claimable
  • It no longer means “the current round withdrawer can withdraw”

• isMemberWithdrawable(id, member)

  • Member-level check
  • Returns true if the circle is active and that specific member is claimable right now

• _claimable(id, member)

  • Internal source of truth for claim eligibility
  • Returns false if the member already claimed or the circle is decommissionable
  • Returns false if the member is not in the circle or their turn has not been reached yet (currentRound < memberIdx)
  • Returns true only when all deposits for that member’s round are complete
  • This enables late claims because it allows currentRound >= memberIdx (not only == memberIdx)

Test update note

The fuzz test now checks isMemberWithdrawable(circleId, recipient) before calling withdraw, which matches the actual recipient being tested and avoids relying on the broader circle-level isWithdrawable result.

[bagelface]

PR Review: feat: add member-level withdrawability and fix viewer canWithdraw for late claims

Overall

The motivation is sound — canWithdraw in the viewer was incorrect for late-claiming members because it only checked isCurrentWithdrawer && isWithdrawable. The new isMemberWithdrawable cleanly solves this, and the viewer change to use it is correct. No security vulnerabilities found.

However, there are a few issues worth addressing before merge:

---

1. O(N²) gas in isWithdrawable — consider short-circuit optimization

isWithdrawable now loops all N members calling _claimable for each (SavingCircles.sol:297). Each _claimable call internally:

• Calls _isDecommissionable(_id) which itself loops all members via _allMembersDepositedForRound — O(N)
• Calls _allMembersDepositedForRound again — O(N)

This makes isWithdrawable O(N²). While this is only a view function (free off-chain), it will cost real gas if called from another contract (e.g. a future integration, a keeper, or a composed DeFi protocol). The _isDecommissionable result is identical for every iteration of the loop and could be hoisted:

function isWithdrawable(uint256 _id) public view override returns (bool) {
    if (!isActive[_id]) return false;
    if (_isDecommissionable(_id)) return false;

    address[] memory members = circleMembers[_id];
    for (uint256 i = 0; i < members.length; i++) {
        if (_claimableSkipDecommCheck(_id, members[i])) return true;
    }
    return false;
}

This would reduce worst-case to O(N) instead of O(N²). This isn't strictly necessary for correctness, but given this function is part of the public interface and may be called on-chain, it's worth the optimization.

2. Missing unit tests for isMemberWithdrawable

This PR adds a new public function to the interface but no dedicated tests. The following scenarios should have explicit coverage:

Scenario	Expected
isMemberWithdrawable on a non-existent / inactive circle	false
isMemberWithdrawable for a non-member address	false (no revert)
isMemberWithdrawable for a member whose turn hasn't arrived	false
isMemberWithdrawable for a member who has already claimed	false
isMemberWithdrawable for a late-claimable member (past round, deposits complete)	true
isMemberWithdrawable when circle is decommissionable	false

Several of these paths are exercised indirectly through existing withdraw tests, but since isMemberWithdrawable is now the source of truth for the viewer's canWithdraw, it deserves first-class test coverage.

3. isWithdrawable semantics are a breaking change — document it

The old isWithdrawable returned whether the current-round withdrawer could claim. The new version returns whether any member can claim. This is a different semantic. If any off-chain or on-chain consumer relied on the old meaning (e.g., a keeper that calls isWithdrawable then withdraw for the current-round member), they'd now get false positives when only a late-claimer is eligible but the current round isn't withdrawable.

Consider adding a @notice annotation on isWithdrawable clarifying the new aggregate semantics, and documenting the change in the interface natspec.

4. Fuzz test change is correct but doesn't expand late-claim coverage

The fuzz test update (SavingCirclesMultiRound.t.sol:532-538) correctly switches from isWithdrawable to isMemberWithdrawable(circleId, recipient), which is more precise. However, recipient is still always storedMembers[circleData.currentIndex] — the current-round member. The fuzz suite never exercises the late-claim path (a past-round member claiming after their round passed). This is the primary new behavior enabled by this PR and would benefit from fuzz coverage.

5. Minor: isMemberWithdrawable natspec

The interface docstring says:

@notice Check if a specific member can withdraw for a circle

"withdraw for a circle" reads ambiguously — it could mean "withdraw on behalf of a circle" rather than "withdraw from a circle". Suggest:

@notice Check if a specific member is eligible to claim their withdrawal from a circle

---

Verdict

The core logic changes are correct and the approach is clean. No vulnerabilities, no insolvency risks with the late-claim model (each claim always draws exactly one round's worth of deposits). The main requests are:

1. Add unit tests for isMemberWithdrawable (blocking)
2. Consider the O(N²) optimization (non-blocking but recommended)
3. Update natspec for the isWithdrawable semantic change (non-blocking)

[franrolotti]

@bagelface 486c156 removes repeated decommission checks in isWithdrawable. For a full optimization, though, we’d need a small model change as tracking per-round funding completion in storage (e.g., funded-member counters or roundFullyFunded flags) and update it in _deposit when a member crosses from < depositAmount to >= depositAmount. Then claimability/decommissionability checks can use O(1) lookups instead of per-member scans. This is very feasible with current structures (roundDeposits, fixed member set after start), but we should do it in a focused follow-up PR to merge this one as it's needed for the current testing.

[franrolotti]

AI Review Summary

I reviewed this branch against dev and the changes are correct for the stated goal: fixing late-claim visibility in the viewer.

What looks good

• isMemberWithdrawable(id, member) was added and wired correctly through SavingCircles and ISavingCircles.
• Viewer now computes canWithdraw using member-level eligibility, which fixes the late-claim mismatch.
• isWithdrawable(id) now reflects aggregate circle-level status (“someone can withdraw”), and the natspec/docs were updated to match that semantic.
• Unit coverage for isMemberWithdrawable is now solid (inactive/non-existent circle, non-member, future-round member, already-claimed member, late-claimable member, decommissionable circle).
• Viewer tests now cover late-claim behavior and decommissionable behavior correctly.
• The decommission check hoist in isWithdrawable is a good optimization step.

Remaining notes

• isWithdrawable is still O(N²) worst-case because each member check still scans a round’s deposits. This is non-blocking here, but a storage-backed round-funded index/flag would be the right follow-up optimization.
• Fuzz update is directionally correct (isMemberWithdrawable(circleId, recipient)), but the helper still picks recipient as current-round member, so it does not fuzz true late-claimer actors yet.

Verdict

No blocking issues found.
The PR is mergeable for the intended fix, with performance/fuzz-depth improvements suitable for a focused follow-up PR.

[Copilot]

Pull request overview

This PR updates withdrawability semantics to support “late claims” at the member level and fixes the viewer’s canWithdraw so it reflects whether the user can claim (not just whether they are the current withdrawer).

Changes:

• Add isMemberWithdrawable(id, member) to ISavingCircles/SavingCircles and use it in SavingCirclesViewer for UserCircleData.canWithdraw.
• Redefine isWithdrawable(id) as an aggregate “someone in this circle can withdraw” signal (any claimable member).
• Update unit and fuzz tests to cover late-claim behavior and to use member-specific withdrawability when selecting a withdrawal recipient.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/unit/SavingCirclesViewerUnit.t.sol	Adds coverage ensuring viewer canWithdraw is true for late-claimable members and false when decommissionable.
test/unit/SavingCirclesUnit.t.sol	Adds unit tests for isMemberWithdrawable across non-existent/inactive/non-member/future-round/already-claimed/late-claimable/decommissionable cases.
test/fuzz/SavingCirclesMultiRound.t.sol	Updates fuzz withdrawal decision to use isMemberWithdrawable(circleId, recipient) instead of circle-level isWithdrawable.
src/interfaces/ISavingCircles.sol	Updates isWithdrawable documentation and adds the new isMemberWithdrawable function to the interface.
src/contracts/SavingCirclesViewer.sol	Fixes canWithdraw to be computed via member-level withdrawability.
src/contracts/SavingCircles.sol	Implements isMemberWithdrawable, changes isWithdrawable semantics, and refactors claimability helpers to support both checks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

isWithdrawable now loops over all members and for each member calls _claimableWithoutDecommissionCheck, which in turn calls _allMembersDepositedForRound (another full members loop). With no member-count cap (see redeemInvite comment), this makes isWithdrawable worst-case O(n^2) and repeatedly re-allocates/copies circleMembers[_id] into memory, which can become very expensive for large circles. Consider caching Circle/currentRound/depositAmount once and avoiding nested full-member scans (e.g., track per-round total deposits / “round funded” status, or refactor _allMembersDepositedForRound to reuse the already-loaded members array).

Suggested change

	address[] memory members = circleMembers[_id];
	for (uint256 i = 0; i < members.length; i++) {
	if (_claimableWithoutDecommissionCheck(_id, members[i])) return true;
	}
	return false;
	Circle memory _circle = circles[_id];
	uint256 currentRound = _currentRoundIndex(_circle);
	if (currentRound >= circleMembers[_id].length) return false;
	address withdrawer = circleMembers[_id][currentRound];
	return _claimableWithoutDecommissionCheck(_id, withdrawer);

[franrolotti]

Context on isWithdrawable: we kept/expanded it as a circle-level aggregate signal (“someone can withdraw now”), while isMemberWithdrawable is the source of truth for user-level eligibility (and what the viewer now uses for canWithdraw, including late claims).

We could consider dropping/deprecating isWithdrawable, but may be a larger change for this PR (wdyt?). We want this PR merged to correctly display the claim button for late claims in the current testing round.

Then we could solve this, but not a blocker right now for our expected circle sizes (<10 members). Am I right here? @RonTuretzky

[bagelface]

Agree on the time complexity not being a blocker, but please make an issue for it so we don't lose track of it.

[RonTuretzky]

agreed

[bagelface]

please open an issue (if there isn't already one) to optimize the time complexity of isWithdrawable. Other than that, lgtm

[franrolotti]

@RonTuretzky do you think we could merge this? bagel already approved

[RonTuretzky]

Approving but please lets track the O(n^2) issue and I have a feeling that there are future viewer changes that need to be made to give the frontend everyone who's withdrawable at once