Skip to content

Fix decommissioned circle data access in viewer #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
franrolotti wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Fix decommissioned circle data access in viewer #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffb863b
feat: preserve decommissioned circle data for viewer access
franrolotti Feb 9, 2026
0fe8d1b
test: handle zero amount and interval in fuzz creation
franrolotti Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/contracts/SavingCircles.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable, E
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;
Copy link
Contributor

@RonTuretzky RonTuretzky Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@exo404 exo404 Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@RonTuretzky RonTuretzky Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@exo404 exo404 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, also in the other PR

Copy link
Contributor Author

@franrolotti franrolotti Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// @dev Requires circle is commissioned by checking if an owner is set
modifier onlyCommissioned(uint256 _id) {
Expand Down Expand Up @@ -88,6 +89,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable, E
isMember[_id][owner] = true;
memberCircles[owner].push(_id);
circleMembers[_id].push(owner);
circleOwners[_id] = owner;

circles[_id] = _circle;
emit CircleCreated(_id, _circle.token, _circle.depositAmount, _circle.depositInterval);
Expand Down Expand Up @@ -153,7 +155,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable, E
}
}

delete circles[_id];
circles[_id].owner = address(0);
emit CircleDecommissioned(_id);
Comment on lines 155 to 159
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@exo404 exo404 Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franrolotti seems important to note

}

Expand Down
47 changes: 41 additions & 6 deletions src/contracts/SavingCirclesViewer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ contract SavingCirclesViewer is ISavingCirclesViewer {

uint256 currentIndex = 0;
for (uint256 i = 0; i < SAVING_CIRCLES.nextId(); i++) {
if (SAVING_CIRCLES.getCircle(i).owner == _user && !_isInArray(i, memberCircleIds)) {
if (_getCircleOwner(i) == _user && !_isInArray(i, memberCircleIds)) {
ownedOnlyIds[currentIndex] = i;
currentIndex++;
}
Expand All @@ -144,7 +144,7 @@ contract SavingCirclesViewer is ISavingCirclesViewer {
uint256[] memory memberCircleIds
) internal view returns (uint256 count) {
for (uint256 i = 0; i < SAVING_CIRCLES.nextId(); i++) {
if (SAVING_CIRCLES.getCircle(i).owner == _user && !_isInArray(i, memberCircleIds)) {
if (_getCircleOwner(i) == _user && !_isInArray(i, memberCircleIds)) {
count++;
}
}
Expand Down Expand Up @@ -265,14 +265,19 @@ contract SavingCirclesViewer is ISavingCirclesViewer {
) internal view returns (UserCircleData memory circleData) {
circleData.circleId = _circleId;

ISavingCircles.Circle memory circle = SAVING_CIRCLES.getCircle(_circleId);
ISavingCircles.Circle memory circle = _getCircle(_circleId);
bool isDecommissioned = SAVING_CIRCLES.isDecommissioned(circle);

if (SAVING_CIRCLES.isDecommissioned(circle)) {
if (circle.owner == address(0)) {
circle.owner = SAVING_CIRCLES.circleOwners(_circleId);
}
Comment on lines +271 to +273
Copy link
Contributor

@RonTuretzky RonTuretzky Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would this ever happen?

Copy link
Contributor

@exo404 exo404 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@franrolotti franrolotti Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@RonTuretzky RonTuretzky Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@RonTuretzky RonTuretzky Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@franrolotti franrolotti Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. On decommission, keep owner untouched.
  2. Mark decommissioned via a dedicated rule (circleEnd = type(uint256).max as you suggested, or a dedicated isDecommissionedById flag).
  3. Update isDecommissioned/guards to use that marker.
  4. Remove circleOwners fallback logic from the viewer entirely.

Is this ok? @RonTuretzky

Copy link
Contributor

@exo404 exo404 Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me lmk if you need help on this

Copy link
Contributor

@RonTuretzky RonTuretzky Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

circleData.circleInfo = circle;

if (isDecommissioned) {
circleData.isDecommissioned = true;
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
return circleData;
}

circleData.circleInfo = circle;
circleData.isDecommissioned = false;
circleData.isDecommissionable = SAVING_CIRCLES.isDecommissionable(_circleId);

Expand Down Expand Up @@ -321,7 +326,7 @@ contract SavingCirclesViewer is ISavingCirclesViewer {

function _setDepositProgress(UserCircleData memory circleData, uint256 _circleId) internal view {
(, uint256[] memory memberBalances) = SAVING_CIRCLES.getMemberBalances(_circleId);
ISavingCircles.Circle memory circle = SAVING_CIRCLES.getCircle(_circleId);
ISavingCircles.Circle memory circle = _getCircle(_circleId);

uint256 membersWithFullDeposits = 0;
for (uint256 i = 0; i < memberBalances.length; i++) {
Expand All @@ -332,6 +337,36 @@ contract SavingCirclesViewer is ISavingCirclesViewer {
circleData.remainingDepositsNeeded = memberBalances.length - membersWithFullDeposits;
}

function _getCircleOwner(uint256 _circleId) internal view returns (address owner) {
ISavingCircles.Circle memory circle = _getCircle(_circleId);
owner = circle.owner;
if (owner == address(0)) {
owner = SAVING_CIRCLES.circleOwners(_circleId);
}
}

function _getCircle(uint256 _circleId) internal view returns (ISavingCircles.Circle memory circle) {
(
address owner,
uint256 currentIndex,
uint256 depositAmount,
address token,
uint256 depositInterval,
uint256 effectiveCircleStartTime,
uint256 circleEnd
) = SAVING_CIRCLES.circles(_circleId);

circle = ISavingCircles.Circle({
owner: owner,
currentIndex: currentIndex,
depositAmount: depositAmount,
token: token,
depositInterval: depositInterval,
effectiveCircleStartTime: effectiveCircleStartTime,
circleEnd: circleEnd
});
}

function _isInArray(uint256 value, uint256[] memory array) internal pure returns (bool) {
for (uint256 i = 0; i < array.length; i++) {
if (array[i] == value) {
Expand Down
7 changes: 7 additions & 0 deletions test/fuzz/SavingCirclesFuzz.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,13 @@ contract SavingCirclesFuzzTest is SavingCirclesTestBase {

ISavingCircles.Circle memory circle = _defaultCircle(alice, _depositAmount, _depositInterval, address(token));

if (_depositAmount == 0 && _depositInterval == 0) {
vm.prank(alice);
vm.expectRevert(ISavingCircles.InvalidDepositInterval.selector);
savingCircles.create(circle);
return;
}

if (_depositAmount == 0) {
vm.prank(alice);
vm.expectRevert(ISavingCircles.InvalidDepositAmount.selector);
Expand Down
Loading