refactor: migrate access control to near-plugins ACL; bump near-sdk; fix burn/claim/migration bugs - #67
Merged
Merged
Conversation
- near-sdk 5.13.0 -> 5.28.3 (latest), requiring rust-toolchain 1.86 -> 1.93 - bump reproducible-build docker image to sourcescan/cargo-near:0.21.1-rust-1.93.0 - bump cargo-near CLI install version in CI workflows to 0.21.1 - opt Contract into near-sdk 5.28's new ContractState trait via #[near_bindgen(contract_state)] (previously implicit) - add required `repository` field to contract/Cargo.toml: cargo-near 0.21.1's stricter NEP-330 metadata validation now requires it for reproducible builds - pass --target wasm32-unknown-unknown to clippy in scripts/lint.sh: near-sdk 5.28 hard-errors when compiled without one of a fixed set of cfgs (near, target_family = "wasm", test, etc.), which host-target clippy doesn't set
…dded commit metadata cargo-near 0.21.1 embeds the exact git commit SHA into the built wasm's NEP-330 metadata, which made scripts/check-contract-hash.sh (and CI's check-binary-hash job) structurally impossible to satisfy: a committed wasm is always built at the parent commit, so it embeds a different SHA than a fresh rebuild at the same HEAD, regardless of source changes. Removes: - scripts/check-contract-hash.sh - Makefile `hash` target - check-binary-hash job in .github/workflows/test.yml and push.yml (and from push.yml's `push` job needs list) Documents the replacement model (post-deploy source verification via SourceScan/near-verify-rs against the embedded metadata) in contract/Cargo.toml's reproducible_build section.
Add near-plugins 0.5.3 dependency and define Roles enum with Oracle, BurnManager, and Maintainer variants for role-based access control. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a #[private] #[init(ignore_state)] migrate() that reads the pre-ACL contract state (still containing the oracles set removed in the previous commit) and grants every account from that set all three new roles (Oracle, BurnManager, Maintainer), so already-registered production oracles keep working immediately after this upgrade deploys.
…with Maintainer Trims claim_model::api::AuthApi to only unlock_account, removing the oracle-management methods now superseded by near-plugins's generated acl_grant_role/acl_revoke_role/acl_get_grantees. Updates the contract's AuthApi impl to match, gating unlock_account with #[access_control_any(roles(Roles::Maintainer))] instead of the removed assert_oracle check. This is the final step in replacing the flat oracle-based access model with near-plugins' role-based AccessControllable; the non-test crate now compiles cleanly for wasm32-unknown-unknown for the first time since the refactor started.
…ed methods Covers record_batch_for_hold (Oracle), burn (BurnManager), and set_claim_period/set_burn_period/clean/unlock_account (Maintainer) with a success case per role and a failure case expecting an "Insufficient permissions" panic, plus a role-separation test proving an Oracle-only account cannot burn.
on_claim_result's failure branch overwrote account.balance with stale pre-claim amounts instead of adding to it, silently discarding any balance credited by record_batch_for_hold while the transfer promise was in flight (record_batch_for_hold doesn't check is_locked).
…anic (PROD-3661) Raising burn_period after an account's burn_since had already been advanced under a smaller burn_period could make the new claimable_window_start smaller than burn_since, underflowing the u32 subtraction and panicking claim()/record_batch_for_hold()/the get_claimable_balance_for_account view for the affected account.
… via explicit args (PROD-3662) migrate() previously granted BurnManager and Maintainer to every former oracle automatically, and never granted StagingManager/UpgradeManager to anyone (leaving the contract un-upgradable via the Upgradable plugin path post-migration). It now takes burn_managers, maintainers, staging_managers, and upgrade_managers as explicit AccountId vectors, so the deployer decides grantees for every role except Oracle, which is still carried over unconditionally from the pre-ACL oracles set.
…rystallizes (PROD-3663) record_batch_for_hold only advanced burn_since inside the balance_to_burn > 0 branch, so it stayed stale whenever balance_to_burn computed to 0 for reasons other than the window not having moved (e.g. balance is 0 after a full claim), even though claimable_window_start had genuinely moved forward. A subsequent deposit would then be backdated to that stale anchor. Advance burn_since forward-only whenever claimable_window_start exceeds it, independent of whether anything crystallized this call. Note: this is a narrow, safe fix for the zero-balance/stale-anchor case. It does not fully close the separate deposit-stacking scenario, where a second deposit arriving before burn_period has elapsed at all gets merged under the same burn_since as the first — that would need per-deposit tracking, a larger design change outside this fix's scope.
…m freeze (PROD-3675) Neither setter had an upper bound. claim()/is_claim_available() gate on claim_period_refreshed_at.is_within_period(now, claim_period) — a single set_claim_period(u32::MAX/2) call (with burn_period raised to match) would make that check true for every existing account indefinitely, freezing claim() contract-wide with one transaction from a Maintainer. Adds MAX_PERIOD_SECS (1 year) to both setters. Deliberately no new minimum bound: the described exploit is specifically about the missing maximum, and this codebase's existing tests extensively rely on small periods (0, 10, 1000 seconds) for fast time-based scenarios — a 1-hour floor would have required rewriting ~15 call sites for no benefit relevant to this fix.
…(PROD-3678) Both constants hold seconds (86_400 = 24h, 2_592_000 = 30d), matching Duration's unit everywhere else in the codebase (now_seconds(), is_within_period), but were suffixed _MS — a footgun for anyone reasoning about the values by name alone. Pure rename, no behavior change: rebuilding produced a byte-identical wasm (same SHA-256), so no wasm commit needed.
…(PROD-3677) The zero-balance early return skipped on_claim_result entirely, so it never refreshed claim_period_refreshed_at (unlike every other claim() path, including the sibling amount_to_claim == 0 case a few lines down). A zero-balance account could call claim() every block with no cooldown. Removed the early return: when balance is 0, get_balance_to_burn's .min(self.balance) clamp already makes amount_to_burn and amount_to_claim both 0, so it now falls through to the existing amount_to_claim == 0 branch, which routes through on_claim_result and refreshes the timestamp like every other path does.
Both methods accepted arbitrary-length Vecs with only NEAR's per-receipt gas envelope as an implicit cap. Adds a shared MAX_BATCH_SIZE guard as defense-in-depth against a misconfigured oracle/maintainer batcher OOG-ing mid-call, or clean's single Clean event covering an oversized batch and degrading off-chain monitoring granularity. Both methods are already role-gated, so this protects against operational error rather than an external attacker.
Same class of issue as accruals (PROD-3671): nothing in contract/src reads or writes AccountRecordLegacy, it was only carried through migrate() and re-initialized empty in init(). Unlike UnorderedSet/UnorderedMap (used for the oracles set and accruals), LookupMap has no way to enumerate or clear its own keys — there's no equivalent leak-cleanup possible here. Dropping old_state.accounts_legacy in migrate() leaves any existing entries exactly as unreachable as they already were, since nothing has ever read them. StorageKey::AccountsLegacy renamed to _AccountsLegacy (matching _Accruals/_OraclesLegacy) since it's now only constructed by migration/tests.rs.
…efault (PROD-3679)
is_enabled has been dead code since it was added: initialized to true, carried through from_legacy, but nothing ever read it or set it to false. Adds set_account_enabled (Roles::Maintainer-gated, mirroring unlock_account/reset_service_call_flag) and gates claim() on it. Deliberately does not gate record_batch_for_hold(): disabling an account blocks withdrawal, it doesn't stop the contract from crediting it — avoids batch-partial-failure semantics for a feature whose actual scope wasn't specified beyond 'prepared for future releases'.
…it behavior
Both comments dated from the migration off nitka/sweat-model and were
now wrong or unclear:
- build-integration.sh claimed integration tests were 'temporarily
disabled' pending that migration — they aren't; it's a completed,
separate cargo workspace with 30 passing tests. Replaced with an
accurate description of what the script actually feeds (the wasm
integration-tests/prepare.rs deploys via CLAIM_WASM).
- helpers.rs attributed BLOCKS_PER_MINUTE to an unremovable dependency
('nitka used 240 blocks per minute') with no way for a reader to
verify or look it up. Replaced with the actual near-workspaces
Worker::fast_forward semantics (block-height delta, not real time)
and framed the constant as this suite's own empirical calibration.
Unlike the oracles admin set (small, bounded), accruals accumulated one entry per record_batch_for_hold timestamp bucket over the contract's entire pre-linear-burn operational history — it could hold far more entries than a single transaction's gas budget can iterate. Calling .clear() on it unconditionally inside migrate() risked an out-of-gas failure mid-migration on any deployment with substantial history. Reverts to just dropping old_state.accruals without attempting to clear it, same as accounts_legacy already does (for a different reason — LookupMap can't enumerate its own keys at all). A safe reclaim needs a separate, paginated cleanup path callable across multiple transactions, not an atomic clear inside migrate(). Removes unordered_map_clear_empties_a_populated_map, which validated a mechanism migrate() no longer uses and would have been misleading context for a future reader.
… (PROD-3671) accruals needs to stay reachable on Contract (carried through migrate() instead of dropped) for two reasons: a future paginated cleanup method needs something to operate on, and this view needs something to read. get_legacy_accruals_count exposes the current entry count so an operator can size the cleanup problem and later confirm when a paginated cleanup (not yet implemented, tracked in PROD-3671) has finished. StorageKey::Accruals is back to its unprefixed name since it's genuinely constructed in production code again. accounts_legacy stays dropped: LookupMap has no len()/iteration capability at all (confirmed against its full API — just get/set/insert/remove/contains_key/entry/flush), so there's no way to build an equivalent size view or cleanup path for it regardless of whether it's kept on Contract.
achernik
reviewed
Jul 2, 2026
redundant_closure_for_method_calls: map(|account| account.into_latest()) -> map(AccountRecordVersioned::into_latest). map_unwrap_or: map(f).unwrap_or(false) -> is_some_and(f). Pure lint fixes, no behavior change — CI was failing since near-sdk 5.28 forced clippy to run --target wasm32-unknown-unknown (see prior near-sdk bump commit), which is the config that actually surfaces these pedantic lints against this module.
achernik
reviewed
Jul 2, 2026
Verified all three (burn::SelfCallback::on_burn, clean::CleanApi::clean, claim::SelfCallback::on_transfer) have zero effect on the CI gate: clippy with --target wasm32-unknown-unknown (the only invocation that runs, since near-sdk 5.28 hard-errors on a plain host-target build) is clean with or without them. They only suppressed warning noise in a separate, non-#[cfg(test)] lib-build pass that cargo test/cargo build also run on host target, where near_bindgen's wasm-only export makes these look uncalled. Removing them matches this codebase's existing tolerance for that exact warning class elsewhere (remaining_gas, on_burn_internal, assert_enough_gas, days_to_seconds, sweat_to_atto all already show the same 'never used' warning with no suppression).
…is_claim_available Both methods needed the same claim_period_refreshed_at/claim_period availability check; claim() previously duplicated it inline (to avoid a second account lookup vs calling the is_claim_available trait method directly). Extracted claim_availability(account, now, claim_period), taking an already-fetched Option<&AccountRecord> instead of an AccountId, so both callers share one implementation: is_claim_available does its own lookup and delegates to it; claim() passes the account it already fetched, keeping the single-lookup optimization from PROD-3667 intact. Pure refactor, no behavior change — full unit and integration suites green before and after.
…_BATCH_SIZE Both currently 150, same value as before — but record_batch_for_hold and clean have different per-account costs, so a single shared constant made it awkward to tune one without affecting the other. Splitting them now, before either needs a different value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch does three things:
AccessControllable(Oracle / BurnManager / Maintainer / StagingManager / UpgradeManager roles), and addsUpgradablefor safe on-chain contract upgrades. Includes amigrate()entrypoint that carries existing oracles over to the new role model.cargo-near.Notable fixes
claim()could silently discard balance credited by an Oracle while the transfer was in flight.claim()/record_batch_for_hold()for affected accounts.set_claim_period/set_burn_periodhad no upper bound, allowing a single call to freeze claiming for every account.Testing