ref: [CR-01]: use DestroyCap design for destroying the wallet#418
ref: [CR-01]: use DestroyCap design for destroying the wallet#4180xNeshi wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughVesting wallet teardown now uses wallet-bound ChangesCap-based teardown redesign
Sequence DiagramssequenceDiagram
participant vesting_wallet_linear
participant vesting_wallet
participant Clock
vesting_wallet_linear->>vesting_wallet: new(...)
vesting_wallet-->>vesting_wallet_linear: wallet, DestroyCap
vesting_wallet_linear->>vesting_wallet: destroy_empty()
vesting_wallet-->>vesting_wallet_linear: DestroyReceipt{wallet_id, params}
vesting_wallet_linear->>Clock: timestamp_ms()
Clock-->>vesting_wallet_linear: current time
vesting_wallet_linear->>vesting_wallet: consume_receipt(receipt, cap, Linear {})
vesting_wallet-->>vesting_wallet_linear: params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
+ Coverage 96.26% 96.30% +0.03%
==========================================
Files 29 29
Lines 2968 2973 +5
Branches 732 732
==========================================
+ Hits 2857 2863 +6
Misses 66 66
+ Partials 45 44 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/finance/sources/vesting_wallet.move (1)
453-473: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
destroy_emptysummary sentence.This doc block still says
DestroyReceipthands back the beneficiary, but the receipt now only carrieswallet_idandparams. That leaves the public API docs out of sync with the actual type.📝 Proposed doc fix
-/// Consume a drained wallet to reclaim its storage rebate, emit `Destroyed`, and hand -/// its beneficiary and schedule parameters back as a `DestroyReceipt<S, P>`. +/// Consume a drained wallet to reclaim its storage rebate, emit `Destroyed`, and hand +/// the wallet's id and schedule parameters back as a `DestroyReceipt<S, P>`.As per path instructions, public functions/modules should have doc comments formatted per
STYLEGUIDE.md; keep API-docs accurate for signature/behavior changes.🤖 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/sources/vesting_wallet.move` around lines 453 - 473, Update the `destroy_empty` doc comment so the summary and Returns section match the current `DestroyReceipt<S, P>` shape: remove any mention that the receipt carries the beneficiary, and describe that it only includes `wallet_id` and `params`. Keep the public API docs in `vesting_wallet.move` aligned with the actual return type and behavior, using the existing `destroy_empty` and `DestroyReceipt` symbols to locate the comment.Source: Path instructions
contracts/finance/README.md (1)
82-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid transferring the example cap to
beneficiaryby default.If someone copies this example with an object-address beneficiary—the teardown case this PR is fixing—
transfer::public_transfer(cap, beneficiary)strands theDestroyCap, because that address cannot drive the later destroy transaction. A separate teardown owner/controller parameter, or an explicit note that this example assumes a sender-controlled beneficiary, would keep the example aligned with the new design.🤖 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/README.md` around lines 82 - 107, The README examples in grant and stream currently transfer the cap directly to beneficiary, which breaks the teardown flow for object-address beneficiaries. Update the example around vesting_wallet_linear::new and create_and_share_continuous to avoid defaulting to transfer::public_transfer(cap, beneficiary); either introduce a separate teardown owner/controller parameter for the cap transfer or add an explicit note that these examples only apply when beneficiary is sender-controlled, so the DestroyCap is not stranded.
🧹 Nitpick comments (3)
contracts/finance/sources/vesting_wallet_linear.move (2)
54-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep grouped imports on one line.
This split import violates the Move import convention for grouped members.
Suggested import formatting
-use openzeppelin_finance::vesting_wallet::{ - Self, - VestingWallet, - VestedAmount, - DestroyReceipt, - DestroyCap -}; +use openzeppelin_finance::vesting_wallet::{Self, VestingWallet, VestedAmount, DestroyReceipt, DestroyCap};As per coding guidelines, “Imports: don’t split
use pkg::mod::{...};across multiple lines; group members when importing the same module.”🤖 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/sources/vesting_wallet_linear.move` around lines 54 - 60, The grouped import from openzeppelin_finance::vesting_wallet is split across multiple lines, which violates the Move import formatting convention. Update the vesting_wallet_linear.move import to keep all members of the same module in a single grouped use statement, preserving the existing symbols Self, VestingWallet, VestedAmount, DestroyReceipt, and DestroyCap together on one line.Sources: Coding guidelines, Path instructions
396-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse receiver syntax for
consume_receipt.
DestroyReceiptis the first argument, and the base tests already usereceipt.consume_receipt(...).- let params = vesting_wallet::consume_receipt(receipt, cap, Linear {}); + let params = receipt.consume_receipt(cap, Linear {});As per coding guidelines, Move 2024 code should prefer
obj.fn(args...)when the first parameter is an object.🤖 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/sources/vesting_wallet_linear.move` at line 396, The call to vesting_wallet::consume_receipt is using function syntax even though DestroyReceipt is the receiver; update the call in the linear vesting flow to use receiver syntax on receipt, matching the existing pattern used in the base tests. Keep the same cap and Linear {} arguments, and ensure the change is applied at the consume_receipt call site in the vesting wallet logic.Sources: Coding guidelines, Path instructions
contracts/finance/examples/vesting_wallet/vesting_quadratic.move (1)
106-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBring the changed public API docs back into STYLEGUIDE format.
destroynow has a newDestroyCapparameter, but the doc-comment still lacks the required#### Parameters/#### Returnssections for public APIs. Also consider qualifying the propagated abort asvesting_wallet::EWrongCapfor API-doc clarity.Proposed doc update
/// #### Aborts -/// - `EWrongCap` if `cap` was minted for a different wallet. +/// #### Parameters +/// - `receipt`: The hot-potato receipt returned by `vesting_wallet::destroy_empty`. +/// - `cap`: The `DestroyCap` minted for the wallet being finalized. +/// - `clock`: The Sui clock used to enforce the schedule-ended gate. +/// +/// #### Returns +/// - Nothing. +/// +/// #### Aborts +/// - `vesting_wallet::EWrongCap` if `cap` was minted for a different wallet. /// - `ENotEnded` if called before the schedule's end (`start_ms + duration_ms`).As per coding guidelines, “Documentation: doc-comments are API reference input; use
///with#### Parameters,#### Returns, and#### Aborts.” As per path instructions, Move code must be reviewed against STYLEGUIDE.md as the source of truth.🤖 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/examples/vesting_wallet/vesting_quadratic.move` around lines 106 - 126, The public API doc for destroy in vesting_quadratic::destroy is missing the required STYLEGUIDE sections for a public function with parameters. Update the doc-comment to include the standard API-reference structure with #### Parameters for receipt, cap, and clock, and #### Returns if applicable, while keeping #### Aborts. Also qualify the propagated cap mismatch abort as vesting_wallet::EWrongCap for clarity and consistency with the core primitive.Sources: Coding guidelines, Path instructions
🤖 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
`@contracts/finance/examples/vesting_wallet/tests/vesting_quadratic_tests.move`:
- Around line 169-174: Add a positive teardown test for the `quadratic::destroy`
path where the `DestroyCap` is held by a sender other than `BENEFICIARY`, to
prove teardown is authorized by cap ownership rather than caller/beneficiary
identity. Extend the existing vesting teardown tests in
`vesting_quadratic_tests.move` by creating a success case that transfers the cap
away from `BENEFICIARY`, then calls `wallet.destroy_empty()` and
`quadratic::destroy(...)` with that non-beneficiary cap holder. Keep the
existing negative wrong-cap coverage, and make sure the new test clearly
exercises the happy path for the refactored authorization behavior.
In `@contracts/finance/sources/vesting_wallet_linear.move`:
- Around line 392-394: The public API docs for the vesting wallet function
currently list an abort constant without qualifying its origin; update the abort
section in the documented method in vesting_wallet_linear.move so the propagated
abort is explicitly attributed to vesting_wallet::EWrongCap. Keep the rest of
the abort causes intact, and ensure the docs accurately reflect that this module
is forwarding the base wallet module’s abort rather than defining the constant
itself.
---
Outside diff comments:
In `@contracts/finance/README.md`:
- Around line 82-107: The README examples in grant and stream currently transfer
the cap directly to beneficiary, which breaks the teardown flow for
object-address beneficiaries. Update the example around
vesting_wallet_linear::new and create_and_share_continuous to avoid defaulting
to transfer::public_transfer(cap, beneficiary); either introduce a separate
teardown owner/controller parameter for the cap transfer or add an explicit note
that these examples only apply when beneficiary is sender-controlled, so the
DestroyCap is not stranded.
In `@contracts/finance/sources/vesting_wallet.move`:
- Around line 453-473: Update the `destroy_empty` doc comment so the summary and
Returns section match the current `DestroyReceipt<S, P>` shape: remove any
mention that the receipt carries the beneficiary, and describe that it only
includes `wallet_id` and `params`. Keep the public API docs in
`vesting_wallet.move` aligned with the actual return type and behavior, using
the existing `destroy_empty` and `DestroyReceipt` symbols to locate the comment.
---
Nitpick comments:
In `@contracts/finance/examples/vesting_wallet/vesting_quadratic.move`:
- Around line 106-126: The public API doc for destroy in
vesting_quadratic::destroy is missing the required STYLEGUIDE sections for a
public function with parameters. Update the doc-comment to include the standard
API-reference structure with #### Parameters for receipt, cap, and clock, and
#### Returns if applicable, while keeping #### Aborts. Also qualify the
propagated cap mismatch abort as vesting_wallet::EWrongCap for clarity and
consistency with the core primitive.
In `@contracts/finance/sources/vesting_wallet_linear.move`:
- Around line 54-60: The grouped import from
openzeppelin_finance::vesting_wallet is split across multiple lines, which
violates the Move import formatting convention. Update the
vesting_wallet_linear.move import to keep all members of the same module in a
single grouped use statement, preserving the existing symbols Self,
VestingWallet, VestedAmount, DestroyReceipt, and DestroyCap together on one
line.
- Line 396: The call to vesting_wallet::consume_receipt is using function syntax
even though DestroyReceipt is the receiver; update the call in the linear
vesting flow to use receiver syntax on receipt, matching the existing pattern
used in the base tests. Keep the same cap and Linear {} arguments, and ensure
the change is applied at the consume_receipt call site in the vesting wallet
logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fecd17f-8dc3-46e9-8f4f-90b592d41f9d
📒 Files selected for processing (9)
contracts/finance/README.mdcontracts/finance/examples/vesting_wallet/tests/pausable_grant_tests.movecontracts/finance/examples/vesting_wallet/tests/splitter_tests.movecontracts/finance/examples/vesting_wallet/tests/vesting_quadratic_tests.movecontracts/finance/examples/vesting_wallet/vesting_quadratic.movecontracts/finance/sources/vesting_wallet.movecontracts/finance/sources/vesting_wallet_linear.movecontracts/finance/tests/vesting_wallet_linear_tests.movecontracts/finance/tests/vesting_wallet_tests.move
| // Permissionless half reclaims the storage rebate; the gated half consumes the | ||
| // receipt with the wallet's `DestroyCap` (parked with the beneficiary by | ||
| // `create_and_share`) and enforces the ended gate. | ||
| let cap = scenario.take_from_sender<DestroyCap>(); | ||
| let receipt = wallet.destroy_empty(); | ||
| quadratic::destroy(receipt, &clock, scenario.ctx()); | ||
| quadratic::destroy(receipt, cap, &clock); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a positive teardown test where the cap holder is not the beneficiary.
This happy path still consumes the parked cap as BENEFICIARY, while the refactor’s core behavior is that DestroyCap ownership—not caller/beneficiary identity—authorizes teardown. The wrong-cap test covers the negative path; add a success case with the cap transferred to a different sender.
As per path instructions, CONTRIBUTING.md requires changed functionality to include tests covering the new authorization/teardown paths.
🤖 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/examples/vesting_wallet/tests/vesting_quadratic_tests.move`
around lines 169 - 174, Add a positive teardown test for the
`quadratic::destroy` path where the `DestroyCap` is held by a sender other than
`BENEFICIARY`, to prove teardown is authorized by cap ownership rather than
caller/beneficiary identity. Extend the existing vesting teardown tests in
`vesting_quadratic_tests.move` by creating a success case that transfers the cap
away from `BENEFICIARY`, then calls `wallet.destroy_empty()` and
`quadratic::destroy(...)` with that non-beneficiary cap holder. Keep the
existing negative wrong-cap coverage, and make sure the new test clearly
exercises the happy path for the refactored authorization behavior.
Source: Path instructions
| /// #### Aborts | ||
| /// - `EWrongCap` if `cap` was minted for a different wallet. | ||
| /// - `ENotEnded` if called before the schedule's end (`start_ms + period_ms * steps`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the propagated abort in the API docs.
EWrongCap is raised by the base wallet module, so documenting it as vesting_wallet::EWrongCap avoids implying this module defines the constant.
-/// - `EWrongCap` if `cap` was minted for a different wallet.
+/// - `vesting_wallet::EWrongCap` if `cap` was minted for a different wallet.As per coding guidelines, public docs must list every abort cause accurately.
📝 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.
| /// #### Aborts | |
| /// - `EWrongCap` if `cap` was minted for a different wallet. | |
| /// - `ENotEnded` if called before the schedule's end (`start_ms + period_ms * steps`). | |
| /// #### Aborts | |
| /// - `vesting_wallet::EWrongCap` if `cap` was minted for a different wallet. | |
| /// - `ENotEnded` if called before the schedule's end (`start_ms + period_ms * steps`). |
🤖 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/sources/vesting_wallet_linear.move` around lines 392 - 394,
The public API docs for the vesting wallet function currently list an abort
constant without qualifying its origin; update the abort section in the
documented method in vesting_wallet_linear.move so the propagated abort is
explicitly attributed to vesting_wallet::EWrongCap. Keep the rest of the abort
causes intact, and ensure the docs accurately reflect that this module is
forwarding the base wallet module’s abort rather than defining the constant
itself.
Sources: Coding guidelines, Path instructions
2 participants
Resolves [CR-01]
PR Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation