Fix decommissioned circle data access in viewer by franrolotti · Pull Request #117 · BreadchainCoop/saving-circles

franrolotti · 2026-02-09T10:09:53Z

Summary

Keep circle storage on decommission while still marking decommissioned by zeroing owner.
Store the original owner in circleOwners so historical data can be displayed.
Update the viewer to reconstruct circles from raw storage so it won’t revert on decommissioned circles.
Fix fuzz test expectation for the zero/zero creation edge case.

Issue

The current decommission deletes circles[_id] and getCircle is guarded by onlyCommissioned. This makes SavingCirclesViewer.getUserCircleData revert for decommissioned circles, so the frontend can’t show historical circle details. This PR keeps the data and restores viewer access.

Notes / Possible flaws

If this upgrade is applied to the existing deployment, older circles won’t have circleOwners populated; after decommission, their original owner may be lost unless migrated.
Storage grows over time because circle records are no longer deleted.
Viewer now bypasses the onlyCommissioned guard by reading raw storage, which is intentional for history but changes the access pattern.

Alternative approach

Drop the owner == address(0) meaning and add an explicit isDecommissionedById flag, optionally storing only a minimal snapshot for display. This is cleaner long‑term but requires changing semantics.
Alternatively, remove onlyCommissioned from getCircle and keep circle data; then the viewer can call getCircle directly without bypassing the guard.

RonTuretzky · 2026-02-09T16:31:47Z

src/contracts/SavingCircles.sol

  mapping(uint256 id => mapping(uint256 nonce => bool used)) public usedNonces;
  mapping(uint256 id => bool active) public isActive;
  mapping(uint256 id => address[] members) public circleMembers;
+  mapping(uint256 id => address owner) public circleOwners;


wouldn't a better design be to designate the 0 index to always be the circle owner?

address(0) is equal to "Decommissioned" or "Stack doesn't exist" in our current semantic. What do you mean by 0 index. If you are pointing to the index in the members array, the owner is already the first in the array

then why do we have this mapping? can't we just use that index instead of defining another variable ?

Yes I agree, also in the other PR

Good point. I added circleOwners as a way to preserve historical owner after decommission() zeroes circle.owner, so the viewer could still display owner data for decommissioned circles.

But I agree the cleaner design is to use the existing circleMembers[id][0].

I’ll update this PR to remove the viewer dependency on circleOwners and reconstruct historical owner from circleMembers[id][0] instead.

RonTuretzky · 2026-02-09T16:42:56Z

src/contracts/SavingCirclesViewer.sol

+    if (circle.owner == address(0)) {
+      circle.owner = SAVING_CIRCLES.circleOwners(_circleId);
+    }


why would this ever happen?

he's updating the local circle variable with the owner after decommissioning the circle, but this approach feels messy imo

It happens after decommission by design: decommission() sets circles[id].owner = address(0) to mark the circle as decommissioned.

That if was my patch to restore historical owner in viewer responses. I agree it’s messy with circleOwners; I’ll switch this to derive owner from circleMembers[id][0].

If we want to move away from owner == address(0) as the decommission flag, I can open a follow-up PR for this or do it in this PR.

you can use circle end = type(uint256).max

i dont understand why we should need to restore a historical owner at any point so we should redesign if possible to avoid that

You’re right, and I agree.

I chose the circleOwners/viewer patch to avoid changing decommission() semantics in this PR, but that was probably the wrong tradeoff because it made everything messy and confusing.

I’ll switch to an explicit decommission marker in contract state and remove the owner-reconstruction logic from the viewer. Concretely:

On decommission, keep owner untouched.

Mark decommissioned via a dedicated rule (circleEnd = type(uint256).max as you suggested, or a dedicated isDecommissionedById flag).

Update isDecommissioned/guards to use that marker.

Remove circleOwners fallback logic from the viewer entirely.

Is this ok? @RonTuretzky

sounds good to me lmk if you need help on this

RonTuretzky · 2026-02-09T16:45:02Z

general note: this feels like a nice to have and outside of what i would slot into the current release

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

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

Copilot · 2026-02-10T16:45:22Z

src/contracts/SavingCirclesViewer.sol

+    circleData.circleInfo = circle;
+
+    if (isDecommissioned) {
      circleData.isDecommissioned = true;


For decommissioned circles, _getUserCircleData returns early after setting only circleInfo and isDecommissioned. This leaves fields like isOwner, isMember, isExpired, completedRounds, and totalRounds at default values, which can be incorrect/misleading even though they can be derived without calling commissioned-only functions. Consider populating the non-reverting fields (e.g., isOwner from restored circle.owner, isMember via SAVING_CIRCLES.isMember, timing/round counts via circle + getCircleMembers) before returning for decommissioned circles.

Suggested change

circleData.isDecommissioned = true;

circleData.isDecommissioned = true;

// For decommissioned circles, we can still safely derive some fields

// without going through commissioned-only helper flows.

circleData.isOwner = (circle.owner == _user);

circleData.isMember = SAVING_CIRCLES.isMember(_circleId, _user);

_setCircleTimingData(circleData, circle);

_setDepositProgress(circleData, _circleId);

Copilot · 2026-02-10T16:45:22Z

src/contracts/SavingCircles.sol

      }
    }

-    delete circles[_id];
+    circles[_id].owner = address(0);
    emit CircleDecommissioned(_id);


Since decommission no longer deletes circles[_id] (it only zeroes owner), isDecommissionable(_id) can now return true for already-decommissioned circles because _isDecommissionable doesn’t check isActive[_id] / decommissioned status and balances are reset to 0 (< depositAmount). Consider updating isDecommissionable/_isDecommissionable to return false when the circle is decommissioned or inactive to avoid misleading results from the public view API.

@franrolotti seems important to note

src/contracts/SavingCircles.sol

franrolotti added 2 commits February 9, 2026 10:50

feat: preserve decommissioned circle data for viewer access

ffb863b

test: handle zero amount and interval in fuzz creation

0fe8d1b

franrolotti requested review from RonTuretzky and exo404 February 9, 2026 10:09

franrolotti linked an issue Feb 9, 2026 that may be closed by this pull request

decommission deletes circle data, causing getUserCircleData to revert #112

Open

RonTuretzky requested a review from Copilot February 9, 2026 16:31

RonTuretzky requested changes Feb 9, 2026

View reviewed changes

Copilot AI reviewed Feb 9, 2026

View reviewed changes

franrolotti self-assigned this Feb 10, 2026

RonTuretzky requested a review from Copilot February 10, 2026 16:37

Copilot started reviewing on behalf of RonTuretzky February 10, 2026 16:37 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

exo404 mentioned this pull request Feb 10, 2026

Show the card details information in the "Your Stack" section #116

Closed

-      circleData.isDecommissioned = true;
+      circleData.isDecommissioned = true;
+      // For decommissioned circles, we can still safely derive some fields
+      // without going through commissioned-only helper flows.
+      circleData.isOwner = (circle.owner == _user);
+      circleData.isMember = SAVING_CIRCLES.isMember(_circleId, _user);
+      _setCircleTimingData(circleData, circle);
+      _setDepositProgress(circleData, _circleId);

Conversation

franrolotti commented Feb 9, 2026

Summary

Issue

Notes / Possible flaws

Alternative approach

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exo404 Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonTuretzky commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

exo404 Feb 10, 2026 •

edited

Loading