Add a revocable vesting wallet extension#6525
Conversation
|
WalkthroughThis PR introduces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/finance/VestingWalletRevocable.test.js (1)
55-80: ⚡ Quick winConsider verifying emitted events during revocation.
The test validates balance changes but doesn't assert that the expected events are emitted when
revoke()is called. Verifying events ensures the contract's complete public API behavior is tested.📋 Proposed enhancement to add event verification
const refund = totalAllocation - (await this.mock.vestedAmount(halfway)); - await expect(() => this.mock.connect(this.revoker).revoke()).to.changeEtherBalances( + await expect(this.mock.connect(this.revoker).revoke()) + .to.emit(this.mock, 'EtherRevoked') + .withArgs(/* expected event arguments */); + + await expect(() => this.mock.connect(this.revoker).revoke()).to.changeEtherBalances( [this.mock, this.revoker], [-refund, refund], );Apply similar event verification to the ERC20 revocation tests as well.
🤖 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/VestingWalletRevocable.test.js` around lines 55 - 80, Add an assertion that calling this.mock.connect(this.revoker).revoke() emits the contract's revocation event: wrap the revoke call in an expect(...).to.emit(this.mock, '<revocationEventName>').withArgs(this.revoker, refund) (or the actual event argument types used by the contract such as beneficiary and refund amount), and do the same pattern for the ERC20 revoke test; locate the revoke invocation in the test (this.mock.connect(this.revoker).revoke()) and add the expect-to.emit assertion immediately around it to verify the event and its args.
🤖 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.
Nitpick comments:
In `@test/finance/VestingWalletRevocable.test.js`:
- Around line 55-80: Add an assertion that calling
this.mock.connect(this.revoker).revoke() emits the contract's revocation event:
wrap the revoke call in an expect(...).to.emit(this.mock,
'<revocationEventName>').withArgs(this.revoker, refund) (or the actual event
argument types used by the contract such as beneficiary and refund amount), and
do the same pattern for the ERC20 revoke test; locate the revoke invocation in
the test (this.mock.connect(this.revoker).revoke()) and add the expect-to.emit
assertion immediately around it to verify the event and its args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee47b840-9301-4bd4-9ec9-5ba1d28a9944
📒 Files selected for processing (3)
contracts/finance/README.adoccontracts/finance/VestingWalletRevocable.soltest/finance/VestingWalletRevocable.test.js
1 participant
Summary
VestingWalletRevocableas aVestingWalletextension with per-asset revocationContext
Closes #6493.
Design
This keeps the beneficiary as the inherited
owner()ofVestingWallet, and introduces a separate immutable revoker account authorized to cancel vesting. On revocation, the historical allocation and revocation timestamp are snapshotted sovestedAmountcontinues to report the same vested total after the contract balance changes.Testing