Experimental: non-custodial renter-as-owner rental primitives - #26
Experimental: non-custodial renter-as-owner rental primitives#26robrigo wants to merge 12 commits into
Conversation
Replace the custodial holders/move machinery with a native lock + title model. During a lease the renter becomes the real AtomicAssets owner, the lister's reclaim right is parked in a `leases` row, and the asset is locked from transfer/burn/offer-out while that row exists. A permissionless `reclaim` force-returns the asset to the title_owner at expiry, so neither party can strand it. - remove the move action, holders table and logmove - add the leases table + check_not_leased guards on internal_transfer (chokepoint, covers transfer + acceptoffer), burnasset and createoffer; setassetdata is deliberately left unguarded (collection-auth gated) - add pretitle/leasestart/leaseextend/delpretitle/reclaim, a config'd rental_market authority gate (setrentmkt, defaults to atomicmarket) and loglock/logreclaim log actions; offers are cleared on lease-start/reclaim - replace the custodial renting characterization tests with a non-custodial lease/lock suite Experimental branch for smart-contract review.
… singleton) - Drop pretitle/delpretitle and the sentinel lease state. leasestart is always the direct trusted-market path (lister consent is captured by AtomicMarket's announcerent). A leases row now always means an active lease. - Store the configured rental market in a new rentalcfg singleton (default atomicmarket, read via get_or_default) instead of appending to config_s, so deploying onto the existing live contract needs no config migration. - Remove offer-clearing on lease-start/reclaim. A pre-existing offer now survives a rental (it cannot settle while the asset is locked) and becomes acceptable again after reclaim. This also removes a renter-controlled unbounded loop on the permissionless reclaim path. - Add a check_rental_market helper; remove the dead renter=="" re-checks. - Update the VeRT suite (lease lifecycle, authority, reclaim, offer survival).
There was a problem hiding this comment.
Pull request overview
This PR prototypes non-custodial “renter-as-owner” rental primitives in the AtomicAssets contract by replacing the legacy custodial holder/move model with an on-chain lease lock (leases) that prevents renter-controlled extraction while enabling permissionless reclaim after expiry.
Changes:
- Removes the custodial
moveaction andholderstable, and introducesleases+rentalcfg(configured rental-market authority) plus new lifecycle actions (leasestart,leaseextend,reclaim,setrentmkt). - Enforces a rental lock across renter-reachable extraction paths (transfer via
internal_transfer,burnasset,createoffer), and addsloglock/logreclaimfor indexers. - Updates tests to drop holders/move behavior and add a dedicated non-custodial rental invariants suite.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Transfer-Offer Actions/transfer.test.js | Removes holder-record transfer tests tied to the deleted custodial holders model. |
| tests/Deposit-Withdraw-Back-Burn Actions/burnasset.test.js | Removes holder-record burn test tied to the deleted holders model. |
| tests/Asset Actions/renting-invariants.test.js | Replaces characterization tests with non-custodial rental lifecycle/lock/reclaim/authority tests. |
| tests/Asset Actions/move.test.js | Deletes the move action test suite (action removed). |
| src/atomicassets.cpp | Implements rentalcfg singleton + leasestart/leaseextend/reclaim, lock enforcement, and new log actions; removes holders logic. |
| include/atomicassets.hpp | Exposes new actions, tables, and helpers; replaces holders accessors with leases accessors; updates internal_transfer signature. |
| include/atomicassets-interface.hpp | Updates the external interface view of the contract tables from holders to leases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| check_rental_market(market); | ||
|
|
||
| if (holders_itr != holders.end()){ | ||
| if (holders_itr->holder != from){ | ||
| check(false, | ||
| ("At least one asset invalidates the 'from:holder' constraint (ID: " + to_string(asset_id) + ")").c_str()); | ||
| } | ||
| leases_t leases = get_leases(); | ||
| auto lease_itr = leases.require_find(asset_id, "Asset is not leased"); | ||
| check(rental_end > lease_itr->rental_end, "rental_end must be later than the current end"); | ||
|
|
There was a problem hiding this comment.
Good catch, fixed in 5a89ff1: leaseextend now requires now < rental_end, so an expired lease can only be reclaimed, never extended. The configured market can no longer race the permissionless reclaim. AtomicMarket only ever extends an active lease, so the normal flow is unaffected; added a VeRT regression for the post-expiry case.
…eriment/noncustodial-rentals # Conflicts: # include/atomicassets-interface.hpp # tests/asset-actions/move.test.js # tests/asset-actions/renting-invariants.test.js
leaseextend only required the new end to be later than the current one, so once a lease had expired the configured market could race the permissionless reclaim and push rental_end into the future, indefinitely blocking the guaranteed revert to the title_owner. Require the lease to still be active (now < rental_end); an expired lease can only be reclaimed. AtomicMarket already only extends an active lease, so this does not affect the normal flow. Adds a VeRT regression.
…ot rolling Add rental_start to the leases table, set when the lease is first opened (leasestart) and left unchanged across extensions (leaseextend). This lets AtomicMarket cap the total rental period from the original start rather than a rolling window from "now". Action signatures are unchanged; only the leases table gains a field.
A leases row always represents an active lease now (pretitle/sentinels were
removed), so renter is never empty. Drop the `if (renter != name(""))` guards
and always notify the renter.
With a single configured rental market, passing and storing a market identity is redundant. leasestart/leaseextend no longer take a market argument; authority is simply require_auth(rentalcfg.rental_market) via check_rental_market(), which returns the configured account for use as the lease row's RAM payer. Drop the leases.market column and the market field from loglock. Tests updated; the authority tests now assert the missing-required-authority path.
| TABLE leases_s { | ||
| uint64_t asset_id; | ||
| name holder; | ||
| name owner; | ||
|
|
||
| uint64_t primary_key() const { return asset_id; }; | ||
| uint64_t by_holder() const { return holder.value; }; | ||
| name title_owner; // lister; reclaim returns the asset here | ||
| name renter; // current AA owner during the lease | ||
| uint32_t rental_start; // sec_since_epoch the lease was first opened (fixed across extensions) |
There was a problem hiding this comment.
Fixed the PR description: the leases row is {asset_id, title_owner, renter, rental_start, rental_end} with no market field. The single rental market lives in the rentalcfg singleton (set via setrentmkt), so it isn't stored per-lease. Code was correct; the description was stale.
| string memo, | ||
| name scope_payer | ||
| name scope_payer, | ||
| bool enforce_lock |
There was a problem hiding this comment.
Updated in ad8fb43. The comment now notes the get_self() exception: a contract can always bill its own RAM, so the privileged leasestart/reclaim paths pass scope_payer=get_self() and need no title_owner/renter signature to create the destination scope.
…_transfer (Copilot review)
The non-custodial model rests on reclaim always returning the asset to the lister at expiry. logreclaim notified the renter via require_recipient, so a renter that is a contract could throw in its handler and abort every reclaim, keeping the asset forever (the old `move` already warned that from/to notifications are "exploitable"). Drop the renter and title_owner recipients from logreclaim. The asset's collection is still notified (on logtransfer and logreclaim), trusting collections not to grief their own collection. Also add a MAX_LEASE_SECONDS (28d) protocol backstop in leasestart/leaseextend so a compromised or buggy rental market can't mint a near-permanent lock; the market keeps its own product cap on top. Tests: new evil-renter fixture proves the renter cannot veto reclaim while the collection still is; plus duration-cap rejections.
Security review — non-custodial reclaim hardeningAudited the reclaim/lock machinery (paired with atomicassets/atomicmarket-contract#11). Lock coverage, inline-action atomicity, royalty scope, and extension capping all check out. One Critical and one High issue were found and are fixed in the latest commit ( 🔴 Critical — permissionless reclaim was renter-vetoable (FIXED)
Fix: dropped Regression test: new 🟠 High — leasestart had no protocol duration cap (FIXED)The AA primitive accepted any 🟡 Medium (your call) — sale + rental coexistenceNot changed (orchestration lives in #11). An unleased asset can be both sale-listed and rent-announced; renting it then blocks the standing sale until reclaim — atomic, no fund loss, but a cheap front-run/grief on a pending purchase. Decide: refuse cross-listing, surface lease-state in the purchase error, or accept-and-document with the 🟢 LowMarket-funded lease-row RAM (resolved by the Critical fix); Tests green: AA 41 suites / 346 pass. |
Post-review cleanup of the non-custodial lease primitives (pairs with
atomicmarket-contract#11):
- Leasing is now opt-in: rentalcfg defaults to name("") (disabled) instead of
atomicmarket, so a fresh deploy is off until governance calls setrentmkt;
setrentmkt(name("")) stays the kill-switch.
- Denormalize collection_name onto the leases row, removing the unreachable
"renter no longer owns the asset" refetch in leaseextend/reclaim.
- Extract send_loglock to drop the duplicated loglock emit.
- Trim the verbose rental comments to the house style: consolidate the
reclaim-veto rationale into logreclaim, drop em-dashes and the instructional
error string, reuse the collection_name local.
| // The rentalcfg singleton defaults to "atomicmarket", so create that | ||
| // account as the authorized market (no setrentmkt needed). |
| uint64_t asset_id; | ||
| name holder; | ||
| name owner; | ||
| name title_owner; // lister; reclaim returns the asset here | ||
| name renter; // current AA owner during the lease | ||
| name collection_name; | ||
| uint32_t rental_start; // sec_since_epoch the lease was first opened (fixed across extensions) |
| // The single account authorized to open/manage leases (leasestart/leaseextend), in its own | ||
| // singleton so it needs no config migration. Leasing is opt-in: name("") (the default, and an | ||
| // absent row) means disabled, so a fresh deploy is off until setrentmkt("atomicmarket"); set it | ||
| // back to name("") to kill-switch all leasing. Not hardcoded - the market account differs per chain. | ||
| TABLE rentalcfg_s { | ||
| name rental_market = name(""); | ||
| }; |
| let owner; // asset owner / lessor | ||
| let holder; // current holder / lessee | ||
| let third; // unrelated third party | ||
| let market; // configured rental market (rentalcfg default = "atomicmarket") |
… review) The test comments still said rentalcfg defaults to "atomicmarket"; it now defaults to disabled (opt-in), and the suite enables leasing via setrentmkt.
… cap Greenfield hardening from the design review - table shapes and log ABIs are free to change now (no mainnet rows) and near-impossible later: - leases row + leasestart carry an opaque market-side rental_id, echoed in loglock and logreclaim, so indexers can join a reclaim to the rental that opened the lease structurally instead of parsing memo text - loglock now carries rental_start: consumers can compute duration and tell a lease start (rental_start == now) from an extension without a table read that races the trace - rentalcfg gains a governance-settable max_lease_seconds (setleasecap), bounded above by the compile-time MAX_LEASE_SECONDS protocol ceiling; the singleton's field set freezes at its first mainnet write, so it is decided now. setrentmkt and setleasecap preserve each other's fields
Custodial rentals are descoped from the V2 release so the rest of V2 can ship without them. The dual-ownership mechanism is removed in full: - move action and logmove notification - holders table (+ holders_s struct, get_holders accessor) in both the contract header and the consumer-facing interface header - holder-erase block in burnasset - holder bookkeeping in internal_transfer No other V2 feature reads holder state, so this is a pure excision. ABI diff vs v2.0.0-rc3: exactly move, logmove, holders(_s) removed. Rentals live on: the custodial implementation is preserved on archive/v2-custodial-rentals (and the v2.0.0-rc1..rc3 tags); the non-custodial rework continues on experiment/noncustodial-rentals (#26). Tests: move.test.js (16) and renting-invariants.test.js (3) deleted; holder-specific cases removed from transfer.test.js and burnasset.test.js. Suite: 40 suites, 324 passing (1 pre-existing skip).
Experimental branch for smart-contract review. Pairs with atomicassets/atomicmarket-contract#11 (the orchestration side); review the two together.
Summary
Reworks V2 rentals from custodial to non-custodial. In the shipped model a lease moves the asset into
atomicmarketand the renter is only aholder; because the ecosystem keys onowner, a rented asset gives the renter no utility. Here the renter becomes the real AtomicAssetsownerfor the lease term, the lister's reclaim right is recorded in aleasesrow, and the asset is locked until a guaranteed return at expiry. Unmodified consumers (games, the public API, drops, snapshots) see the renter as owner with no changes.Design
leasestable{ asset_id, title_owner, renter, collection_name, rental_start, rental_end, rental_id }, keyed byasset_id. A row's existence means the asset is locked, and the table is the single source of truth for lock state.rental_idis an opaque market-side id (AtomicMarket's rental counter), stored at lease-open and echoed in the lease logs so indexers can join a reclaim to the rental that opened the lease structurally instead of parsing memo text.check_not_leasedguards every renter-reachable extraction path. It runs insideinternal_transfer(the chokepoint fortransferandacceptoffer, via anenforce_lockflag) and directly inburnassetandcreateoffer.setassetdatais left unguarded by design, since it is collection-auth gated and never renter-reachable.leasestartflips ownership from lister to renter, writing the lease row before the move so there is no unlocked window;leaseextendbumps the end time; permissionlessreclaimreturns the asset to thetitle_ownerat expiry under the contract's own authority, so it needs no renter signature.loglockandlogreclaimemit traces for indexers:loglockcarriesrental_start(so a lease-open, whererental_start == now, is distinguishable from an extension without a table read) and both carry therental_id.rental_market(therentalcfgsingleton) gatesleasestartandleaseextend. Leasing is opt-in:rental_marketdefaults toname("")(disabled), so a fresh deploy stays off until governance callssetrentmkt("atomicmarket"), and setting it back toname("")disables all leasing. Lister consent is captured upstream by AtomicMarket'sannouncerent.leasestartandleaseextendcap a lease, and its total extended window measured from the fixedrental_start, at a governance-settablemax_lease_seconds(setleasecap, contract authority), which is itself bounded by the compile-time 28-dayMAX_LEASE_SECONDSprotocol ceiling. This bounds the impact if the configured market is ever compromised. The singleton's field set is deliberately finalized now, since it freezes at its first mainnet write.Removed
The custodial machinery: the
moveaction, theholderstable andget_holders(), andlogmove.Tests
VeRT: 41 suites / 348 pass (1 skipped).
tests/asset-actions/renting-invariants.test.jscovers the lock guards on every path, the deliberatesetassetdatanon-guard, the lifecycle (including the fixedrental_start/rental_idacross extensions), the configured-market authority, the opt-in default, the duration cap, and thesetleasecapbounds and cross-preservation withsetrentmkt. A fixture contract proves a hostile renter contract cannot abort the permissionless reclaim.Notes for reviewers
reclaimnotifies only the asset's collection, not the renter or title_owner, so a hostile renter cannot abort the guaranteed return by throwing in a notification handler. A collection notify-account can still abort it, which is accepted under the same trust model that already lets a collection gate transfers of its assets. (Corollary: a collection notify contract that throws onloglockselectively vetoes lease creation for its assets — a de facto per-collection opt-out.)setrentmktalready requires the contract's own authority (the same authority as an upgrade), so hardcoding would not reduce trust, and the market account name can differ per chain. One AtomicAssets + one AtomicMarket per network is a design invariant, so no multi-market machinery exists or is planned.rental_idis fully opaque to this contract: no checks, no index, 8 bytes of market-paid row RAM. It exists because the required indexer follow-on (nft-data / eosio-contract-api) needs a structural reclaim-to-rental join, and log/table shapes are free to change only while the tables are empty.leaseextendcap anchors to the fixedrental_startwhile a reclaim + re-lease gets a fresh window: deliberate asymmetry, since a re-lease necessarily transits the reclaimable state the cap exists to guarantee.