Automatic claims#157
Conversation
|
|
||
| /** | ||
| * @notice Gelato resolver-style checker for automatic claims across every circle | ||
| * @return canExec Whether Gelato should execute the claims |
There was a problem hiding this comment.
There is a discrepancy here. need to either update this comment to "Whether Gelato can execute the claims" or change the name of the variable.
| (bool success,) = address(automaticSavingCircles).call(execPayload); | ||
| assertTrue(success); | ||
|
|
||
| assertTrue(savingCircles.hasClaimed(baseCircleId, alice)); |
There was a problem hiding this comment.
check these values are false and balance is correct before the automated claim? maybe not needed since we dont seem to be checking pre-state for other tests
| * @param _index Current write index in the output arrays | ||
| * @return nextIndex Updated write index after appending any eligible targets | ||
| */ | ||
| function _populateEligibleAutomatedClaimsForCircle( |
There was a problem hiding this comment.
Naming of this function is a bit confusing, since it implies it's populating an array but really it's just returning the next index. The comments and function name prescribe a use to the function, which I think isn't ideal. Consider renaming to something like "_getNextEligibleAutomationClaimIndexForCircle"
| revert ISavingCircles.NotWithdrawable(); | ||
| } | ||
|
|
||
| SAVING_CIRCLES.withdrawFor(_circleId, _member); |
There was a problem hiding this comment.
_executeAutomatedClaimTarget fetches getCircleMembers and calls _getMemberIndex to check membership, then calls _isEligibleForAutomatedClaim, which calls isMemberWithdrawable → _claimable → _activeClaimableCheck → _getMemberIndex — so membership is verified twice with two separate getCircleMembers calls. Since withdrawFor → _withdraw has onlyMember(_id, _member) as a hard guard, the explicit NotMember revert at line 239 is also redundant. Consider dropping the pre-check and relying on withdrawFor to enforce it, or at least share the member list with _isEligibleForAutomatedClaim to avoid the duplicate fetch.
| if (!SAVING_CIRCLES.isActive(_circleId)) return false; | ||
| if (_circle.effectiveCircleStartTime == 0) return false; | ||
| if (SAVING_CIRCLES.isDecommissionable(_circleId)) return false; | ||
| if (block.timestamp < _roundEndTime(_circle, _memberIndex)) return false; |
There was a problem hiding this comment.
This assumes _memberIndex (the position in getCircleMembers()) equals the member's payout round index. That invariant holds as long as member order in the array is the canonical payout order, but it is implicit. Worth adding a comment asserting this assumption so a future refactor of how member order is stored does not silently break claim eligibility.
There was a problem hiding this comment.
I think we make this assumption throughout the contract, so can probably skip this
| function _countEligibleAutomatedClaimsForCircle(uint256 _circleId) internal view returns (uint256 eligibleCount) { | ||
| try SAVING_CIRCLES.getCircle(_circleId) returns (ISavingCircles.Circle memory _circle) { | ||
| if (!SAVING_CIRCLES.isActive(_circleId)) return 0; | ||
| if (SAVING_CIRCLES.isDecommissionable(_circleId)) return 0; |
There was a problem hiding this comment.
isActive and isDecommissionable are checked here (and again in _populateEligibleAutomatedClaimsForCircle) before calling _isEligibleForAutomatedClaim, which already checks both conditions (lines 438–440). The early-exit guards are redundant and could diverge from the authoritative check over time. Consider removing them from _countEligibleAutomatedClaimsForCircle and _populateEligibleAutomatedClaimsForCircle and letting _isEligibleForAutomatedClaim be the single gate.
There was a problem hiding this comment.
I generally agree with this comment, but assuming the early-exit guards aren't too expensive it can be worth leaving in just to future proof. isDecommissionable loops over members, so this is sort of a heavy check.
| members = new address[](0); | ||
| } | ||
|
|
||
| execPayload = abi.encodeCall(IAutomaticSavingCircles.batchExecuteAutomatedClaims, (circleIds, members)); |
There was a problem hiding this comment.
When automationExecutor == address(0), canExec is false but execPayload is still a valid (empty-arrays) encoded call. This is safe (the call would revert on onlyAutomationExecutor), but Gelato's own resolver examples use execPayload = "" when canExec = false. Low priority, but worth aligning for consistency with Gelato conventions.
|
|
||
| function _fundEnableAndApprove(address _member, uint256 _amount) internal { | ||
| token.mint(_member, _amount); | ||
|
|
There was a problem hiding this comment.
Missing parity with the deposit side: there is no test that a failed individual claim target (e.g. already claimed, or round time not yet passed) emits AutomatedClaimFailed and does not block the rest of the batch. Worth adding to match the coverage pattern for deposits.
bagelface
left a comment
There was a problem hiding this comment.
Got a few comments on here that I think are worth addressing.
Summary
This PR adds Gelato-powered automatic claims to the existing
AutomaticSavingCirclesautomation extension.Gelato can now resolve and execute claimable payouts automatically once both required conditions are true:
What Changed
Added a new automatic claim flow:
claimChecker()scans all circles and returns executable calldata when any member is eligible to claim.getEligibleAutomatedClaims()returns all eligible(circleId, member)claim targets.batchExecuteAutomatedClaims(...)executes claim targets through Gelato’s configured executor.executeAutomatedClaimTarget(...)is used internally so failed targets do not block the full batch with the try/catch pattern.AutomatedClaimFailed(...)is emitted when an individual claim target fails during batch execution.Claim eligibility requires:
SavingCircles.isMemberWithdrawable(circleId, member)returns true.This means Gelato only claims when deposits are complete and the time condition is satisfied.
SavingCircles Change
withdrawFor(...)is now truly permissionless for valid claim recipients. The caller no longer needs to be a circle member; instead, the target member must be a valid member and must be withdrawable.This is required because the automation contract is not a member of each circle, but it needs to trigger claims on behalf of eligible members. The payout safety checks still remain in place through
_claimable(...).Tests
Added coverage for:
claimChecker()returns the correct batch execution payload,withdrawFor(...)for valid recipients,Validation:
forge testFull suite passes: 156 tests.