Repository navigation
Retire reward and reserve creation carriers - #670
Conversation
6f2d37e to
517de37
Compare
517de37 to
8cbc553
Compare
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Second-pass review at 8cbc553e. Current-head CI is green. I have framed the generated-model concern as a coordinated companion-PR and release-order requirement rather than a request to retain the retired behavior in this PR. The remaining findings are inline.
| APPROVED request can no longer be challenged and its held items release after clear. Read the | ||
| request deadline from `sysio.bond` before relying on step 5. A frozen queue step still issues requests, so `crank` during the freeze gives every WAITING |
There was a problem hiding this comment.
There is no way to read that deadline from this playbook; no command reads sysio.bond. It is bonded_at + window_sec on the requests row and only exists once the request is BONDED. Worth stating too: a hold placed after the deadline still works until someone calls approve, and rslvinvalid has the same cutoff.
There was a problem hiding this comment.
Addressed. The playbook now reads sysio.bond::requests through the bondrequests table and explains pagination and bonded_at + window_sec. OPEN requests have no running approval deadline. Passing the deadline enables approval but does not close challenges or rslvinvalid until approval actually happens. A new regression freezes the cord, passes the deadline, successfully holds the request, and rules it invalid; the existing regression separately covers approval while frozen and subsequent challenge refusal.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| WAITING envelope has no request yet, so run `crank` first. `CHALLENGER` must not be `sysio.synd` or `sysio.bond`; contract accounts are supported through ledger settlement. The challenger pays the request's hold bond plus the pair's `challenge_extra`; both transfers go into | ||
| custody, so the challenge runs while the depot is frozen. `rslvinvalid` rules a request bonded or not, |
There was a problem hiding this comment.
The challenger has to hold the hold bond in that shadow token before the freeze: sysio.liq::transfer to a non-custody account is refused while the cord is pulled, so it cannot be funded during one. "Before an incident" should list it.
There was a problem hiding this comment.
Addressed. The pre-incident checklist now requires funding challengers with the correct shadow LIQ token for the hold bond plus challenge_extra before a freeze, since a transfer to a non-custody account is refused while the cord is pulled.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| clio -u "$DEPOT_URL" push action sysio.synd finishreturn "[$REQUEST_ID]" -p sysio@active | ||
| clio -u "$DEPOT_URL" push action sysio.synd refundreturn "[$REQUEST_ID]" -p sysio@active |
There was a problem hiding this comment.
finishreturn and refundreturn for the same request in one block: pasted together, the first erases the row and the refund then fails. Separate blocks would be safer. The reads that prove "never paid, not pending" were also dropped from this section, and desyndicationsPaid no longer has a command form anywhere in the playbook.
There was a problem hiding this comment.
Addressed. Paid-return completion and irreversibly canceled-return refund are in separate command blocks. The section restores executable Ethereum desyndicationsPaid/pending-payout reads and Solana pending-payout reads. A refund requires evidence of irreversible cancellation, no payment, and no remaining payable obligation; a pending payout must retain the return row.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| /// Default minimum return quantity, in shadow base units. | ||
| static constexpr uint64_t DEFAULT_MIN_DESYNDICATE = 1; |
There was a problem hiding this comment.
This default is never read: desyndicate refuses a pair with no row, and setconfig always writes the argument. I'd drop it and the member initializer. As it stands the floor has no value: setconfig accepts 1, the fixtures pass 1, and the docs name no launch value. Whatever is chosen has to clear what liqsol-core can pay: a release that rounds to zero distribution shares is refused there and can never settle.
There was a problem hiding this comment.
Addressed. Removed the unused default constant and member initializer. The approved launch/bootstrap choice is one shared gross minimum of 1,000,000 base units, equivalent to 0.001 SOL at nine decimals, for both shadow pairs. It is intentionally arbitrary and is documented as a launch floor, without adding per-chain configuration policy. The companion harness supplies the required field in bootstrap and retained flow configurations and tests both bootstrap rows. Tiny-value fee/budget edge tests keep explicit fixture settings. This is not a guarantee that every possible future distribution-share ratio can pay that amount.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| BOOST_REQUIRE_EQUAL(wasm_assert_msg(message), setconfig(SOLANA, LIQSOL, | ||
| {.min_desyndicate = static_cast<uint64_t>(asset::max_amount) + 1})); |
There was a problem hiding this comment.
The burst clause rejects this one (the fixture's default burst is below 2^62), so the asset-range clause is never the deciding one, and min == desynd_burst is never accepted. Suggest {.desynd_burst = 0, .min_desyndicate = max + 1} refused and {.desynd_burst = UNIT, .min_desyndicate = UNIT} accepted.
There was a problem hiding this comment.
Addressed. The out-of-asset-range case now uses desynd_burst = 0, so the asset-range check decides it. Added acceptance at min_desyndicate == desynd_burst == UNIT and at the asset maximum with an unlimited burst. These run in the passing syndication suite.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| // Small treasury liquidity deliberately reaches the second fundclaim cap. | ||
| BOOST_FIXTURE_TEST_CASE(generic_staking_rewards_fund_claim_and_dedupe_through_dispatch, sysio_dispatch_tester) { | ||
| // Funded pre-launch imports link and settle the exact token balance. | ||
| BOOST_FIXTURE_TEST_CASE(funded_import_claims_link_and_settle, sysio_dispatch_tester) { |
There was a problem hiding this comment.
With this rewrite nothing delivers type 60950, or any undeclared type; the drop now rests on default: in dispatch_attestation. Could retired_attestations_do_not_mutate_collateral_or_block_liq also send static_cast<AttestationType>(60950) and a never-declared number?
There was a problem hiding this comment.
Addressed. The mixed-envelope regression now includes raw removed values 60950 and 60958 and a never-declared 65000 with malformed payloads alongside an active LIQ attestation. It verifies no collateral or DClaim mutation and successful active LIQ processing. Raw discriminants are injected through protobuf reflection rather than an invalid enum cast. The dispatch suite passes.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| /// (2^62-1); imported credits cap at | ||
| /// the asset maximum rather than abort. A single staker balance approaching 2^62 atomic WIRE units | ||
| /// is not reachable in practice — the cap exists purely to preserve the never-throw contract. | ||
| /// is not reachable in practice; imports retain the established saturation behavior. |
There was a problem hiding this comment.
The cap is not for imports; importseed is privileged and already check()s. It is for linkswept, which is reached from OPP dispatch (nodeownreg -> recordlink -> linkswept) and must not abort. Also credit_wire is now only called with name{}, so wacct and the linked branch are dead.
There was a problem hiding this comment.
Addressed. The cap comment now attributes saturation to linkswept, which must preserve its OPP parent, while privileged imports retain their rejecting checks. Replaced the dead linked/unlinked helper with credit_unmapped and removed the unused account argument/branch. Added a regression that sweeps another imported identity into a nearly full pending balance, saturates at the asset maximum, and removes the unmapped row without aborting.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| /// Governance: mint `quantity` back to `holder` after its outpost refused | ||
| /// a de-syndication (reconciled from the outpost log). Requires this | ||
| /// contract's authority. |
There was a problem hiding this comment.
This still opens with the old rule and then says the opposite. One sentence would do: a privileged supply repair, never for a return tracked in sysio.synd::returns. recredit_mints_a_refused_desyndication_back and a_recredit_is_seen_by_the_next_comparison (its body calls refundreturn) describe the old rule too, and the playbook still has "Never recredit it."
There was a problem hiding this comment.
Addressed. The action documentation now states one rule: privileged supply repair, never recovery of a return tracked in sysio.synd::returns. Renamed the supply-repair regression and the refundreturn comparison regression to match their behavior. The playbook now says to retain a pending return and never refund it, rather than the ambiguous “Never recredit it.”
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
| // TWO call sites remain, and every one targets a PROTOCOL-CONTROLLED account. What makes each | ||
| // safe differs, and the difference is the thing to preserve: |
There was a problem hiding this comment.
Both remaining sites are safe for the same reason now (no code deployed), so "What makes each safe differs" no longer holds. Other comments the removal left behind: "unlike sysio.dclaim" below and in the claimpay doc, "capital" in the payepoch doc, "or staking credits" above source_chain_binding_ok, onreward in safe_ops.hpp and claimable.hpp, and the now-unused kv_global.hpp include in sysio.liq.hpp.
There was a problem hiding this comment.
Addressed all listed leftovers: both automatic emission destinations are described as safe because they have no deployed code; removed the DClaim contrast, capital wording in payepoch, staking-credit binding description, retired onreward examples, and unused kv_global.hpp include. The rebuilt contracts and relevant LIQ/emissions/dispatch suites pass.
Updated in 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
8cbc553 to
04703cf
Compare
Require a configurable gross desyndication minimum and retire external staking reward and reserve creation carriers, DClaim reward cursors, treasury-funded LIQ bonuses, and obsolete reserve and underwriting bootstrap settings. Preserve funded launch imports and distribute only supplied WIRE yield. Document pair-scoped incident snapshots and reconciliation, approval and intervention deadlines during freezes, prefunded challengers, and request-keyed return completion or refund. Cover minimum boundaries, unknown retired and undeclared wire discriminants, saturated linked imports, and interventions after approval eligibility. Include rebuilt contract artifacts. Use fresh pre-launch state without compatibility decoders or reserved schema identifiers. Companion bootstrap uses one shared minimum of 1,000,000 base units, equivalent to 0.001 SOL at nine decimals. Change-Id: Ic9e7a85b546c4e72b54884851ffe45f8fe2a3d9e
04703cf to
2b1430a
Compare
valthon
changed the title
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed current head 2b1430ab1ea0ff6b55118362a2e4e160120b2aab.
I accept the compatibility push-back: this is explicitly a pre-release, fresh-state schema, so the retired enum/bootstrap identifiers do not need compatibility reservations or a major-version/deprecated-export layer. I verified the other requested fixes and resolved those five threads. The four companion PRs cover the active consumers, current-head Sysio CI is green, and I found no new functional code defect.
One pre-merge release-coordination gate remains in the original thread: publish all three generated model targets from this exact head at a version unused across the registries (1.0.53+) before merging, then update/pin and rerun the companions. The master push path would otherwise auto-select 1.0.52 from npm while Cargo 1.0.52 contains the old schema, and the duplicate-Cargo path reports success.
Non-blocking cleanup: contracts/tests/sysio.synd_tests.cpp:4094-4097 still describes sysio.liq::recredit and says nothing is recorded in sysio.synd, although the test invokes sysio.synd::refundreturn. A few mismatch test comments also still describe sequence-keyed/new rows rather than one row overwritten per pair.
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Approved at 2b1430ab1ea0ff6b55118362a2e4e160120b2aab. The compatibility push-back is accepted for this pre-release, fresh-state schema. Companion coverage is complete; retain the documented coordinated 1.0.53+ model publication and consumer retest before merge.
…return dispatch test #670 added min_desyndicate to sysio.synd::setconfig, and every configuration now supplies it. The dispatch test this branch adds, a_bond_releases_the_envelope_and_comes_back_after_the_window, configured its pair without it, so the action was refused once master was merged in. It now passes 1, as #670's own calls with no desyndication bucket do.
CertiK WNS-63 (WIRE-413) is fixed on master by Wire-Network#594 (links sweep unmapped credits), Wire-Network#639 (credits never expire) and Wire-Network#670 (onreward retired). The SVM createlink sweep had no unit test; the new one also checks that a credit sharing the key's 8-byte bychainad prefix stays put. A link sweeps once and createlink does not check that the DClaim import is sealed, so the upgrade-order doc now says to finish the import before the first link. Change-Id: I50e69a120633181dd3d9e0dd100c3e5a663af844
This follow-on to #662 requires a gross return minimum and retires external staking-reward and reserve-creation carriers plus treasury-funded LIQ bonuses. Funded pre-launch imports, account linking, claims, and yield distribution from actual supplied WIRE remain supported.
Changes:
min_desyndicateinsysio.synd::setconfigandsyndconfig, validate the positive asset/burst bounds, and check the gross quantity before fees, burns, or bucket updates. Remove the unused initializer. The companion bootstrap uses one shared 1,000,000-unit minimum for both shadow pairs: equivalent to 0.001 SOL at nine decimals, intentionally arbitrary, with no per-chain minimum policy.ReserveCreateprotobuf, enum, reflection, dispatch-case, and serializer surfaces, DClaim reward cursors,fundclaim, and the LIQ kicker/shortfall machinery. Remove obsolete reserve/underwriting bootstrap fields. This is pre-release and uses fresh state; no backward-compatibility decoders or reserved identifiers are provided.(chain_code, token_code), snapshotting each overwrite and requiring explicit reconciliation. A healthy report does not erase the incident.bonded_at + window_secapproval deadline, prefund challengers before a freeze, separate return variables from pair reconciliation, and separate paid-return completion from irreversibly canceled-return refund with external evidence reads.Companion coordination:
The fresh audit confirmed remaining consumers on Ethereum/Solana
nextand Toolsmaster. The companion set is now published as PRs ready for review:master: current producer ABI regeneration, the required minimum, retired capital-action removal, and client regressions. Replaces the stale declaration candidate in Libraries Perf issue: avoid vector copy at each insertion #97.master: shared 1,000,000-unit minimum, retired reward flow/emitter/decoder and kicker/shortfall cleanup, raw fallback regression for removed reward/reserve-creation values, retained flow alignment, and the native Andon implementation already reviewed in Tools Addclio convert abi_to_hashandconvert wasm_to_hash#115.next: remove the retired mock reward emitter and local deployment/parity wiring, and stale reserve-creation API references.next: remove the retired reward and reserve-creation production enum matches, fixture codecs, decoder option, and unused test constant. Excludes unrelated migration/development work.Each companion contains one commit against its current target. They are published for coordinated review; none is claimed merged. Validation uses local sibling artifacts, not a newly published model or SDK. Merge/publish the matching SDK and coordinate consumer merges with the OPP model release before landing the schema removal.
Use an explicit fresh OPP release version, 1.0.53 or later: Cargo 1.0.52 is already occupied by the earlier schema. Coordinate companion merges and model/SDK publication before merging the schema removal. No major-version compatibility work is proposed for this pre-release schema. Producer LIQ collateral valuation remains outside this change.
Current update validation: