test+docs(rentals): review follow-ups (idempotent endrent, invalid-listing cancel, delphi extension, wiki rewrite) - #12
Merged
robrigo merged 3 commits intoJul 2, 2026
Conversation
There was a problem hiding this comment.
Pull request overview
Adds regression coverage and updates documentation for the non-custodial rentals flow (renter-as-owner, AtomicAssets leases as the lock source of truth), addressing gaps surfaced by an independent security review without changing contract code.
Changes:
- Add smoke/regression tests for: delphi-priced rental extension settlement, idempotent
endrent, and permissionlesscancelrenton invalid listings. - Rewrite Rentals wiki page to describe the shipped non-custodial model and current table semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/market-smoke.test.js | Adds 3 new rental regression tests covering oracle settlement on extension, idempotent endrent, and invalid-listing cancelrent. |
| docs/wiki/Rentals.md | Updates lifecycle, action semantics, and integration notes to match non-custodial rentals and leases-based lock state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+829
to
+831
| await expect( | ||
| atomicmarket.actions.endrent([ASSET1]).send('renter2@active') | ||
| ).resolves.not.toThrow(); |
robrigo
force-pushed
the
experiment/noncustodial-rentals-review-followups
branch
from
July 2, 2026 17:14
5c45633 to
36abc33
Compare
Comment on lines
+831
to
+833
| await expect( | ||
| atomicmarket.actions.endrent([ASSET1]).send('renter2@active') | ||
| ).resolves.not.toThrow(); |
robrigo
force-pushed
the
experiment/noncustodial-rentals
branch
from
July 2, 2026 17:27
acccaa8 to
92465d0
Compare
robrigo
force-pushed
the
experiment/noncustodial-rentals-review-followups
branch
from
July 2, 2026 17:27
36abc33 to
97091e7
Compare
robrigo
force-pushed
the
experiment/noncustodial-rentals
branch
from
July 2, 2026 17:32
92465d0 to
a07f751
Compare
…hi extension Regression tests for three paths the security review flagged as unguarded: - endrent on an already-reclaimed asset is a no-op, not a throw - an invalid listing (owner moved the asset) is cancellable by anyone, while a still-valid one rejects a non-owner cancel - a delphi-priced rental extension settles the added hours at the oracle rate via the leaseextend path
The page still described the old custodial holder model, which the contract now rejects. Rewrite to renter-as-owner: leases is the source of truth, no rental-memo custody step, permissionless un-vetoable reclaim, idempotent endrent, and the current immutable rentals table shape.
robrigo
force-pushed
the
experiment/noncustodial-rentals-review-followups
branch
from
July 2, 2026 17:32
97091e7 to
771d9ed
Compare
robrigo
added a commit
that referenced
this pull request
Jul 3, 2026
Rentals are descoped from the V2 release so the rest of V2 can ship without them. Removed in full: - actions announcerent / cancelrent / rentasset / endrent / payrentram - log actions lognewrent / logrentstart / logrental - rentals table (+ rentals_s, rentalends index, get_rentals accessor) - the "rental" memo branch in receive_asset_transfer - holders_s / get_holders from the AtomicAssets interface header (the custodial holdership mechanism is being removed from AtomicAssets V2 in the companion PR atomicassets-contract#27) - ricardian clauses, wiki page, README/CLAUDE.md/api-integration.md sections internal_payout_sale, consume_counter and the counters table are shared with sales/auctions and stay; only the rental call sites are gone. The migrate action never seeded a rental counter, so no migration change. ABI diff vs v2.0.0-rc1: exactly the 8 rental actions + rentals table (and their structs) removed. Rentals live on: the custodial implementation is preserved on archive/v2-custodial-rentals + the v2.0.0-rc1 tag; the non-custodial rework continues on experiment/noncustodial-rentals (#11, #12). Tests: 8 rental cases + the rentals table helper removed from market-smoke; auctions.test.js kept (renter accounts there are generic bidders). Suite: 6 suites, 220 passing.
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
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.
Stacked on
experiment/noncustodial-rentals(AtomicMarket #11). Follow-ups from the independent security review of the non-custodial rentals contracts: no critical/high issues were found, and these close three of the cheap gaps it surfaced. Tests and docs only — no contract.cppchanges, no wasm rebuild.What's here
Regression tests (
tests/market-smoke.test.js), each pinning a path the review flagged as un-covered:endrent— a secondendrenton an already-reclaimed asset must be a no-op, not a throw. Guards the early-return path added in658ef1e(src/atomicmarket.cpp:2094-2097) so a keeper reclaim followed by anendrent, or two racingendrentcalls, can't brick.cancelrenton an invalid listing — after the lister moves the asset out of band the listing is invalid and anyone may cancel it; a still-valid listing still rejects a non-owner cancel. Covers theis_rental_invalidbranch (src/atomicmarket.cpp:1918-1922).leaseextendinline path (no second ownership flip, lease end advanced, payouts doubled).Docs (
docs/wiki/Rentals.md) — the page still described the old custodial holder model, which the contract now explicitly rejects (arental-memo transfer aborts). Rewritten to the shipped non-custodial renter-as-owner model:leasesis the single source of truth, no custody step, permissionless un-vetoablereclaim, idempotentendrent, and the current immutablerentalstable shape (dropped the removedholder/rental_end/asset_transferredfields).Verification
npx jest— 6 suites / 235 tests pass (was 232; +3 new). Nomake buildneeded.