fix(#420): creator fees accrue and are claimed PER ASSET - #463
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
Every asset's creator cut accumulated into one market-wide counter (`WrapperConfigV16::creator_fee_claimable_atoms`) while `WithdrawCreatorFee` authorised against ASSET 0's `asset_admin` only. In a multi-asset market the base deployer could withdraw fees earned on assets 1..N, and those assets' creators could never claim their own. WHAT CHANGED `AssetOracleProfileV16` gains `creator_fee_claimable_atoms`. Both accrual paths credit the LEG'S OWN asset, and tag 90 takes an `asset_index`, authorises against that asset's `asset_admin`, and debits that asset's counter. THERE IS NO LAYOUT COST, and that is not luck. `ASSET_ORACLE_PROFILE_LEN` grows 400 -> 408, but the profile sits inside a fixed 512-byte `ASSET_ORACLE_WRAPPER_LEN` slot with 112 bytes spare, so `MARKET_ASSET_SLOT_LEN` is unchanged and NO offset moves. Deployed markets have those bytes zeroed (they were slot padding), so each asset reads 0 and accrues fresh. This is the opposite of the config counter's situation, where `WRAPPER_CONFIG_LEN` is exactly 576 and growing it would shift `MARKET_GROUP_OFF` and brick every deployed market. NOTHING ALREADY EARNED IS STRANDED. The pre-existing global balance stays in `cfg.creator_fee_claimable_atoms` and remains claimable by asset 0's admin — the only party who could ever claim it. A claim debits the per-asset counter first and falls back to the legacy pot, so a migrated market stops touching the old value and that value can only shrink. ABI: tag 90 goes 17 -> 19 bytes, `asset_index` appended AFTER `amount` so the tag and the u128 keep their offsets and an old 17-byte caller fails to DECODE rather than being silently read as asset 0. Rides the next deploy alongside #412 and #451. TWO BUGS THE TESTS CAUGHT IN MY OWN CHANGE 1. The first version of the single-trade site did its own profile read/write. That write was CLOBBERED a few lines later by the function's existing write-back of an `oracle_profile` read earlier — the accrual silently vanished. Now it mutates the in-scope profile, as the batch path does. 2. The batch site initially removed the config accrual while the per-leg credit did not yet exist, which would have DROPPED batch creator fees entirely. Neither would have been obvious by reading the diff. Both were caught by `..._creator_fee_accrual_is_written_back_to_the_account_...` and the batch multi-leg test, which is what those tests are for. Also removed a bounds check I had added against `config.max_market_slots`: that is a different basis from the one `read_asset_oracle_profile` already enforces, and it rejected a legitimate asset 1 as InvalidInstruction. One invariant, one owner. VERIFIED new test `asset0_admin_cannot_drain_asset1_creator_fees` — asset 0's admin is REFUSED against asset 1 and the market is left byte-identical, and asset 1's own admin CAN claim (positive control, without which the refusal proves nothing) NEGATIVE CONTROL with the authority check reverted to asset 0, that test FAILS — the theft succeeds; restored, it passes batch test rewritten to assert each asset receives ITS leg's cut, and keeps the original conservation assertion: the per-asset credits still sum to every leg's creator cut wire test updated for the deliberate break, and now also pins that the OLD 17-byte payload is REFUSED rather than read as asset 0 full CI suite 604 passed / 21 failed — "OK: failing set matches the allowlist exactly" UPSTREAM: `creator_fee_claimable_atoms` does not exist at aeyakovenko/percolator-prog — the creator-fee claim is ours alone. Nothing to coordinate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
dcccrypto
force-pushed
the
fix/420-per-asset-creator-fee
branch
from
September 4, 2026 00:24
c3b6657 to
6cb3c70
Compare
dcccrypto
added a commit
that referenced
this pull request
Sep 4, 2026
`last_fee_slot` is anchored at ACCOUNT CREATION and advanced only by SyncMaintenanceFee. While the rate is 0 nobody cranks, so it stays pinned at creation for the market's whole life — and the moment `marketauth` raises the rate, the next PERMISSIONLESS crank bills `new_rate x (the account's entire age)`, including every slot the rate was 0, capped only by capital. A small-looking rate takes up to 100% of a flat account, and `UpdateMaintenanceFeePerSlot` + `SyncMaintenanceFee` bundle atomically so the victim never gets a turn. That defeats #428's safety argument, which Live-gated the rate change on the assumption a user can withdraw before a hostile fee bites. They cannot: the charge is retroactive AND instantaneous. ── WHY THIS IS IN THE WRAPPER, AFTER A FIRST ATTEMPT IN THE ENGINE FAILED ────── My first attempt capped `dt` inside `sync_account_fee_to_slot_not_atomic` and broke `v16_spec_tests.rs`. spec.md §981 fixes what the ENGINE does with a given anchor: it "charges recurring wrapper-owned fees exactly once over `[last_fee_slot_i, anchor]`" and advances `last_fee_slot_i = anchor`. The retroactivity is SPECIFIED. But CHOOSING the anchor is the wrapper's job, and §981's only constraint on it is "Live anchors must be `<= current_slot`". A smaller anchor is fully compliant. So the fix is two spec-compliant engine calls instead of one: [last_fee_slot, checkpoint] at the PREVIOUS rate <- what was actually owed [checkpoint, now] at the CURRENT rate <- ordinary accrual `UpdateMaintenanceFeePerSlot` records `(slot, previous_rate)` when — and only when — the rate actually changes, so a no-op re-assert cannot move the checkpoint and silently forgive owed fees. STATE: two fields on `AssetOracleProfileV16`, asset 0's copy holding the market-wide value, following the existing precedent that asset 0's profile carries market-wide oracle state the config mirrors. `ASSET_ORACLE_PROFILE_LEN` 408 -> 432, still inside the fixed 512-byte slot (80 spare), so `MARKET_ASSET_SLOT_LEN` is unchanged and NO offset moves. This is why it is not in `WrapperConfigV16`: `WRAPPER_CONFIG_LEN` is exactly 576 and growing it shifts `MARKET_GROUP_OFF`, bricking every deployed market. Zero means "never changed", which is what a deployed market reads after an in-place upgrade — correct, since a market whose rate never changed has no retroactive window. ── (B) HAS NO SAFE PLACE TO SIT, AND THAT IS A FINDING ───────────────────────── The decision asked for a `max_accrual_dt_slots` per-crank bound as belt and braces. I implemented it. It cannot go anywhere useful once (A) exists: * on the POST-checkpoint leg it under-collects ordinary accrual from any account that simply has not been cranked lately — legitimately owed fees, not hostile ones. Six existing tests assert those exact amounts and were right to; all six failed with the cap in place. * on the PRE-checkpoint leg it is WORSE than useless: the leg stops short of the checkpoint, and the second call then bills the remaining pre-change window at the NEW rate — re-creating the exact confiscation (A) prevents. The new test fails with the account drained to zero. With (A) there is no unbounded HOSTILE window left. A cap would only defer legitimately-owed fees, so it is deliberately absent and the reasoning is in-source. VERIFIED new test `..._raising_the_maintenance_rate_is_not_retroactive` — rate 0 for ~900 slots, then raised and cranked immediately (the bundled attack). Only the 10 post-raise slots are billed: capital 950. NEGATIVE CONTROL with the checkpoint leg disabled, that test FAILS with capital 0 — the account fully drained. Restored, 950. full CI suite 605 passed / 21 failed — "OK: failing set matches the allowlist exactly". The six accrual tests pass UNTOUCHED. STACKED ON #420 (prog#463): both grow `AssetOracleProfileV16`, so this branch is based on that one to avoid a conflict. Merge #463 first. UPSTREAM: `sync_account_fee_to_slot_not_atomic` exists at `av` (tip 8eb7142a) with the same retroactive shape, so this is not a divergence we introduced — and it is worth filing upstream, since spec.md is Toly's and upstream's own engine has the same exposure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
dcccrypto
added a commit
that referenced
this pull request
Sep 4, 2026
`last_fee_slot` is anchored at ACCOUNT CREATION and advanced only by SyncMaintenanceFee. While the rate is 0 nobody cranks, so it stays pinned at creation for the market's whole life — and the moment `marketauth` raises the rate, the next PERMISSIONLESS crank bills `new_rate x (the account's entire age)`, including every slot the rate was 0, capped only by capital. A small-looking rate takes up to 100% of a flat account, and `UpdateMaintenanceFeePerSlot` + `SyncMaintenanceFee` bundle atomically so the victim never gets a turn. That defeats #428's safety argument, which Live-gated the rate change on the assumption a user can withdraw before a hostile fee bites. They cannot: the charge is retroactive AND instantaneous. ── WHY THIS IS IN THE WRAPPER, AFTER A FIRST ATTEMPT IN THE ENGINE FAILED ────── My first attempt capped `dt` inside `sync_account_fee_to_slot_not_atomic` and broke `v16_spec_tests.rs`. spec.md §981 fixes what the ENGINE does with a given anchor: it "charges recurring wrapper-owned fees exactly once over `[last_fee_slot_i, anchor]`" and advances `last_fee_slot_i = anchor`. The retroactivity is SPECIFIED. But CHOOSING the anchor is the wrapper's job, and §981's only constraint on it is "Live anchors must be `<= current_slot`". A smaller anchor is fully compliant. So the fix is two spec-compliant engine calls instead of one: [last_fee_slot, checkpoint] at the PREVIOUS rate <- what was actually owed [checkpoint, now] at the CURRENT rate <- ordinary accrual `UpdateMaintenanceFeePerSlot` records `(slot, previous_rate)` when — and only when — the rate actually changes, so a no-op re-assert cannot move the checkpoint and silently forgive owed fees. STATE: two fields on `AssetOracleProfileV16`, asset 0's copy holding the market-wide value, following the existing precedent that asset 0's profile carries market-wide oracle state the config mirrors. `ASSET_ORACLE_PROFILE_LEN` 408 -> 432, still inside the fixed 512-byte slot (80 spare), so `MARKET_ASSET_SLOT_LEN` is unchanged and NO offset moves. This is why it is not in `WrapperConfigV16`: `WRAPPER_CONFIG_LEN` is exactly 576 and growing it shifts `MARKET_GROUP_OFF`, bricking every deployed market. Zero means "never changed", which is what a deployed market reads after an in-place upgrade — correct, since a market whose rate never changed has no retroactive window. ── (B) HAS NO SAFE PLACE TO SIT, AND THAT IS A FINDING ───────────────────────── The decision asked for a `max_accrual_dt_slots` per-crank bound as belt and braces. I implemented it. It cannot go anywhere useful once (A) exists: * on the POST-checkpoint leg it under-collects ordinary accrual from any account that simply has not been cranked lately — legitimately owed fees, not hostile ones. Six existing tests assert those exact amounts and were right to; all six failed with the cap in place. * on the PRE-checkpoint leg it is WORSE than useless: the leg stops short of the checkpoint, and the second call then bills the remaining pre-change window at the NEW rate — re-creating the exact confiscation (A) prevents. The new test fails with the account drained to zero. With (A) there is no unbounded HOSTILE window left. A cap would only defer legitimately-owed fees, so it is deliberately absent and the reasoning is in-source. VERIFIED new test `..._raising_the_maintenance_rate_is_not_retroactive` — rate 0 for ~900 slots, then raised and cranked immediately (the bundled attack). Only the 10 post-raise slots are billed: capital 950. NEGATIVE CONTROL with the checkpoint leg disabled, that test FAILS with capital 0 — the account fully drained. Restored, 950. full CI suite 605 passed / 21 failed — "OK: failing set matches the allowlist exactly". The six accrual tests pass UNTOUCHED. STACKED ON #420 (prog#463): both grow `AssetOracleProfileV16`, so this branch is based on that one to avoid a conflict. Merge #463 first. UPSTREAM: `sync_account_fee_to_slot_not_atomic` exists at `av` (tip 8eb7142a) with the same retroactive shape, so this is not a divergence we introduced — and it is worth filing upstream, since spec.md is Toly's and upstream's own engine has the same exposure. Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D Co-authored-by: dcccrypto <dcccrypto@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Decision A — route per-asset; each asset's creator gets its own fees.
The bug
Every asset's creator cut accumulated into one market-wide counter (
WrapperConfigV16::creator_fee_claimable_atoms) whileWithdrawCreatorFeeauthorised against asset 0'sasset_adminonly. In a multi-asset market the base deployer could withdraw fees earned on assets 1..N, and those assets' creators could never claim their own.What changed
AssetOracleProfileV16gainscreator_fee_claimable_atoms. Both accrual paths credit the leg's own asset, and tag 90 takes anasset_index, authorises against that asset'sasset_admin, and debits that asset's counter.No layout cost, and not by luck.
ASSET_ORACLE_PROFILE_LENgrows 400 → 408, but the profile sits inside a fixed 512-byte slot with 112 bytes spare —MARKET_ASSET_SLOT_LENis unchanged and no offset moves. Deployed markets have those bytes zeroed (slot padding), so each asset reads 0 and accrues fresh.That's the opposite of the config counter's situation, where
WRAPPER_CONFIG_LENis exactly 576 and growing it would shiftMARKET_GROUP_OFFand brick every deployed market.Nothing already earned is stranded. The pre-existing global balance stays in
cfg.creator_fee_claimable_atoms, still claimable by asset 0's admin — the only party who could ever claim it. A claim debits the per-asset counter first and falls back to the legacy pot, so a migrated market stops touching the old value and it can only shrink.ABI: tag 90 goes 17 → 19 bytes,
asset_indexappended afteramountso the tag and u128 keep their offsets and an old 17-byte caller fails to decode rather than being read as asset 0. Rides the next deploy alongside #412 and #451.Two bugs the tests caught in my own change
Neither would have been obvious from reading the diff. Both were caught by
..._creator_fee_accrual_is_written_back_to_the_account_...and the batch multi-leg test — which is what those tests are for.I also removed a bounds check I'd added against
config.max_market_slots: a different basis from the oneread_asset_oracle_profilealready enforces, and it rejected a legitimate asset 1 asInvalidInstruction. One invariant, one owner.Verified
Upstream
creator_fee_claimable_atomsdoesn't exist ataeyakovenko/percolator-prog— the creator-fee claim is ours alone. Nothing to coordinate.🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D