Conversation
📝 WalkthroughWalkthroughAdds a payment subsystem: PaymentBase (fundsOwner, paymentToken, token recovery, elevated access), EpochPayments (epoch-based admin-managed per-epoch allocations, epoch lifecycle, claiming and revocation), and VestingPayments (multi-schedule vesting with creation, revocation, release, and query APIs). Introduces IPaymentBase, IEpochPayments, and IVestingPayments interfaces, comprehensive Forge unit tests for VestingPayments and EpochPayments, deployment scripts for vesting/epoch contracts, Foundry config updates, and minor ancillary tooling and config changes. Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin
participant Epoch as EpochPayments
participant ERC20 as paymentToken (ERC20)
participant Recipient
rect rgb(240,248,255)
note right of Epoch: Admin creates/updates epoch allocations
Admin->>Epoch: setEpochPayments(recipients[], amounts[])
Epoch-->>Epoch: _startNextEpoch() (checks minEpochDuration, set start time)
Epoch-->>Admin: NextEpochSet(epoch)
Epoch-->>Admin: EpochAllocationSet(recipient, epoch, amount) [per recipient]
end
rect rgb(255,250,240)
note right of Epoch: Recipient claims allocated amount for an epoch
Recipient->>Epoch: claimEpoch(epoch)
Epoch-->>Epoch: validate, mark claimed, totalClaimed += amount
Epoch->>ERC20: safeTransferFrom(fundsOwner, recipient, amount)
Epoch-->>Recipient: ClaimedEpoch(recipient, epoch, amount)
end
sequenceDiagram
autonumber
participant Admin
participant Vesting as VestingPayments
participant ERC20 as paymentToken (ERC20)
participant Recipient
rect rgb(240,248,255)
note right of Vesting: Admin creates vesting schedules
Admin->>Vesting: createSchedules(schedules[])
Vesting-->>Admin: ScheduleCreated(id, recipient, start, cliff, duration, amount)
end
rect rgb(245,255,250)
note right of Vesting: Recipient requests vested funds over time
Recipient->>Vesting: release(vestingId)
Vesting-->>Vesting: compute releasable, update distributed
Vesting->>ERC20: safeTransferFrom(fundsOwner, recipient, amount)
Vesting-->>Recipient: Released(id, recipient, amount)
end
rect rgb(255,240,245)
note right of Vesting: Admin may revoke active schedule
Admin->>Vesting: revokeVesting(vestingId)
Vesting-->>Admin: Revoked(id, recipient, unreleased, totalVested)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (19)
protocol/contracts/interfaces/admin/IPaymentBase.sol (1)
13-17: Generalize “TGLD”-specific natspec to cover any payment token.The interface is reused across multiple payment flows and tokens. The docs “owner of the TGLD funds” and “TGLD address” are misleading; please generalize to “payment token” to avoid confusion. This was raised previously on the base file as well.
Apply:
- /// @notice The owner of the TGLD funds + /// @notice The owner of the payment token funds ... - /// @notice TGLD address + /// @notice Payment token addressprotocol/contracts/admin/PaymentBase.sol (3)
13-16: Generalize natspec: not TGLD-specific.- /// @notice The owner of the TGLD funds + /// @notice The owner of the payment token funds
22-27: Critical: Mutators are public and unguarded in the base. Convert to internal helpers to remove a footgun.As written, setFundsOwner, recoverToken, and setPaymentToken are publicly callable on any inheritor that forgets to override with access control, leading to immediate fund theft or configuration corruption. Prior feedback suggested making these internal helpers. Adopt that pattern so derived contracts implement the external functions with their own modifiers.
Proposed minimal refactor (pattern):
-import { IPaymentBase } from "contracts/interfaces/admin/IPaymentBase.sol"; +// import removed; derived contracts will implement the interface -abstract contract PaymentBase is IPaymentBase { +abstract contract PaymentBase { @@ - function setFundsOwner(address _fundsOwner) public virtual { + function _setFundsOwner(address _fundsOwner) internal { if (_fundsOwner == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); } /// @dev Elevated access should revoke approval from old `fundsOwner` for this contract fundsOwner = _fundsOwner; emit FundsOwnerSet(_fundsOwner); } @@ - function recoverToken(address _token, address _to, uint256 _amount) public virtual { + function _recoverToken(address _token, address _to, uint256 _amount) internal { + if (_to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); } emit CommonEventsAndErrors.TokenRecovered(_to, _token, _amount); IERC20(_token).safeTransfer(_to, _amount); } @@ - function setPaymentToken(address _token) public virtual { + function _setPaymentToken(address _token) internal { if (_token == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); } paymentToken = IERC20(_token); emit PaymentTokenSet(_token); }Then, in each derived contract (e.g., TeamPayments/EpochPayments/TempleGoldVesting), implement the IPaymentBase surface:
function setFundsOwner(address _fundsOwner) external override onlyElevatedAccess { _setFundsOwner(_fundsOwner); } function recoverToken(address _token, address _to, uint256 _amount) external override onlyElevatedAccess { _recoverToken(_token, _to, _amount); } function setPaymentToken(address _token) external override onlyElevatedAccess { _setPaymentToken(_token); }If you prefer to keep PaymentBase implementing IPaymentBase, at minimum add an abstract modifier (or import the access base) so these functions are protected in the base. I can draft that variant if you prefer.
Also applies to: 35-38, 44-48
44-48: Changing paymentToken mid-flight is dangerous; constrain or make immutable.Given Payment struct does not encode a token, switching paymentToken while allocations exist can silently denominate future claims in a different token. Either:
- Make paymentToken immutable via constructor and drop setter; or
- Require totalAllocation == 0 (and no active epochs/vestings) before permitting a change.
If keeping a setter in derived contracts, enforce an invariant like:
if (totalAllocation() != 0) revert CommonEventsAndErrors.InvalidState();protocol/contracts/interfaces/admin/ITeamPayments.sol (2)
7-13: Consider uint40 for timestamps.As previously suggested, using uint40 for start/duration provides longer runway and packs efficiently with a bool. Not required, but forward-looking.
6-6: Extend IPaymentBase to reflect actual surface.TeamPayments relies on fundsOwner and paymentToken from the base. Extending IPaymentBase improves discoverability and consistency.
-interface ITeamPayments { +import { IPaymentBase } from "contracts/interfaces/admin/IPaymentBase.sol"; + +interface ITeamPayments is IPaymentBase {protocol/contracts/interfaces/templegold/ITempleGoldVesting.sol (3)
5-5: Remove unused import.IERC20 is imported but not used in the interface.
Apply this diff:
-import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
15-15: Clean up commented-out event.FundsOwnerSet is commented out. Delete if not part of the interface to prevent drift.
Apply this diff:
- // event FundsOwnerSet(address indexed _fundsOwner);
21-27: Revisit timestamp widths (uint32) vs uint40.Past feedback suggested using uint40 for timestamps. With uint32, overflow occurs around 2106. Aligning with IVestingPayments (uint40) would future-proof and reduce divergence.
If you want, I can produce a scoped migration plan (interfaces + impl + tests) in a follow-up.
protocol/contracts/admin/TeamPayments.sol (1)
244-250: Prevent double-allocation and totalAllocation drift for epoch payments.Currently, setEpochPayments can overwrite an existing recipient’s amount for the same epoch, inflating totalAllocation without reverting.
Apply this diff to add guards consistent with EpochPayments:
function _setEpochAllocation(uint256 _epoch, address _recipient, uint256 _amount) private { if (_amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); } if (_recipient == address(0)) { revert CommonEventsAndErrors.InvalidAddress();} - epochPayments[_recipient][_epoch] = _amount; + if (claimedEpochs[_recipient][_epoch]) { revert AlreadyClaimed(); } + if (epochPayments[_recipient][_epoch] != 0) revert EpochAlreadySet(); + epochPayments[_recipient][_epoch] = _amount; totalAllocation += _amount; emit EpochAllocationSet(_recipient, _epoch, _amount); }protocol/contracts/templegold/TempleGoldVesting.sol (8)
61-64: Avoid copying each schedule: use calldata reference inside the loopIndexing a
VestingSchedule[] calldataallows binding a per-iterationcalldatareference without copying to memory.Apply:
- VestingSchedule memory _schedule; + VestingSchedule calldata _schedule; for (uint256 i; i < _length; ++i) { _schedule = _schedules[i];
68-73: Sanity checks enforce a non-zero cliff and future start — confirm product requirements
cliff <= startdisallows a 0-second cliff;start < block.timestampdisallows “start now”. Both are fine if intentional, but they do constrain configurability.Do we explicitly want to forbid immediate start and zero cliff schedules? If not, consider relaxing to
cliff < start→cliff < startonly if you must, or change tocliff < start→ allowcliff == start. Likewise consider allowingstart <= block.timestamp. I can draft the exact diff once confirmed.
94-101: Revocation auto-releases vested amount — consider parity with VestingPaymentsHere you force-release vested funds on revoke. In VestingPayments, this path is intentionally disabled to avoid potential taxable events for recipients. Divergent behavior across sibling contracts is easy to miss by integrators.
Do we want TempleGoldVesting to mirror VestingPayments and not force-release on revoke? If yes, remove the auto-release block. I can provide a parity patch.
25-31: Minor nits: typos and formatting
- “Schdules” → “Schedules”.
- The doc on Line 29 has an extra indent.
- /// @notice Schdules for recipients + /// @notice Schedules for recipients - /// @notice Recipient vesting counts for generating IDs. An account can have multiple vesting schedules + /// @notice Recipient vesting counts for generating IDs. An account can have multiple vesting schedules
147-153: Index getter returns zero value for empty set — prefer explicit revert and OOB guardReturning a zero bytes32 on empty set is ambiguous. VestingPayments reverts with
NoVesting(). Also, callers get a generic OZ revert on out-of-bounds.Recommended:
- /// @dev revert with an array out-of-bounds access if only `id = _vestingIds.at(_index);` is used - if (_vestingIds.length() == 0) { return id; } - id = _vestingIds.at(_index); + uint256 len = _vestingIds.length(); + if (len == 0) { revert NoVesting(); } + if (_index >= len) { revert CommonEventsAndErrors.InvalidParam(); } + id = _vestingIds.at(_index);If
NoVesting()isn’t in this interface, swap to a repository-standard error.
191-194: isVestingRevoked reports true for unknown IDs — change to precise semanticsReturning
truefor a non-existent ID conflates “never existed/was removed” with “revoked”. This can mislead clients and on-chain integrations.Apply:
- if(!vestingIdExists(_vestingId)) { return true; } - return schedules[_vestingId].revoked; + if (!vestingIdExists(_vestingId)) { return false; } + return schedules[_vestingId].revoked;
210-220: Underflow on empty holder history in getLastVestingScheduleForHolder
holdersVestingCount[_recipient] - 1will underflow and revert when the count is zero. VestingPayments already guards this.Apply:
- function getLastVestingScheduleForHolder( - address _recipient - ) external view override returns (VestingSchedule memory) { - return - schedules[ - computeVestingScheduleIdForAddressAndIndex( - _recipient, - holdersVestingCount[_recipient] - 1 - ) - ]; - } + function getLastVestingScheduleForHolder( + address _recipient + ) external view override returns (VestingSchedule memory schedule) { + uint256 count = holdersVestingCount[_recipient]; + if (count == 0) { return schedule; } + return schedules[computeVestingScheduleIdForAddressAndIndex(_recipient, count - 1)]; + }
309-318: Critical: releasable math can exceed total amount after end — cap elapsedHere you use
(block.timestamp - start) / durationdirectly, which can exceed 1 when past the duration, enabling over-release. Use the same capped elapsed path as other functions.Apply:
- if ((block.timestamp <= _schedule.cliff) || _schedule.revoked) { - return 0; - } else { - uint256 _vested = TempleMath.mulDivRound(_schedule.amount, block.timestamp - _schedule.start, _schedule.duration, false); - return _vested - _schedule.distributed; - } + if ((block.timestamp <= _schedule.cliff) || _schedule.revoked) { + return 0; + } + uint256 _vested = _calculateTotalVestedAt(_schedule, uint32(block.timestamp)); + return _vested - _schedule.distributed;Please add/keep a unit test that warps far past
start + durationand asserts the release amount is capped.protocol/contracts/admin/VestingPayments.sol (1)
71-76: Same configurability question as TGLD: future-only start and non-zero cliffYou require
startstrictly in the future andcliff > start. Confirm this reflects product policy; otherwise we can allow immediate start and 0-second cliff.
🧹 Nitpick comments (34)
protocol/contracts/interfaces/admin/IPaymentBase.sol (2)
8-9: Index token in PaymentTokenSet for easier off-chain filtering.Consider indexing the token address to simplify event queries and subgraph filters.
- event PaymentTokenSet(address token); + event PaymentTokenSet(address indexed token);
31-37: Clarify expected access control in docs for mutators.These functions should be callable only by elevated roles (executor/admin). Please add natspec noting that implementations must enforce access control (e.g., onlyElevatedAccess/onlyExecutor). This helps prevent accidental exposure in future implementers.
Example:
- function setFundsOwner(address _fundsOwner) external; + /// @dev MUST be protected by elevated access in implementers. + function setFundsOwner(address _fundsOwner) external; ... - function recoverToken(address _token, address _to, uint256 _amount) external; + /// @dev MUST be protected by elevated access in implementers. + function recoverToken(address _token, address _to, uint256 _amount) external; ... - function setPaymentToken(address _token) external; + /// @dev MUST be protected by elevated access in implementers. + function setPaymentToken(address _token) external;protocol/contracts/admin/PaymentBase.sol (3)
50-52: Clamp elapsed-time helper to avoid underflow reverts and improve reuse.If _end < _start this reverts today. Safer to clamp to 0.
- function _getElapsedTime(uint32 _start, uint32 _end, uint32 _duration) internal pure returns (uint32) { - return _end - _start > _duration ? _duration : _end - _start; - } + function _getElapsedTime(uint32 _start, uint32 _end, uint32 _duration) internal pure returns (uint32) { + if (_end <= _start) return 0; + uint32 elapsed = _end - _start; + return elapsed > _duration ? _duration : elapsed; + }
35-38: Nit: validate non-zero recipient on token recovery.Avoid accidental burns: require _to != address(0). See diff above.
18-27: Doc nit: the “revoke approval from old fundsOwner” note is not actionable by the contract.The contract cannot revoke allowances on behalf of the old owner. Reword to indicate an operational runbook step or remove to prevent confusion.
Suggested wording: “Operational note: the previous fundsOwner should revoke token approvals granted to this contract.”
protocol/contracts/interfaces/admin/IEpochPayments.sol (3)
8-21: Remove commented-out legacy types for a clean interface.The commented Payment struct and events add noise and risk confusion. Please remove or move to design notes.
- // /// @notice Struct for team payment information - // struct Payment { - // /// @notice Start time for payment - // uint40 start; - // /// @notice Duration of payment - // uint40 duration; - // /// @notice Is payment valid or cancel - // bool isCancelled; - // /// @notice Amount claimed out of the total amount - // uint128 claimed; - // /// @notice Total payment amount - // uint128 amount; - // }
22-30: Align events and API surface.MinimumEpochDurationSet and NextEpochSet are emitted but there are no corresponding setters in this interface. If the intention is that implementations manage epoch progression internally, consider either:
- Adding the setters to this interface; or
- Dropping the events here and exposing them in a more appropriate admin interface.
31-37: Check unused/undocumented errors.Errors like CannotStartEpoch/EpochAlreadySet are defined here but not referenced by any function in this interface. Confirm they are used by the implementing contract’s public API; otherwise, prune to reduce surface area.
protocol/test/forge/unit/admin/TeamPayments.t.sol (8)
42-43: Fix test contract name typo.Typo in contract name: TeampPaymentsAccessTest → TeamPaymentsAccessTest. Consistency improves readability and avoids future copy/paste mistakes.
-contract TeampPaymentsAccessTest is TeamPaymentsTestBase { +contract TeamPaymentsAccessTest is TeamPaymentsTestBase {
55-61: Fix test name typo.Typo: test_acceess_setPayments → test_access_setPayments.
- function test_acceess_setPayments() public { + function test_access_setPayments() public {
315-317: Fix minor typos in comments (“smae”, “urnreleased”).Nit-level cleanup for clarity.
- // smae time 0 releasable + // same time, 0 releasable @@ - // smae time 0 releasable + // same time, 0 releasable @@ - // 0 urnreleased + // 0 unreleasedAlso applies to: 371-373, 279-281
464-475: Add negative test: unauthorized recoverToken must revert.We prove positive path only. Add a test to ensure only elevated roles can recover tokens.
Example:
+ function test_access_recover_token_team_payments() public { + uint256 amount = 1 ether; + deal(address(fakeToken), address(payment), amount, true); + vm.startPrank(unauthorizedUser); + vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidAccess.selector)); + payment.recoverToken(address(fakeToken), alice, amount); + }
102-111: Consider asserting invariants around paymentToken changes.Given the risks of switching tokens mid-flight, consider adding tests that:
- Prohibit setPaymentToken when totalAllocation > 0; or
- Assert no state/accounting regressions if you intend to allow it.
I can draft the test after you decide the policy.
477-483: Remove unused local variable.start is never used. Drop it to clear warnings.
- uint256 start = block.timestamp; - - ITeamPayments.Payment memory _payment =_createPayment(); + ITeamPayments.Payment memory _payment = _createPayment();
503-507: Remove unused local variable.period is not used in this test.
- uint256 period = 56 weeks; - uint256 amount = 1_000 ether;
531-542: DRY: local time helpers duplicate PaymentBase logic.Not harmful, but you can inline the same clamp logic as proposed in PaymentBase to keep calculations consistent across code and tests.
protocol/contracts/interfaces/admin/ITeamPayments.sol (2)
28-36: Clarify ‘Payment’ amount semantics in docs.Explicitly state that amount is the total amount linearly vested over duration, denominated in the payment token’s smallest unit.
- * @param _payments Amounts array to set + * @param _payments Array of payment schedules. Each schedule's `amount` is the TOTAL to be released linearly over `duration` (in token units).
88-95: Fix typo “provieed” and prefer uint32 for ‘at’ to match struct width (optional).
- Typo fix.
- Since start/duration are uint32, consider uint32 for at. If you expect far-future timestamps or external callers passing uint256, keeping uint256 is fine.
- * @notice Get claimable payment at a provieed time + * @notice Get claimable payment at a provided time - function getClaimablePaymentAt(address recipient, uint256 at) external view returns (uint256); + function getClaimablePaymentAt(address recipient, uint256 at) external view returns (uint256);protocol/test/forge/unit/templegold/TempleGoldVesting.t.sol (3)
390-391: Remove duplicate assertion.holdersVestingCount(alice) is asserted twice consecutively.
Apply this diff:
- assertEq(vesting.holdersVestingCount(alice), 0); + // already asserted above
448-456: Strengthen event assertions by scoping the emitter.When expecting Released, consider specifying the emitting contract to avoid false positives if multiple contracts emit similarly named events.
Example:
vm.expectEmit(address(vesting)); emit Released(_id, alice, releasable);
16-25: Add coverage for RecipientChanged flow.RecipientChanged is declared but not exercised. Add a test that changes a recipient mid-vesting and verifies ownership transfer for subsequent release.
I can draft a test that: creates a schedule, warps past cliff, changes recipient to a new EOA, asserts only the new recipient can release, and validates event fields. Want me to add it?
protocol/contracts/interfaces/admin/IVestingPayments.sol (4)
19-34: Token-agnostic interface should not reference TGLD in comments.This interface is generic, but comments mention TGLD specifically. This can mislead downstream users.
Apply this diff to generalize:
- /// @notice Amount of TGLD for whole vesting + /// @notice Amount of payment token for whole vesting ... - /// @notice Amount of TGLD distributed to recipient + /// @notice Amount of payment token distributed to recipient
36-45: Remove commented-out API surface or implement it.VestingSummary.vestedAtEnd is commented out. Either include it and wire it through getVestingSummary, or remove to keep the interface clean.
Apply this diff to remove the dead code:
- // /// @notice total vested at end time of summary - // uint128 vestedAtEnd;
1-11: Consider standardizing timestamp widths (uint40) across vesting interfaces.This interface uses uint40 for time fields, while ITempleGoldVesting uses uint32. The inconsistency increases cognitive load and complicates shared tooling. Using uint40 everywhere avoids the 2106 problem.
If aligning is feasible, migrate TempleGold vesting fields to uint40 in a follow-up PR to avoid breaking this one.
141-148: Name alignment with TempleGold interface.Function isActiveVestingId() mirrors vestingIdExists() in ITempleGoldVesting. Consider unifying naming to reduce confusion.
If you prefer the “exists” naming, rename here and in the implementation, adding a temporary alias for backwards compatibility if needed.
protocol/contracts/admin/EpochPayments.sol (2)
86-106: Optional: Guard against zero minEpochDuration or document behavior.If minEpochDuration is zero (default), admins can advance epochs every call. Either require a non-zero value to be set before first call or document that zero means “no gating.”
Minimal guard:
function setEpochPayments( address[] calldata recipients, uint256[] calldata amounts ) external override onlyElevatedAccess { + if (minEpochDuration == 0 && currentEpoch != 0) { revert CommonEventsAndErrors.InvalidParam(); } // attempt to start next epoch uint256 epoch = _startNextEpoch();
14-15: Remove unused import.TempleMath is not used in this contract.
Apply this diff:
-import { TempleMath } from "contracts/common/TempleMath.sol";protocol/contracts/admin/TeamPayments.sol (2)
238-242: Use a more appropriate revert for “existing payment already set.”Reverting with InvalidAddress() is misleading for the “already has a payment” case.
Apply this diff:
- if (payments[_recipient].amount != 0) { revert CommonEventsAndErrors.InvalidAddress(); } + if (payments[_recipient].amount != 0) { revert CommonEventsAndErrors.InvalidParam(); }
71-73: Prefer explicit uint256 loop counters.Use uint256 for loop indices for consistency with Solidity style and to avoid implicit aliasing of uint.
Apply this diff:
- for (uint i; i < _length; ++i) { + for (uint256 i; i < _length; ++i) {Also applies to: 90-92
protocol/contracts/templegold/TempleGoldVesting.sol (1)
78-81: Duplicate ID check is effectively redundant
keccak256(abi.encodePacked(holder, index))with a monotonically increasing per-holder index cannot collide for the same holder. TheEnumerableSet.addguard is fine, but the comment “very low possibility of duplicate” is misleading.- /// @dev very low possibility of duplicate vesting IDs generated but still checking. + /// @dev IDs are deterministic per (holder, index); add() is a defensive guard.protocol/contracts/admin/VestingPayments.sol (4)
145-152: Strengthen index validation and improve UXYou already revert on zero length with
NoVesting(). Add an explicit out-of-bounds guard to avoid opaque OZ EnumerableSet errors.- /// @dev revert if zero vesting Ids length - if (_activeVestingIds.length() == 0) { revert NoVesting(); } - /// @dev revert with an array out-of-bounds access if only `id = _activeVestingIds.at(_index);` is used - id = _activeVestingIds.at(_index); + uint256 len = _activeVestingIds.length(); + if (len == 0) { revert NoVesting(); } + if (_index >= len) { revert CommonEventsAndErrors.InvalidParam(); } + id = _activeVestingIds.at(_index);
65-67: Micro-gas: avoid redundant i++ checks inside for-loopOptional: switch to manual increment with unchecked block to save gas in the creation loop. Clear, measurable when creating many schedules.
- for (uint256 i; i < _length; ++i) { + for (uint256 i; i < _length;) { _schedule = _schedules[i]; ... + unchecked { ++i; } }
29-31: Minor typo“Schdules” → “Schedules”.
- /// @notice Schdules for recipients + /// @notice Schedules for recipients
308-315: Type consistency: _releaseTime width differs across sibling contractsHere
_releaseTimeisuint40, while TempleGoldVesting usesuint32in analogous code. Not a bug, but consider unifying to avoid casts and keep interfaces symmetrical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/PaymentBase.sol(1 hunks)protocol/contracts/admin/TeamPayments.sol(1 hunks)protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IPaymentBase.sol(1 hunks)protocol/contracts/interfaces/admin/ITeamPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/contracts/interfaces/templegold/ITempleGoldVesting.sol(1 hunks)protocol/contracts/templegold/TempleGoldVesting.sol(1 hunks)protocol/test/forge/unit/admin/TeamPayments.t.sol(1 hunks)protocol/test/forge/unit/templegold/TempleGoldVesting.t.sol(1 hunks)
🔇 Additional comments (8)
protocol/contracts/admin/EpochPayments.sol (1)
156-171: Good: revokeEpochPayment keeps the epoch reusable.Not marking claimed on revoke allows admins to re-set the allocation later. This addresses earlier feedback about reusability.
protocol/contracts/admin/TeamPayments.sol (1)
171-184: LGTM: Claim logic validates and updates state correctly.Claim path checks cancellation, existence, and releasable amount, updates claimed and totalClaimed, then transfers via SafeERC20. Event fields look correct.
protocol/contracts/templegold/TempleGoldVesting.sol (4)
125-127: Override of setPaymentToken is pure-revert — verify base signature and intentOverriding with
pureand an unconditional revert is unusual. Ensure this exactly matches the base signature in PaymentBase (visibility, mutability, and custom errors). If the goal is to forbid token changes, consider overriding withexternal onlyElevatedAccessthat reverts with a domain-specific error.Would you like me to generate a small patch to align the signature and emit a clearer custom error?
159-165: Release flow updates state before external call — good orderingYou increment
distributedand decrementtotalVestedAndUnclaimedbeforesafeTransferFrom, which provides reentrancy resistance without a guard. The recipient check in_validateReleaseblocks arbitrary reentry via token hooks.
34-42: Constructor bypasses PaymentBase constructor — align initialization pathsVestingPayments invokes
PaymentBase(_paymentToken), while here you setpaymentTokenandfundsOwnerdirectly. If PaymentBase emits events or enforces invariants in its constructor, those are skipped here.If PaymentBase has a token-accepting constructor, prefer:
- ) TempleElevatedAccess(_rescuer, _executor){ - fundsOwner = _fundsOwner; - paymentToken = IERC20(_paymentToken); - } + ) TempleElevatedAccess(_rescuer, _executor) PaymentBase(_paymentToken) { + _setFundsOwner(_fundsOwner); + }Confirm exact base API names; I can tailor the patch once you confirm PaymentBase’s constructor and setter signatures.
266-275: Summary window filters only schedules fully inside [_from, _to] — confirm intentYou exclude schedules that overlap the window at either edge. Depending on analytics needs, clients may expect overlapping schedules to be included with partial accruals.
If inclusion of overlaps is desired, I can propose a bounded-interval accrual helper and update the loop accordingly.
protocol/contracts/admin/VestingPayments.sol (2)
241-246: Consistent elapsed-time capping — goodBoth the “current time” and
_atpaths go through_calculateTotalVestedAtwhich caps elapsed via_getElapsedTime. This avoids over-release after maturity.Also applies to: 308-315
317-321: Release flow ordering and eventing — solidState is updated before token transfer; Released event carries the caller. Matches best practices and aligns with the FundsOwner allowance model.
Also applies to: 158-165
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
protocol/contracts/interfaces/admin/IPaymentBase.sol (3)
8-10: Remove PaymentTokenSet event (paymentToken is now immutable).With paymentToken immutable and no setter in the interface, PaymentTokenSet is misleading dead weight. Drop it or, if you really want an audit trail, emit it once in the constructor of implementors—right now there's no place to emit it.
Apply this diff:
- event PaymentTokenSet(address token);
11-11: Drop NotImplemented error from the interface.There are no interface methods that reference or use NotImplemented(). Keeping it here invites confusion and unused-code drift.
- error NotImplemented();
19-31: Clarify access-control expectations in NatSpec for mutators.Interface methods setFundsOwner and recoverToken should be guarded by elevated access in implementations. Encode this expectation in the NatSpec to prevent accidental exposure during implementation.
- * @notice Set funds owner + * @notice Set funds owner + * @dev MUST be access controlled (e.g., onlyElevatedAccess) in implementors. @@ - * @notice Recover ERC20 token + * @notice Recover ERC20 token + * @dev MUST be access controlled (e.g., onlyElevatedAccess) in implementors.protocol/contracts/admin/PaymentBase.sol (2)
27-32: _setFundsOwner internalization is good; consider zero-cost early return on no-op set.Not critical, but skipping state write and event when the value is unchanged can reduce noise and gas in admin workflows.
function _setFundsOwner(address _fundsOwner) internal { if (_fundsOwner == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); } /// @dev Elevated access should revoke approval from old `fundsOwner` for this contract - fundsOwner = _fundsOwner; - emit FundsOwnerSet(_fundsOwner); + if (fundsOwner != _fundsOwner) { + fundsOwner = _fundsOwner; + emit FundsOwnerSet(_fundsOwner); + } }
40-43: Harden _recoverToken with basic argument checks.Optional but defensive: prevent burning to address(0) and meaningless zero-amount transfers; both catch misconfigurations early.
function _recoverToken(address _token, address _to, uint256 _amount) internal { + if (_to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); } + if (_amount == 0) { revert CommonEventsAndErrors.InvalidAmount(); } emit CommonEventsAndErrors.TokenRecovered(_to, _token, _amount); IERC20(_token).safeTransfer(_to, _amount); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/PaymentBase.sol(1 hunks)protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IPaymentBase.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- protocol/contracts/interfaces/admin/IEpochPayments.sol
- protocol/contracts/admin/VestingPayments.sol
- protocol/contracts/interfaces/admin/IVestingPayments.sol
- protocol/contracts/admin/EpochPayments.sol
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.218Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
📚 Learning: 2025-08-21T19:18:05.218Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.218Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
Applied to files:
protocol/contracts/interfaces/admin/IPaymentBase.solprotocol/contracts/admin/PaymentBase.sol
🔇 Additional comments (2)
protocol/contracts/admin/PaymentBase.sol (2)
16-21: Nice: paymentToken is immutable and set in the constructor.This aligns with prior feedback to avoid mutable payment tokens. Good improvement in the trust model.
10-18: All PaymentBase inheritors correctly implement guarded externals and initialize fundsOwnerI’ve verified that each contract inheriting PaymentBase in protocol/contracts
- Implements setFundsOwner(...) and recoverToken(...) overrides
- Applies onlyElevatedAccess on those externals
- Assigns fundsOwner in its constructor
Specifically:
• VestingPayments.sol — setFundsOwner & recoverToken present with onlyElevatedAccess, fundsOwner initialized (line 44)
• EpochPayments.sol — setFundsOwner & recoverToken present with onlyElevatedAccess, fundsOwner initialized (line 51)
• TempleGoldVesting.sol — setFundsOwner & recoverToken present with onlyElevatedAccess, fundsOwner initialized (line 40)No further changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (17)
protocol/contracts/interfaces/admin/IVestingPayments.sol (17)
1-3: Put SPDX identifier first (before pragma).Most linters and SPDX tooling expect the license identifier as the very first line. Reordering avoids formatting warnings in CI.
Apply:
-pragma solidity ^0.8.20; // SPDX-License-Identifier: AGPL-3.0-or-later +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.20;
8-15: Index both old and new recipients in RecipientChanged.Indexing the previous recipient aids log filtering and off-chain indexing without adding runtime cost.
-event RecipientChanged(bytes32 _vestingId, address _oldRecipient, address indexed _recipient); +event RecipientChanged(bytes32 _vestingId, address indexed _oldRecipient, address indexed _recipient);
16-20: Disambiguate/align error taxonomy with CommonEventsAndErrors.Given the codebase uses CommonEventsAndErrors elsewhere, consider aligning error names or documenting throw conditions. E.g., CannotRelease is ambiguous (nothing vested vs. not recipient vs. revoked). Either clarify via comments or split into more specific errors (NothingToRelease, OnlyRecipient, VestingRevoked). Keeps client integrations predictable.
If you prefer not to add new errors, please add NatSpec “@dev throws when …” notes at call sites in the implementation for consistency with existing error docs.
21-36: Make interface docs token-agnostic; minor storage packing nit.
- The struct comments reference “TGLD”. Since this is the generic IVestingPayments, use token-agnostic phrasing to avoid coupling to a specific asset.
- Optional micro-optimization: place
bool revokedadjacent touint128 distributedso they share a slot (both fit with 128+8), instead of cohabiting withaddress, but note you’ll still consume 3 slots either way. Not critical.Doc-only tweak:
- /// @notice Amount of TGLD for whole vesting + /// @notice Total token amount allocated for this vesting schedule uint128 amount; - /// @notice Amount of TGLD distributed to recipient + /// @notice Amount already released to the recipient uint128 distributed; - /// @notice Recipient of vesting + /// @notice Vesting recipient (beneficiary) address recipient;If you want to adopt the packing nit:
- /// @notice Recipient of vesting - address recipient; - /// @notice If vesting is revoked - bool revoked; + /// @notice If vesting is revoked + bool revoked; + /// @notice Recipient of vesting + address recipient;
38-47: Tighten VestingSummary docs and consider type consistency.
- Doc phrasing: “distributed tgld” → generic, capitalized.
- Consider whether
vestedshould beuint256to align with getters returninguint256(e.g., getTotalVestedAt). Today it’s safe (per-schedule cap = amount uint128), but the mixed types force casts in clients.Doc-only tweak:
- /// @notice distributed tgld + /// @notice Amount already released to the recipient uint128 distributed; - /// @notice total vested at current block.timestamp + /// @notice Total vested at the current block timestamp (may exceed distributed) uint128 vested;Optional API consistency tweak:
- uint128 vested; + uint256 vested;Note: this is a breaking change for downstream users; only do it if you want strict consistency with the query functions.
55-57: Naming consistency: “holder” vs “recipient”.The interface alternates between “holder” and “recipient” for the same concept. Pick one term across functions and parameter names to reduce cognitive overhead (e.g., “recipient” everywhere).
If you keep both terms, please add a brief comment at the top explaining they are synonymous in this context.
83-86: Parameter name mismatches interface wording.Function name uses “…ForHolder” but the parameter is
_recipient. Align for clarity.- function computeNextVestingScheduleIdForHolder( - address _recipient + function computeNextVestingScheduleIdForHolder( + address holder ) external view returns (bytes32);
94-97: Document zero-based indexing and bounds.Given the swap-and-pop note, clarify whether
_indexis zero-based and that it must be < current length to avoid confusion for integrators.
98-103: Release authorization semantics: doc the expected caller.NatSpec says “Caller must be recipient.” If admins can also force-release (common in vesting systems), either add an admin release function or explicitly state that only the beneficiary can call this.
Would you like me to draft an
adminRelease(bytes32 _vestingId, address to)variant for emergency recoveries?
104-111: Consider exposing the ID computation salt/algorithm.
computeVestingScheduleIdForAddressAndIndexispure, which is good. Ensure the implementation uses the same formula on-chain (e.g., keccak256(holder, index, domainSeparator/version)). If not, add a doc line describing the exact encoding to prevent mismatches.
117-120: “Last vesting schedule” definition can be ambiguous.Is “last” the highest index, most recently created, or last active (non-revoked)? Please clarify in NatSpec and, if necessary, rename to
getLatestVestingScheduleForHolder.
145-147: Use calldata for external array parameter.Saves a memory copy for external calls.
- function getVestingSummary( - bytes32[] memory _ids - ) external view returns (VestingSummary[] memory summary); + function getVestingSummary( + bytes32[] calldata _ids + ) external view returns (VestingSummary[] memory summary);
149-154: Consider adding a global count getter.You expose
getVestingIds()(which can be heavy) andholdersVestingCount(holder). A lightweightgetVestingCount()can help indexers/UIs without pulling the whole set.If helpful, I can propose a minimal EnumerableSet-backed signature for this.
160-168: Releasable amount vs. revoked flows.You surface
revokedAccountsReleasable(account)but only a per-IDrelease. If the intended UX is “accumulate revoked releasables and allow a single claim,” consider addingreleaseRevoked()orreleaseAll(account)for gas efficiency and simpler UX. Otherwise, document how a user claims therevokedAccountsReleasableamount.I can draft an interface addition if that matches product requirements.
169-176: Type alignment between getters and summary.Getters return
uint256while the summary usesuint128. Safe today (per-schedule cap =amount), but client code will cast. Consider returninguint256inVestingSummaryor switch getters touint128. Pick one style and stick with it.
132-139: RenamegetTotalVestingAtCurrentTimefor naming consistencyTo avoid confusion between “vesting” and “vested,” please rename the function across the interface and all implementations:
• In
protocol/contracts/interfaces/admin/IVestingPayments.sol(around line 138), change:- function getTotalVestingAtCurrentTime(bytes32 _vestingId) external view returns (uint256); + function getTotalVestedAtCurrentTime(bytes32 _vestingId) external view returns (uint256);• In
protocol/contracts/admin/VestingPayments.sol(around line 204), update the override and its callers:- function getTotalVestingAtCurrentTime(bytes32 _vestingId) external view override returns (uint256) { + function getTotalVestedAtCurrentTime(bytes32 _vestingId) external view override returns (uint256) { VestingSchedule memory _schedule = schedules[_vestingId]; - return _getTotalVestingAtCurrentTime(_schedule); + return _getTotalVestingAtCurrentTime(_schedule); }• In
protocol/contracts/templegold/TempleGoldVesting.sol(around line 240), update the override signature:- function getTotalVestingAtCurrentTime(bytes32 _vestingId) external view override returns (uint256) { + function getTotalVestedAtCurrentTime(bytes32 _vestingId) external view override returns (uint256) {• In
protocol/contracts/interfaces/templegold/ITempleGoldVesting.sol(around line 126), update the interface signature similarly.• Update any external callers (tests, scripts, other contracts) that reference
getTotalVestingAtCurrentTime.This renaming is purely cosmetic and backward-breaking; ensure you coordinate updates across all dependents (including auto-generated artifacts like
slither.db.json).
1-183: Standardize Solidity version and pin compiler for determinismThe repository currently relies on
auto_detect_solc = trueinprotocol/foundry.toml, allowing Forge to pick any compatible 0.8.x compiler that satisfies each file’s pragma. However, our grep acrosscontracts/andtest/surfaces a mix of pragmas—^0.8.0,^0.8.4,^0.8.18,^0.8.20, and^0.8.22. While this works under auto-detection, it can lead to subtle discrepancies between environments and hinders reproducible builds.I recommend:
- Choose a single baseline pragma (for example, upgrade all files to
^0.8.20, or bump to^0.8.22/^0.8.24to leverage the latest patches).- Update every contract, interface, mock, and test to use that chosen pragma version.
- In
protocol/foundry.toml, replaceauto_detect_solc = truewith an explicitsolc_version = "0.8.xx"to lock in the compiler.- Run
forge fmtand re-run the full test suite to verify consistency under the pinned compiler.This will ensure all contributors compile against the exact same Solidity version and avoid any “works on my machine” scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/contracts/admin/VestingPayments.sol
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.218Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
protocol/test/forge/unit/admin/VestingPayments.t.sol (3)
685-686: Remove duplicate assertion in test_computeNextVestingScheduleIdForHolder.Line 686 is a duplicate of line 685, providing no additional test coverage.
Apply this diff to remove the duplicate:
assertEq(vesting.holdersVestingCount(alice), 0); - assertEq(vesting.holdersVestingCount(alice), 0);
73-83: Consider adding more initialization validation tests.The initialization tests could be more comprehensive by validating edge cases like invalid addresses for rescuer/executor and testing the immutability of paymentToken.
Would you like me to generate additional initialization test cases to improve coverage, such as testing invalid constructor parameters for rescuer and executor addresses?
397-927: Consider organizing tests into more focused test contracts.The
VestingPaymentsTestcontract is quite large (530+ lines) and tests multiple aspects. Consider splitting it into more focused test contracts likeVestingPaymentsCreateTest,VestingPaymentsRevokeTest, andVestingPaymentsReleaseTestfor better organization and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/contracts/interfaces/admin/IVestingPayments.sol
- protocol/contracts/admin/VestingPayments.sol
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
🔇 Additional comments (3)
protocol/test/forge/unit/admin/VestingPayments.t.sol (3)
16-171: LGTM! Well-structured test base setup.The test base contract is well-organized with proper initialization of dependencies, event definitions, and helper functions. The setup correctly configures TempleGold with distribution parameters and contract whitelisting.
173-222: LGTM! Comprehensive access control testing.The access control tests properly validate both failure and success cases for all elevated access functions.
224-395: LGTM! Thorough view function testing.The view function tests comprehensively cover edge cases including zero vesting counts, pre-cliff periods, post-cliff calculations, and revoked schedules.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
protocol/contracts/admin/EpochPayments.sol (1)
153-162: Epoch start gate off-by-one; equality should pass.Revert only when now < start + minDuration. Current ‘>=’ blocks at the boundary.
- if (currentEpochCache != 0 && startTime + minEpochDuration >= block.timestamp) { revert CannotStartEpoch(currentEpochCache+1); } + if (currentEpochCache != 0 && block.timestamp < startTime + minEpochDuration) { + revert CannotStartEpoch(currentEpochCache + 1); + }protocol/test/forge/unit/admin/VestingPayments.t.sol (5)
794-811: Remove dead/unreachable block in test_release_no_amount.The first block’s values are overwritten immediately.
- // revoke and release - skip(1 weeks); - vm.startPrank(executor); - balance = templeGold.balanceOf(alice); - totalVestedAndUnclaimed = vesting.totalVestedAndUnclaimed(); - releasable = vesting.getReleasableAmount(_id); - - skip(3 weeks); + // Continue testing release after more time + skip(4 weeks);
115-117: Use uint40 for time fields to avoid truncation in _getScheduleTwo.Current uint32 casts risk silent truncation; keep widths consistent with VestingSchedule (uint40).
- uint40 start = uint32(block.timestamp) + 1 weeks; - uint40 cliff = uint32(start + 16 weeks); + uint40 start = uint40(block.timestamp) + 1 weeks; + uint40 cliff = uint40(start + 16 weeks);
130-132: Fix time field widths in _getScheduleThree.These should be uint40 to match the struct and avoid narrowing.
- uint32 start = uint32(block.timestamp) + 4 weeks; - uint32 cliff = uint32(start + 12 weeks); - uint32 duration = 56 weeks; + uint40 start = uint40(block.timestamp) + 4 weeks; + uint40 cliff = uint40(start + 12 weeks); + uint40 duration = 56 weeks;
439-444: Correct start-time casts during schedule validation.Use uint40, not uint32.
- _schedule.start = uint32(block.timestamp - 1); + _schedule.start = uint40(block.timestamp - 1); schedules[0] = _schedule; vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidParam.selector)); vesting.createSchedules(schedules); - _schedule.start = uint32(block.timestamp); + _schedule.start = uint40(block.timestamp);
845-851: Populate _ids and assert zero releasable before cliff.The test computes releasable but doesn’t assert it, and leaves _ids[0] unset.
// alice: skip to below cliff skip(_schedule1.cliff-1); bytes32[] memory _ids = new bytes32[](2); - uint256 releasable = vesting.getReleasableAmount(_id1); - IVestingPayments.VestingSummary[] memory summary = vesting.getVestingSummary(_ids); + _ids[0] = _id1; + uint256 releasable = vesting.getReleasableAmount(_id1); + assertEq(releasable, 0, "Should have zero releasable before cliff"); + IVestingPayments.VestingSummary[] memory summary = vesting.getVestingSummary(_ids);
🧹 Nitpick comments (14)
protocol/contracts/interfaces/admin/IEpochPayments.sol (2)
14-19: Remove or use EpochAlreadySet.The error is declared but unused in the implementation. Either wire it into the “initial set-only” path or drop it from the interface to avoid API noise.
31-39: Expose setMinimumEpochDuration in the interface.You emit MinimumEpochDurationSet and the implementation exposes setMinimumEpochDuration, but the interface does not. Add it so integrators can rely on the API surface via IEpochPayments.
Apply this diff to IEpochPayments.sol:
@@ function updateEpochPayments( address[] calldata recipients, uint256[] calldata amounts ) external; + /** + * @notice Set minimum duration of an epoch + * @param duration Minimum duration in seconds + */ + function setMinimumEpochDuration(uint256 duration) external; +protocol/contracts/admin/EpochPayments.sol (4)
17-21: Docs mismatch: “Team Payments” vs EpochPayments.Update the natspec title/notice to reflect this is the epoch-based payments contract.
- * @title Team Payments - * @notice New team payments contract + * @title Epoch Payments + * @notice Discrete, admin-managed epoch allocations and claims
129-132: Optional: add reentrancy guard to claimEpoch.State updates happen before transfer which is good, but adding nonReentrant is a cheap hardening.
Additional changes outside this hunk:
@@ -import { TempleMath } from "contracts/common/TempleMath.sol"; +import { TempleMath } from "contracts/common/TempleMath.sol"; +import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; @@ -contract EpochPayments is IEpochPayments, PaymentBase, TempleElevatedAccess { +contract EpochPayments is IEpochPayments, PaymentBase, TempleElevatedAccess, ReentrancyGuard { @@ - function claimEpoch(uint256 epoch) external override { + function claimEpoch(uint256 epoch) external override nonReentrant {
36-44: Nit: clarify comments.“Epoch payments. variable payments” and “Epochs” are vague. Prefer “Per-recipient per-epoch allocations” and “Epoch start timestamps.”
- /// @notice Epoch payments. variable payments + /// @notice Per-recipient per-epoch allocations @@ - /// @notice Epochs + /// @notice Epoch start timestamps
73-91: Add duplicate-recipient handling tests forupdateEpochPayments.To ensure robust batch semantics in
EpochPayments.sol, expand the test suite with scenarios covering duplicate recipients:
- Create a new test file (e.g.
protocol/test/forge/unit/admin/EpochPayments.t.sol) if one doesn’t already exist.- Single-call duplicates
– CallupdateEpochPayments([alice, alice], [amt, amt])and assert the expected behavior (revert withAlreadyClaimed, sum allocations, or other designed outcome).- Successive-update duplicates
– CallupdateEpochPayments([alice], [amt1]), then again withupdateEpochPayments([alice], [amt2])in the same epoch; verify consistency with intended logic (e.g., replacement vs. accumulation vs. revert).- Edge cases
– Mix duplicates and unique addresses in a single batch.
– Zero-amount entries for duplicated recipients (should revert viaExpectedNonZeroin_setEpochAllocation).protocol/test/forge/unit/admin/VestingPayments.t.sol (8)
675-679: Scope expectEmit to the vesting contract.Improves correctness and noise-resilience.
- vm.expectEmit(); + vm.expectEmit(address(vesting)); emit CommonEventsAndErrors.TokenRecovered(alice, address(fakeToken), amount); vm.startPrank(executor);
685-686: Remove duplicate assertion.- assertEq(vesting.holdersVestingCount(alice), 0); assertEq(vesting.holdersVestingCount(alice), 0);
335-336: Fix off-by-one in comment to match 29-week skip.- // 1 week + (cliff - start)) + 11 weeks + // 1 week + (cliff - start) + 12 weeks = 29 weeks total → 28 weeks vested since start skip(29 weeks);
188-188: Avoid bytes32(bytes(...)) casts; use clear literals.Use bytes32(0) and bytes32("...") for clarity and to avoid dynamic bytes casts.
- vesting.revokeVesting(bytes32(bytes(""))); + vesting.revokeVesting(bytes32(0));- vesting.revokeVesting(bytes32(bytes(""))); + vesting.revokeVesting(bytes32(0));- vesting.release(bytes32(bytes(""))); + vesting.release(bytes32(0)); - vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidParam.selector)); - vesting.release(bytes32(bytes("0x123"))); + vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidParam.selector)); + vesting.release(bytes32("0x123"));Also applies to: 606-607, 733-737
247-250: Index 0 is a clearer sentinel for empty vesting IDs.Minor readability improvement.
- vesting.getVestingIdAtIndex(1); + vesting.getVestingIdAtIndex(0);
33-37: Remove unused fork ID variable.Keeps tests lean; the fork is still created.
- uint256 public arbitrumOneForkId; - function setUp() public { - arbitrumOneForkId = fork("arbitrum_one"); + fork("arbitrum_one");
281-287: Also assert zero releasable exactly at start.Matches the test name “less_than_equal_to_start”.
// go to less than start time vm.warp(schedule.start-1); assertEq(vesting.getReleasableAmount(ids[0]), 0); + // exactly at start + vm.warp(schedule.start); + assertEq(vesting.getReleasableAmount(ids[0]), 0);
706-708: Assert inactive after revoke.Strengthens the invariant being tested.
// revoke immediately vesting.revokeVesting(_id); assertEq(vesting.isVestingRevoked(_id), true); + assertEq(vesting.isActiveVestingId(_id), false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
📚 Learning: 2025-08-21T19:18:05.268Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
Applied to files:
protocol/contracts/admin/EpochPayments.sol
🔇 Additional comments (5)
protocol/contracts/interfaces/admin/IEpochPayments.sol (1)
7-13: Interface surface and event set look solid.Event coverage is appropriate for off-chain indexing and admin ops.
protocol/contracts/admin/EpochPayments.sol (3)
54-57: LGTM: setFundsOwner override defers to PaymentBase.Consistent with PaymentBase mutability change for paymentToken; no setPaymentToken surface here.
102-117: LGTM: revoke path validates claim status and adjusts accounting.With the delta fix in _setEpochAllocation, revoke remains accurate.
59-71: LGTM: epoch creation plus initial allocations flow.Starting next epoch before setting allocations is sane; array length checks are correct.
protocol/test/forge/unit/admin/VestingPayments.t.sol (1)
16-71: Test harness initialization looks solid.Setup wires contracts, permissions, and whitelisting correctly; good coverage baseline.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (17)
protocol/contracts/interfaces/admin/IVestingPayments.sol (5)
81-84: Param naming mismatch with function nameFunction says “ForHolder” but param is
_recipient. Align for clarity.- function computeNextVestingScheduleIdForHolder( - address _recipient - ) external view returns (bytes32); + function computeNextVestingScheduleIdForHolder( + address _holder + ) external view returns (bytes32);
47-49: Consistent param naming (use leading underscore throughout)Unify style to reduce friction in tooling/ABIs generated from NatSpec.
- function revokedAccountsReleasable(address account) external view returns (uint256); + function revokedAccountsReleasable(address _account) external view returns (uint256); - function holdersVestingCount(address holder) external view returns (uint256); + function holdersVestingCount(address _holder) external view returns (uint256); - function computeVestingScheduleIdForAddressAndIndex( - address holder, - uint256 index - ) external pure returns (bytes32); + function computeVestingScheduleIdForAddressAndIndex( + address _holder, + uint256 _index + ) external pure returns (bytes32); - function getVestingScheduleByAddressAndIndex( - address recipient, - uint256 index - ) external view returns (VestingSchedule memory); + function getVestingScheduleByAddressAndIndex( + address _recipient, + uint256 _index + ) external view returns (VestingSchedule memory);Also applies to: 53-55, 105-109, 120-129
130-137: Name consistency: “Vesting” vs “Vested”APIs mix “Vesting” and “Vested”. Prefer “Vested” for amounts accrued by time. Consider renaming for clarity. Breaking change if applied now.
- function getTotalVestingAtCurrentTime(bytes32 _vestingId) external view returns (uint256); + // Consider renaming for consistency with getTotalVestedAt(...) + function getTotalVestedAtCurrentTime(bytes32 _vestingId) external view returns (uint256);Also applies to: 167-174
151-152: Consider pagination for vesting ids to avoid heavy returnsReturning all IDs can be large and brittle for integrators. Add a paginated variant while keeping the existing method.
Proposed additional signature (new method):
function getVestingIds(uint256 _offset, uint256 _limit) external view returns (bytes32[] memory ids);
38-45: Optional: widen VestingSummary.vested to uint256Most getters return uint256 amounts; widening avoids repeated casting for clients. Not required functionally since amount is uint128.
- uint128 vested; + uint256 vested;protocol/test/forge/unit/admin/EpochPayments.t.sol (5)
48-48: Typo in contract name.Rename
TeampPaymentsAccessTesttoTeamPaymentsAccessTest(orEpochPaymentsAccessTest) for clarity.-contract TeampPaymentsAccessTest is EpochPaymentsTestBase { +contract TeamPaymentsAccessTest is EpochPaymentsTestBase {
393-404: Strengthen assertions: verify totalAllocation delta on update.After updating Alice’s epoch-1 allocation from 1e18 to 3e18,
totalAllocationshould reflect the +2e18 delta (or equal new total if no other allocations exist). Add an assertion.function test_updateEpochAllocations_update_allocation() public { test_setEpochPayments_first_epoch(); + uint256 totalBefore = payment.totalAllocation(); address[] memory recipients = new address[](1); uint256[] memory amounts = new uint256[](1); amounts[0] = 3e18; recipients[0] = alice; assertEq(payment.epochPayments(alice, 1), 1e18); vm.expectEmit(address(payment)); emit EpochAllocationSet(alice, 1, 3e18); payment.updateEpochPayments(recipients, amounts); assertEq(payment.epochPayments(alice, 1), 3e18); + assertEq(payment.totalAllocation(), totalBefore - 1e18 + 3e18); }
160-161: Whitespace nit.Extra leading space before
assertEqon Line 160; remove for consistency.- assertEq(payment.claimedEpochs(mike, 1), true); + assertEq(payment.claimedEpochs(mike, 1), true);
512-523: Test name mismatch.Rename
test_recover_token_team_paymentstotest_recover_token_epoch_paymentsto match the subject under test.-function test_recover_token_team_payments() public { +function test_recover_token_epoch_payments() public {
83-86: Close pranks or prefer vm.prank for single calls.Several tests start pranks but don’t stop them. While Foundry resets between tests, local clarity improves with
vm.stopPrank()or usingvm.prank(...)for single-call cases.Example:
-vm.startPrank(executor); -payment.setFundsOwner(alice); +vm.prank(executor); +payment.setFundsOwner(alice);Also applies to: 88-95, 97-101, 102-105, 107-111, 113-120
protocol/contracts/interfaces/admin/IEpochPayments.sol (7)
8-13: Index epoch in events for efficient querying.Add
indexedtoepochin events; it’s a primary filter dimension for analytics and subgraph consumers.- event CancelledEpochPayment(address indexed recipient, uint256 epoch, uint256 amountRevoked); - event ClaimedEpoch(address indexed recipient, uint256 epoch, uint256 amount); - event EpochAllocationSet(address indexed recipient, uint256 epoch, uint256 amount); + event CancelledEpochPayment(address indexed recipient, uint256 indexed epoch, uint256 amountRevoked); + event ClaimedEpoch(address indexed recipient, uint256 indexed epoch, uint256 amount); + event EpochAllocationSet(address indexed recipient, uint256 indexed epoch, uint256 amount); event MinimumEpochDurationSet(uint256 duration); event NextEpochSet(uint256 epoch);
28-33: Clarify epoch numbering and duration semantics in docs.Document that:
currentEpoch()starts at 0 and increments to 1 on firstsetEpochPayments.- “Minimum epoch duration” is strict (next epoch allowed only when
block.timestamp > start + minEpochDuration) or inclusive (>=). Tests assume strict.-/// @notice Keep track of the current active epoch +/// @notice Current active epoch (0 before first epoch; increments to 1 on first start) function currentEpoch() external view returns (uint256); -/// @notice Minimum epoch duration +/// @notice Minimum epoch duration; next epoch can start only when +/// block.timestamp [operator] epochStartTimes[currentEpoch] + minEpochDuration. +/// Specify whether [operator] is '>' (strict) or '>=' (inclusive). function minEpochDuration() external view returns (uint256);
34-41: Tighten NatSpec on getters.Make the mappings explicit for integrators.
-/// @notice Epoch payments. variable payments +/// @notice Amount allocated to recipient for a given epoch. function epochPayments(address recipient, uint256 epoch) external view returns (uint256); -/// @notice Mapping of claimed epochs for recipient/epoch pair +/// @notice Whether recipient has claimed for epoch. function claimedEpochs(address recipient, uint256 epoch) external view returns (bool); -/// @notice Epochs to start time mapping +/// @notice Start timestamp for an epoch; 0 if unset. function epochStartTimes(uint256 epoch) external view returns (uint256);
44-51: Specify preconditions for setting next epoch.Explicitly state inputs must be non-empty, arrays equal-length, recipients nonzero, amounts > 0, and that this targets
currentEpoch + 1.- * @notice Set initial epoch payments for the next epoch + * @notice Set initial epoch payments for the next epoch (currentEpoch + 1). + * Requirements: recipients/amounts non-empty, equal length; recipients != 0; amounts > 0.
53-61: Clarify update scope and accounting.Note that updates affect the current epoch only and
totalAllocationadjusts by delta when modifying an existing allocation.- * @notice Update existing or set new payments for the current epoch + * @notice Update existing or set new payments for the current epoch; totalAllocation reflects deltas.
63-69: Constrain revoke semantics in docs.State that revocation is for the current epoch, requires nonzero existing allocation, and reverts if already claimed.
- * @notice Revoke epoch payment for contributor + * @notice Revoke caller-managed epoch payment for a contributor. + * Only for the current epoch; reverts if amount == 0 or already claimed.
71-75: Clarify claim target.Make it explicit that the caller claims their own allocation for the specified epoch.
- * @notice Claim for epoch + * @notice Claim caller's allocation for `epoch`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/PaymentBase.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/test/forge/unit/admin/EpochPayments.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/contracts/admin/EpochPayments.sol
- protocol/contracts/admin/PaymentBase.sol
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Foundry Tests (ubuntu-latest, 20)
🔇 Additional comments (1)
protocol/contracts/interfaces/admin/IVestingPayments.sol (1)
5-7: Extends IPaymentBase (aligned with immutable paymentToken change)Using retrieved learnings: PaymentBase.paymentToken is immutable and setters were removed from the interface. This extension looks correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
protocol/contracts/interfaces/admin/IVestingPayments.sol (2)
10-13: Index ScheduleCreated id for efficient off-chain filteringIndex the vesting id; keeps within the 3-index limit and aligns with the other events.
- event ScheduleCreated( - bytes32 _id, address indexed _recipient, uint40 _start, + event ScheduleCreated( + bytes32 indexed _id, address indexed _recipient, uint40 _start, uint40 _cliff, uint40 _duration, uint128 _amount );
143-146: Fix: external array param must be calldata (won’t compile in ^0.8.20)External functions cannot take dynamically-sized memory parameters.
- function getVestingSummary( - bytes32[] memory _ids - ) external view returns (VestingSummary[] memory summary); + function getVestingSummary( + bytes32[] calldata _ids + ) external view returns (VestingSummary[] memory summary);
🧹 Nitpick comments (6)
protocol/contracts/interfaces/admin/IVestingPayments.sol (2)
8-15: Unify event id parameter namesUse a consistent name for the id across events.
- event RecipientChanged(bytes32 indexed _vestingId, address indexed _oldRecipient, address indexed _recipient); + event RecipientChanged(bytes32 indexed _id, address indexed _oldRecipient, address indexed _recipient);
1-3: Nit: put SPDX before pragma (style)Common Solidity style prefers SPDX first line.
-pragma solidity ^0.8.20; -// SPDX-License-Identifier: AGPL-3.0-or-later +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.20;protocol/contracts/interfaces/admin/IEpochPayments.sol (1)
1-3: Nit: put SPDX before pragma (style)Keeps headers consistent across interfaces.
-pragma solidity ^0.8.20; -// SPDX-License-Identifier: AGPL-3.0-or-later +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.20;protocol/test/forge/unit/admin/EpochPayments.t.sol (3)
230-243: Avoid leaked prank dependencies; convert this “test_*” into a helper or stop the prankThis function starts an executor prank and relies on it staying active for callers, which is brittle when tests call each other.
Option A (preferred): rename to a helper and stop prank.
- function test_setEpochPayments_first_epoch() public { + function _bootstrapFirstEpoch() internal { assertEq(payment.currentEpoch(), 0); assertEq(payment.epochStartTimes(1), 0); assertEq(payment.epochStartTimes(0), 0); vm.startPrank(executor); payment.setMinimumEpochDuration(4 weeks); address[] memory recipients = new address[](1); uint256[] memory amounts = new uint256[](1); recipients[0] = alice; amounts[0] = 1e18; vm.expectEmit(address(payment)); emit NextEpochSet(1); payment.setEpochPayments(recipients, amounts); + vm.stopPrank(); }Then update call sites to use
_bootstrapFirstEpoch();and wrap privileged calls withvm.prank(executor)as needed.If you intentionally rely on prank leakage (as discussed earlier), consider adding a brief comment at the call sites to make it explicit.
294-299: Make expected event assertions explicit per eventCall expectEmit for each expected log for clarity.
- vm.expectEmit(true, true, false, true, address(payment)); - emit NextEpochSet(3); - emit EpochAllocationSet(recipients[0], 3, amounts[0]); - emit EpochAllocationSet(recipients[1], 3, amounts[1]); + vm.expectEmit(true, true, false, true, address(payment)); + emit NextEpochSet(3); + vm.expectEmit(true, true, false, true, address(payment)); + emit EpochAllocationSet(recipients[0], 3, amounts[0]); + vm.expectEmit(true, true, false, true, address(payment)); + emit EpochAllocationSet(recipients[1], 3, amounts[1]);
565-572: Check emitter address for TokenRecovered eventAlign with other tests by asserting the emitting contract.
- vm.expectEmit(); + vm.expectEmit(address(payment)); emit CommonEventsAndErrors.TokenRecovered(alice, address(fakeToken), amount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/test/forge/unit/admin/EpochPayments.t.sol(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/contracts/admin/EpochPayments.sol
- protocol/test/forge/unit/admin/VestingPayments.t.sol
- protocol/contracts/admin/VestingPayments.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T10:46:38.185Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/test/forge/unit/admin/EpochPayments.t.sol:238-242
Timestamp: 2025-08-28T10:46:38.185Z
Learning: In Solidity test files using Foundry's vm.startPrank(), if a test function calls vm.startPrank() without vm.stopPrank(), the prank remains active when that function is called by other test functions, allowing subsequent calls to execute with the same privileges.
Applied to files:
protocol/test/forge/unit/admin/EpochPayments.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Foundry Tests (ubuntu-latest, 20)
🔇 Additional comments (3)
protocol/contracts/interfaces/admin/IEpochPayments.sol (1)
7-20: LGTM: interface shape and event indexingEvents/errors/read API look coherent; array params correctly use calldata.
protocol/test/forge/unit/admin/EpochPayments.t.sol (2)
26-33: LGTM: approval done with single-call prankAvoids leaking prank context from setUp().
186-191: LGTM: minEpochDuration test now asserts behaviorGood concise check of setter path.
* feat: simplify interface for epoch payments * feat: tweaks to vesting payments and deps * feat: nits * nits update * slither * add revoke releasable to schedule struct * move elevated access to base constructor --------- Co-authored-by: princetonbishop <princetonbishop@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
protocol/foundry.toml (2)
29-29: Persisting invariant corpus: add gitignore and wire up CI artifacts.If you intend to keep local fuzz seeds, add this directory to .gitignore and (optionally) upload it as a CI artifact between runs to improve convergence.
+.gitignore +fuzz-corpus/Confirm that
corpus_diris recognized by your Foundry version under[profile.default.invariant](some options have moved across releases).
54-63: Confirm lint rule validity and refine ignore globs
Runforge fmt --list-lints(or equivalent) to ensure all specified rule IDs (asm-keccak256,mixed-case-variable, etc.) are supported by your Foundry version. Additionally, narrow theignoreglobs (e.g. target onlycontracts/test/**/*rather than**/node_modules/**/*and other broad patterns) to avoid silencing valuable lint feedback in tests.protocol/contracts/interfaces/admin/IPaymentBase.sol (2)
16-21: Clarify access control in NatSpec for setFundsOwner
Document that this is permissioned (only elevated access) to make API expectations explicit.Apply:
/** * @notice Set funds owner * @param _fundsOwner Funds owner +* @dev Permissioned: only elevated access */ function setFundsOwner(address _fundsOwner) external;
22-28: Clarify access control in NatSpec for recoverToken
Same note for recoverToken to avoid misuse by integrators reading the interface docs.Apply:
/** * @notice Recover ERC20 token * @param _token Token address * @param _to Recipient address * @param _amount Amount to recover + * @dev Permissioned: only elevated access */ function recoverToken(address _token, address _to, uint256 _amount) external;protocol/contracts/interfaces/admin/IEpochPayments.sol (1)
56-65: Doc/signature mismatch for updateEpochPayments
Docs say “current epoch” but the function accepts an epoch parameter. Align the comment or drop the param in a future interface rev.Apply:
- /** - * @notice Update existing or set new payments for the current epoch + /** + * @notice Update existing or set new payments for the specified epoch * @param recipients Recipients array * @param amounts Amounts array to set */protocol/contracts/interfaces/admin/IVestingPayments.sol (2)
16-19: Prefer uint256 for event amount typesEvent params aren’t storage-optimized; using uint256 avoids off-chain decode friction and implicit widening. Safe to keep uint128 in storage.
- event ScheduleCreated( - bytes32 indexed _id, address indexed _recipient, uint40 _start, - uint40 _cliff, uint40 _duration, uint128 _amount - ); + event ScheduleCreated( + bytes32 indexed _id, address indexed _recipient, uint40 _start, + uint40 _cliff, uint40 _duration, uint256 _amount + );
54-56: NatSpec mismatches actual accounting semanticsImplementation tracks “outstanding obligation” (amount yet to be claimed after revokes and claims), not strictly “vested and unclaimed.” Update the docstring to prevent integrator confusion.
- /// @notice Total vesting token vested and unclaimed + /// @notice Total unclaimed vesting obligation (sum of undistributed amounts minus revoked portions)protocol/contracts/admin/EpochPayments.sol (1)
121-129: Epoch start boundary off-by-one: allow start at exact boundaryPermit starting the next epoch when now == start + minEpochDuration. Current <= reverts at the boundary.
- if (currentEpochCache != 0 && block.timestamp <= startTime + minEpochDuration) { revert CannotStartEpoch(currentEpochCache+1); } + if (currentEpochCache != 0 && block.timestamp < startTime + minEpochDuration) { revert CannotStartEpoch(currentEpochCache + 1); }protocol/contracts/admin/VestingPayments.sol (2)
120-138: Explicit error on revoked-with-zero flow; avoid leaking InvalidParamIf a schedule is revoked and nothing remains to claim, release() currently falls through and reverts with InvalidParam via isActiveVestingId(false). Emit a clearer domain error.
function release(bytes32 _vestingId) external override { VestingSchedule storage _schedule = schedules[_vestingId]; if (_schedule.start == 0) { revert CommonEventsAndErrors.InvalidParam(); } if (_schedule.recipient != msg.sender) { revert CommonEventsAndErrors.InvalidAccess(); } if (_schedule.revoked) { uint256 amount = _schedule.revokedReleasable; if (amount > 0) { _schedule.revokedReleasable = 0; _release(_schedule, amount); emit Released(_vestingId, msg.sender, amount); return; } + revert CannotRelease(); } if (!isActiveVestingId(_vestingId)) { revert CommonEventsAndErrors.InvalidParam(); } uint256 _releasableAmount = _calculateReleasableAmount(_schedule); if (_releasableAmount == 0) { revert FullyVested(); } _release(_schedule, _releasableAmount); emit Released(_vestingId, msg.sender, _releasableAmount); }
120-138: Optional: prune fully settled schedules from active setTo keep enumerations lean, remove the vestingId from _activeVestingIds once fully distributed on the final release (non-revoked path).
- _release(_schedule, _releasableAmount); - emit Released(_vestingId, msg.sender, _releasableAmount); + _release(_schedule, _releasableAmount); + if (_schedule.distributed == _schedule.amount) { + // best-effort; ignore failure if already removed elsewhere + _activeVestingIds.remove(_vestingId); + } + emit Released(_vestingId, msg.sender, _releasableAmount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
protocol/.gitignore(1 hunks)protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/PaymentBase.sol(1 hunks)protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IEpochPayments.sol(1 hunks)protocol/contracts/interfaces/admin/IPaymentBase.sol(1 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/foundry.toml(3 hunks)protocol/lib/forge-std(1 hunks)protocol/package.json(1 hunks)protocol/slither.requirements.txt(1 hunks)protocol/test/forge/unit/admin/EpochPayments.t.sol(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- protocol/lib/forge-std
- protocol/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/test/forge/unit/admin/VestingPayments.t.sol
- protocol/test/forge/unit/admin/EpochPayments.t.sol
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-21T19:18:05.268Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
Applied to files:
protocol/contracts/admin/PaymentBase.solprotocol/contracts/interfaces/admin/IPaymentBase.solprotocol/contracts/admin/EpochPayments.sol
📚 Learning: 2025-08-29T05:08:58.252Z
Learnt from: frontier159
PR: TempleDAO/temple#1213
File: protocol/contracts/admin/PaymentBase.sol:22-26
Timestamp: 2025-08-29T05:08:58.252Z
Learning: In PaymentBase.sol constructor, it's acceptable to not validate _fundsOwner for zero address - the team trusts that deployment procedures handle this appropriately, and validation exists in the setFundsOwner function.
Applied to files:
protocol/contracts/admin/PaymentBase.solprotocol/contracts/interfaces/admin/IPaymentBase.solprotocol/contracts/admin/EpochPayments.sol
📚 Learning: 2025-08-30T23:20:17.493Z
Learnt from: frontier159
PR: TempleDAO/temple#1214
File: protocol/scripts/deploys/mainnet/team-payments/deploy.ts:9-9
Timestamp: 2025-08-30T23:20:17.493Z
Learning: In Temple DAO's protocol/scripts/deploys/mainnet/team-payments/deploy.ts, the user prefers to keep hard-coded imports for epoch snapshot JSON files (like epoch29b.json) rather than parametrizing them. Avoid suggesting refactoring to make snapshot files runtime parameters.
Applied to files:
protocol/contracts/admin/EpochPayments.sol
📚 Learning: 2025-08-29T10:36:03.181Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1213
File: protocol/contracts/admin/VestingPayments.sol:38-39
Timestamp: 2025-08-29T10:36:03.181Z
Learning: In VestingPayments.sol, when fixing revocation accounting issues for multiple schedules per account, the preferred approach is to add revokedReleasable directly to the VestingSchedule struct rather than maintaining separate mappings, leveraging the existing schedules mapping keyed by vestingId for cleaner architecture.
Applied to files:
protocol/contracts/interfaces/admin/IVestingPayments.solprotocol/contracts/admin/VestingPayments.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Foundry Tests (ubuntu-latest, 20)
🔇 Additional comments (8)
protocol/foundry.toml (2)
52-53: LGTM: fmt tweaks.
wrap_comments = trueand the newline at EOF improve formatting consistency.
13-16: Pin Foundry v1.1.0+ in CI
The flagsdynamic_test_linking,legacy_assertions, andassertions_revertrequire Foundry ≥ 1.1.0. Manually verify your CI/toolchain is pinned to v1.1.0 or newer to prevent unknown-key errors and ensure expected assertion semantics.protocol/slither.requirements.txt (1)
1-2: Pin upgrades verified in CI (solc-select + slither)
The validation script failed to create a venv (ensurepip missing) and didn’t find solc-select/solc/slither (installed to ~/.local/bin, not on PATH). Confirm your CI image includes python3-venv and adds pip-installed bins to PATH (or adjust the setup) so these tools are callable.protocol/contracts/interfaces/admin/IEpochPayments.sol (1)
12-17: Confirm lifecycle API that emits NextEpochSet
The interface emits NextEpochSet but doesn’t expose an explicit “roll/start next epoch” function. Verify implementers emit this event along the intended path (e.g., within setEpochPayments or a dedicated function) and that tests assert it.protocol/contracts/interfaces/admin/IVestingPayments.sol (1)
14-21: LGTM: indexed vesting IDs and calldata array fix landedEvents now index the vesting id; getVestingSummary correctly uses calldata. Nice off-chain UX and gas wins.
Also applies to: 159-161
protocol/contracts/admin/EpochPayments.sol (1)
106-119: Delta-based totalAllocation accounting looks solidAdjusting by (new - prev) prevents double-counting on updates or duplicates in a batch. Good catch and implementation.
protocol/contracts/admin/VestingPayments.sol (2)
41-45: Good: per-schedule revokedReleasable matches team preferenceStoring revokedReleasable on the schedule (not per-recipient mappings) simplifies accounting for multiple schedules per account. Using retrieved learnings.
Also applies to: 92-95
28-76: Input validation coverage is thoroughStart/cliff/duration invariants and zero-address/amount checks are tight; prevents malformed schedules at creation.
| // Claimable delta at revoke time | ||
| uint256 releasableNow = vestedNow - _schedule.distributed; | ||
| // Persist per-schedule claimable checkpoint | ||
| _schedule.revokedReleasable = releasableNow.encodeUInt128(); |
There was a problem hiding this comment.
Was a specific test case added to check multiple revoke's for the same user? It doesn't look like it, but that's the exact case this was intended to fix. So that should 💯 be added in (unless i just missed it in the existing tests)
There was a problem hiding this comment.
This was in the existing test previous commit as
function test_revokeVesting_invalid_id_after_revoke() public {
test_revokeVesting_immediate_revoke_after_creation();
// use previous index to match id
bytes32 _id = vesting.computeVestingScheduleIdForAddressAndIndex(alice, 0);
vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidParam.selector));
vesting.revokeVesting(_id);
}
Unless I misunderstood something?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
protocol/contracts/interfaces/admin/IVestingPayments.sol (2)
14-21: Indexed vesting ids in events — LGTMIndexing
_idenables efficient off-chain filtering and respects the 3-index limit.
45-52: Using uint256 in VestingSummary — LGTMAligns with downstream ABI usage and reviewer guidance.
🧹 Nitpick comments (6)
protocol/contracts/interfaces/admin/IVestingPayments.sol (6)
1-3: Move SPDX identifier to the first lineSPDX should precede pragma for tooling and license scanners.
-pragma solidity ^0.8.20; // SPDX-License-Identifier: AGPL-3.0-or-later +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.20;
14-21: Unify event param naming to_vestingIdfor consistencyMatches function params elsewhere and improves readability. ABI unchanged.
- event Revoked(bytes32 indexed _id, address indexed _recipient, uint256 _unreleased, uint256 _totalVested); - event Released(bytes32 indexed _id, address indexed _recipient, uint256 _amount); + event Revoked(bytes32 indexed _vestingId, address indexed _recipient, uint256 _unreleased, uint256 _totalVested); + event Released(bytes32 indexed _vestingId, address indexed _recipient, uint256 _amount); event ScheduleCreated( - bytes32 indexed _id, address indexed _recipient, uint40 _start, + bytes32 indexed _vestingId, address indexed _recipient, uint40 _start, uint40 _cliff, uint40 _duration, uint128 _amount ); - event RecipientChanged(bytes32 indexed _id, address indexed _oldRecipient, address indexed _recipient); + event RecipientChanged(bytes32 indexed _vestingId, address indexed _oldRecipient, address indexed _recipient);
22-25: Add specific error for recipient changesConsider a targeted error to improve revert clarity.
error CannotRelease(); error InvalidScheduleId(); error FullyVested(); + error InvalidRecipient();
112-116: Consider returning released amount fromreleaseQuality-of-life for integrators; event parsing not required.
- function release(bytes32 _vestingId) external; + function release(bytes32 _vestingId) external returns (uint256 released);
131-145: Rename params to_holderfor consistency with function namesNatSpec/param names are not part of ABI; safe rename.
- function getLastVestingScheduleForHolder( - address _recipient + function getLastVestingScheduleForHolder( + address _holder ) external view returns (VestingSchedule memory); @@ - function getVestingScheduleByAddressAndIndex( - address _recipient, + function getVestingScheduleByAddressAndIndex( + address _holder, uint256 _index ) external view returns (VestingSchedule memory);
101-111: Provide paginated accessors to avoid unbounded array returnsLarge sets can be heavy even for off-chain callers.
function getVestingIdAtIndex( uint256 _index ) external view returns (bytes32 id); @@ - function getVestingIds() external view returns (bytes32[] memory ids); + function getVestingIds() external view returns (bytes32[] memory ids); + function getVestingIdsPaginated(uint256 _offset, uint256 _limit) + external + view + returns (bytes32[] memory ids);Also applies to: 167-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
protocol/contracts/admin/EpochPayments.sol(1 hunks)protocol/contracts/admin/VestingPayments.sol(1 hunks)protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol(0 hunks)protocol/contracts/interfaces/admin/IVestingPayments.sol(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
💤 Files with no reviewable changes (1)
- protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/test/forge/unit/admin/VestingPayments.t.sol
- protocol/contracts/admin/VestingPayments.sol
- protocol/contracts/admin/EpochPayments.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:36:03.200Z
Learnt from: princetonbishop
PR: TempleDAO/temple#1213
File: protocol/contracts/admin/VestingPayments.sol:38-39
Timestamp: 2025-08-29T10:36:03.200Z
Learning: In VestingPayments.sol, when fixing revocation accounting issues for multiple schedules per account, the preferred approach is to add revokedReleasable directly to the VestingSchedule struct rather than maintaining separate mappings, leveraging the existing schedules mapping keyed by vestingId for cleaner architecture.
Applied to files:
protocol/contracts/interfaces/admin/IVestingPayments.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Foundry Tests (ubuntu-latest, 20)
- GitHub Check: Slither (ubuntu-latest, 20)
🔇 Additional comments (4)
protocol/contracts/interfaces/admin/IVestingPayments.sol (4)
26-43: Struct packing looks optimal (3 storage slots)Order achieves tight packing and aligns with our prior learning to include
revokedReleasablein the struct.
83-86: Calldata for external array — LGTMEfficient and compiles under ^0.8.20.
20-21: Expose recipient change method to pair withRecipientChangedevent — verification requiredFile: protocol/contracts/interfaces/admin/IVestingPayments.sol
IVestingPayments declares event RecipientChanged but the interface has no method to change a vesting recipient; either add an admin-only setter to the interface or confirm the implementation exposes this via another interface. Repository search returned no matches—verify the implementation in protocol/contracts/admin/VestingPayments.sol and act accordingly.
event RecipientChanged(bytes32 indexed _id, address indexed _oldRecipient, address indexed _recipient); + /** + * @notice Change the recipient for a vesting schedule + * @param _vestingId Vesting Id + * @param _newRecipient New recipient address + */ + function changeRecipient(bytes32 _vestingId, address _newRecipient) external;
58-68: schedules getter tuple matches VestingSchedule fields — no action required. The VestingSchedule struct in protocol/contracts/interfaces/admin/IVestingPayments.sol lists fields in the exact order used by the schedules(...) return tuple, and VestingPayments declares the mapping aspublic override schedules, so the auto-generated getter signature is consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts (2)
97-101: Replace "0x" placeholders with the zero address (or add validation) to avoid invalid-address runtime errors"0x" is not a valid EVM address and will throw if accidentally used in a connect/call. Standardize on the canonical zero address.
Apply this minimal change (repeat for each placeholder TGLD):
- TGLD: "0x", + TGLD: "0x0000000000000000000000000000000000000000",Optionally, add a small guard wherever these addresses are consumed to assert non-zero on production networks.
Also applies to: 140-144, 183-187, 226-230, 269-273, 312-316, 355-359
52-56: Add typed connector/address getter for ADMIN.VESTING.TGLDFound direct usage at protocol/scripts/deploys/sepolia/admin/99-post-deploy.ts:17 — VestingPayments__factory.connect(TEMPLEGOLD_ADDRESSES.ADMIN.VESTING.TGLD, owner). Expose a typed connector or at least an address getter (mirror existing TEMPLE_GOLD connectors) and update consumers to use it instead of directly indexing the nested address path.
protocol/scripts/deploys/sepolia/admin/01-vesting-tgld.ts (1)
17-25: Annotate constructor args for readability (optional).Apply this diff:
await deployAndMine( 'VESTING_TGLD', factory, factory.deploy, - TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, - ownerAddress, - ownerAddress, - TEMPLEGOLD_ADDRESSES.TEMPLE_GOLD.TEMPLE_GOLD + TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, // rescuer + ownerAddress, // fundsOwner + ownerAddress, // owner/admin + TEMPLEGOLD_ADDRESSES.TEMPLE_GOLD.TEMPLE_GOLD // paymentTokenprotocol/scripts/deploys/sepolia/admin/99-post-deploy.ts (4)
43-76: Strongly type schedule inputs to catch struct mismatches at compile time.Type the schedules to the exact input type expected by createSchedules.
Apply this diff:
- const schedules = [ + const schedules: Parameters<VestingPayments["createSchedules"]>[0] = [ { cliff: cliff, start: start, duration: ONE_WEEK, amount: ethers.utils.parseEther("40"), revoked: false, distributed: 0, revokedReleasable: 0, recipient: "0x7Ab8375c4F1167b92a1fD649Bc2f5e8E6aa7d4c4" },Repeat the same typing for the second schedules array below.
85-139: Repeat: add strict typing for the second schedules array.Apply this diff:
- const schedules = [ + const schedules: Parameters<VestingPayments["createSchedules"]>[0] = [ { cliff: cliff, start: start, duration: ONE_WEEK, amount: ethers.utils.parseEther("40"),
141-152: Avoid BigNumber.toNumber() for unbounded counts (minor).Safer loop without potential overflow:
- const count = (await vesting.holdersVestingCount(holder)).toNumber(); - for (let i = 0; i < count; i++) { + const count = await vesting.holdersVestingCount(holder); + for (let i = 0; i < count.toNumber(); i++) {
32-35: Guard approval amount (optional).Approving 10B tokens is convenient but risky if accidentally uncommented on prod. Consider env‑driven caps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts(8 hunks)protocol/scripts/deploys/sepolia/admin/01-vesting-tgld.ts(1 hunks)protocol/scripts/deploys/sepolia/admin/02-epoch-payments.ts(1 hunks)protocol/scripts/deploys/sepolia/admin/99-post-deploy.ts(1 hunks)protocol/scripts/deploys/sepolia/deploymentArgs/0xD96595caDE5AADa556F34F546b2992E702aA43e3.js(1 hunks)protocol/test/forge/unit/admin/VestingPayments.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- protocol/test/forge/unit/admin/VestingPayments.t.sol
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: princetonbishop
PR: TempleDAO/temple#1169
File: protocol/contracts/admin/PaymentBase.sol:18-48
Timestamp: 2025-08-21T19:18:05.268Z
Learning: In PaymentBase.sol, paymentToken has been changed to be immutable and the setPaymentToken function has been removed from the interface, eliminating the need for access-controlled overrides in inheriting contracts.
📚 Learning: 2025-08-30T23:20:17.523Z
Learnt from: frontier159
PR: TempleDAO/temple#1214
File: protocol/scripts/deploys/mainnet/team-payments/deploy.ts:9-9
Timestamp: 2025-08-30T23:20:17.523Z
Learning: In Temple DAO's protocol/scripts/deploys/mainnet/team-payments/deploy.ts, the user prefers to keep hard-coded imports for epoch snapshot JSON files (like epoch29b.json) rather than parametrizing them. Avoid suggesting refactoring to make snapshot files runtime parameters.
Applied to files:
protocol/scripts/deploys/sepolia/admin/02-epoch-payments.tsprotocol/scripts/deploys/sepolia/admin/99-post-deploy.ts
🧬 Code graph analysis (3)
protocol/scripts/deploys/sepolia/admin/02-epoch-payments.ts (2)
protocol/scripts/deploys/helpers.ts (2)
ensureExpectedEnvvars(716-728)deployAndMine(609-655)protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts (1)
getDeployedTempleGoldContracts(363-370)
protocol/scripts/deploys/sepolia/admin/99-post-deploy.ts (2)
protocol/scripts/deploys/helpers.ts (2)
ensureExpectedEnvvars(716-728)mine(582-585)protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts (1)
getDeployedTempleGoldContracts(363-370)
protocol/scripts/deploys/sepolia/admin/01-vesting-tgld.ts (2)
protocol/scripts/deploys/helpers.ts (2)
ensureExpectedEnvvars(716-728)deployAndMine(609-655)protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts (1)
getDeployedTempleGoldContracts(363-370)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Foundry Tests (ubuntu-latest, 20)
🔇 Additional comments (3)
protocol/scripts/deploys/mainnet/templegold/contract-addresses.ts (1)
269-273: Confirm sepolia ADMIN.VESTING.TGLD is the deployed Vesting contractPlease verify 0xD96595caDE5AADa556F34F546b2992E702aA43e3 is the expected Vesting contract on Sepolia (bytecode, ABI match, ownership). Link the deploy tx or Etherscan contract page in the PR description.
protocol/scripts/deploys/sepolia/admin/02-epoch-payments.ts (1)
14-25: Confirm constructor arg order and address config for Sepolia.
- Ensure EpochPayments constructor matches (rescuer, fundsOwner, owner, paymentToken).
- Verify TEMPLEGOLD_ADDRESSES has Sepolia entries for CORE.RESCUER_MSIG and EXTERNAL.MAKER_DAO.DAI_TOKEN.
protocol/scripts/deploys/sepolia/admin/01-vesting-tgld.ts (1)
16-25: LGTM; confirm admin/fundsOwner choices and Sepolia address map.
- Using TEMPLE_GOLD as immutable payment token is correct per recent PaymentBase changes.
- Verify CORE.RESCUER_MSIG and TEMPLE_GOLD.TEMPLE_GOLD are configured for sepolia.
- If fundsOwner and owner should differ (ops vs admin), pass distinct addresses.
| 'VESTING_TGLD', | ||
| factory, | ||
| factory.deploy, | ||
| TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, | ||
| ownerAddress, | ||
| ownerAddress, | ||
| TEMPLEGOLD_ADDRESSES.EXTERNAL.MAKER_DAO.DAI_TOKEN | ||
| ); |
There was a problem hiding this comment.
Wrong deployment label: using VESTING_TGLD for EpochPayments will mis-tag outputs.
This will produce verification files/log lines under the VESTING_TGLD key and can overwrite/confuse Vesting addresses.
Apply this diff:
- 'VESTING_TGLD',
+ 'EPOCH_PAYMENTS_DAI',
factory,
factory.deploy,
- TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG,
- ownerAddress,
- ownerAddress,
- TEMPLEGOLD_ADDRESSES.EXTERNAL.MAKER_DAO.DAI_TOKEN
+ TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, // rescuer
+ ownerAddress, // fundsOwner
+ ownerAddress, // owner/admin
+ TEMPLEGOLD_ADDRESSES.EXTERNAL.MAKER_DAO.DAI_TOKEN // paymentToken📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'VESTING_TGLD', | |
| factory, | |
| factory.deploy, | |
| TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, | |
| ownerAddress, | |
| ownerAddress, | |
| TEMPLEGOLD_ADDRESSES.EXTERNAL.MAKER_DAO.DAI_TOKEN | |
| ); | |
| 'EPOCH_PAYMENTS_DAI', | |
| factory, | |
| factory.deploy, | |
| TEMPLEGOLD_ADDRESSES.CORE.RESCUER_MSIG, // rescuer | |
| ownerAddress, // fundsOwner | |
| ownerAddress, // owner/admin | |
| TEMPLEGOLD_ADDRESSES.EXTERNAL.MAKER_DAO.DAI_TOKEN // paymentToken | |
| ); |
🤖 Prompt for AI Agents
In protocol/scripts/deploys/sepolia/admin/02-epoch-payments.ts around lines 18
to 25, the deployment is incorrectly labeled 'VESTING_TGLD' which will tag
EpochPayments outputs as vesting; change the label to the correct key (e.g.
'EPOCH_PAYMENTS' or the project constant used for EpochPayments) so the
deployment/verification files and logs are written under the EpochPayments key
instead of VESTING_TGLD.
| module.exports = [ | ||
| "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266", | ||
| "0xC785695710292c042a2de8A0Ba16F3a054cC2eAD", | ||
| "0xC785695710292c042a2de8A0Ba16F3a054cC2eAD", | ||
| "0x2ae6318e34bb97ae3755AFcE75559452aA223A5D" | ||
| ]; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Sepolia constructor args look wrong (Hardhat default EOA, duplicate addresses).
- "0xf39F..." is the default Hardhat account, unlikely to be your Sepolia RESCUER_MSIG or admin.
- The 2nd and 3rd args are identical; confirm fundsOwner and owner are intentionally the same.
- Verify arg order matches the contract constructor (likely: rescuer, fundsOwner, owner, paymentToken).
Regenerate this file from an actual Sepolia deploy or fix the values before verification.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show target file ==="
sed -n '1,200p' protocol/scripts/deploys/sepolia/deploymentArgs/0xD96595caDE5AADa556F34F546b2992E702aA43e3.js || true
echo
echo "=== list deploymentArgs dir ==="
ls -la protocol/scripts/deploys/sepolia/deploymentArgs || true
echo
echo "=== search for references to this args file ==="
rg -n "0xD96595caDE5AADa556F34F546b2992E702aA43e3" || true
echo
echo "=== search for usage of deploymentArgs directory ==="
rg -n "deploymentArgs" || true
echo
echo "=== search for deploy scripts under protocol/scripts/deploys/sepolia ==="
rg -n --hidden --no-ignore 'deploy' protocol/scripts/deploys/sepolia || true
echo
echo "=== search for common constructor names (rescuer|fundsOwner|paymentToken) in contracts ==="
rg -n --hidden --no-ignore '\brescuer\b|\bfundsOwner\b|\bpaymentToken\b' protocol || trueLength of output: 132197
Sepolia constructor args are incorrect — replace Hardhat default & fix duplicate addresses
- File: protocol/scripts/deploys/sepolia/deploymentArgs/0xD96595caDE5AADa556F34F546b2992E702aA43e3.js — first arg is 0xf39F... (Hardhat default EOA), 2nd and 3rd args are identical (0xC785...).
- Rescuer should be a multisig (Gnosis Safe); 0xf39F... must not be used on Sepolia.
- Constructor order is (rescuer, executor, fundsOwner, paymentToken) — verify against protocol/contracts/admin/VestingPayments.sol (and protocol/test/forge/unit/admin/VestingPayments.t.sol).
- Action: regenerate this file from the actual Sepolia deploy or replace the four entries with the correct rescuer/executor/fundsOwner/token addresses before running verification.
🤖 Prompt for AI Agents
In
protocol/scripts/deploys/sepolia/deploymentArgs/0xD96595caDE5AADa556F34F546b2992E702aA43e3.js
around lines 3–8 the constructor args are wrong: the first entry is the Hardhat
default EOA (0xf39F...) and the 2nd and 3rd entries are duplicates; constructor
expects (rescuer, executor, fundsOwner, paymentToken). Replace the first value
with the actual Sepolia rescuer (Gnosis Safe multisig) and ensure executor and
fundsOwner are the correct, distinct Sepolia addresses (no duplicates), keeping
the correct paymentToken address; regenerate the file from the real Sepolia
deploy or update the four entries manually and verify the order against
protocol/contracts/admin/VestingPayments.sol and
protocol/test/forge/unit/admin/VestingPayments.t.sol before running
verification.
2 participants
Description
What does this PR solve?
Fixes # (issue)
Checklist
Summary by CodeRabbit
New Features
Tests
Chores