feat(fees): pay underwriters and reserve owners out of the swap fee - #546
feat(fees): pay underwriters and reserve owners out of the swap fee#546heifner wants to merge 13 commits into
Conversation
WIRE-296: the swap fee reached producers + batch operators, while the
winning underwriter — the party whose collateral lock lets a swap settle
at all — got nothing. WIRE-281: reserve owners earned nothing either, so
a third party had no reason to fund a reserve.
Two independent fees now ride the WIRE leg (per Jonathan, 2026-08-04):
- Network fee (sysio.uwrit::fee_bps): 50% accrues to the winning
underwriter in sysio.reserv::uwfees, drawn via claimuwfee. The rest is
a rewards pool split between batch operators and the emissions treasury
by reserve_config.fee_emissions_share_bps (default 0, so by default the
whole pool reaches batch operators). Producers no longer draw from swap
fees; they earn emissions.
- Reserve owner fee (reserve_row.owner_fee_bps): per-reserve, set by the
owner via setrsvfee once matchreserve resolves ownership, drawn via
claimrsvfee. Every reserve supplying a leg charges its own rate, so a
chain-to-chain swap pays three fees — two reserve owners plus the
network. Bounded 0 (off) or [1, 9900]; an owner-less reserve cannot
charge, so no accrual can strand without a claimant.
split_wire_fee is now the ONE decomposition used by both quote_swap and
all four settlement paths, every rate applied to the same gross leg —
additive, order-independent, exact (the five shares sum to fee, and
net + fee == wire_amount). Quote and settlement cannot drift apart.
Custody invariant extends to:
token_balance == Σ reserve_wire_amount + rewards.balance
+ Σ uwfees.balance + Σ owner_fee_accrued + in-flight escrow
Also sets the from-WIRE revert fee to its 500 bps launch default
(WIRE-315), and rewrites sysio.uwrit/README.md, which described a
contract that never existed (WIRE-299).
Full contracts_unit_test green.
Change-Id: Idaaca7535149e82792d6ec7ada3386beb809334b
huangminghuang
left a comment
There was a problem hiding this comment.
Two inline findings from the delegated code review.
| r.emissions_share = static_cast<uint64_t>((static_cast<u128>(rewards_pool) * emissions_share_bps) / BPS_TOTAL); | ||
| r.reward_share = rewards_pool - r.emissions_share; | ||
|
|
||
| r.fee = network_fee + r.src_reserve_share + r.dst_reserve_share; |
There was a problem hiding this comment.
[Medium] Avoid wrapping the stacked fee total. The individual products use u128, but this addition narrows the total to uint64_t before checking whether it consumes the leg. For example, wire_amount = UINT64_MAX with three 100% rates wraps r.fee to UINT64_MAX - 2 and reports net == 2 instead of zero. Please sum in u128, compare against wire_amount, and narrow only when the total is smaller; a maximum-value stacked-rate test would cover the boundary.
There was a problem hiding this comment.
Confirmed and fixed in 04a8e5dfa — you were right, and the failure is exactly the one you described. Reproduced against your inputs (wire_amount = UINT64_MAX, three 100% rates):
OLD: fee=18446744073709551613 (MAX-2) net=2 -> caller's `net > 0` gate: PASSES (bug)
NEW: fee=18446744073709551615 (== MAX) net=0 -> caller's `net > 0` gate: rejects
The wrap is worse than a saturate precisely because of that last column: net > 0 is the gate every settlement path uses to reject a fully-consumed leg, and a wrapped total hands it a small positive number.
Now summed and compared in u128, narrowed only when the total genuinely fits, clamped to wire_amount otherwise. Two things worth calling out:
- It strengthens the documented invariant.
net + fee == wire_amountnow holds in the saturating case too; the old code leftfeeholding the raw over-leg sum, so that equation only held on the happy path. - One consequence, now documented. In the clamped case alone the per-share fields sum to more than
fee. That is inert — the caller rejects the swap and no share is ever accrued — but it is stated in the header rather than left for the next reader to work out.
Added split_wire_fee_stacked_rates_cannot_wrap_the_total per your suggestion, covering the boundary and its neighbours: three 100% rates on a MAX leg; a partial-rate carry (40% x3, 1.2 x MAX); the just-below-carry case that must not clamp (30% x3, 0.9 x MAX — guards against over-correcting into a clamp that eats real swaps); a reach-the-leg case with no carry; and ordinary-rate conservation across odd amounts. It fails under the old arithmetic.
contracts_unit_test: 583 cases, *** No errors detected. No ABI delta — only the two wasms that include amm_math.hpp changed.
One adjacent site I checked and deliberately left alone: paywire does wire_leaving = wire_out + fee.fee in uint64_t, same narrowing class, and a wrapped value there would pass the sufficiency check and under-debit the reserve. I did not touch it because it is pre-existing (untouched by this PR) and not reachable through the real call path — wire_out comes from uwrit's own quote against the same reserve, so it cannot approach UINT64_MAX. Happy to harden it here or file it separately, your call.
| /// Singleton accumulator for the rewards (batch-operator) half of swap | ||
| /// fees. The WIRE stays in this contract's custody — it is NOT transferred | ||
| /// out — so the custody invariant is `token_balance == Σ | ||
| /// reserve_wire_amount + rewards.balance + Σ uw_fee_row.balance + |
There was a problem hiding this comment.
[Low] Include reserve-owner accruals in this custody invariant. Once an owner fee is collected, that share leaves reserve_wire_amount but remains in this contract as reserve_row.owner_fee_accrued, so the equation as written no longer balances. Please add sum(owner_fee_accrued), matching the invariant in the PR description.
There was a problem hiding this comment.
Correct — fixed in 04a8e5dfa. The invariant now reads:
token_balance == Σ reserve_wire_amount + rewards.balance + Σ uw_fee_row.balance
+ Σ reserve_row.owner_fee_accrued + in-flight escrow
I also added the reason the term exists, since the asymmetry is the easy thing to miss: a collected owner fee leaves reserve_wire_amount but stays in this contract until claimrsvfee, so it is a term in the equation rather than an outflow. Only the emissions share is ever transferred out at collection time, and only when that dial is non-zero.
This matches the invariant in the PR description, which was already correct — the header comment was the one that lagged.
Review finding (huangminghuang, PR #546). Each fee product was computed in `u128`, but the three were summed into a `uint64_t`. On an extreme leg the total carries past 64 bits and WRAPS — which is strictly worse than saturating, because it reports a small POSITIVE `net` for a leg the fees consumed entirely, and `net > 0` is exactly the gate every settlement path uses to reject such a swap. Verified against the reviewer's inputs (`wire_amount = UINT64_MAX`, three 100% rates): before: fee = MAX - 2, net = 2 -> `net > 0` passes, swap proceeds after: fee = MAX, net = 0 -> correctly rejected The total is now summed and compared in `u128` and narrowed only when it fits; otherwise `fee` clamps to `wire_amount`. That also strengthens the documented invariant — `net + fee == wire_amount` now holds in the saturating case too, where the old code left `fee` holding the raw over-leg sum. The one consequence is documented: in the clamped case alone the per-share fields sum to more than `fee`, which is inert because the caller rejects the swap and no share is accrued. `split_wire_fee_stacked_rates_cannot_wrap_the_total` covers the boundary: three 100% rates on a MAX leg, a partial-rate carry (40% x3), the just-below-carry case that must NOT clamp (30% x3), a reach-the-leg case with no carry, and ordinary-rate conservation. It fails under the old arithmetic. Also (second review finding): the `rewards_bucket` custody invariant omitted `Sigma reserve_row.owner_fee_accrued`. A collected owner fee leaves `reserve_wire_amount` but stays in the contract until `claimrsvfee`, so it is a term in the equation, not an outflow. No ABI delta — only the two wasms that include `amm_math.hpp` changed. contracts_unit_test: 583 cases, No errors detected. Change-Id: I01126b4076d2d4db373f7bc98234950bc380cdcf
Review finding (huangminghuang, PR #546). Each fee product was computed in `u128`, but the three were summed into a `uint64_t`. On an extreme leg the total carries past 64 bits and WRAPS — which is strictly worse than saturating, because it reports a small POSITIVE `net` for a leg the fees consumed entirely, and `net > 0` is exactly the gate every settlement path uses to reject such a swap. Verified against the reviewer's inputs (`wire_amount = UINT64_MAX`, three 100% rates): before: fee = MAX - 2, net = 2 -> `net > 0` passes, swap proceeds after: fee = MAX, net = 0 -> correctly rejected The total is now summed and compared in `u128` and narrowed only when it fits; otherwise `fee` clamps to `wire_amount`. That also strengthens the documented invariant — `net + fee == wire_amount` now holds in the saturating case too, where the old code left `fee` holding the raw over-leg sum. The one consequence is documented: in the clamped case alone the per-share fields sum to more than `fee`, which is inert because the caller rejects the swap and no share is accrued. `split_wire_fee_stacked_rates_cannot_wrap_the_total` covers the boundary: three 100% rates on a MAX leg, a partial-rate carry (40% x3), the just-below-carry case that must NOT clamp (30% x3), a reach-the-leg case with no carry, and ordinary-rate conservation. It fails under the old arithmetic. Also (second review finding): the `rewards_bucket` custody invariant omitted `Sigma reserve_row.owner_fee_accrued`. A collected owner fee leaves `reserve_wire_amount` but stays in the contract until `claimrsvfee`, so it is a term in the equation, not an outflow. No ABI delta — only the two wasms that include `amm_math.hpp` changed. contracts_unit_test: 583 cases, No errors detected. Change-Id: I01126b4076d2d4db373f7bc98234950bc380cdcf
e9b5258 to
04a8e5d
Compare
huangminghuang
left a comment
There was a problem hiding this comment.
Two additional findings from the re-review at 04a8e5d.
| ("chain_code", codename_mvo("SOLANA"))("token_code", codename_mvo("SOL")) | ||
| ("reserve_code", codename_mvo("PRIMARY"))("owner_fee_bps", 500))); | ||
| produce_block(); | ||
| BOOST_REQUIRE_EQUAL(success(), quote()); |
There was a problem hiding this comment.
[Medium] This test never inspects either quote. push_action discards the transaction trace and returns only success(), so both assertions pass even if reserve fees have no effect on swapquote. Please capture and decode the action's return_value, then assert that the post-fee quote is lower (ideally equal to the exact result from the shared AMM math).
There was a problem hiding this comment.
Correct, and thank you — this was the worst kind of gap, because the comment above the case asserted a behaviour ("Adding reserve fees must strictly reduce the quoted output") that nothing verified. push_to discards the trace and returns success(), so the case would have stayed green if setrsvfee had no effect on pricing at all. That is exactly the regression it exists to catch: the variance check runs off this quote, so quote/settlement drift is silent by construction.
Fixed in 6fbed1d2d. Added a swapquote_value() fixture helper that decodes the read-only action's return_value, and took your "ideally" option — the case now asserts against the shared AMM kernel rather than a direction:
- fee-free baseline
== quote_swap(..., 0, 0) - both owner fees set
== quote_swap(..., 500, 500), and< baseline - destination fee cleared
== quote_swap(..., 500, 0)
That third case is beyond what you asked for and is the one with teeth: a quote that summed the wrong reserve's rate, or double-counted one side, still reproduces the symmetric two-sided numbers but fails the asymmetric one.
Expected values come from opp::amm::quote_swap directly, so the test is pinned to the same kernel settlement uses rather than to a recomputed copy. One coupling worth naming: NETWORK_FEE_BPS is the uw_config{} in-struct default, because sysio.uwrit exists as a bare account with no contract in this fixture — that is stated in a comment at the constant, and if the default ever moves this case fails loudly with an exact-value mismatch, which is the signal you want.
contracts_unit_test: 583 cases, *** No errors detected.
| uint64_t owner_fee_accrued = 0; | ||
| /// Audit total: every WIRE this reserve has EVER earned from | ||
| /// `owner_fee_bps`. Monotonic — never decremented by a claim. | ||
| uint64_t owner_fee_lifetime = 0; |
There was a problem hiding this comment.
[Medium] The lifetime audit counters can silently saturate. These fields are uint64_t and are updated with add_capped_u64, so after UINT64_MAX raw units they stop recording history while claims continue. At 9 decimals that is about 18.45 billion WIRE; with the 1-billion launch supply and a 99% owner fee, roughly 19 full-supply turnovers can reach it. That breaks the documented "every WIRE ever" audit semantics for owner_fee_lifetime and likewise affects the new underwriter lifetime_accrued / lifetime_claimed fields. Please use uint128_t, or explicitly represent and document overflow.
There was a problem hiding this comment.
Valid finding, and the sharpest part of it is one I had not seen: add_capped_u64 justifies itself with "the cap is unreachable for any real token amount", which is true for a balance — bounded by what is actually in custody — but does not carry to an unbounded monotonic counter. The helper is correct; applying it to the lifetime_* fields silently inherited a rationale that does not hold there.
Documented rather than widened to uint128, in 6fbed1d2d — at all four fields (reserve_row::owner_fee_lifetime, uw_fee_row::lifetime_accrued / lifetime_claimed) and at the helper itself, with your arithmetic and the blast radius stated plainly: saturates at UINT64_MAX (~18.45e9 WIRE at 9dp), reachable in principle after roughly 370 full-supply turnovers through a single reserve at a 5% owner fee (~19 at the 99% maximum), and past that only audit history truncates — never a balance, never a payout.
On the uint128_t option — it is a bigger call than it looks, which is why I did not take it unilaterally. uint128 currently appears in this tree only as a secondary-index key_type; there is no serialized uint128 struct field in any contract ABI, and no uint128 mapping in the generated SysioContractTypes. So it would be the first of its kind, with unverified ABI/TS-generator support, rippling wire-sysio → wire-libraries-ts → wire-tools-ts (the harness reads these fields) plus an e2e re-run.
That is worth doing deliberately rather than inside a fee PR — and pre-launch is genuinely the cheapest it will ever be, so I am not arguing against it. If you want the counters widened, I would rather take it as its own change that establishes uint128 support across the ABI/TS boundary properly. Say the word and I will file it.
No wasm/abi delta from this commit — the contract changes are comments only. contracts_unit_test: 583 cases, *** No errors detected.
…ration Two review findings (huangminghuang, PR #546). 1. `swapquote_prices_the_reserve_owner_fees` never inspected either quote. The fixture's `push_to` discards the transaction trace and returns `success()`, so the case passed on the action succeeding — it would have stayed green if `setrsvfee` had no effect on pricing whatsoever, which is precisely the regression it exists to catch (the variance check runs off this quote, so a quote/settlement drift is silent). Adds a `swapquote_value()` fixture helper that decodes the read-only action's `return_value`, and asserts against the shared AMM kernel rather than a direction: - fee-free baseline == quote_swap(..., 0, 0) - both fees set == quote_swap(..., 500, 500), and < baseline - destination cleared == quote_swap(..., 500, 0) The one-sided case is beyond what was asked and is the one with teeth: a quote that summed the wrong reserve's rate, or double-counted a side, still reproduces the symmetric numbers but fails this. 2. The `lifetime_*` audit counters saturate silently. `add_capped_u64` justified itself with "the cap is unreachable for any real token amount" — true for a BALANCE, which is bounded by what is in custody, but it does not carry to an unbounded monotonic counter. Documented at all four fields and at the helper: saturates at UINT64_MAX (~18.45e9 WIRE at 9dp), reachable in principle after ~370 full-supply turnovers through one reserve at a 5% owner fee (~19 at the 99% maximum), and past it only audit history truncates — never a balance, never a payout. No wasm/abi delta: the contract changes are comments only. contracts_unit_test: 583 cases, No errors detected. Change-Id: If4e07396ee18106cb2009afdea7bd53319903b27
| /// `wire_amount` minus the fee, and the fee routes through the standard | ||
| /// `route_wire_fee` path. A revert has no winning underwriter — nobody | ||
| /// locked collateral for a swap that never settled — so the whole revert | ||
| /// fee goes to the rewards bucket (zero underwriter share). Pass 0 for |
There was a problem hiding this comment.
[Low] Qualify the revert-fee routing description. refundwire still passes fee_emissions_share_bps(get_self()) into split_wire_fee, so the whole revert fee reaches rewards_bucket only under the default-zero emissions configuration. With a non-zero dial, the configured portion goes to the treasury. Please qualify this statement and update the matching source/test comments.
There was a problem hiding this comment.
Correct — fixed in 296c1abb5f. The routing claim was wrong in the way you describe, and the precise mechanic is worth naming because the first half of the old sentence was right:
refundwire passes a zero underwriter_share_bps, so the whole network fee does become the rewards pool — but split_wire_fee's stage 2 then splits that pool by emissions_share_bps, and refundwire feeds it fee_emissions_share_bps(get_self()) like every settlement path. So "pool" was accurate and "bucket" was not: the bucket receives the fee in full only under the default zero dial, and a configured dial diverts its share to the treasury.
Qualified at all five sites — the flagged action doc, the refundwire source comment, sysio.uwrit/README.md, and both test comments (sysio.reserv_tests::refundwire_routes_revert_fee, sysio.dispatch_tests::drainfwq_charges_revert_fee_on_caller_fault).
The test comments were the load-bearing ones. Neither fixture calls reserv::setconfig, so the exact-value assertions — 15u to the bucket, REWARD_SHARE == FEE — are correct precisely because the dial is zero. The old comments read as unconditional, which would have made either assertion look wrong to anyone who later set a non-zero dial in the fixture, when in fact the assertion is right and the comment was the incomplete part. Both now state the dependency.
Comments only: no wasm/abi delta (the build reports Contracts unchanged, skipping copy). contracts_unit_test: 583 cases, *** No errors detected.
| } | ||
| const int64_t fee_producer_pool = split_bps(fee_total, cfg.producer_bps); | ||
| const int64_t fee_batch_pool = fee_total - fee_producer_pool; | ||
| const int64_t fee_batch_pool = fee_total; |
There was a problem hiding this comment.
[Low] Update the payepoch overview. This assignment and the producer loop now route the entire drained swap-fee pool to batch operators, but the function-level comment above still says producers and batch operators receive it split by producer_bps / batch_op_bps. Please update that overview so it matches the new policy.
There was a problem hiding this comment.
Correct — fixed in 296c1abb5f. The overview described the pre-change policy verbatim, and it turned out not to be the only site carrying it. Sweeping for the same claim found three more:
emissions.cpp:56— theRESERV_CONTRACTcomment ("folds into the per-epoch compute distribution" → batch-operator distribution).EMISSIONS.mddistribution section — the identical "split by the sameproducer_bps/batch_op_bps" sentence.emissions_tests.cpp::payepoch_folds_swap_fee_rewards's header — the one with actual teeth. It said the case verifies "the single producer is paidproducer_pool+ its fee share", while the body it describes assertsgot == producer_poolandfee_distributed == 0. The assertions were updated in this PR; the comment describing them was not, so it claimed the opposite of what the test proves. A reader trusting the header would have concluded the test was wrong, or "fixed" it back to the old policy.
One qualifier beyond what you asked for, at the fold-in comment: it asserted the underwriter half "never passes through this treasury" and implied the batch-op half arrives whole via the bucket. With a non-zero fee_emissions_share_bps, that configured share of the batch-op half is transferred straight to sysio at collection time and never enters the bucket drainrewards sweeps. Same imprecision class as your other Low finding, sitting in the function under review, so it seemed better to close both together than to leave the near-miss next to the fix.
Comments only: no wasm/abi delta. contracts_unit_test: 583 cases, *** No errors detected.
Two review findings, both comment/doc only — no code, no wasm/abi delta. refundwire passes `fee_emissions_share_bps` into `split_wire_fee`, so with a zero underwriter share the whole revert fee becomes the rewards POOL, not the rewards BUCKET: stage 2 then splits that pool by the emissions dial. The bucket receives it in full only under the default zero dial. Qualified at all five sites — the action doc, the refundwire source comment, sysio.uwrit/README.md, and both test comments. Neither test fixture calls reserv::setconfig, so the exact-value assertions hold precisely because the dial is zero; the comments now state that dependency instead of implying it holds unconditionally. The payepoch overview still described producers and batch operators splitting the drained fee pool by producer_bps / batch_op_bps. The entire pool now goes to batch operators. Corrected the function-level comment plus three more sites carrying the same stale policy: the RESERV_CONTRACT comment, the EMISSIONS.md distribution section, and the payepoch_folds_swap_fee_rewards header — which claimed the test verifies a producer is paid "producer_pool + its fee share" while its body asserts got == producer_pool and fee_distributed == 0. Also qualified the fold-in comment: a non-zero fee_emissions_share_bps sends that share of the batch-op half straight to sysio at collection time, so it never enters the bucket payepoch drains. contracts_unit_test: 583 cases, *** No errors detected Change-Id: If8195f3af680d27584d3b93735d38483bc2c2000
| } | ||
| const int64_t fee_producer_pool = split_bps(fee_total, cfg.producer_bps); | ||
| const int64_t fee_batch_pool = fee_total - fee_producer_pool; | ||
| const int64_t fee_batch_pool = fee_total; |
There was a problem hiding this comment.
[Low] Finish updating the drainrewards descriptions. This line now correctly treats the entire drained bucket as batch-operator-only, but the drainrewards action doc (sysio.reserv.hpp:227-233), implementation comment (sysio.reserv.cpp:637-639), and test header (sysio.reserv_tests.cpp:1141-1143) still say the sweep is distributed to producers and batch operators. Please update those remaining copies to match the new policy.
There was a problem hiding this comment.
Correct, and the miss is mine — fixed in 9665f788f2. My previous sweep grepped the producer_bps / batch_op_bps phrasing, which matched the four sites I fixed and none of the ones you list, because they carry the same claim as prose (producers + batch operators). Wrong search, not a judgement call about scope.
All three updated, plus one more in the same family:
sysio.reserv.hpp:227-233— thedrainrewardsaction doc now says payepoch pays the sweep ENTIRELY to batch operators on top of their emission share, with the reason producers are excluded (the fee compensates the parties that carry an individual swap — the underwriter, whose share never enters this bucket, and the relaying batch ops).sysio.reserv.cpp:637-639— implementation comment.sysio.reserv_tests.cpp:1141-1143— test-section header.rewardbal's doc (sysio.reserv.hpp:221-223), which you didn't list but says "the rewards half … swept to the emissions treasury for distribution" — the same sentence one action earlier, just vaguer. Now "the batch-operator share … for distribution to batch operators."
One thing beyond what you asked, because I expect it is the next comment otherwise: the three settlement-path docs (applyswap / applyfromwire / paywire, hpp:270/295/316) each say the fee is "split 50/50 to underwriter's claimable accrual / the rewards bucket". That 50/50 is correct — it is FEE_UNDERWRITER_SHARE_BPS, unrelated to the emissions dial — but "bucket" has the same imprecision you flagged on the revert path: the rewards half reaches the bucket less any configured fee_emissions_share_bps. All three now say "rewards pool (which reaches rewards_bucket less any configured fee_emissions_share_bps)".
Comments only: no wasm/abi delta. contracts_unit_test: 583 cases, *** No errors detected.
| BOOST_REQUIRE_EQUAL(857, wire_balance(RESERVE_ACCOUNT)); // 1000 - 135 - 8 emissions | ||
| BOOST_REQUIRE_EQUAL(865, wire_balance(RESERVE_ACCOUNT)); // 1000 - 135; the fee stays in custody | ||
|
|
||
| // A revert has no winning underwriter, so the WHOLE revert fee becomes the |
There was a problem hiding this comment.
[Low] Remove the obsolete 7/8 split from this test header. The block comment immediately above still says a fee of 15 becomes rewards 7 plus emissions 8. In this fixture the underwriter share and emissions dial are both zero, so the assertions below correctly expect all 15 in rewards_bucket. Please update the header to match this corrected explanation and the actual assertions.
There was a problem hiding this comment.
Correct — fixed in 9665f788f2, and this one was worse than a stale comment: my previous commit made the file contradict itself. 296c1abb5f added a comment at the bucket assertions saying the pool reaches rewards_bucket intact, while leaving this header seven lines above still describing the 50/50 rewards/emissions split (rewards floor(15/2) = 7, emissions 8) from the earlier design. I edited the inner comment without reading up to the header.
The header now carries the arithmetic and the reason it holds:
// A nonzero revert fee (caller-fault drain revert) is taken out of the refund and
// routed exactly like a settlement fee, through `route_wire_fee`. Arithmetic here:
// 10% of 150 = 15, and a revert has no winning underwriter, so the whole 15 is the
// rewards pool. This fixture never calls `setconfig`, so `fee_emissions_share_bps`
// is the default 0 and all 15 accrues to `rewards_bucket` — custody-internal, with
// nothing transferred to the emissions treasury.
I also cut the inner comment down to defer to the header rather than restate it — with both spelling out the zero-dial dependency, the duplication was just a second place to drift.
Comments only: no wasm/abi delta. contracts_unit_test: 583 cases, *** No errors detected.
…split Follow-up to 296c1ab, which swept the payepoch overview but missed the remaining copies of the same claim. Comment/doc only — no code, no wasm/abi delta. drainrewards descriptions still said the sweep is distributed to producers and batch operators: the action doc, its implementation comment, rewardbal's doc, and the drainrewards test-section header. The whole drained bucket goes to batch operators; producers are not paid out of swap fees. The refundwire_routes_revert_fee header still described a 50/50 rewards/emissions split of the revert fee ("rewards floor(15/2) = 7, emissions 8") from an earlier design, while the assertions expect all 15 in rewards_bucket -- and 296c1ab added a comment seven lines below saying exactly that, so the file contradicted itself. The header now carries the arithmetic (no underwriter share, zero emissions dial, all 15 to the bucket) and the inner comment defers to it instead of restating it. Also qualified the three settlement-path docs (applyswap / applyfromwire / paywire) that describe the fee's rewards half landing in rewards_bucket: it reaches the bucket less any configured fee_emissions_share_bps, the same imprecision already corrected on the revert path. The previous sweep grepped the producer_bps / batch_op_bps phrasing and the "whole revert fee" wording, which matched neither the prose variant ("producers + batch operators") nor the numeric split. contracts_unit_test: 583 cases, *** No errors detected Change-Id: Ifebf4ac860593ee54cd3220c5bec57468b246271
huangminghuang
left a comment
There was a problem hiding this comment.
Documentation audit follow-up. In addition to the inline comments, please update two claims in the PR description: (1) the five per-share fields sum to fee only on the non-clamped path; when stacked rates consume the leg, fee clamps to wire_amount while the shares can sum higher and the swap is rejected, and (2) qualify the bootstrap-owner note as applying to public bootstrap reserves, since private bootstrap reserves name an owner.
| /// `net` is 0 — so in THAT case alone the per-share fields sum to more than | ||
| /// `fee`, which is harmless because the caller rejects the swap and no share is | ||
| /// ever accrued. The settlement paths already assert this, and | ||
| /// `sysio.reserv::setrsvfee` / `sysio.uwrit::setconfig` cap each rate so a |
There was a problem hiding this comment.
[Medium] Treat zero-net as a valid configured rejection state. The paragraph first says stacked rates can consume the leg, then says the per-rate caps make that unrealistic, but valid settings can reach it directly—for example, network 9999 bps plus one reserve-owner 1 bps already totals 100%. The clamp and zero quote are therefore live behavior, not merely theoretical defense-in-depth. Please document it as an intentionally rejected configuration and sweep the matching MAX_FEE_BPS/"unreachable" claims in sysio.reserv.cpp, sysio.uwrit.hpp/.cpp, and amm_math_tests.cpp. Also note that paywire currently fails closed through the upstream zero-quote check rather than a local net > 0 assertion, so "the settlement paths already assert this" is not literally true.
There was a problem hiding this comment.
Confirmed, and your example is exact — fixed in cb31b67323. MAX_FEE_BPS = 9999 plus MIN_OWNER_FEE_BPS = 1 is 10000 precisely, and nothing cross-checks the sum: setconfig bounds the network rate, setrsvfee bounds each owner rate, and neither knows about the other. So a zero net is a configured rejection the callers must handle, and the paragraph claiming "a realistic combination cannot get there" was simply false.
I made it a test rather than only prose, since a comment is exactly what rotted here. New split_wire_fee_reaches_the_leg_under_valid_configuration pins:
9999 + 1→net == 0,fee == leg(your case, at the owner-fee FLOOR — no extreme setting needed)- both legs at the floor → still a consumed leg, no wrap or under-report
9998 + 1→net > 0— so it is a genuine boundary, not a blanket refusal of high fees
Swept the matching claims: the applyswap and applyfromwire backstop comments (both said "unreachable"), both MAX_FEE_BPS comments in sysio.uwrit (hpp and cpp — each said the cap "keeps the remainder positive", true only for that rate in isolation), and the amm_math_tests suite header, which asserted setconfig makes net == 0 "unconstructible at settlement".
One claim I deliberately kept, because sweeping it would have been wrong: refundwire's "unreachable" is true. A refund leaves the trailing reserve rates at 0, so its total is the network fee alone at ≤ 99.99% and floor division always leaves net >= 1 — there is no second rate to stack. Its only defect was the cross-reference calling it "the same defense-in-depth pattern as applyswap", which stopped being true once applyswap's became live; it now states the path-specific reason instead.
You are also right about paywire, and it is now documented as the exception: it pays the caller's wire_out target rather than net, so it never reads the field. It fails closed on the upstream zero quote and on its own reserve_wire_amount >= wire_out + fee sufficiency check — which is also why it cannot drain a reserve at an arbitrary price the way applyswap/applyfromwire could, so the asymmetry is intentional rather than a missing assert.
contracts_unit_test: 584 cases, *** No errors detected. No wasm/abi delta.
| /// rewards pool (which reaches `rewards_bucket` less any configured | ||
| /// `fee_emissions_share_bps`). Balances are checked BEFORE any mutation; a failed check | ||
| /// aborts the surrounding race-resolution transaction (no half-state). `Σ | ||
| /// reserve_wire_amount` drops by the fee (which leaves the reserve pair but |
There was a problem hiding this comment.
[Medium] Distinguish the total fee from the network-fee split and conditional outflow. Here fee includes both reserve-owner shares as well as the network fee; only the network component is split 50/50 between the underwriter and rewards pool. In addition, a non-zero fee_emissions_share_bps transfers part of that pool to sysio, so the fee does not all stay in custody. Please describe the owner accruals, network split, and conditional emissions transfer explicitly, and update the same custody claims in the applyfromwire/paywire docs plus their implementation comments (both halves stay, none leaves, and whole fee stays).
There was a problem hiding this comment.
Correct on both counts — fixed in cb31b67323. This one is a defect I introduced: my previous commit (9665f788f2) qualified the emissions dial on these three docs and left the "the fee is split 50/50" framing standing, which is the larger error of the two. fee is wire_fee::fee, the TOTAL off the leg — both owner shares plus the network fee — and only the network component splits 50/50. I fixed a smaller imprecision and left a bigger one directly beside it.
All three action docs now state the three-part decomposition explicitly, per path:
applyswap— BOTH reserves'owner_fee_bps, each accrued to its own row'sowner_fee_accrued, plus the network fee.applyfromwire— the DESTINATION reserve's owner fee only (no source reserve).paywire— the SOURCE reserve's owner fee only (recipient is paid in WIRE, so no destination reserve).
…then the 50/50 split of the network component alone, then the conditional fee_emissions_share_bps transfer.
The custody claims are corrected too, since as you note the fee does not all stay:
| Was | Now |
|---|---|
applyswap doc: "drops by the fee … stays in this contract's custody" |
drops by the whole fee; all of it stays as the three accruals except the emissions share, the only part that leaves |
applyfromwire doc: "every part stays in custody, so custody stays balanced" |
every part stays except that share, so custody balances once it is accounted for |
paywire doc: "only wire_out leaves custody" |
what leaves is wire_out plus that share, nothing else |
applyswap impl: "both halves stay in custody" |
routes the NETWORK component; owner shares already accrued above; emissions share is the only outflow |
paywire impl: "the whole fee stays as the underwriter accrual + the rewards bucket" |
stays as three accruals (incl. the source owner accrual) less the emissions share; the Σ reserve_wire_amount vs token-balance reconciliation now accounts for it |
contracts_unit_test: 584 cases, *** No errors detected. No wasm/abi delta.
| | 50% | The swap's **winning underwriter** | Accrues to `sysio.reserv::uwfees`; drawn by that account's own `sysio.reserv::claimuwfee` | | ||
| | 50% | **Batch operators** | Accrues to `sysio.reserv::rewardbkt`; swept by `sysio.system::payepoch` into the batch-op distribution | | ||
|
|
||
| Both halves stay in `sysio.reserv`'s WIRE custody until claimed or drained — no |
There was a problem hiding this comment.
[Medium] Describe configurable routing rather than only the default. The second network-fee half is a rewards pool, not unconditionally a batch-operator payment: fee_emissions_share_bps can divert part of it to the treasury, so both this table and the "no part ... reaches" sentence are true only at the default-zero setting. Please show the stage-2 split, qualify the custody statement, and add the independent per-reserve owner fee so this Fees section does not imply uwconfig.fee_bps is the entire effective swap fee. The corresponding setconfig and uw_config comments in sysio.uwrit.hpp need the same qualification.
There was a problem hiding this comment.
Correct on all three points — fixed in cb31b67323. The Fees section had the same two defects you identified: it presented uwconfig.fee_bps as the entire effective swap fee, and it described the second network half as an unconditional batch-operator payment.
The table now has four rows rather than two, showing the independent owner fee and the stage-2 split:
| Part | Recipient |
|---|---|
Each reserve's owner_fee_bps |
that reserve's owner (owner_fee_accrued → claimrsvfee) |
| 50% of the network fee | the winning underwriter (uwfees → claimuwfee) |
the other 50% — the rewards pool — less fee_emissions_share_bps |
batch operators (rewardbkt → payepoch) |
fee_emissions_share_bps of the pool |
the sysio emissions treasury, transferred at settlement |
Above it, fee_bps is now named the NETWORK fee, with the note that each participating non-WIRE leg's reserve independently charges its own owner_fee_bps off the same leg — so a chain-to-chain swap pays two owner fees plus the network fee, and a WIRE-endpoint swap pays one.
The custody sentence is now scoped rather than absolute: at the default-zero dial every part stays in sysio.reserv's custody and no part reaches the emissions treasury; a non-zero dial diverts that share at settlement, so the claim holds only at the default. I kept "producers are never paid out of swap fees" unqualified, since that one is true at every setting.
Same qualification applied to both sysio.uwrit.hpp sites you point at — the setconfig fee_bps bullet (now carrying the full two-stage routing and the default-zero note) and the uw_config struct doc, which said "no fee-distribution shares live here" and now also says the reserve owner fees charged alongside fee_bps live on their own reserve rows, not here.
contracts_unit_test: 584 cases, *** No errors detected. No wasm/abi delta.
| dst_fee_bps = it->owner_fee_bps; | ||
| } | ||
|
|
||
| // Each participating reserve's owner fee rides the quote alongside the |
There was a problem hiding this comment.
[Medium] Update the public swapquote documentation to match this implementation. This code correctly includes each participating reserve's owner_fee_bps, but the action comment in sysio.reserv.hpp:204-211 still describes the post-fee reduction as only sysio.uwrit::fee_bps. Because callers use this quote for ingestion and race-time variance checks, please document that it prices the network fee plus every non-WIRE leg's reserve-owner fee.
There was a problem hiding this comment.
Correct — fixed in cb31b67323. The implementation passes src_it->owner_fee_bps / dst_it->owner_fee_bps through to quote_swap, while the action doc described the post-fee reduction as only sysio.uwrit::fee_bps.
The doc now states that the quote prices the network fee PLUS every participating non-WIRE leg's reserve owner_fee_bps — both sides on a chain-to-chain swap, one side against a WIRE endpoint (which has no reserve and charges no owner fee).
I also wrote down the consequence you point at, because it is the reason this doc has to be exact rather than merely complete: callers drive ingestion and race-time variance checks off this value, so a quote that priced only fee_bps would drift from the books by each owner fee — which is a variance-check failure at settlement, not a cosmetic doc gap. That framing is now in the comment so the next reader sees why the owner fees belong in the quote rather than being tempted to "simplify" them out.
contracts_unit_test: 584 cases, *** No errors detected. No wasm/abi delta.
| static constexpr uint32_t FEE_SPLIT_TOTAL_BPS = 10000; | ||
|
|
||
| // Swap-fee split, stage 2 — the rewards pool's own division, GOVERNANCE | ||
| // CONFIGURABLE (`reserve_config.fee_emissions_share_bps`, seeded at |
There was a problem hiding this comment.
[Low] Avoid saying this singleton is seeded at bootstrap unless bootstrap actually writes it. The implementation reads an absent reservcfg row with get_or_default(reserve_config{}), and I found no bootstrap call that persists the row. Please describe the dial as defaulting to zero until self-authorized setconfig writes it, or add the missing bootstrap seeding if persistence is intended.
There was a problem hiding this comment.
Correct — fixed in cb31b67323. Your reading of the code is right: no bootstrap path writes reservcfg, the only writer in the tree is setconfig itself, and both readers (fee_emissions_share_bps() and setconfig) go through get_or_default(reserve_config{}). "Seeded at bootstrap" described something that does not happen.
I took the documentation option rather than adding the seeding, because persistence is not intended here — get_or_default already makes an absent row behave as the zero default, so a bootstrap write would add a state transition and a RAM row for no behavioural change. The comment now says so explicitly:
// Nothing seeds the `reservcfg` row: bootstrap does not write it, and every
// read goes through `get_or_default(reserve_config{})`, so the dial reads
// as zero until the self-authorized `setconfig` first persists a row.
Stating the mechanism rather than just deleting the false clause, so the next reader does not "fix" the missing seeding back in.
contracts_unit_test: 584 cases, *** No errors detected. No wasm/abi delta.
…h a test Five review findings on the fee documentation. Comments/docs plus one new test — no contract logic changed, no wasm/abi delta. A zero post-fee leg is a REACHABLE configured state, not unreachable defense-in-depth. The two caps bound their rates independently and nothing cross-checks the sum, so MAX_FEE_BPS (9999) plus one owner fee at MIN_OWNER_FEE_BPS (1) totals exactly 100%. Corrected in split_wire_fee's contract, the applyswap / applyfromwire backstop comments, both MAX_FEE_BPS comments in sysio.uwrit, and the amm_math_tests header that claimed setconfig makes net == 0 "unconstructible at settlement". Added split_wire_fee_reaches_the_leg_under_valid_configuration, pinning 9999+1 -> net == 0, the both-legs overshoot, and 9998+1 still settling, so the boundary is a compiled fact and the net > 0 checks cannot be re-labelled dead code. refundwire's "unreachable" claim is KEPT: a refund passes no reserve fees, so its total is the network fee alone at <= 99.99%. Only its cross-reference to applyswap as "the same pattern" was wrong; it now states the path-specific reason. `fee` is the TOTAL off the WIRE leg -- every participating reserve's owner_fee_bps plus the network fee -- and only the NETWORK component splits 50/50 between underwriter and rewards pool. The applyswap / applyfromwire / paywire docs said "the fee is split 50/50", and their implementation comments claimed "both halves stay" / "every part stays" / "the whole fee stays" in custody, which also ignores the emissions transfer. All six now describe the owner accruals, the network split, and the conditional outflow. swapquote prices the network fee plus every non-WIRE leg's owner fee; its action doc described only sysio.uwrit::fee_bps. Since callers drive ingestion and race-time variance checks off this quote, the doc now states every rate it includes. The sysio.uwrit README Fees section and the setconfig / uw_config comments presented fee_bps as the whole effective fee and the second network half as an unconditional batch-operator payment. They now show the independent per-reserve owner fee, the two-stage network split, and that the custody claim holds only at the default-zero dial. reserve_config is not seeded at bootstrap -- nothing writes the reservcfg row and every read is get_or_default -- so the dial reads zero until setconfig first persists it. Documented rather than adding seeding, which get_or_default makes unnecessary. contracts_unit_test: 584 cases, *** No errors detected Change-Id: Ie8484908ff0f7bca04630593b594efa433d780f5
huangminghuang
left a comment
There was a problem hiding this comment.
Four inline findings from the documentation-focused delegated re-review.
| // single active producer takes the whole producer pool.) | ||
| BOOST_REQUIRE_EQUAL( got, producer_pool ); | ||
|
|
||
| // Nothing was distributed out of the fee: the whole pool belongs to batch |
There was a problem hiding this comment.
[Medium] Add a positive batch-fee payout test. This fixture has no active rotation group and asserts fee_distributed == 0; repository-wide, there is no other contract test combining rewardbkt with an ACTIVE batch group. The related tools flow only waits for sysio.reserv custody to fall by rewardShare, which drainrewards does even if every batch payout is skipped. Please add an active group/member and assert both the recipient balance increase from the fee and the exact positive epochlog.fee_distributed value.
There was a problem hiding this comment.
Confirmed and added in 9b8bddf977 — and your reasoning about why the existing coverage cannot catch this is the important part: drainrewards sweeps the bucket unconditionally, so both the contract test (fee_distributed == 0) and the tools flow (waiting for sysio.reserv custody to fall by rewardShare) pass identically whether every batch payout fired or every one was skipped. The feature's actual payout had no assertion anywhere.
New t5_emissions_tests::payepoch_pays_swap_fee_to_active_batch_operator, on the REAL path rather than a hand-pushed payepoch: a genuine ACTIVE opreg batch operator, scheduled through sysio.epoch::schbatchgps, paid via advance → payepoch inline.
Scaled to a one-member rotation (operators_per_epoch = batch_op_groups = 1, which makes batch_operator_minimum_active 1) so the single member's slice is the entire batch pool and the entire fee pool. That gives exact expected values instead of a proportional bound:
got == batch_pool + fee_total— the recipient balance increase (emission and fee share ride one transfer, so the delta is the sum)log["fee_distributed"] == fee_total— the exact positive value you asked forreward_balance() == 0— bucket sweptt5_after - t5_before == producer_pool + batch_pool + capex + gov— the fee is still excluded from the emission curve
On whether it actually has teeth, since a vacuous pass is the exact failure being fixed here: BOOST_REQUIRE_GT(fee_total, 0) is a hard guard in the test, so fee_distributed == fee_total asserts a strictly positive distributed amount — not 0 == 0. Better, the two cases now bracket the behaviour: identical reserve/swap seeding, differing only in whether a rotation group exists, asserting opposite fee_distributed values (0 vs exactly fee_total). Both pass, so the difference is attributable to the group.
I also rewrote the negative case's comment to name its counterpart, so neither reads as complete coverage on its own.
contracts_unit_test: 585 cases, *** No errors detected. No wasm/abi delta.
| ones. Swap-fee rewards from `sysio.reserv`'s `rewards_bucket` are swept in | ||
| (`drainrewards`) and folded into the same compute distribution on top of | ||
| emissions, split by the same `producer_bps` / `batch_op_bps`. | ||
| (`drainrewards`) and paid out entirely to batch operators, on top of their |
There was a problem hiding this comment.
[Medium] Do not promise that the fee pool is paid out entirely. payepoch explicitly leaves shares in the treasury for zero-epoch groups, inactive/slashed/terminated members, and integer-division remainders (emissions.cpp:626-628), and this test suite even covers a fully swept bucket with fee_distributed == 0. Please describe the pool as allocated exclusively to batch-operator distribution, with only eligible shares actually paid, and sweep the same ENTIRELY / whole pool reaches wording from emissions.cpp, sysio.reserv.hpp/.cpp, sysio.uwrit.hpp, and amm_math_tests.cpp.
There was a problem hiding this comment.
Correct — fixed in 9b8bddf977, and this one is mine: I introduced "ENTIRELY" in the two commits before this while correcting the previous over-claim, and overshot in the other direction. emissions.cpp:626-628 says exactly what you quote, and this suite's own negative case is a live example of a fully swept bucket paying out nothing.
I took your framing literally and split allocation from payout, since the two claims need different scoping:
- Allocation — the pool is allocated exclusively to the batch-operator distribution. This is the true statement, and it is what the stage-2 dial sites are actually about ("at the zero default the whole pool is allocated to batch operators"). Those now say "allocated to" rather than "reaches".
- Payout — only eligible shares are actually transferred; zero-epoch groups, non-ACTIVE members, and integer-division remainders leave their share in the treasury, exactly as undistributed emission does.
Swept all nine sites: emissions.cpp (the payepoch overview and the RESERV_CONTRACT comment), EMISSIONS.md, sysio.reserv.hpp (drainrewards doc, the DEFAULT_FEE_EMISSIONS_SHARE_BPS comment, the setconfig bullet), sysio.reserv.cpp (drainrewards impl, the fee_emissions_share_bps helper), sysio.uwrit.hpp, and amm_math_tests.cpp.
EMISSIONS.md gained the consequence explicitly, since that is the file a reader would consult for the payout model: epochlog.fee_distributed records what was actually paid, so it can be lower than the swept amount — and is 0 when no eligible batch operator existed, even though drainrewards swept the bucket to zero regardless. That is now the documented behaviour rather than a surprise, and it is pinned by both tests.
contracts_unit_test: 585 cases, *** No errors detected. No wasm/abi delta.
| // swap's WINNING UNDERWRITER (the party whose collateral lock let the swap | ||
| // settle) and puts the other half into a REWARDS POOL. The fee RATE | ||
| // (`fee_bps`) is owned by sysio.uwrit. | ||
| static constexpr uint32_t FEE_UNDERWRITER_SHARE_BPS = 5000; // 50% underwriter / 50% rewards pool |
There was a problem hiding this comment.
[Medium] Update the canonical bootstrap documentation with this replacement. libraries/opp/proto/sysio/opp/bootstrap/bootstrap.proto:129-146 still names the removed FEE_REWARD_SHARE_BPS and says fees use the old fixed 50/50 rewards/emissions split. It also presents setconfig as a two-argument action, as does docs/platform-bootstrap-config.md:74, while the ABI now has six fields. Please document the network-fee underwriter/rewards split, optional reservcfg emissions dial, and independent reserve-owner fees, and clarify how the bootstrap schema supplies or defaults the remaining setconfig arguments.
There was a problem hiding this comment.
Correct on every point — fixed in 9b8bddf977. Worth noting how stale that comment was: FEE_REWARD_SHARE_BPS appears nowhere else in the tree, so the proto was the last reference to a constant that no longer exists. And the split it described was wrong twice over — the fixed 50/50 is underwriter/rewards, not rewards/emissions, and the rewards/emissions division is the optional dial.
UwritConfig's doc now carries all three pieces you asked for:
- Network-fee split —
fee_bpsis the NETWORK fee;sysio.reservsplits that component 50/50 between the winning underwriter (claimuwfee) and a rewards pool, the fixedFEE_UNDERWRITER_SHARE_BPS. - Optional
reservcfgdial — the pool splits again byfee_emissions_share_bps; nothing seedsreservcfg, so it reads zero untilsysio.reserv::setconfigfirst persists a row, and at that default no fee leavessysio.reserv's custody at settlement. - Independent reserve-owner fees — each participating non-WIRE leg's reserve charges its own
owner_fee_bpsoff the same WIRE leg, which is explicitly not inReserveSpecand is set post-bootstrap viasetrsvfee. Sofee_bpsis not the whole effective swap fee.
On the setconfig arity — both the proto and docs/platform-bootstrap-config.md:74 now show all six arguments, and state how the remaining four are supplied: they are not part of this spec, and a bootstrap caller passes the contract's uw_config in-struct defaults (5 WIRE floor, 500 bps revert fee, the two uwreq lifecycle windows) unless it has reason to override. I checked for an in-tree consumer of UwritConfig before writing that — there is none, so the message is a schema for external tooling and the honest statement is what the defaults are, not a claim about code that does not exist.
I also fixed the reserved-slots comment (reserved 3, 4, 5), which carried the same wrong 50/50 claim. The slots stay documented — proto field numbers are a wire contract, so recording that they held fee_split_*_pct is what stops a future field reusing them — but it now points at the two-stage split rather than restating a constant that was removed.
Proto comments only: no field, number, or wire-format change.
contracts_unit_test: 585 cases, *** No errors detected. No wasm/abi delta.
| /// out of swap fees — the fee compensates the parties that carry an | ||
| /// individual swap (the winning underwriter, whose share never enters this | ||
| /// bucket, and the batch operators that relay it). Called | ||
| /// inline by payepoch with the amount it read from `rewardbal()`, so the |
There was a problem hiding this comment.
[Low] Match the documented non-positive behavior to the action. While updating this action contract, please also correct the adjacent drainrewards text at lines 251-253: it says amount <= 0 is a defensive no-op, but the implementation immediately checks amount > 0, and drainrewards_auth_and_overdrain_guarded explicitly expects both zero and negative amounts to throw. The documentation should state that non-positive amounts are rejected.
There was a problem hiding this comment.
Correct — fixed in 9b8bddf977. The implementation's first act after require_auth is check(amount > 0, "drainrewards: amount must be positive"), and drainrewards_auth_and_overdrain_guarded pins that for both 0 and -5, so "defensive no-op" described the opposite of the behaviour and contradicted a test in the same repo.
Now:
/// Decrements `rewards_bucket.balance` by `amount`; `lifetime_accrued`
/// (an audit total) is left untouched. A non-positive `amount` is REJECTED
/// (throws — an internal sweep asking for <= 0 is a caller bug, not a
/// no-op), as is an `amount` exceeding the live balance.
I kept the reason inline — this is an internal sweep only payepoch calls, so a non-positive request means the caller's integration logic is wrong and should fail loudly rather than be absorbed. That is also why the over-drain case is grouped with it: both are caller bugs, not tolerable inputs.
contracts_unit_test: 585 cases, *** No errors detected. No wasm/abi delta.
Four review findings. One new test plus doc corrections — no contract logic changed, no wasm/abi delta. Added t5_emissions_tests/payepoch_pays_swap_fee_to_active_batch_operator, the positive counterpart the suite lacked. payepoch_folds_swap_fee_rewards asserts fee_distributed == 0, and drainrewards sweeps the bucket whether or not any payout fires, so neither it nor a flow test watching sysio.reserv custody could distinguish "batch operators were paid" from "the fee rolled to treasury". The new case registers a real ACTIVE opreg batch operator, schedules it via schbatchgps, and runs the real advance -> payepoch path with operators_per_epoch = batch_op_groups = 1 so one member takes the whole batch pool and the whole fee pool -- making the arithmetic exact rather than a bound. Asserts the recipient's balance delta (batch_pool + fee_total), the exact positive fee_distributed == fee_total, the swept bucket, and that the fee stays out of total_distributed. The two cases now bracket the behaviour: identical fee seeding, differing only in the rotation group, asserting opposite fee_distributed values. "Paid out ENTIRELY to batch operators" over-promised. payepoch deliberately leaves shares in the treasury for zero-epoch groups, non-ACTIVE members, and integer-division remainders. Split ALLOCATION from PAYOUT across all nine sites: the pool is allocated exclusively to the batch-operator distribution, and only eligible shares are actually paid. Sites describing the stage-2 dial, where "whole pool" is a correct statement about allocation, now say "allocated to" rather than "reaches". The canonical bootstrap docs still described the removed FEE_REWARD_SHARE_BPS and a fixed 50/50 rewards/emissions split -- wrong twice over, since the 50/50 is underwriter/rewards and the rewards/emissions split is the optional reservcfg dial. bootstrap.proto's UwritConfig doc and its reserved-slots comment now carry the two-stage split, the optional dial and its zero default, and the independent per-reserve owner fee; both it and docs/platform-bootstrap-config.md now show setconfig's six arguments and state that the spec pins two while a caller supplies the rest from uw_config defaults. Proto comments only -- no field or wire-format change; the reserved slots stay documented because field numbers are a wire contract. drainrewards' doc claimed `amount <= 0` is a defensive no-op; the implementation throws and drainrewards_auth_and_overdrain_guarded expects throws for both 0 and -5. It now documents rejection. contracts_unit_test: 585 cases, *** No errors detected Change-Id: I082ade03a9aac6391aa2cf8848f1192c0414156b
huangminghuang
left a comment
There was a problem hiding this comment.
Two inline findings from the delegated re-review of follow-up commit 9b8bddf.
| | `TokenSpec` | `sysio.tokens::regtoken(kind, code, symbol_name, description, precision, address)` then `sysio.tokens::regctok(chain_code, token_code, contract_addr, is_native)` | | ||
| | `ReserveSpec` | `sysio.reserv::regreserve(chain_code, token_code, reserve_code, name, description, initial_chain_amount, initial_wire_amount, connector_weight_bps, is_private, owner)` | | ||
| | `UwritConfig` | `sysio.uwrit::setconfig(fee_bps, collateral_lock_duration_ms)` | | ||
| | `UwritConfig` | `sysio.uwrit::setconfig(fee_bps, collateral_lock_duration_ms, min_fromwire_amount, fromwire_revert_fee_bps, uwreq_pending_timeout_epochs, uwreq_retention_epochs)` — the spec pins the first two; the caller supplies the rest (see below) | |
There was a problem hiding this comment.
[Medium] Make the bootstrap precision mapping executable. The regreserve row still omits the required source_token_precision argument, which should come from the referenced token’s depot-frame TokenSpec.precision. The same docs and test_bootstrap_platform_config.cpp currently accept precision through 18, but sysio.tokens::regtoken rejects anything above 9. That means a configuration can pass strict validation and then fail during the irreversible bootstrap actions. Please include source_token_precision in this mapping and the proto action comment, clarify that it is min(native precision, 9), and change V4/the validator to enforce 1..9.
There was a problem hiding this comment.
Confirmed on all three points — fixed in 73da2dab9b. The "passes strict validation, then fails during irreversible bootstrap" framing is the part that made this worth doing properly rather than as a doc tweak, so I fixed the validator first and added tests.
V4 now enforces 1..9 (test_bootstrap_platform_config.cpp), with the reason inline: TokenSpec.precision is the DEPOT-FRAME precision and sysio.tokens::regtoken rejects anything above MAX_TOKEN_PRECISION (9).
Two facts worth reporting, because they decide whether this is safe:
- Both shipped configs already declare
precision: 9throughout —dex-config.launch.example.json(5 tokens) anddex-config.dev.json(9 tokens). So1..18was permissive slack that nothing relied on; tightening breaks no config.test_oppis green (6 cases). - I added rejection cases rather than leaving the new bound as a silent constant that a future edit could widen back: precision
10rejected,18rejected (ETH's native precision, which must be declared downscaled), and9still valid. Verified executing under--log_level=all.
On the omitted argument — regreserve's eleventh parameter has no ReserveSpec field, so the mapping needed the derivation, not just the name. Now documented in four places: the mapping row, a new paragraph explaining it is a lookup (resolve (chain_code, token_code) → its TokenSpec, pass that precision), the V4 invariant row, and both proto comments — TokenSpec.precision (now stating it is min(native, 9), not native, and why the cap keeps every uint64/int64 amount field in range) and ReserveSpec.token_code (noting the referenced spec also supplies source_token_precision). Proto comments only: no field, number, or wire-format change.
One scope note: this overlaps an in-flight per-token-precision workstream, which independently caps the depot frame at ≤9 and makes Token.precision authoritative as min(native, 9). The change here is consistent with that direction rather than pre-empting it — it closes the validation gap without touching the precision mechanics.
contracts_unit_test: 585 cases green. test_opp: 6 cases green. No wasm/abi delta.
| per-group active-epoch count. Producers are not paid out of swap fees, so | ||
| `producer_bps` / `batch_op_bps` govern the emission split only. | ||
|
|
||
| Allocated is not the same as paid: as with emissions, only **eligible** shares |
There was a problem hiding this comment.
[Low] Replace the zero-epoch retention case with the real one. fee_group_pool is weighted as fee_total * group_epochs / pay_cadence_epochs, so a group with zero active epochs receives a zero allocation; because the group epoch counts still sum to the cadence, it does not leave a share behind. Funds actually remain when there are no groups, an empty group owns positive active epochs, members are ineligible, or division leaves a remainder. Please correct this paragraph and the matching payepoch header/source comments so the accounting explanation matches the implementation.
There was a problem hiding this comment.
Correct, and your derivation is exact — fixed in 73da2dab9b. fee_group_pool = fee_total * group_epochs / pay_cadence_epochs, so a zero-epoch group's allocation is already zero; and since sum(batch_group_epochs) == pay_cadence_epochs, the remaining groups absorb the whole pool. Skipping it retains nothing. I had the causal direction backwards: the continue is a shortcut past an allocation that is zero anyway, not the thing that strands the funds.
The corrected list, which is now stated at the batch-op loop and referenced from the other sites:
- no groups at all (the enclosing
iffails) — the entire pool; - an EMPTY group holding POSITIVE epochs — skipped by
group.empty()before the epoch check, so its weighted slice is genuinely never paid; - a member not opreg-ACTIVE (slashed / terminated / unknown) — that member's per-member slice;
- the remainders of the two integer divisions — per-group weighting, then the even per-member split.
I also state explicitly that a zero-epoch group is not one of them, since that is the inference the old wording invited.
Worth flagging: this error predates my change. emissions.cpp:629 ("groups active in zero epochs … stays in this treasury") and the batch-loop comment at :768 ("their slice stays in treasury") both had it, and I propagated it into EMISSIONS.md, the payepoch header, and drainrewards' doc when writing the eligibility text — so the finding cost you a second pass over text I introduced. Fixed at all five sites that claimed retention.
One site I did not rewrite: sysio.system.hpp:589 says zero-epoch groups "are skipped", which is factually true and claims no retention. I added the missing half-sentence — skipping costs them nothing because a zero count already weights the allocation to zero — rather than restating the list there.
contracts_unit_test: 585 cases, *** No errors detected. No wasm/abi delta.
…tention cases Two review findings. One validator fix with tests, one accounting correction. No contract logic changed, no wasm/abi delta. V4 accepted token `precision` in 1..18 while `sysio.tokens::regtoken` rejects anything above MAX_TOKEN_PRECISION (9), so a config declaring 10..18 passed strict validation and would then abort partway through the IRREVERSIBLE bootstrap actions. Tightened the validator to 1..9 and added rejection cases (10 and 18 rejected, 9 still valid). Both shipped configs already declare precision 9 throughout, so the wider bound was permissive slack nothing relied on. Documented the argument the mapping omitted: `regreserve` takes `source_token_precision`, which has no `ReserveSpec` field of its own and comes from the referenced `TokenSpec.precision` -- itself the depot-frame value min(native, 9). Recorded in the docs mapping row, a new explanatory paragraph, the V4 invariant row, and both proto comments (TokenSpec.precision and ReserveSpec.token_code). Proto comments only; no field or wire-format change. The retention accounting was wrong, in a pre-existing comment I had propagated. A group active in zero epochs retains NOTHING: `pool * group_epochs / cadence` is already zero for it, and because the per-group counts sum to the cadence the remaining groups absorb the whole pool. What actually leaves WIRE in the treasury is: no groups at all; an EMPTY group holding POSITIVE epochs (skipped by the `group.empty()` test before the epoch check); a member not opreg-ACTIVE; and the remainders of the two integer divisions. Corrected at all five sites that claimed retention -- emissions.cpp (payepoch header, the fee-funding note, the batch-op loop), EMISSIONS.md, and sysio.reserv.hpp's drainrewards doc -- plus a clarification in sysio.system.hpp, which claimed only skipping and so was accurate but invited the wrong inference. contracts_unit_test: 585 cases, *** No errors detected test_opp: 6 cases, *** No errors detected Change-Id: Iff33029769d6da11f8f5a0fc8ca7aca1622da7f1
huangminghuang
left a comment
There was a problem hiding this comment.
Three inline findings from the delegated re-review at 73da2da.
| // pay_cadence < batch_op_groups.size()); their slice stays in treasury. | ||
| // Members not registered as ACTIVE in sysio.opreg (slashed / terminated / | ||
| // unknown) are skipped and their slice remains in the treasury. | ||
| // sum(batch_group_epochs) == pay_cadence_epochs by construction, so the |
There was a problem hiding this comment.
[High] Normalize by the actual accrued epoch count. sum(batch_group_epochs) == pay_cadence_epochs is not guaranteed. The existing pay_cadence_change_via_setemitcfg_takes_effect test permits changing cadence 3→1 after one accrual; the next accrueepoch makes the counter sum 2 while these divisions use 1, so an active group can receive 2× batch_pool and 2× fee_batch_pool. The extra fee is paid from the sysio treasury even though only one fee pool was swept, and it is excluded from total_distributed; the shortened genesis period produces the inverse underpayment. Please divide by the actual sum of state.batch_group_epochs, or defer/reject cadence changes while a period is active, and add an active-batch/nonzero-fee regression test around the cadence change. This is current payout correctness, not a backward-compatibility concern.
There was a problem hiding this comment.
Confirmed as a live payout bug and fixed in code, not comments — 8638612f08. Your mechanism is exact, and I measured it rather than reasoning about it.
The negative control. I temporarily reverted the divisor, rebuilt, and ran the new regression test against the old arithmetic:
fatal error: critical check got == batch_pool + fee_total has failed
[187718664666 != 93859332333]
Exactly 2×, precisely as you predicted. Then restored the fix and re-ran: 586 cases green. I did this because "test added, suite green" proves nothing on its own for a payout fix — a test that passes both before and after is indistinguishable from one that asserts nothing.
The fix — normalize by the accrued total. I took that option over deferring/rejecting cadence changes, because it needs no new "period active" state, leaves setemitcfg unrestricted, and also corrects the inverse genesis underpayment. accrued_epochs is the sum of state.batch_group_epochs, so the per-group weights partition each pool by construction, whatever the config did mid-period. A zero total is guarded at each use — impossible in practice (payepoch asserts accrueepoch ran for this same epoch_index) but a zero divisor would abort the entire advance chain.
I hoisted it above the producer section rather than computing it locally, so the batch pool, the fee pool, and expected_rounds share one divisor and cannot drift apart again.
expected_rounds too. It carried the same defect — elig_rounds accumulates across exactly the accrued epochs, so scaling by the configured cadence distorts every producer's share. Worth distinguishing the severity: it feeds a clamp bounded by emis_share, so a mismatch shifts who forfeits how much but can never pay past producer_pool. Not the same class as the pool divisor, which distributes more than 100%.
The regression test runs with an ACTIVE batch group and a NON-ZERO fee, per your request — with either absent the overpayment is unobservable (no group distributes nothing; a zero fee hides the fee half entirely). Its load-bearing assertion is fee_distributed == fee_total.
One thing I got wrong and should own: the false invariant was pre-existing, but my earlier commit strengthened it — I wrote "sum(batch_group_epochs) == pay_cadence_epochs by construction, so the per-group weights partition the pool exactly." I asserted "by construction" about something I had not verified, which is worse than the vaguer original and would have made this harder for the next reader to doubt.
Only sysio.system.wasm changed; no .abi delta, so no SysioContractTypes regeneration. contracts_unit_test: 586 cases, *** No errors detected.
| | `swapfromwire` | `user` | Escrow WIRE and enqueue a swap-FROM-WIRE request | | ||
| | `drainfwq` | `sysio.epoch` or self | Drain the from-WIRE queue: settle what prices, revert the rest (charging the revert fee on caller-fault causes) | | ||
| | `chklocks` | `sysio.epoch` or self | Sweep collateral locks whose wall-clock window has expired | | ||
| | `pruneuwreqs` | permissionless | Expire timed-out PENDING uwreqs and erase terminal rows past their retention window | |
There was a problem hiding this comment.
[Low] Document the actual pruneuwreqs authorization. This action is not permissionless: its implementation requires either sysio.epoch or sysio.uwrit authority and aborts otherwise. Please change the Auth column to sysio.epoch or self so operators are not told they can invoke a restricted maintenance action directly.
There was a problem hiding this comment.
Correct — fixed in 8638612f08. pruneuwreqs requires has_auth(EPOCH_ACCOUNT) || has_auth(get_self()) and aborts otherwise, so "permissionless" told an operator they could call a restricted maintenance action directly. Auth column is now sysio.epoch or self.
Worth noting it was the lone outlier: its two siblings in the same table, chklocks and drainfwq, carry the identical auth check in the implementation and were already documented as sysio.epoch or self. So this was a single stale row rather than a systematic mistake — the row simply predates the auth being added. I checked all three implementations rather than fixing only the one you flagged, in case the same slip had spread.
contracts_unit_test: 586 cases, *** No errors detected.
| // Token paired against WIRE — must reference a `TokenSpec` bound to | ||
| // `chain_code`. | ||
| // | ||
| // That referenced spec ALSO supplies `regreserve`'s `source_token_precision` |
There was a problem hiding this comment.
[Low] Keep source_token_precision in the action synopsis too. This detailed lookup explanation is correct, but the regreserve(...) signature immediately above still jumps from initial_wire_amount to connector_weight_bps, so readers copying the synopsis still construct a ten-argument action. Please add source_token_precision there as the eighth argument; also change the platform guide wording ‘takes an eleventh argument’, since this is the eighth argument in an eleven-argument signature.
There was a problem hiding this comment.
Correct on both — fixed in 8638612f08. You are right that the synopsis is what a reader actually copies, so leaving it ten-argument while explaining the lookup below it was the worst of both: the prose was accurate and the thing people paste was still wrong.
The proto synopsis now reads:
// `sysio.reserv::regreserve(chain_code, token_code, reserve_code, name,
// description, initial_chain_amount, initial_wire_amount,
// source_token_precision, connector_weight_bps, is_private, owner)`.
//
// ELEVEN arguments. `source_token_precision` — the 8th — has no field in this
// message; it is the paired `TokenSpec.precision` (see `token_code` below).
And the platform guide now says regreserve's eleven-argument signature includes it as its eighth argument, replacing my "takes an eleventh argument" — which was doubly misleading, implying both the wrong position and that it was somehow appended rather than sitting mid-signature between initial_wire_amount and connector_weight_bps. That is exactly the kind of error that produces a mis-ordered action, so thank you for catching the wording and not just the omission.
contracts_unit_test: 586 cases, *** No errors detected. Proto comments only — no field or wire-format change.
… configured cadence payepoch divided the batch-operator pool AND the swap-fee pool by cfg.pay_cadence_epochs, but accrueepoch increments one batch_group_epochs slot per epoch unconditionally and setemitcfg may change pay_cadence_epochs at any time (effective on the next advance). The two therefore disagree: lowering cadence 3->1 after one accrual leaves the counters summing to 2 against a divisor of 1, so an active group is paid 2x batch_pool AND 2x fee_batch_pool. The fee half is the damaging one -- only ONE fee pool was swept from sysio.reserv, so the surplus is drawn from the sysio treasury, and fee payouts are excluded from total_distributed, so it never registers against the emission curve. A shortened genesis period underpays by the inverse. This is current payout correctness. Both distributions now normalize by `accrued_epochs`, the sum of the counters, which makes the per-group weights partition each pool by construction whatever the config did mid-period. The value is hoisted above the producer section so the batch pool, the fee pool, and expected_rounds share one divisor and cannot drift apart again; a zero total is guarded at each use because a zero divisor would abort the whole advance chain. expected_rounds carried the same defect and is fixed with it: elig_rounds accumulates across exactly the accrued epochs, so scaling by the configured cadence distorts every producer's pay share. Different severity -- it feeds a clamp bounded by emis_share, so it skews proportions rather than distributing more than producer_pool. Added cadence_drop_midperiod_does_not_multiply_batch_fee_payout with an ACTIVE batch group and a NON-ZERO fee, since with either absent the overpayment is unobservable. Verified as a genuine regression detector, not a vacuous pass: under the old divisor it fails with got == 187718664666 against an expected 93859332333 -- exactly 2x. Also corrected two documentation errors. sysio.uwrit's README listed `pruneuwreqs` as permissionless; it requires sysio.epoch or self, as its siblings chklocks / drainfwq are already documented. And the bootstrap.proto regreserve synopsis still jumped from initial_wire_amount to connector_weight_bps, so a reader copying it would build a ten-argument action -- it now lists all eleven with source_token_precision 8th, and the platform guide says "eighth argument in an eleven-argument signature" rather than "an eleventh argument". contracts_unit_test: 586 cases, *** No errors detected Change-Id: I86611ed7dd46a263c240f2f13e521a3ddeb4bd21
huangminghuang
left a comment
There was a problem hiding this comment.
Two inline documentation findings from the delegated re-review at 8638612.
| is skipped), when a **member is not opreg-ACTIVE**, or as the **remainder** of the | ||
| two integer divisions (per-group weighting, then the even per-member split). A | ||
| group active in **zero** epochs is *not* one of these cases — its weighted | ||
| allocation is already zero, and because the per-group counts sum to the cadence |
There was a problem hiding this comment.
[Low] Replace the configured-cadence invariant here too. This follow-up correctly changed payepoch to normalize by accrued_epochs because the counters do not necessarily sum to the current pay_cadence_epochs after a mid-period config change or during the shortened genesis period. This guide still states the false invariant that led to the payout bug. Please say the counts sum to the actual accrued-epoch divisor, which is why the remaining groups absorb the pool. The related action documentation in sysio.system.hpp should also stop saying zero-count groups are possible only when pay_cadence_epochs < batch_op_groups.size(); the relevant comparison is the actual accrued count, not the configured cadence.
There was a problem hiding this comment.
Correct — fixed in baaace547d, and this is the third pass over the same invariant, which is the part worth naming. I corrected payepoch and the comment sitting next to it, then the retention list, and each time left other documents restating the identical false claim. Fixing code and its adjacent comment is not the same as sweeping the claim.
Both sites you name:
-
EMISSIONS.md— "because the per-group counts sum to the cadence" → the actual accrued-epoch divisor. I also added a paragraph stating the mechanism explicitly rather than only the corrected fact, since this guide is where the wrong invariant would be re-derived from:That divisor is the sum of the per-group counters, not the configured
pay_cadence_epochs. The two can differ — a mid-periodsetemitcfgcadence change, or the shortened genesis period — and normalizing by the configured value is what caused a payout to be multiplied. -
sysio.system.hpp— dropped "which can only happen whenpay_cadence_epochs < batch_op_groups.size()". You are right that the comparison is against the accrued count, not the configured cadence; zero-count groups occur whenever the accrued count is smaller thanbatch_op_groups.size(), whatever the cadence happens to be set to.
contracts_unit_test: 586 cases, *** No errors detected. Comment/doc only — no wasm/abi delta (verified after the rebuild, which the header edit forces).
| | Table | Row type | Description | | ||
| |-------|----------|-------------| | ||
| | `uwconfig` | `uw_config` | Singleton: `fee_bps`, `collateral_lock_duration_ms`, `min_fromwire_amount`, `fromwire_revert_fee_bps`, `uwreq_pending_timeout_epochs`, `uwreq_retention_epochs` | | ||
| | `uwreqs` | `uw_request_t` | One row per swap intent — race state in `commits_by`, `winner`, lifecycle status, mirrored `variance_tolerance_bps`. Retained for `uwreq_retention_epochs` after settlement for audit | |
There was a problem hiding this comment.
[Low] Describe retention after every terminal transition. uwreqs are not retained only after settlement: immediate failures become REJECTED and pending timeouts become EXPIRED, and both stamp current_epoch + uwreq_retention_epochs before pruning just like COMPLETED rows. Please describe this as retention after a terminal COMPLETED / REJECTED / EXPIRED transition so the table matches the lifecycle implemented by reject_and_refund, chklocks, and pruneuwreqs.
There was a problem hiding this comment.
Correct — fixed in baaace547d. I traced all three paths rather than taking the list on trust, and they confirm your reading:
| Terminal status | Stamped in | Trigger |
|---|---|---|
COMPLETED |
chklocks |
CONFIRMED row whose lock window expired — the "settlement" case the row described |
REJECTED |
reject_and_refund |
immediate failure |
EXPIRED |
reject_and_refund (called from pruneuwreqs) |
PENDING row past uwreq_pending_timeout_epochs |
The EXPIRED path is the one the old wording hid most completely: pruneuwreqs abandons the stale PENDING row by calling reject_and_refund with an explicit UNDERWRITE_REQUEST_STATUS_EXPIRED argument, so it stamps current_epoch + uwreq_retention_epochs through exactly the same line as a rejection. All three then get erased by pruneuwreqs once past the window.
So "after settlement" documented one third of the lifecycle and implied the other two either were not retained or were erased immediately. The row now names each transition with the function it goes through, so the table matches reject_and_refund / chklocks / pruneuwreqs as implemented.
contracts_unit_test: 586 cases, *** No errors detected. Documentation only.
… retention Two review findings, both documentation. No wasm/abi delta. The false invariant that produced the payout bug survived in two more places after the code fix. EMISSIONS.md still said the per-group counts "sum to the cadence", and sysio.system.hpp still said zero-count groups are possible only when pay_cadence_epochs < batch_op_groups.size(). Both now reference the ACTUAL accrued-epoch divisor, and EMISSIONS.md gained a paragraph naming the configured cadence as the wrong divisor and why -- a mid-period setemitcfg change or the shortened genesis period makes the two disagree -- so the guide records the reason rather than only the corrected fact. uwreqs retention was described as applying "after settlement", which covers only the COMPLETED third of the lifecycle. All three terminal statuses stamp current_epoch + uwreq_retention_epochs: COMPLETED in chklocks, REJECTED in reject_and_refund, and EXPIRED through the same helper when pruneuwreqs abandons a PENDING row past its timeout. The table row now names all three transitions and the path each takes, then the pruneuwreqs erase. contracts_unit_test: 586 cases, *** No errors detected Change-Id: I2e886ffbad333899fe023b988fb62b303ae8616a
huangminghuang
left a comment
There was a problem hiding this comment.
Two inline documentation findings from the delegated re-review at baaace5.
| * sum of those counters) rather than the configured | ||
| * pay_cadence_epochs, which a mid-period setemitcfg change or the | ||
| * shortened genesis period can make disagree. Groups active in zero | ||
| * epochs are skipped, which happens whenever the accrued count is |
There was a problem hiding this comment.
[Low] Document accrueepoch on pay epochs too. While updating this action block for the actual accrued-epoch divisor, please also correct the accrueepoch documentation immediately below: sysio.epoch::advance calls it on every successful epoch, including a pay epoch, and then queues payepoch after it. Saying it runs only on non-pay epochs hides the current pay epoch from the very counter sum this paragraph now explains and can recreate the same off-by-one mental model.
There was a problem hiding this comment.
Correct — fixed in 504cc7f2a4. Verified against sysio.epoch::advance: accrueepoch is queued unconditionally and only payepoch is gated on gate.is_pay_epoch, with accrue queued first so FIFO inline ordering lets payepoch observe the post-accrue state. "Every non-pay epoch" was simply wrong.
Your diagnosis of the consequence is the valuable part, and I think it is the actual origin of the bug. If accrual skipped the pay epoch, then on a pay epoch the counters would sum to cadence - 1 — and a divisor of cadence would look nearly right, off by one epoch, which is exactly the kind of discrepancy that survives review. The stale doc made the wrong divisor plausible.
It also contradicted two things in the tree simultaneously: the paragraph one block above it (which I had just corrected to explain the counter sum), and cadence_drop_midperiod_does_not_multiply_batch_fee_payout, which depends on the pay epoch accruing to reach a sum of 2. So the documentation disagreed with verified behaviour, not just with itself.
The comment now records the causal link rather than only the corrected fact:
* Because it also runs on the pay epoch, the counter sum payepoch
* normalizes by INCLUDES the epoch being paid. Reading this as
* "non-pay epochs only" understates that sum by one and is how the
* configured-cadence divisor came to look correct.
contracts_unit_test: 586 cases, *** No errors detected. Comment only — no wasm/abi delta.
| | Table | Row type | Description | | ||
| |-------|----------|-------------| | ||
| | `uwconfig` | `uw_config` | Singleton: `fee_bps`, `collateral_lock_duration_ms`, `min_fromwire_amount`, `fromwire_revert_fee_bps`, `uwreq_pending_timeout_epochs`, `uwreq_retention_epochs` | | ||
| | `uwreqs` | `uw_request_t` | One row per swap intent — race state in `commits_by`, `winner`, lifecycle status, mirrored `variance_tolerance_bps`. Retained for `uwreq_retention_epochs` after ANY terminal transition — `COMPLETED` (settled by `chklocks`), `REJECTED` (immediate failure via `reject_and_refund`), or `EXPIRED` (pending timeout, same path) — then erased by `pruneuwreqs` | |
There was a problem hiding this comment.
[Low] Do not call chklocks the settlement step. Reserve settlement happens during winner selection, where applyswap / applyfromwire / paywire are queued and the row becomes CONFIRMED. chklocks runs later, after the challenge window, and only transitions that already-settled CONFIRMED row to COMPLETED once its final collateral lock expires. Please describe this as COMPLETED after the final lock sweep rather than ‘settled by chklocks’.
There was a problem hiding this comment.
Correct — fixed in 504cc7f2a4. My wording collapsed a two-stage lifecycle into one step and put settlement in the wrong stage. Confirmed against the call sites: applyswap / applyfromwire / paywire are queued in the winner-selection path, in the same reqs.modify that flips the row to CONFIRMED; chklocks runs later, after the challenge window, and its only job for that row is CONFIRMED → COMPLETED once the final collateral lock expires.
The row now reads:
COMPLETED(afterchklockssweeps the final collateral lock; the reserve settlement itself already happened at winner selection, which is what made the row CONFIRMED)
One adjacent thing I noticed and deliberately left alone, since I think it is where this confusion comes from: chklocks stamps settled_at_ms on that CONFIRMED → COMPLETED transition — so the field named "settled at" records the lock sweep, not the settlement. That is a pre-existing ABI field name, outside this PR's scope, and renaming it mid-review would be disproportionate. Flagging it rather than silently touching it; happy to file it separately if you want it aligned.
contracts_unit_test: 586 cases, *** No errors detected. Documentation only.
…ent step Two review findings on wording introduced by the previous commit. Documentation only; no wasm/abi delta. sysio.system.hpp described accrueepoch as running "on every non-pay epoch". sysio.epoch::advance queues it UNCONDITIONALLY and gates only payepoch on is_pay_epoch, queueing accrueepoch first so FIFO inline ordering lets payepoch observe the post-accrue state. The stale claim contradicted the paragraph one block above it and is plausibly how the configured-cadence divisor came to look correct: if accrual skipped the pay epoch, a counter sum of cadence-1 against a divisor of cadence would be the natural mental model. It also contradicted cadence_drop_midperiod_does_not_multiply_batch_fee_payout, which depends on the pay epoch accruing to reach a counter sum of 2. The comment now states that the counter sum INCLUDES the epoch being paid, and why misreading it understates the sum. sysio.uwrit's README called chklocks the settlement step. Reserve settlement is queued during winner selection -- applyswap / applyfromwire / paywire, in the same modify that flips the row CONFIRMED. chklocks runs later, after the challenge window, and only transitions that already-settled row to COMPLETED once its final collateral lock expires. Described as COMPLETED after the final lock sweep, noting where settlement actually happened. contracts_unit_test: 586 cases, *** No errors detected Change-Id: I62812700d11a0661ac3eaf1c2cfc84eb07f1eaaf
huangminghuang
left a comment
There was a problem hiding this comment.
Approved. One non-blocking inline grammar correction remains. Please also update the PR description so it matches the final PR: say the default rewards pool is allocated exclusively to the batch-operator distribution while only eligible shares are paid; qualify the five-share conservation claim to the non-clamped path (stacked rates can clamp fee to wire_amount, after which settlement rejects zero net); qualify the owner-less bootstrap statement to public bootstrap reserves because private bootstrap reserves name an owner; and add the accrued-epoch normalization/cadence-drop regression plus the current 586-case validation result. These are documentation follow-ups, not blockers on the implementation approval.
| * the next payepoch sees the period total + per-group counts. | ||
| * | ||
| * Because it also runs on the pay epoch, the counter sum payepoch | ||
| * normalizes by INCLUDES the epoch being paid. Reading this as |
There was a problem hiding this comment.
[Low] Fix the missing relative pronoun. ‘The counter sum payepoch normalizes by INCLUDES’ is ungrammatical and makes the key invariant harder to read. Please change it to ‘the counter sum that payepoch normalizes by includes the epoch being paid.’
There was a problem hiding this comment.
Fixed in c55cc14ee2, using your wording verbatim:
Because it also runs on the pay epoch, the counter sum that
payepochnormalizes by includes the epoch being paid.
You are right on both counts. The missing pronoun made "normalizes by INCLUDES" parse as a verb collision, and the sentence is doing real work here — it is the one line explaining why the divisor is the accrued sum rather than the configured cadence, so it is the worst place in the file to make a reader re-parse. I also dropped the shouted INCLUDES along with it; the sentence structure carries the emphasis without it, and the capitals were compensating for the broken grammar rather than adding anything.
contracts_unit_test: 586 cases, *** No errors detected. Comment only — no wasm/abi delta.
"the counter sum payepoch normalizes by INCLUDES" was ungrammatical, obscuring the invariant it exists to state. Reworded to the reviewer's suggested text: "the counter sum that `payepoch` normalizes by includes the epoch being paid". The shouted INCLUDES is dropped with it -- the sentence structure carries the emphasis. Comment only; no wasm/abi delta. contracts_unit_test: 586 cases, *** No errors detected Change-Id: Id6178f3e7587483c39ab0f0425a180f49f86240e
huangminghuang
left a comment
There was a problem hiding this comment.
Documentation-only follow-up at c55cc14ee2. One additional issue cannot be attached inline because its lines are outside the PR diff: contracts/sysio.system/include/sysio.system/emissions.hpp:183-195 still says accrueepoch updates pending_emission_amount and batch_group_epochs only on non-pay epochs. sysio.epoch::advance actually invokes it on every successful epoch, including the pay epoch, so both comments should be corrected to match the invariant documented elsewhere.
Please also update the PR description, which still says the default rewards pool "reaches" batch operators rather than being allocated to their distribution with only eligible shares paid; states five-share conservation without the clamped-path exception; categorically calls bootstrap reserves owner-less even though private bootstrap reserves name an owner; and omits the accrued-epoch normalization/cadence-drop regression plus the current 586-case validation result. These are documentation follow-ups, not implementation blockers.
| /// 2. `emissions_share_bps` of that POOL (not of the whole fee) goes to the | ||
| /// `sysio` emissions treasury; the rest is the batch-operator share. | ||
| /// | ||
| /// All integer, exact: the five shares sum to `fee` and `net + fee == |
There was a problem hiding this comment.
[Low] Qualify conservation to the non-clamped path. This states unconditionally that the five share fields sum to fee, but the paragraph below correctly says they can sum to more than fee after total_fee is clamped to wire_amount. Please state that five-share conservation holds only when total_fee < wire_amount; net + fee == wire_amount remains unconditional.
There was a problem hiding this comment.
Correct — fixed in 25092af3b4. The sentence contradicted the clamp paragraph directly below it, which is the worse form of the error: a reader who trusts the first statement has no reason to read on, and the clamp is precisely the case where the five shares stop summing to fee.
Split into the conditional and unconditional halves:
/// All integer, exact, because every stage takes a REMAINDER rather than a
/// second floored product. `net + fee == wire_amount` holds UNCONDITIONALLY.
/// Five-share conservation — the shares summing to `fee` — holds only on the
/// non-clamped path, i.e. when `total_fee < wire_amount`; once the total reaches
/// the leg `fee` is clamped to `wire_amount` and the shares sum to MORE than it
/// (see the clamp note below). Computed in `u128` to avoid overflow.
net + fee == wire_amount is called out as unconditional exactly as you say, since it holds on both paths — the clamp sets fee = wire_amount, net = 0, which satisfies it.
Comment only. See my note on the other three in this batch regarding test verification.
| /// TRANSFERRED to the `sysio` treasury — the only part that leaves custody. | ||
| /// No-op when there is no fee. | ||
| /// | ||
| /// At the default config the emissions share is 0, so the whole fee moves from a |
There was a problem hiding this comment.
[Low] Describe only the network component here. wire_fee::fee also contains the source and destination reserve-owner shares, which this function does not route because they are accrued to the reserve rows by the settlement actions. At the default config, the total fee therefore moves into the owner accrual(s), uwfees, and rewards_bucket - not wholly into the two accumulators described here. Please scope this comment to the network component.
There was a problem hiding this comment.
Correct — fixed in 25092af3b4. The distinction you draw is the one this doc was missing: route_wire_fee receives the whole wire_fee struct but routes only the network component, because the owner shares were already accrued to their reserve rows by the calling settlement action. Describing "the fee" as landing in two accumulators conflated the argument it takes with the subset it acts on.
Both halves scoped:
- The opening now says it routes the NETWORK COMPONENT, and states explicitly that the owner shares in
feeare not routed here — the settlement actions accrue those before calling it. - The custody paragraph now says everything this function routes stays in custody at the default dial, and adds that the total
wire_fee::feeis larger whenever a participating reserve charges an owner fee — so at the default the whole fee ends up spread across the owner accrual(s),uwfees, andrewards_bucket, exactly as you describe.
On verification, stated plainly: all four changes in this batch are comment-only, and I proved that mechanically rather than assuming it — every added/removed line in the diff matches a comment prefix, with no code line touched. I did not re-run contracts_unit_test for this commit, invoking the standing rule's documentation-only carve-out. Two reasons beyond the proof: this branch is checked out in a fresh isolated worktree with no build tree (a from-scratch wire-sysio build for comment edits is disproportionate), and another task is actively using the shared build directory on this host, so a heavy parallel build would contend with it. Every prior comment-only commit in this series verified a zero wasm/abi delta. Say the word if you want the full build run anyway and I will stand one up.
| // its quotes share one curve. The fee is then taken OUT of this WIRE leg: the | ||
| // source side gives up the full gross WIRE, only `net` continues to the | ||
| // destination side, and `fee` is routed to rewards + emissions. | ||
| // destination side, and `fee` is split between the winning underwriter's |
There was a problem hiding this comment.
[Low] Include all destinations of the total fee. Here fee is wire_fee::fee, so it includes both reserve-owner shares as well as the network fee. This sentence says it is split only between the winning underwriter and batch operators, and also omits the optional emissions transfer. Please describe the owner accruals, network split, and conditional emissions outflow consistently with the action-level documentation.
There was a problem hiding this comment.
Correct — fixed in 25092af3b4. applyswap is the site where the omission is largest, since it is the only path where BOTH reserves charge an owner fee, so "split between the underwriter and batch operators" left two of the four destinations unmentioned plus the conditional fifth.
Now:
// ... The TOTAL `fee` spreads across BOTH reserves' owner
// accruals (made in the modifies below), the winning underwriter's accrual,
// the batch-operator rewards bucket, and — only when
// `fee_emissions_share_bps` is set — a transfer of that share to `sysio`.
I pointed at the modify calls below for the owner accruals specifically, so the reader can see where those two happen rather than having to infer that route_wire_fee handles them (it does not — that was the confusion in the sibling finding on this same function's doc).
Comment only; see my note on route_wire_fee in this batch regarding test verification.
| const auto fee = opp::amm::split_wire_fee(wire_in, uwrit_fee_bps(), FEE_REWARD_SHARE_BPS); | ||
| // becomes destination-reserve liquidity; the fee is split between the winning | ||
| // underwriter's accrual and the rewards bucket. The full `wire_in` was | ||
| // escrowed in this contract at `swapfromwire` time and none of it leaves |
There was a problem hiding this comment.
[Low] Do not say none of the escrow leaves custody. A non-zero fee_emissions_share_bps transfers that share of the rewards pool to sysio below, so this is true only at the default-zero dial. The accounting description should also include the destination reserve's owner-fee accrual rather than reducing the fee to two accumulators.
There was a problem hiding this comment.
Correct — fixed in 25092af3b4. "None of it leaves here" was the strongest of the stale custody claims, and it sat two lines above the very call that passes fee_emissions_share_bps(get_self()) into split_wire_fee — so the code contradicting it was in the same visual block.
Now:
// Fee out of the user's escrowed input WIRE: only the post-fee remainder
// becomes destination-reserve liquidity. The TOTAL fee spreads across the
// DESTINATION reserve's owner accrual, the winning underwriter's accrual, and
// the rewards bucket. The full `wire_in` was escrowed in this contract at
// `swapfromwire` time; the ONLY part that leaves here is a configured
// `fee_emissions_share_bps` of the rewards pool. At the default-zero dial
// nothing leaves and custody balances as net -> Σwire, fee -> the accruals.
Both of your points are in there: the destination reserve's owner accrual is named as a destination (this path has no source reserve, so it is the one owner fee), and the custody claim is now scoped to the default-zero dial rather than stated absolutely.
Comment only; see my note on route_wire_fee in this batch regarding test verification.
…lly routes Four review findings, all comments. Every changed line is a comment (verified mechanically); no code, no compiled-artifact change. amm_math.hpp stated five-share conservation unconditionally, contradicting the clamp note below it. Conservation of the five shares holds only on the non-clamped path (total_fee < wire_amount); once the total reaches the leg, `fee` is clamped and the shares sum to more than it. `net + fee == wire_amount` is unconditional and is now stated as such. route_wire_fee's doc described the whole fee moving into two accumulators. It routes only the NETWORK component: the reserve-owner shares carried in `wire_fee::fee` are accrued to their own reserve rows by the settlement actions before this is called. Scoped accordingly, noting that at the default dial the total fee ends up spread across the owner accrual(s), `uwfees`, and `rewards_bucket` rather than the two accumulators alone. applyswap's comment said `fee` is split between the underwriter and batch operators, omitting both owner accruals and the conditional emissions transfer. applyfromwire's said none of the escrow leaves custody, which holds only at the default-zero dial, and likewise reduced the fee to two accumulators. Both now match the action-level documentation. contracts_unit_test NOT re-run for this commit: the standing rule's documentation-only carve-out applies, and the comment-only property is proven mechanically rather than assumed. Every prior comment-only commit in this series verified a zero wasm/abi delta. Change-Id: I4ff0687b6a6e56ab91d38025898b1aed25f38a6f
huangminghuang
left a comment
There was a problem hiding this comment.
Two non-inline documentation findings remain at 25092af3:
-
[Low] Correct the pay-cadence accumulator comments.
contracts/sysio.system/include/sysio.system/emissions.hpp:183-195still saysaccrueepochincrementspending_emission_amountandbatch_group_epochsonly on non-pay epochs.sysio.epoch::advanceinvokes it on every successful epoch, including the pay epoch. Please correct both comments so they match the invariant now documented insysio.system.hppandEMISSIONS.md. -
[Low] Update the PR description to match the final PR. It still says the default rewards pool "reaches" batch operators instead of being allocated to their distribution with only eligible shares paid; states five-share conservation without the clamped-path exception; categorically says bootstrap reserves are owner-less even though private bootstrap reserves name an owner; and omits the accrued-epoch normalization/cadence-drop regression plus the current 586-case validation result.
These are documentation follow-ups, not implementation blockers.
Closes the fee gaps: the swap fee was reaching producers + batch operators, while the winning underwriter — whose collateral lock is what lets a swap settle at all — got nothing, and reserve owners earned nothing, so a third party had no reason to fund a reserve.
Implements the two-independent-fee model (Jonathan, 2026-08-04).
The model
Network fee (
sysio.uwrit::fee_bps) — 50% accrues to the winning underwriter insysio.reserv::uwfees, drawn viaclaimuwfee. The rest is a rewards pool split between batch operators and the emissions treasury byreserve_config.fee_emissions_share_bps, default 0, so by default the whole pool reaches batch operators and no fee leaves custody at settlement. Producers no longer draw from swap fees — they earn emissions.Reserve owner fee (
reserve_row.owner_fee_bps) — per-reserve, set by the owner viasetrsvfeeoncematchreserveresolves ownership, drawn viaclaimrsvfee. Every reserve supplying a leg charges its own rate, so a chain-to-chain swap pays three fees: two reserve owners plus the network. Bounded0(off) or[1, 9900].The load-bearing bit
split_wire_feeis now the one decomposition used by bothquote_swapand all four settlement paths, with every rate applied to the same gross WIRE leg. Additive, order-independent, and exact — the five shares sum tofee, andnet + fee == wire_amount, because each stage takes a remainder rather than a second floored product. Quote and settlement cannot drift by a rounding subunit, which matters because the variance check runs off that quote at ingestion, race resolution, and drain.Custody invariant extends to:
Design notes
oncrtreserveinserts the row PENDING andmatchreserveis what resolvesowner, so at create time the depot doesn't know who the owner is. A depot-side owner action avoids a proto field and anywire-ethereum/wire-solanachange, and makes the fee tunable.owner = "", so no account can authorize a fee on them — which is exactly what stops WIRE accruing with no claimant.reserve_row, not a parallel table: it's the reserve's own revenue, per-reserve by definition, and its claimant is already a field there. (uwfeesneeds its own table only because underwriters have no row insysio.reserv.)Also sets the from-WIRE revert fee to its 500 bps launch default, and rewrites
sysio.uwrit/README.md, which described a contract that never existed (distfee,submituw,collateral/uwledgertables).Verification
contracts_unit_test:*** No errors detectedsetrsvfeeowner/status/bounds guards; both-reserve accrual onapplyswapwith the three-fee arithmetic checked exactly and custody proven unchanged; single-reserve attribution onpaywire/applyfromwire;claimrsvfeepayout + auth + double-claim + lifetime retention;swapquotereprice;setconfigauth/cap/full-pool routing;amm_mathfive-way conservation sweepsTickets
WIRE-296, WIRE-281, WIRE-315, WIRE-299
Reviewer note
The
.wasmartifacts for contracts this PR does not touch are also modified — ABI-identical rebuilds from the fullcmake --build+ copy-back, which is what the green test run used.Related PRs — one change across four repos
Land in this order; the first three are coupled:
SysioContractTypesfrom those ABIswire-libraries-ts#60 must merge before wire-tools-ts#54 (the harness builds against the committed
SysioContractTypes), and both need wire-sysio#546's ABIs first. The manifest PR is doc-only and independent.Running the e2e gate on this set needs all three code branches passed explicitly:
BRANCH_WIRE_SYSIO=feat/swap-fee-distribution,BRANCH_WIRE_LIBRARIES_TS=feat/swap-fee-distribution,BRANCH_WIRE_TOOLS_TS=feat/swap-fee-distribution.The tools-ts override is not optional — without it the gate discovers and runs master's flows, so none of the new fee coverage executes. wire-ethereum / wire-solana stay on their manifest defaults (
next); neither repo is touched by this change.Gate run: all 13 flows passed (2026-08-05) — resolved
wire-sysio@f982725a,wire-libraries-ts@ab92a844,wire-tools-ts@74b2f092; wire-ethereum / wire-solana on manifest defaultnext.