feat: add allowance spend_vault module#402
Open
kosedogus wants to merge 1 commit into
Open
Conversation
Add the openzeppelin_allowance package: a shared, untyped Vault that escrows multiple coin types as object-owned address balances and grants per-(cap, coin) spending allowances to capability holders. An owner funds the vault and issues SpenderCaps with capped, optionally expiring, revocable budgets that delegates draw on demand via spend, without surrendering custody. Includes capability-gated spend, owner-side set/revoke/revoke_all/suspend controls, CAS-guarded updates, and unconditional owner withdraw/teardown. Ships the module, unit tests, integration examples under examples/spend_vault, and a package README; registers the package in the CI matrix and adds a CHANGELOG entry and the contracts index row.
kosedogus
requested review from
0xNeshi,
bidzyyys,
ericnordelo and
immrsd
as code owners
📝 WalkthroughWalkthroughThis PR introduces the Changesopenzeppelin_allowance Package
Sequence Diagram(s)sequenceDiagram
participant Owner
participant Spender
participant spend_vault
rect rgba(135, 206, 235, 0.5)
note over Owner, spend_vault: Setup
Owner->>spend_vault: new(ctx) → (Vault, OwnerCap)
Owner->>spend_vault: deposit(vault, coin, ctx)
Owner->>spend_vault: mint_cap(vault, owner_cap, ctx) → SpenderCap
Owner->>spend_vault: set_allowance(vault, owner_cap, cap_id, budget, expiry, none, clock, ctx)
Owner->>spend_vault: share(vault)
Owner->>Spender: transfer(SpenderCap)
end
rect rgba(144, 238, 144, 0.5)
note over Spender, spend_vault: Spending
Spender->>spend_vault: spend(vault, spender_cap, amount, clock, ctx) → Balance~T~
spend_vault->>spend_vault: verify binding, check allowance/expiry/budget, decrement remaining
spend_vault-->>Spender: Balance~T~ + emits Spent
end
rect rgba(255, 179, 71, 0.5)
note over Owner, spend_vault: Owner management & exit
Owner->>spend_vault: revoke(vault, owner_cap, cap_id, ctx)
Owner->>spend_vault: withdraw(vault, owner_cap, amount, ctx) → Balance~T~
Owner->>spend_vault: destroy(vault, owner_cap, ctx)
spend_vault-->>Owner: emits VaultDestroyed
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
==========================================
+ Coverage 78.88% 81.19% +2.30%
==========================================
Files 23 24 +1
Lines 1781 2053 +272
Branches 642 695 +53
==========================================
+ Hits 1405 1667 +262
- Misses 341 351 +10
Partials 35 35
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: 10
🤖 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/allowance/examples/spend_vault/defi_keeper.move`:
- Around line 66-75: The create function in defi_keeper.move currently combines
object creation with sharing in a single step and returns only the ID. Refactor
it to follow the two-step initialization pattern: modify the function to return
the Service object itself instead of just the service_id, and remove the
internal transfer::share_object(service) call. This allows callers to receive
the Service object and handle the sharing step separately, aligning with the
package's composability architecture pattern as specified in ARCHITECTURE.md.
- Around line 63-127: Add STYLEGUIDE-required structured documentation sections
to all public functions in this module. For each public function (create,
register, execute_topup, unregister, vault_id, is_registered), augment the
existing descriptive comments by adding #### Parameters, #### Returns, and ####
Aborts sections. The Parameters section should describe each function parameter,
Returns should describe the return value or type, and Aborts should list all
assertion conditions that cause the function to abort, including those from
internal calls like EWrongVaultForService, ENotOperator, ENotRegistered, and
ENoAllowance from the vault library.
In `@contracts/allowance/examples/spend_vault/direct_delegation.move`:
- Around line 27-167: The public functions open_allowance, spend_to_wallet,
change_budget, suspend, revoke_one_coin, drain_one_coin, and tear_down are
missing required documentation sections per STYLEGUIDE.md. Add structured
documentation sections to each public function: #### Parameters (listing each
parameter with its type and purpose), #### Returns (describing the return value
or lack thereof), and #### Aborts (documenting all abort conditions). Ensure
these sections are added to the comment blocks above each public function
definition while preserving the existing narrative descriptions.
- Around line 36-59: The open_allowance function currently performs internal
transfer::public_transfer and vault.share operations, which reduces PTB
composability. Refactor the function to follow a two-step
create/return-then-share model by removing the internal
transfer::public_transfer call that sends the cap to the delegate and removing
the internal vault.share() call. Instead, return the cap, vault, and owner_cap
from the function so that callers can compose the sharing and transfer
operations themselves outside the function, making it more compatible with PTBs
and aligned with the shared-object pattern guidance.
In `@contracts/allowance/examples/spend_vault/example_coin.move`:
- Around line 45-49: The `init_for_testing` function is missing required
documentation sections per STYLEGUIDE.md guidelines for public functions. Add
three new documentation sections to the existing docblock above
`init_for_testing`: add a `#### Parameters` section describing the ctx parameter
and what it represents, add a `#### Returns` section documenting the return
value (or indicating it returns nothing), and add a `#### Aborts` section
listing any conditions that would cause the function to abort (including any
aborts from the init call it makes).
In `@contracts/allowance/sources/spend_vault.move`:
- Line 679: In the documentation comment for the spend_vault.move file around
line 679, change the hyphenated word "mis-sourced" to the unhyphenated form
"missourced" to pass the typo checker and resolve the CI failure. The comment
currently reads "mistyped or mis-sourced cap_id" and should be updated to remove
the hyphen from "mis-sourced".
- Line 10: In the comment block in spend_vault.move around line 10, replace the
hyphenated compound words "mis-delivery" and "mis-authorization" with their
non-hyphenated versions "misdelivery" and "misauthorization" respectively to
resolve the CI typo checker failures that occur when hyphenated forms of "mis-"
prefix words are used.
In `@contracts/allowance/tests/spend_vault/isolation_tests.move`:
- Around line 365-368: The test function
batch_mixed_sequence_deposit_spend_revoke_one_tx has a name and documentation
describing a deposit→spend→revoke sequence, but the actual test body only
implements deposit and spend operations, missing the revoke step. Either rename
the function to remove "revoke" from its name to match the actual behavior
(e.g., batch_mixed_sequence_deposit_spend_one_tx), or add the missing revoke
operation to the test body to match the documented intent.
In `@contracts/allowance/tests/spend_vault/sv_test_utils.move`:
- Around line 31-42: The test coin type structs (USDC, SUIT, DEEP, FOO) are
declared before the constants (NO_EXPIRY, MAX_U64, START_MS), which violates the
mandated Move section ordering. Reorder the code so the constants section comes
before the structs section, moving the NO_EXPIRY, MAX_U64, and START_MS constant
definitions above the struct definitions for USDC, SUIT, DEEP, and FOO.
In `@contracts/allowance/tests/spend_vault/withdraw_tests.move`:
- Around line 123-140: The test
withdraw_succeeds_with_maximal_adversarial_ledger describes creating three
allowance states (live grant, suspended grant, and unlimited grant) but only
creates two. Add a third capability (cap_c) by calling spend_vault::mint_cap to
create the unlimited grant. Set its allowance using spend_vault::set_allowance
with the appropriate sentinel value parameters that represent an unlimited grant
state, then transfer cap_c to SPENDER just like cap_a and cap_b. This will
ensure all three allowance branches mentioned in the test comment are actually
tested.
🪄 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: e8aa2141-1fe6-49da-a308-8e47b439a216
⛔ Files ignored due to path filters (1)
contracts/allowance/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/test.ymlCHANGELOG.mdcontracts/README.mdcontracts/allowance/Move.tomlcontracts/allowance/README.mdcontracts/allowance/examples/spend_vault/defi_keeper.movecontracts/allowance/examples/spend_vault/direct_delegation.movecontracts/allowance/examples/spend_vault/example_coin.movecontracts/allowance/examples/spend_vault/tests/defi_keeper_tests.movecontracts/allowance/examples/spend_vault/tests/direct_delegation_tests.movecontracts/allowance/examples/spend_vault/tests/example_coin_tests.movecontracts/allowance/sources/spend_vault.movecontracts/allowance/tests/spend_vault/cap_budget_tests.movecontracts/allowance/tests/spend_vault/composability_tests.movecontracts/allowance/tests/spend_vault/fund_tests.movecontracts/allowance/tests/spend_vault/isolation_tests.movecontracts/allowance/tests/spend_vault/lifecycle_tests.movecontracts/allowance/tests/spend_vault/reads_tests.movecontracts/allowance/tests/spend_vault/revoke_tests.movecontracts/allowance/tests/spend_vault/spend_tests.movecontracts/allowance/tests/spend_vault/sv_test_utils.movecontracts/allowance/tests/spend_vault/withdraw_tests.move
Comment on lines
+63
to
+127
| /// Create and share a service pinned to `vault_id`. The creator becomes the | ||
| /// operator, the only address the cap-borrowing entrypoint accepts. Returns the | ||
| /// service's object id so callers can address the shared object. | ||
| public fun create(vault_id: ID, ctx: &mut TxContext): ID { | ||
| let service = Service { | ||
| id: object::new(ctx), | ||
| operator: ctx.sender(), | ||
| vault_id, | ||
| caps: table::new(ctx), | ||
| }; | ||
| let service_id = object::id(&service); | ||
| transfer::share_object(service); | ||
| service_id | ||
| } | ||
|
|
||
| /// Hand a cap into the service's custody, keyed by the registering sender. | ||
| /// | ||
| /// The binding check is the custody-boundary rule for ANY protocol that accepts a | ||
| /// `SpenderCap`: validate `spender_cap_vault_id` against the vault you intend to | ||
| /// spend from, on-chain, BEFORE taking the cap. | ||
| public fun register(s: &mut Service, cap: SpenderCap, ctx: &mut TxContext) { | ||
| assert!(cap.spender_cap_vault_id() == s.vault_id, EWrongVaultForService); | ||
| s.caps.add(ctx.sender(), cap); | ||
| } | ||
|
|
||
| /// Draw `amount` of coin `T` from `user`'s allowance and return the funds for the | ||
| /// caller to route (into a position, a `Coin`, ...). Generic over `T`, so the same | ||
| /// custodied cap serves every coin the owner budgeted it for; asking for a coin the | ||
| /// owner never granted aborts inside the library (`ENoAllowance`), so this fails | ||
| /// safe. | ||
| /// | ||
| /// SENDER-GATED: the operator check below is the security boundary. The library | ||
| /// never checks who calls `spend`, so the custody layer must. | ||
| public fun execute_topup<T>( | ||
| s: &mut Service, | ||
| v: &mut Vault, | ||
| user: address, | ||
| amount: u64, | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ): Balance<T> { | ||
| assert!(ctx.sender() == s.operator, ENotOperator); | ||
| assert!(s.caps.contains(user), ENotRegistered); | ||
|
|
||
| let cap = s.caps.borrow(user); | ||
| v.spend<T>(cap, amount, clock, ctx) | ||
| } | ||
|
|
||
| /// Take a cap back out of custody. The grant is untouched: it stays live in the | ||
| /// vault; only custody of the cap changes hands. | ||
| public fun unregister(s: &mut Service, ctx: &mut TxContext): SpenderCap { | ||
| assert!(s.caps.contains(ctx.sender()), ENotRegistered); | ||
| s.caps.remove(ctx.sender()) | ||
| } | ||
|
|
||
| // === View helpers === | ||
|
|
||
| /// The vault this service is pinned to. | ||
| public fun vault_id(s: &Service): ID { | ||
| s.vault_id | ||
| } | ||
|
|
||
| /// Whether `user` currently has a cap in custody. | ||
| public fun is_registered(s: &Service, user: address): bool { | ||
| s.caps.contains(user) |
There was a problem hiding this comment.
Public function docs need required Parameters/Returns/Aborts sections.
The public API comments are descriptive, but they are missing the STYLEGUIDE-required structured sections and complete abort listings.
As per coding guidelines, STYLEGUIDE.md requires /// docs for public APIs with #### Parameters, #### Returns, and complete #### Aborts.
🤖 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/allowance/examples/spend_vault/defi_keeper.move` around lines 63 -
127, Add STYLEGUIDE-required structured documentation sections to all public
functions in this module. For each public function (create, register,
execute_topup, unregister, vault_id, is_registered), augment the existing
descriptive comments by adding #### Parameters, #### Returns, and #### Aborts
sections. The Parameters section should describe each function parameter,
Returns should describe the return value or type, and Aborts should list all
assertion conditions that cause the function to abort, including those from
internal calls like EWrongVaultForService, ENotOperator, ENotRegistered, and
ENoAllowance from the vault library.
Source: Coding guidelines
Comment on lines
+66
to
+75
| public fun create(vault_id: ID, ctx: &mut TxContext): ID { | ||
| let service = Service { | ||
| id: object::new(ctx), | ||
| operator: ctx.sender(), | ||
| vault_id, | ||
| caps: table::new(ctx), | ||
| }; | ||
| let service_id = object::id(&service); | ||
| transfer::share_object(service); | ||
| service_id |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use two-step service initialization (return object first, share separately).
create currently shares internally and returns only ID. Returning the Service object and sharing in a separate step would align with the package’s composability/shared-object architecture pattern.
As per coding guidelines, ARCHITECTURE.md prescribes two-step shared-object flows (create/return then share) and prefers returning objects over internal transfer/share in create-style APIs.
🤖 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/allowance/examples/spend_vault/defi_keeper.move` around lines 66 -
75, The create function in defi_keeper.move currently combines object creation
with sharing in a single step and returns only the ID. Refactor it to follow the
two-step initialization pattern: modify the function to return the Service
object itself instead of just the service_id, and remove the internal
transfer::share_object(service) call. This allows callers to receive the Service
object and handle the sharing step separately, aligning with the package's
composability architecture pattern as specified in ARCHITECTURE.md.
Source: Coding guidelines
Comment on lines
+27
to
+167
| /// One PTB that creates the vault, funds it, mints a cap for `delegate`, grants a | ||
| /// budget, shares the vault, and returns the OwnerCap for the caller to keep or route. | ||
| /// | ||
| /// Order matters: every fund / mint / grant step must precede `share`, because the | ||
| /// Vault is only addressable as a shared input in LATER transactions. The Vault has | ||
| /// no `drop`, so the tx fails unless it is consumed by `share` (or `destroy`). | ||
| /// | ||
| /// Returns the `OwnerCap` by value rather than self-transferring it, so the caller | ||
| /// (or the enclosing PTB) decides its destination: composable. | ||
| public fun open_allowance<T>( | ||
| funding: Coin<T>, | ||
| delegate: address, | ||
| budget: u64, | ||
| expires_at_ms: u64, // pass std::u64::max_value!() for "no expiry" | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ): OwnerCap { | ||
| let (mut vault, owner_cap) = spend_vault::new(ctx); | ||
|
|
||
| // Permissionless top-up. Confers no rights; the funds become the owner's pool. | ||
| vault.deposit(funding, ctx); | ||
|
|
||
| // Bare cap, no budget yet. Returned by value, so we choose its destination. | ||
| let cap = vault.mint_cap(&owner_cap, ctx); | ||
| let cap_id = object::id(&cap); | ||
| transfer::public_transfer(cap, delegate); | ||
|
|
||
| // Create the (cap, T) budget. `option::none()` = no CAS guard on a fresh create. | ||
| vault.set_allowance<T>(&owner_cap, cap_id, budget, expires_at_ms, option::none(), clock, ctx); | ||
|
|
||
| // Make the vault spendable, then hand owner authority back to the caller. | ||
| vault.share(); | ||
| owner_cap | ||
| } | ||
|
|
||
| // === Delegate spends === | ||
|
|
||
| /// The delegate draws `amount` of `T` and gets it back as a wallet `Coin<T>`. | ||
| /// | ||
| /// `spend` returns `Balance<T>`, which has no `drop`: it must be consumed in the | ||
| /// same PTB. Here we turn it into a `Coin` and return it, so the caller (or the | ||
| /// enclosing PTB) decides where the coin goes: composable. A spend aborts (with a | ||
| /// distinct, deterministic code) if the cap is wrong, the (cap, T) entry is missing, | ||
| /// the grant expired, or the amount is zero or over budget. It can also fail with the | ||
| /// `InsufficientFundsForWithdraw` execution status at `redeem_funds` if the pool is | ||
| /// short (surfaced via the SDK / a dry run, not a Move abort code). | ||
| /// | ||
| /// Note `ctx: &mut TxContext`: both `into_coin` (to mint the Coin) and `spend` take | ||
| /// `&mut TxContext`, so one parameter serves both. | ||
| public fun spend_to_wallet<T>( | ||
| vault: &mut Vault, | ||
| cap: &SpenderCap, | ||
| amount: u64, | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ): Coin<T> { | ||
| let bal = vault.spend<T>(cap, amount, clock, ctx); | ||
| bal.into_coin(ctx) | ||
| } | ||
|
|
||
| // === Owner manages the grant === | ||
|
|
||
| /// Raise / lower / renew with the race-free CAS idiom: read, then write with | ||
| /// `expected = Some(current)` in the SAME PTB. If a spend was sequenced between the | ||
| /// read and the write, the call aborts `EUnexpectedAllowance` instead of clobbering. | ||
| public fun change_budget<T>( | ||
| vault: &mut Vault, | ||
| owner_cap: &OwnerCap, | ||
| cap_id: ID, | ||
| new_budget: u64, | ||
| new_expires_at_ms: u64, | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ) { | ||
| let current = vault.allowance<T>(cap_id); | ||
| vault.set_allowance<T>( | ||
| owner_cap, | ||
| cap_id, | ||
| new_budget, | ||
| new_expires_at_ms, | ||
| option::some(current), | ||
| clock, | ||
| ctx, | ||
| ); | ||
| } | ||
|
|
||
| /// Suspend a grant without removing it: zero the budget but keep the entry + cap | ||
| /// alive. The next `spend<T>` aborts `EAllowanceExceeded` (NOT `ENoAllowance`), so | ||
| /// the spender knows to ask the owner to raise rather than to ask for a new grant. | ||
| public fun suspend<T>( | ||
| vault: &mut Vault, | ||
| owner_cap: &OwnerCap, | ||
| cap_id: ID, | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ) { | ||
| // new_amount = 0 = suspend. A finite expiry must still be future, so reuse "no expiry". | ||
| vault.set_allowance<T>( | ||
| owner_cap, | ||
| cap_id, | ||
| 0, | ||
| std::u64::max_value!(), | ||
| option::none(), | ||
| clock, | ||
| ctx, | ||
| ); | ||
| } | ||
|
|
||
| /// Owner kill-switch for one coin. Idempotent: returns whether anything was removed | ||
| /// (`false` is the typo'd-cap_id / wrong-coin signal). Cannot be raced into failure. | ||
| public fun revoke_one_coin<T>( | ||
| vault: &mut Vault, | ||
| owner_cap: &OwnerCap, | ||
| cap_id: ID, | ||
| ctx: &mut TxContext, | ||
| ): bool { | ||
| vault.revoke<T>(owner_cap, cap_id, ctx) | ||
| } | ||
|
|
||
| // === Owner exit + teardown === | ||
|
|
||
| /// Drain one coin's settled pool to the owner. Needs the AccumulatorRoot (0xacc). | ||
| /// Run this in its OWN tx, never in the same PTB as a `spend` / `withdraw` on this | ||
| /// vault: a same-checkpoint pool drop makes the settled read over-ask and abort (the | ||
| /// settled-vs-live pool skew), retry-safe next checkpoint. | ||
| public fun drain_one_coin<T>( | ||
| vault: &mut Vault, | ||
| owner_cap: &OwnerCap, | ||
| root: &AccumulatorRoot, | ||
| ctx: &mut TxContext, | ||
| ): Balance<T> { | ||
| vault.withdraw_all<T>(owner_cap, root, ctx) | ||
| } | ||
|
|
||
| /// Tear down the vault. PRECONDITION: every coin already drained via | ||
| /// `withdraw_all<T>` (enumerate types off-chain with `suix_getAllBalances`), or any | ||
| /// remaining funds strand permanently at the dead vault address. `destroy` drains | ||
| /// only the budget ledger and deletes the UIDs; it does not touch the pool. | ||
| public fun tear_down(vault: Vault, owner_cap: OwnerCap, ctx: &mut TxContext) { | ||
| vault.destroy(owner_cap, ctx); | ||
| } |
There was a problem hiding this comment.
Public API docs are missing required STYLEGUIDE sections.
The exported functions in this module have narrative comments, but they do not include the required structured #### Parameters, #### Returns, and complete #### Aborts sections.
As per coding guidelines, STYLEGUIDE.md mandates those sections for public APIs.
🤖 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/allowance/examples/spend_vault/direct_delegation.move` around lines
27 - 167, The public functions open_allowance, spend_to_wallet, change_budget,
suspend, revoke_one_coin, drain_one_coin, and tear_down are missing required
documentation sections per STYLEGUIDE.md. Add structured documentation sections
to each public function: #### Parameters (listing each parameter with its type
and purpose), #### Returns (describing the return value or lack thereof), and
#### Aborts (documenting all abort conditions). Ensure these sections are added
to the comment blocks above each public function definition while preserving the
existing narrative descriptions.
Source: Coding guidelines
Comment on lines
+36
to
+59
| public fun open_allowance<T>( | ||
| funding: Coin<T>, | ||
| delegate: address, | ||
| budget: u64, | ||
| expires_at_ms: u64, // pass std::u64::max_value!() for "no expiry" | ||
| clock: &Clock, | ||
| ctx: &mut TxContext, | ||
| ): OwnerCap { | ||
| let (mut vault, owner_cap) = spend_vault::new(ctx); | ||
|
|
||
| // Permissionless top-up. Confers no rights; the funds become the owner's pool. | ||
| vault.deposit(funding, ctx); | ||
|
|
||
| // Bare cap, no budget yet. Returned by value, so we choose its destination. | ||
| let cap = vault.mint_cap(&owner_cap, ctx); | ||
| let cap_id = object::id(&cap); | ||
| transfer::public_transfer(cap, delegate); | ||
|
|
||
| // Create the (cap, T) budget. `option::none()` = no CAS guard on a fresh create. | ||
| vault.set_allowance<T>(&owner_cap, cap_id, budget, expires_at_ms, option::none(), clock, ctx); | ||
|
|
||
| // Make the vault spendable, then hand owner authority back to the caller. | ||
| vault.share(); | ||
| owner_cap |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Refactor open_allowance to a two-step create/return flow instead of internal share/transfer.
This helper currently performs internal transfer::public_transfer and vault.share(), which makes the flow less PTB-composable and deviates from the repo’s shared-object pattern guidance.
As per coding guidelines, ARCHITECTURE.md requires shared-object integrations to follow a two-step create/return-then-share model and to prefer returning objects over internal transfer::* where possible.
🤖 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/allowance/examples/spend_vault/direct_delegation.move` around lines
36 - 59, The open_allowance function currently performs internal
transfer::public_transfer and vault.share operations, which reduces PTB
composability. Refactor the function to follow a two-step
create/return-then-share model by removing the internal
transfer::public_transfer call that sends the cap to the delegate and removing
the internal vault.share() call. Instead, return the cap, vault, and owner_cap
from the function so that callers can compose the sharing and transfer
operations themselves outside the function, making it more compatible with PTBs
and aligned with the shared-object pattern guidance.
Source: Coding guidelines
Comment on lines
+45
to
+49
| /// Run `init` under test, minting the fixed supply to the sender so an example vault | ||
| /// can be funded. | ||
| #[test_only] | ||
| public fun init_for_testing(ctx: &mut TxContext) { | ||
| init(EXAMPLE_COIN {}, ctx) |
There was a problem hiding this comment.
Add required public-API doc sections to init_for_testing.
This docblock is missing the required #### Parameters, #### Returns, and #### Aborts sections for a public function.
As per coding guidelines, STYLEGUIDE.md requires public APIs to include /// docs with #### Parameters, #### Returns, and a complete #### Aborts listing.
🤖 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/allowance/examples/spend_vault/example_coin.move` around lines 45 -
49, The `init_for_testing` function is missing required documentation sections
per STYLEGUIDE.md guidelines for public functions. Add three new documentation
sections to the existing docblock above `init_for_testing`: add a `####
Parameters` section describing the ctx parameter and what it represents, add a
`#### Returns` section documenting the return value (or indicating it returns
nothing), and add a `#### Aborts` section listing any conditions that would
cause the function to abort (including any aborts from the init call it makes).
Source: Coding guidelines
| /// > One untyped cap now spans N coin budgets, so a leaked cap | ||
| /// > exposes the SUM across all coins the owner granted it. The library never | ||
| /// > inspects holder identity, provenance, or intent; transfer of the cap is | ||
| /// > transfer of authority, and mis-delivery is mis-authorization. There is no |
There was a problem hiding this comment.
Fix CI-blocking typos: hyphenated "mis-" words.
The pipeline is failing on mis-delivery and mis-authorization. While these are valid compound words, the typo checker doesn't recognize the hyphenated form.
📝 Suggested fix
-/// transfer of authority, and mis-delivery is mis-authorization. There is no
+/// transfer of authority, and misdelivery is misauthorization. There is no🧰 Tools
🪛 GitHub Actions: Typo Check / 0_Check for typos.txt
[warning] 10-10: typos warning: "mis" should be "miss" or "mist".
[warning] 10-10: typos warning: "mis" should be "miss" or "mist".
[warning] 10-10: typos warning: "mis" should be "miss" or "mist".
[error] 10-10: typos error: mis should be miss, mist.
[error] 10-10: typos error: mis should be miss, mist.
🪛 GitHub Actions: Typo Check / Check for typos
[warning] 10-10: typos: "mis" should be "miss" or "mist".
[warning] 10-10: typos: "mis" should be "miss" or "mist".
[warning] 10-10: typos: "mis" should be "miss" or "mist".
[error] 10-10: typos (error): mis should be miss, mist.
[error] 10-10: typos (error): mis should be miss, mist.
🪛 GitHub Check: Check for typos
[warning] 10-10:
"mis" should be "miss" or "mist".
[warning] 10-10:
"mis" should be "miss" or "mist".
🤖 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/allowance/sources/spend_vault.move` at line 10, In the comment
block in spend_vault.move around line 10, replace the hyphenated compound words
"mis-delivery" and "mis-authorization" with their non-hyphenated versions
"misdelivery" and "misauthorization" respectively to resolve the CI typo checker
failures that occur when hyphenated forms of "mis-" prefix words are used.
Source: Pipeline failures
| /// **`cap_id` is the SPENDER cap's object id** (`object::id(&spender_cap)`, the | ||
| /// id `mint_cap` minted), NOT the OwnerCap's id. It is UNVALIDATED by design | ||
| /// (kept a bare ID) and the owner gate only checks the OwnerCap, so a | ||
| /// mistyped or mis-sourced `cap_id` silently targets a different budget. The |
There was a problem hiding this comment.
Fix CI-blocking typo: "mis-sourced".
The pipeline is failing on mis-sourced. Use the unhyphenated form to pass the typo checker.
📝 Suggested fix
-/// mistyped or mis-sourced `cap_id` silently targets a different budget. The
+/// mistyped or missourced `cap_id` silently targets a different budget. The📝 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
| /// mistyped or mis-sourced `cap_id` silently targets a different budget. The | |
| /// mistyped or missourced `cap_id` silently targets a different budget. The |
🧰 Tools
🪛 GitHub Actions: Typo Check / 0_Check for typos.txt
[error] 679-679: typos error: mis should be miss, mist.
🪛 GitHub Actions: Typo Check / Check for typos
[error] 679-679: typos (error): mis should be miss, mist.
🪛 GitHub Check: Check for typos
[warning] 679-679:
"mis" should be "miss" or "mist".
🤖 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/allowance/sources/spend_vault.move` at line 679, In the
documentation comment for the spend_vault.move file around line 679, change the
hyphenated word "mis-sourced" to the unhyphenated form "missourced" to pass the
typo checker and resolve the CI failure. The comment currently reads "mistyped
or mis-sourced cap_id" and should be updated to remove the hyphen from
"mis-sourced".
Source: Pipeline failures
Comment on lines
+365
to
+368
| fun batch_mixed_sequence_deposit_spend_revoke_one_tx() { | ||
| // A mixed interleaving in ONE tx (deposit, then spend, then revoke a | ||
| // different coin) all compose: no one-call-per-tx assumption anywhere. | ||
| let mut s = ts::begin(OWNER); |
There was a problem hiding this comment.
Rename this test or add the missing revoke step.
Line 365 and its comments describe a deposit→spend→revoke sequence, but the body only does deposit and spend. This makes the test intent and actual coverage diverge.
Suggested minimal fix (rename to match behavior)
-#[test]
-fun batch_mixed_sequence_deposit_spend_revoke_one_tx() {
- // A mixed interleaving in ONE tx (deposit, then spend, then revoke a
- // different coin) all compose: no one-call-per-tx assumption anywhere.
+#[test]
+fun batch_mixed_sequence_deposit_spend_one_tx() {
+ // A mixed interleaving in ONE tx (deposit, then spend) composes:
+ // no one-call-per-tx assumption.Also applies to: 369-385
🤖 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/allowance/tests/spend_vault/isolation_tests.move` around lines 365
- 368, The test function batch_mixed_sequence_deposit_spend_revoke_one_tx has a
name and documentation describing a deposit→spend→revoke sequence, but the
actual test body only implements deposit and spend operations, missing the
revoke step. Either rename the function to remove "revoke" from its name to
match the actual behavior (e.g., batch_mixed_sequence_deposit_spend_one_tx), or
add the missing revoke operation to the test body to match the documented
intent.
Comment on lines
+31
to
+42
| // === Test coin types (distinct defining types so BudgetKeys differ) === | ||
|
|
||
| public struct USDC has drop {} | ||
| public struct SUIT has drop {} // a stand-in "SUI"-like coin (avoids the real sui::sui::SUI) | ||
| public struct DEEP has drop {} | ||
| public struct FOO has drop {} // junk type for un-griefability / wrong-coin tests | ||
|
|
||
| // === Common constants (consts are module-private in Move; expose via fns) === | ||
|
|
||
| const NO_EXPIRY: u64 = 18_446_744_073_709_551_615; // u64::MAX sentinel | ||
| const MAX_U64: u64 = 18_446_744_073_709_551_615; | ||
| const START_MS: u64 = 1_700_000_000_000; // a fixed "now" base for clock tests |
There was a problem hiding this comment.
Reorder constants and structs to match mandated Move section ordering.
At Line 33-Line 42, structs are declared before constants. The repository Move order requires constants before structs.
Suggested reorder
-// === Test coin types (distinct defining types so BudgetKeys differ) ===
-
-public struct USDC has drop {}
-public struct SUIT has drop {} // a stand-in "SUI"-like coin (avoids the real sui::sui::SUI)
-public struct DEEP has drop {}
-public struct FOO has drop {} // junk type for un-griefability / wrong-coin tests
-
// === Common constants (consts are module-private in Move; expose via fns) ===
const NO_EXPIRY: u64 = 18_446_744_073_709_551_615; // u64::MAX sentinel
const MAX_U64: u64 = 18_446_744_073_709_551_615;
const START_MS: u64 = 1_700_000_000_000; // a fixed "now" base for clock tests
+
+// === Test coin types (distinct defining types so BudgetKeys differ) ===
+
+public struct USDC has drop {}
+public struct SUIT has drop {} // a stand-in "SUI"-like coin (avoids the real sui::sui::SUI)
+public struct DEEP has drop {}
+public struct FOO has drop {} // junk type for un-griefability / wrong-coin testsAs per coding guidelines: “Keep Move section ordering exactly: imports; // === Errors ===; // === Constants ===; // === Structs ===; …”.
📝 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
| // === Test coin types (distinct defining types so BudgetKeys differ) === | |
| public struct USDC has drop {} | |
| public struct SUIT has drop {} // a stand-in "SUI"-like coin (avoids the real sui::sui::SUI) | |
| public struct DEEP has drop {} | |
| public struct FOO has drop {} // junk type for un-griefability / wrong-coin tests | |
| // === Common constants (consts are module-private in Move; expose via fns) === | |
| const NO_EXPIRY: u64 = 18_446_744_073_709_551_615; // u64::MAX sentinel | |
| const MAX_U64: u64 = 18_446_744_073_709_551_615; | |
| const START_MS: u64 = 1_700_000_000_000; // a fixed "now" base for clock tests | |
| // === Common constants (consts are module-private in Move; expose via fns) === | |
| const NO_EXPIRY: u64 = 18_446_744_073_709_551_615; // u64::MAX sentinel | |
| const MAX_U64: u64 = 18_446_744_073_709_551_615; | |
| const START_MS: u64 = 1_700_000_000_000; // a fixed "now" base for clock tests | |
| // === Test coin types (distinct defining types so BudgetKeys differ) === | |
| public struct USDC has drop {} | |
| public struct SUIT has drop {} // a stand-in "SUI"-like coin (avoids the real sui::sui::SUI) | |
| public struct DEEP has drop {} | |
| public struct FOO has drop {} // junk type for un-griefability / wrong-coin tests |
🤖 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/allowance/tests/spend_vault/sv_test_utils.move` around lines 31 -
42, The test coin type structs (USDC, SUIT, DEEP, FOO) are declared before the
constants (NO_EXPIRY, MAX_U64, START_MS), which violates the mandated Move
section ordering. Reorder the code so the constants section comes before the
structs section, moving the NO_EXPIRY, MAX_U64, and START_MS constant
definitions above the struct definitions for USDC, SUIT, DEEP, and FOO.
Source: Coding guidelines
Comment on lines
+123
to
+140
| // Build a vault carrying a live grant, a suspended grant, and an unlimited | ||
| // grant across two caps; withdraw still succeeds (no ledger consult). No | ||
| // spender state can block the owner exit. Non-root path only. | ||
| let mut s = ts::begin(OWNER); | ||
| let clk = u::clock_at(u::start_ms(), s.ctx()); | ||
| let (mut v, oc) = spend_vault::new(s.ctx()); | ||
| let vid = object::id(&v); | ||
| spend_vault::deposit(&v, sui::coin::mint_for_testing<USDC>(1_000, s.ctx()), s.ctx()); | ||
| // cap A: a live finite grant. | ||
| let cap_a = spend_vault::mint_cap(&v, &oc, s.ctx()); | ||
| let cid_a = object::id(&cap_a); | ||
| spend_vault::set_allowance<USDC>(&mut v, &oc, cid_a, 500, MAXU64, option::none(), &clk, s.ctx()); | ||
| // cap B: a suspended grant (remaining == 0) + an unlimited grant (sentinel). | ||
| let cap_b = spend_vault::mint_cap(&v, &oc, s.ctx()); | ||
| let cid_b = object::id(&cap_b); | ||
| spend_vault::set_allowance<USDC>(&mut v, &oc, cid_b, 0, MAXU64, option::none(), &clk, s.ctx()); | ||
| transfer::public_transfer(cap_a, SPENDER); | ||
| transfer::public_transfer(cap_b, SPENDER); |
There was a problem hiding this comment.
withdraw_succeeds_with_maximal_adversarial_ledger does not actually create an unlimited allowance state.
At Line 123-Line 138, the test describes live+suspended+unlimited states, but only finite (cap A) and suspended (cap B) grants are set. This leaves the unlimited branch untested.
Suggested patch to include an unlimited grant
// cap B: a suspended grant (remaining == 0) + an unlimited grant (sentinel).
let cap_b = spend_vault::mint_cap(&v, &oc, s.ctx());
let cid_b = object::id(&cap_b);
spend_vault::set_allowance<USDC>(&mut v, &oc, cid_b, 0, MAXU64, option::none(), &clk, s.ctx());
+ let cap_c = spend_vault::mint_cap(&v, &oc, s.ctx());
+ let cid_c = object::id(&cap_c);
+ spend_vault::set_allowance<USDC>(&mut v, &oc, cid_c, MAXU64, MAXU64, option::none(), &clk, s.ctx());
transfer::public_transfer(cap_a, SPENDER);
transfer::public_transfer(cap_b, SPENDER);
+ transfer::public_transfer(cap_c, SPENDER);🤖 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/allowance/tests/spend_vault/withdraw_tests.move` around lines 123 -
140, The test withdraw_succeeds_with_maximal_adversarial_ledger describes
creating three allowance states (live grant, suspended grant, and unlimited
grant) but only creates two. Add a third capability (cap_c) by calling
spend_vault::mint_cap to create the unlimited grant. Set its allowance using
spend_vault::set_allowance with the appropriate sentinel value parameters that
represent an unlimited grant state, then transfer cap_c to SPENDER just like
cap_a and cap_b. This will ensure all three allowance branches mentioned in the
test comment are actually tested.
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
1 participant
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.
Resolves #178
Validity checks performed:
/sui-bttin ultracode/sui-basic-reviewin ultracodeCoverage note: the funds-accumulator paths (
withdraw_all, the&AccumulatorRoot-taking reads, and the native pool-short abort) cannot run inthe Move unit-test VM (it cannot construct an
AccumulatorRoot); they areexercised by a separate on-chain test harness, available on request.
PR Checklist
Summary by CodeRabbit
New Features
spend_vaultmodule for capability-based multi-coin spending allowances, featuring budget limits, optional expiration times, revocation controls, and owner-side management functions.Documentation