Add Stacks Card details

[exo404]

To solve the issue #116 , I implemented 2 view functions

First one is circleState that calculates the Circle state and returns:

• 0 if NotStarted
• 1 if Active
• 2 if Active and deposits are in progress
• 3 if Active and deposits are completed
• 4 if the circle expired
• 5 if Decommissioned
• 6 if there is a missed deposit

Second one is roundState:

• 0 if round not started
• 1 if deposits are in progress
• 2 if deposits are completed
• 3 if deposits are claimed

I'm also adding tests for this and a new struct with a new function in SavingCircleViewer contract to expose this data to the frontend.

[bagelface]

I think it'd be more appropriate to use an enum for circle state
https://solidity-by-example.org/enum/

[exo404]

dfd3906

[bagelface]

Can you elaborate a bit on how these changes help solve the issue described in #114. I'm not quite following

[bagelface]

PR Review: Add Stacks Card details (circleState / roundState)

Summary

This PR adds two view functions (circleState, roundState) and a viewer batch helper. The intent is solid — surfacing circle lifecycle state to the frontend. However, there are two bugs that make documented states unreachable and one case that causes a revert on expired circles. These need to be fixed before merge.

---

Bug 1 (High): circleState — State 4 (Expired) is unreachable

if (currentRound >= circleMembers[_id].length) {
    state = 4;                                        // ← set to Expired
}
if (currentRound > 0) {                               // ← always true when expired, OVERWRITES state 4

When a circle is expired (currentRound >= members.length), state is set to 4. But the code immediately falls through into the if (currentRound > 0) block (not else if), which will always be true for an expired circle. That block unconditionally overwrites state to 6, 2, or 3.

Additionally, inside that block, _allMembersDepositedForRound(_id, currentRound, ...) queries deposits for a round index that no member was ever assigned to — it will always return false, so an expired circle incorrectly reports as state 2 (deposits in progress).

Fix: Use else if or return early after setting state 4:

if (currentRound >= circleMembers[_id].length) {
    state = 4;
} else if (currentRound > 0) {
    // ... existing logic
}

There's also no test for circleState on an expired circle — test_CircleStateWhenCircleExpired is absent.

---

Bug 2 (High): roundState — State 3 (Claimed) is unreachable (dead code)

} else if (_allMembersDepositedForRound(_id, currentRound, _circle.depositAmount)) {
    state = 2;                                          // all deposited → state 2
} else if (_claimable(_id, circleMembers[_id][currentRound])) {
    state = 3;                                          // ← never reached
}

_claimable internally requires _allMembersDepositedForRound(_id, memberIdx, ...) to return true (line 436). Since memberIdx for circleMembers[_id][currentRound] equals currentRound, _claimable and _allMembersDepositedForRound check the same round. So:

• If _allMembersDepositedForRound returns true → state = 2 (we never reach the state 3 check)
• If _allMembersDepositedForRound returns false → _claimable also returns false → state = 1

State 3 can never be returned. The check ordering makes it dead code. There's also no test for it (which would have caught this since no assertion for state 3 is possible).

If the intent is to differentiate "deposits complete, not yet claimed" from "deposits complete AND claimed":

if (_allMembersDepositedForRound(_id, currentRound, _circle.depositAmount)) {
    if (_claimable(_id, circleMembers[_id][currentRound])) {
        state = 3; // claimable
    } else {
        state = 2; // deposited but already claimed (or decommissionable)
    }
}

Note: the natspec says state 3 = "Claimed" but the code checks _claimable (i.e., can claim). The naming is also misleading — worth clarifying.

---

Bug 3 (Medium): roundState reverts on expired circles

} else if (_claimable(_id, circleMembers[_id][currentRound])) {

When currentRound >= circleMembers[_id].length, circleMembers[_id][currentRound] causes an array out-of-bounds revert. A view function should not revert on valid state — an expired circle is a normal lifecycle stage.

The fix for Bug 1 (adding the else if) means circleState would handle this correctly, but roundState has no such guard. Add an early return:

if (currentRound >= circleMembers[_id].length) return 0; // or a new "expired" state

---

Issue 4 (Low): roundState returns misleading state for non-existent circles

For a non-existent circle (effectiveCircleStartTime == 0, empty members array), _currentRoundIndex returns 0 and _allMembersDepositedForRound returns true (empty loop). So roundState returns 2 ("deposits complete") for circles that don't exist.

Consider adding an onlyCommissioned guard, or at minimum checking circleMembers[_id].length > 0.

---

Issue 5 (Low): Raw magic numbers for state values

States 0–6 are documented in natspec but represented as bare uint8 literals. This hurts readability and makes a control-flow bug (like #1 and #2 above) harder to spot in review. Consider defining an enum or named constants:

uint8 constant CIRCLE_NOT_STARTED = 0;
uint8 constant CIRCLE_ACTIVE = 1;
// ...

---

Tests

The tests are well-structured and use the new _depositRoundForAllMembers helper effectively. However:

• Missing: circleState on an expired circle (state 4) — adding this test would immediately reveal Bug 1
• Missing: roundState on a claimed round (state 3) — adding this test would immediately reveal Bug 2
• Missing: roundState called on an expired circle — would reveal Bug 3 (revert)
• Missing: roundState on a non-existent circle — would reveal Issue 4

Viewer changes

The getCirclesState batch function in SavingCirclesViewer is clean and follows the existing patterns well.

---

Verdict

The architecture is good and the direction is correct. The two unreachable-state bugs and the expired-circle revert need to be fixed before merge. Once the control flow is corrected (primarily if → else if in circleState, and reordering/restructuring checks in roundState), and corresponding tests are added, this is ready to go.

[exo404]

Can you elaborate a bit on how these changes help solve the issue described in #114. I'm not quite following

The frontend currently misses a way to retrieve data about Circle states so I'm adding new functions to support this behaviour. I typed #114 but the issue is #116 sorry

[exo404]

PR Review: Add Stacks Card details (circleState / roundState)

Summary

This PR adds two view functions (circleState, roundState) and a viewer batch helper. The intent is solid — surfacing circle lifecycle state to the frontend. However, there are two bugs that make documented states unreachable and one case that causes a revert on expired circles. These need to be fixed before merge.

Bug 1 (High): circleState — State 4 (Expired) is unreachable

if (currentRound >= circleMembers[_id].length) {
    state = 4;                                        // ← set to Expired
}
if (currentRound > 0) {                               // ← always true when expired, OVERWRITES state 4

When a circle is expired (currentRound >= members.length), state is set to 4. But the code immediately falls through into the if (currentRound > 0) block (not else if), which will always be true for an expired circle. That block unconditionally overwrites state to 6, 2, or 3.

Additionally, inside that block, _allMembersDepositedForRound(_id, currentRound, ...) queries deposits for a round index that no member was ever assigned to — it will always return false, so an expired circle incorrectly reports as state 2 (deposits in progress).

Fix: Use else if or return early after setting state 4:

if (currentRound >= circleMembers[_id].length) {
    state = 4;
} else if (currentRound > 0) {
    // ... existing logic
}

There's also no test for circleState on an expired circle — test_CircleStateWhenCircleExpired is absent.

Bug 2 (High): roundState — State 3 (Claimed) is unreachable (dead code)

} else if (_allMembersDepositedForRound(_id, currentRound, _circle.depositAmount)) {
    state = 2;                                          // all deposited → state 2
} else if (_claimable(_id, circleMembers[_id][currentRound])) {
    state = 3;                                          // ← never reached
}

_claimable internally requires _allMembersDepositedForRound(_id, memberIdx, ...) to return true (line 436). Since memberIdx for circleMembers[_id][currentRound] equals currentRound, _claimable and _allMembersDepositedForRound check the same round. So:

* If `_allMembersDepositedForRound` returns `true` → state = 2 (we never reach the state 3 check)

* If `_allMembersDepositedForRound` returns `false` → `_claimable` also returns `false` → state = 1

State 3 can never be returned. The check ordering makes it dead code. There's also no test for it (which would have caught this since no assertion for state 3 is possible).

If the intent is to differentiate "deposits complete, not yet claimed" from "deposits complete AND claimed":

if (_allMembersDepositedForRound(_id, currentRound, _circle.depositAmount)) {
    if (_claimable(_id, circleMembers[_id][currentRound])) {
        state = 3; // claimable
    } else {
        state = 2; // deposited but already claimed (or decommissionable)
    }
}

Note: the natspec says state 3 = "Claimed" but the code checks _claimable (i.e., can claim). The naming is also misleading — worth clarifying.

Bug 3 (Medium): roundState reverts on expired circles

} else if (_claimable(_id, circleMembers[_id][currentRound])) {

When currentRound >= circleMembers[_id].length, circleMembers[_id][currentRound] causes an array out-of-bounds revert. A view function should not revert on valid state — an expired circle is a normal lifecycle stage.

The fix for Bug 1 (adding the else if) means circleState would handle this correctly, but roundState has no such guard. Add an early return:

if (currentRound >= circleMembers[_id].length) return 0; // or a new "expired" state

Issue 4 (Low): roundState returns misleading state for non-existent circles

For a non-existent circle (effectiveCircleStartTime == 0, empty members array), _currentRoundIndex returns 0 and _allMembersDepositedForRound returns true (empty loop). So roundState returns 2 ("deposits complete") for circles that don't exist.

Consider adding an onlyCommissioned guard, or at minimum checking circleMembers[_id].length > 0.

Issue 5 (Low): Raw magic numbers for state values

States 0–6 are documented in natspec but represented as bare uint8 literals. This hurts readability and makes a control-flow bug (like #1 and #2 above) harder to spot in review. Consider defining an enum or named constants:

uint8 constant CIRCLE_NOT_STARTED = 0;
uint8 constant CIRCLE_ACTIVE = 1;
// ...

Tests

The tests are well-structured and use the new _depositRoundForAllMembers helper effectively. However:

* **Missing:** `circleState` on an expired circle (state 4) — adding this test would immediately reveal Bug 1

* **Missing:** `roundState` on a claimed round (state 3) — adding this test would immediately reveal Bug 2

* **Missing:** `roundState` called on an expired circle — would reveal Bug 3 (revert)

* **Missing:** `roundState` on a non-existent circle — would reveal Issue 4

Viewer changes

The getCirclesState batch function in SavingCirclesViewer is clean and follows the existing patterns well.

Verdict

The architecture is good and the direction is correct. The two unreachable-state bugs and the expired-circle revert need to be fixed before merge. Once the control flow is corrected (primarily if → else if in circleState, and reordering/restructuring checks in roundState), and corresponding tests are added, this is ready to go.

dfd3906
d9fdbd0
1f44f30
3580e39

[bagelface]

would prefer this to just be an "if/else" for better readability

[exo404]

ce61768

[bagelface]

one small comment. please fix the warnings about the function ordering too

[exo404]

one small comment. please fix the warnings about the function ordering too

70d8d58