Feature/vesting wallet factory#6524
Closed
TenHam3 wants to merge 14 commits into
Closed
Conversation
…espond to one beneficiary in a contract
…d schedules are created correctly
…the schedule and releasing the tokens
…ently and dont conflict with each other
…racts into feature/VestingWalletFactory
…ract per beneficiary VestingWallet and multi-beneficiary VestingWalletFactory
|
Contributor
WalkthroughThis pull request introduces 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
test/finance/VestingWalletFactory.test.js (3)
29-65: ⚡ Quick winAssert post-revert invariants in input-validation tests.
The revert reason checks are good, but adding invariant checks (e.g.,
scheduleCountunchanged and no token movement) hardens these tests against accidental writes before revert.Based on learnings: In tests that exercise revert semantics, verify no balance changes occurred to ensure no unintended state changes.Diff suggestion
describe('createVestingSchedule input validation', function () { it('rejects zero address beneficiary', async function () { + const beforeCount = await this.factory.scheduleCount(); + const ownerBalBefore = await this.token.balanceOf(this.owner.address); + const factoryBalBefore = await this.token.balanceOf(this.factory.target); + await expect( this.factory.createVestingSchedule( ethers.ZeroAddress, this.token.target, this.start, this.duration, this.amount, ), ).to.be.revertedWith('VestingWalletFactory: beneficiary is zero address'); + + expect(await this.factory.scheduleCount()).to.equal(beforeCount); + expect(await this.token.balanceOf(this.owner.address)).to.equal(ownerBalBefore); + expect(await this.token.balanceOf(this.factory.target)).to.equal(factoryBalBefore); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/finance/VestingWalletFactory.test.js` around lines 29 - 65, The input-validation tests for createVestingSchedule only assert revert reasons but don't verify post-revert invariants; update each test (those calling this.factory.createVestingSchedule) to also assert that no state or token movements occurred after the revert by checking the factory's scheduleCount (or equivalent getter) remains unchanged and relevant ERC20 balances (this.token.target balance of the factory/contract and this.beneficiary.address balance) are unchanged; use the same fixtures (this.factory, this.token.target, this.beneficiary.address, this.start, this.duration, this.amount) to read pre-call values, perform the call expecting revert, then re-read and assert equality.
215-221: ⚡ Quick winStrengthen per-release isolation assertions with full balance deltas.
These checks currently only assert recipient gains. Include factory and the other beneficiary in each tx assertion to catch accidental cross-schedule leakage.
Diff suggestion
it('each beneficiary receives only their own allocation', async function () { await time.increaseTo.timestamp(this.end, false); const tx1 = await this.factory.release(0n); const tx2 = await this.factory.release(1n); - await expect(tx1).to.changeTokenBalances(this.token, [this.beneficiary], [this.amount]); - await expect(tx2).to.changeTokenBalances(this.token, [this.beneficiary2], [this.amount2]); + await expect(tx1).to.changeTokenBalances( + this.token, + [this.factory, this.beneficiary, this.beneficiary2], + [-this.amount, this.amount, 0n], + ); + await expect(tx2).to.changeTokenBalances( + this.token, + [this.factory, this.beneficiary, this.beneficiary2], + [-this.amount2, 0n, this.amount2], + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/finance/VestingWalletFactory.test.js` around lines 215 - 221, Update the assertions in the test "each beneficiary receives only their own allocation" to assert full balance deltas for each release: when calling this.factory.release(0n) (tx1) include this.factory and both beneficiaries in the changeTokenBalances expectation so the factory's balance decreases by amount and the other beneficiary's balance is unchanged, and likewise when calling this.factory.release(1n) (tx2) include this.factory and both beneficiaries so the factory decreases by amount2 and the first beneficiary's balance is unchanged; modify the expect lines referencing tx1, tx2, this.token, this.beneficiary, this.beneficiary2, this.amount, and this.amount2 to check all three balances per release.
126-174: ⚡ Quick winAdd an explicit permissionless-caller release test.
release()is a core permissionless behavior, but current tests only execute it as the default signer. Add one case where a non-owner/non-beneficiary triggers release.Diff suggestion
describe('release', function () { beforeEach(async function () { @@ it('updates released on the schedule', async function () { await time.increaseTo.timestamp(this.end); await this.factory.release(0n); expect((await this.factory.getSchedule(0n)).released).to.equal(this.amount); }); + + it('allows any account to trigger release', async function () { + await time.increaseTo.timestamp(this.end); + await expect(this.factory.connect(this.other).release(0n)).to.changeTokenBalances( + this.token, + [this.factory, this.beneficiary], + [-this.amount, this.amount], + ); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/finance/VestingWalletFactory.test.js` around lines 126 - 174, Add a test that verifies release() is permissionless by calling this.factory.release(0n) from a non-owner/non-beneficiary signer (e.g., this.other) after time has advanced to this.end; assert it transfers funds (use changeTokenBalances), emits ERC20Released with expected args, and updates the schedule's released field (via getSchedule(0n)); ensure you first create the vesting schedule with createVestingSchedule and set this.other to be a different signer than this.beneficiary and the factory owner.contracts/finance/VestingWalletFactory.sol (3)
28-37: ⚡ Quick winConsider validating
tokenaddress.The function validates
beneficiarybut nottoken. Iftokenisaddress(0), the call will revert deep insafeTransferFromwith a confusing low-level error rather than a clear validation message.🛡️ Suggested fix
function createVestingSchedule( address beneficiary, address token, uint64 startTimestamp, uint64 durationSeconds, uint256 amount ) external onlyOwner returns (uint256 scheduleId) { require(beneficiary != address(0), "VestingWalletFactory: beneficiary is zero address"); + require(token != address(0), "VestingWalletFactory: token is zero address"); require(amount > 0, "VestingWalletFactory: amount is zero"); require(durationSeconds > 0, "VestingWalletFactory: duration is zero");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/finance/VestingWalletFactory.sol` around lines 28 - 37, The createVestingSchedule function validates beneficiary, amount, and duration but misses checking the token address; add a require(token != address(0), "VestingWalletFactory: token is zero address") at the start of createVestingSchedule to fail fast with a clear message (before any downstream calls like safeTransferFrom) so errors are not raised deep in token transfer logic.
75-81: ⚡ Quick winCalling
releaseon a non-existent schedule produces a confusing revert.If
scheduleIddoesn't exist, the function attemptssafeTransferwithtoken = address(0)andbeneficiary = address(0), causing a low-level revert. An explicit existence check would provide a clearer error.🛡️ Suggested fix
function release(uint256 scheduleId) external { VestingSchedule storage schedule = _schedules[scheduleId]; + require(schedule.beneficiary != address(0), "VestingWalletFactory: schedule does not exist"); uint256 amount = releasable(scheduleId); schedule.released += amount; SafeERC20.safeTransfer(IERC20(schedule.token), schedule.beneficiary, amount); emit ERC20Released(scheduleId, schedule.token, amount); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/finance/VestingWalletFactory.sol` around lines 75 - 81, The release function can operate on a non-existent VestingSchedule in _schedules, leading to a confusing low-level revert when token or beneficiary is address(0); add an explicit existence check at the start of release (before calling releasable or modifying schedule) such as requiring a non-zero token or beneficiary (e.g., require(schedule.token != address(0) || schedule.beneficiary != address(0), "VestingWalletFactory: schedule does not exist")), so the function reverts with a clear message; keep the rest of the logic (calling releasable, updating schedule.released, SafeERC20.safeTransfer, emit ERC20Released) unchanged.
2-2: 💤 Low valueIncorrect file name in header comment.
The header references
VestingWallet.solbut this file isVestingWalletFactory.sol.📝 Suggested fix
-// OpenZeppelin Contracts (last updated v5.6.0) (finance/VestingWallet.sol) +// OpenZeppelin Contracts (last updated v5.6.0) (finance/VestingWalletFactory.sol)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/finance/VestingWalletFactory.sol` at line 2, The header comment at the top incorrectly references "VestingWallet.sol"; update that comment to the correct file name "VestingWalletFactory.sol" so the file header matches the actual contract (VestingWalletFactory.sol) and any SPDX/license or descriptive comment lines reflect the correct filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/demoFactory.js`:
- Line 10: The current formatter fmt uses Number/BigInt which is unsafe for
values > Number.MAX_SAFE_INTEGER; change fmt to use ethers.formatUnits so large
token amounts are accurately formatted (e.g., call ethers.formatUnits(n, 18) or
ethers.formatUnits(n, DECIMALS) if DECIMALS/UNIT is defined) and ensure ethers
is imported; accept BigInt/string/BigNumber inputs and pass them to
ethers.formatUnits rather than converting to Number.
In `@scripts/gasCompare.js`:
- Around line 10-16: The variable N is currently parsed loosely and can be NaN
or partially parsed (e.g., "10foo"), causing downstream failures in main
(including a BigInt divide-by-zero when arrays are empty); update the
parsing/validation of N so it only accepts a strict positive integer: validate
the raw env string first (e.g., /^\d+$/) or use Number() + Number.isInteger and
ensure > 0, then set N accordingly and throw a clear Error from main (or before
calling main) if validation fails; reference the N constant and main function
when applying this change so the early-fail prevents the later divide-by-zero.
---
Nitpick comments:
In `@contracts/finance/VestingWalletFactory.sol`:
- Around line 28-37: The createVestingSchedule function validates beneficiary,
amount, and duration but misses checking the token address; add a require(token
!= address(0), "VestingWalletFactory: token is zero address") at the start of
createVestingSchedule to fail fast with a clear message (before any downstream
calls like safeTransferFrom) so errors are not raised deep in token transfer
logic.
- Around line 75-81: The release function can operate on a non-existent
VestingSchedule in _schedules, leading to a confusing low-level revert when
token or beneficiary is address(0); add an explicit existence check at the start
of release (before calling releasable or modifying schedule) such as requiring a
non-zero token or beneficiary (e.g., require(schedule.token != address(0) ||
schedule.beneficiary != address(0), "VestingWalletFactory: schedule does not
exist")), so the function reverts with a clear message; keep the rest of the
logic (calling releasable, updating schedule.released, SafeERC20.safeTransfer,
emit ERC20Released) unchanged.
- Line 2: The header comment at the top incorrectly references
"VestingWallet.sol"; update that comment to the correct file name
"VestingWalletFactory.sol" so the file header matches the actual contract
(VestingWalletFactory.sol) and any SPDX/license or descriptive comment lines
reflect the correct filename.
In `@test/finance/VestingWalletFactory.test.js`:
- Around line 29-65: The input-validation tests for createVestingSchedule only
assert revert reasons but don't verify post-revert invariants; update each test
(those calling this.factory.createVestingSchedule) to also assert that no state
or token movements occurred after the revert by checking the factory's
scheduleCount (or equivalent getter) remains unchanged and relevant ERC20
balances (this.token.target balance of the factory/contract and
this.beneficiary.address balance) are unchanged; use the same fixtures
(this.factory, this.token.target, this.beneficiary.address, this.start,
this.duration, this.amount) to read pre-call values, perform the call expecting
revert, then re-read and assert equality.
- Around line 215-221: Update the assertions in the test "each beneficiary
receives only their own allocation" to assert full balance deltas for each
release: when calling this.factory.release(0n) (tx1) include this.factory and
both beneficiaries in the changeTokenBalances expectation so the factory's
balance decreases by amount and the other beneficiary's balance is unchanged,
and likewise when calling this.factory.release(1n) (tx2) include this.factory
and both beneficiaries so the factory decreases by amount2 and the first
beneficiary's balance is unchanged; modify the expect lines referencing tx1,
tx2, this.token, this.beneficiary, this.beneficiary2, this.amount, and
this.amount2 to check all three balances per release.
- Around line 126-174: Add a test that verifies release() is permissionless by
calling this.factory.release(0n) from a non-owner/non-beneficiary signer (e.g.,
this.other) after time has advanced to this.end; assert it transfers funds (use
changeTokenBalances), emits ERC20Released with expected args, and updates the
schedule's released field (via getSchedule(0n)); ensure you first create the
vesting schedule with createVestingSchedule and set this.other to be a different
signer than this.beneficiary and the factory owner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef4444a7-cad8-4890-839d-2c5182ba51f6
📒 Files selected for processing (4)
contracts/finance/VestingWalletFactory.solscripts/demoFactory.jsscripts/gasCompare.jstest/finance/VestingWalletFactory.test.js
|
|
||
| const DECIMALS = 18n; | ||
| const UNIT = 10n ** DECIMALS; | ||
| const fmt = n => (Number(n) / Number(UNIT)).toFixed(2); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file and surrounding context
cat scripts/demoFactory.jsRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 5689
🏁 Script executed:
# Verify the precision loss concern by testing Number conversion behavior
node <<'EOF'
// Test the precision loss with the actual values from the demo
const DECIMALS = 18n;
const UNIT = 10n ** DECIMALS;
const values = [
{ name: "totalSupply (3000 tokens)", value: 3000n * UNIT },
{ name: "Alice (1000 tokens)", value: 1000n * UNIT },
{ name: "Bob (500 tokens)", value: 500n * UNIT },
{ name: "Carol (1500 tokens)", value: 1500n * UNIT },
];
const SAFE_INTEGER_LIMIT = 2n ** 53n - 1n;
console.log(`Safe integer limit: ${SAFE_INTEGER_LIMIT}`);
console.log(`That's: ${Number(SAFE_INTEGER_LIMIT).toExponential()}\n`);
values.forEach(({ name, value }) => {
const exceedsLimit = value > SAFE_INTEGER_LIMIT;
const current = (Number(value) / Number(UNIT)).toFixed(2);
const safe = Number("1000"); // example of what ethers.formatUnits would yield
console.log(`${name}:`);
console.log(` Value: ${value.toString()} (exceeds safe limit: ${exceedsLimit})`);
console.log(` Current approach: ${current}`);
});
console.log(`\nConclusion: Values EXCEED JavaScript's safe integer limit (2^53 - 1)`);
EOFRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 650
Use ethers.formatUnits() instead of Number(BigInt) for precise token formatting.
The values in this demo exceed JavaScript's safe integer limit (2^53 - 1), making direct BigInt-to-Number conversion unsafe even though the output happens to appear correct. ethers.formatUnits(n, 18) handles this safely and is the idiomatic approach for ethers.js v6.
Proposed fix
-const fmt = n => (Number(n) / Number(UNIT)).toFixed(2);
+const fmt = n => Number(ethers.formatUnits(n, 18)).toFixed(2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/demoFactory.js` at line 10, The current formatter fmt uses
Number/BigInt which is unsafe for values > Number.MAX_SAFE_INTEGER; change fmt
to use ethers.formatUnits so large token amounts are accurately formatted (e.g.,
call ethers.formatUnits(n, 18) or ethers.formatUnits(n, DECIMALS) if
DECIMALS/UNIT is defined) and ensure ethers is imported; accept
BigInt/string/BigNumber inputs and pass them to ethers.formatUnits rather than
converting to Number.
Comment on lines
+10
to
+16
| const N = parseInt(process.env.N ?? '5'); | ||
| const AMOUNT = ethers.parseEther('1000'); | ||
| const DURATION = BigInt(365 * 24 * 3600); // 1 year in seconds | ||
|
|
||
| async function main() { | ||
| if (N < 1) throw new Error('N must be at least 1'); | ||
|
|
Contributor
There was a problem hiding this comment.
Validate N strictly to prevent invalid-input runtime failures.
Line 10/Line 15 currently allow NaN or partially parsed values (e.g., N=abc, N=10foo), which can cascade into a BigInt divide-by-zero at Line 103 when arrays are empty. Fail fast with a strict positive-integer check.
Suggested fix
-const N = parseInt(process.env.N ?? '5');
+const rawN = process.env.N ?? '5';
+const N = Number(rawN);
const AMOUNT = ethers.parseEther('1000');
const DURATION = BigInt(365 * 24 * 3600); // 1 year in seconds
async function main() {
- if (N < 1) throw new Error('N must be at least 1');
+ if (!Number.isInteger(N) || N < 1) {
+ throw new Error(`N must be a positive integer; received "${rawN}"`);
+ }📝 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.
Suggested change
| const N = parseInt(process.env.N ?? '5'); | |
| const AMOUNT = ethers.parseEther('1000'); | |
| const DURATION = BigInt(365 * 24 * 3600); // 1 year in seconds | |
| async function main() { | |
| if (N < 1) throw new Error('N must be at least 1'); | |
| const rawN = process.env.N ?? '5'; | |
| const N = Number(rawN); | |
| const AMOUNT = ethers.parseEther('1000'); | |
| const DURATION = BigInt(365 * 24 * 3600); // 1 year in seconds | |
| async function main() { | |
| if (!Number.isInteger(N) || N < 1) { | |
| throw new Error(`N must be a positive integer; received "${rawN}"`); | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gasCompare.js` around lines 10 - 16, The variable N is currently
parsed loosely and can be NaN or partially parsed (e.g., "10foo"), causing
downstream failures in main (including a BigInt divide-by-zero when arrays are
empty); update the parsing/validation of N so it only accepts a strict positive
integer: validate the raw env string first (e.g., /^\d+$/) or use Number() +
Number.isInteger and ensure > 0, then set N accordingly and throw a clear Error
from main (or before calling main) if validation fails; reference the N constant
and main function when applying this change so the early-fail prevents the later
divide-by-zero.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4131
This is an implementation of the multi-vesting approach requested in this issue.
Design: A single VestingWalletFactory contract manages all beneficiaries. Each call to createVestingSchedule() stores a VestingSchedule struct in a mapping keyed by an auto-incrementing scheduleId, rather than deploying a new contract. The factory holds all tokens in escrow and transfers them to the beneficiary on release(scheduleId).
Vesting model: Linear vesting between a start timestamp and start + duration, consistent with the existing VestingWallet schedule. vestedAmount() and releasable() are exposed as view functions.
Access control: Only the owner can create schedules (onlyOwner). release() is permissionless - anyone can trigger a release for any schedule, which sends tokens directly to the stored beneficiary address.
Gas efficiency: Writing a struct to storage (~40k gas) is an order of magnitude cheaper than deploying a new VestingWallet (~500–700k gas). A benchmarking script comparing both approaches confirms the factory becomes meaningfully cheaper as the number of beneficiaries grows, with the factory's one-time deployment cost amortized across all schedules.
Known limitations: Schedules are immutable once created (no revocation or modification), only ERC20 tokens are supported (no ETH), and there is no cliff support. Each schedule is also tied to a single token, so multiple tokens for one beneficiary require separate schedules.
The implementation, tests, and gas benchmark are complete and runnable locally via Hardhat.
Note: This implements the core multi-beneficiary vesting portion of the issue: a single contract managing multiple vesting schedules for multiple beneficiaries. It does not include the additional features discussed, such as dss-vest-style streaming, revocability, or per-schedule manager roles.
PR Checklist
npx changeset add)