Deployment/testnet/2024-05-09/gip-0088#1337
Draft
RembrandtK wants to merge 23 commits intomainfrom
Draft
Conversation
…ion-start anchor getMaxNextClaim() under SCOPE_PENDING used block.timestamp as the window start, but update() applies new terms retroactively from _agreementCollectionStartAt and never advances lastCollectionAt. When (now - collectionStart) exceeded (rcau.endsAt - now), the pre-update envelope under-reserved escrow relative to the very next collect(), risking PaymentsEscrow.collect reverts on insufficient balance via RAM._reconcileAgreement. Switch the windowStart to _agreementCollectionStartAt for Accepted agreements (the only state where update() can promote the pending RCAU); other states keep block.timestamp since pending is structurally unreachable for promotion there. Adds three regression tests in getMaxNextClaim.t.sol asserting the envelope invariant (post-update collect amount <= pre-update getMaxNextClaim): deterministic never-collected path, post-prior-collect path, and a bounded fuzz across (elapsed, rate, maxSec, window).
…ents After the upcoming invariant fix, selfMintingOffset cycles 0 -> X -> 0 on every distributeIssuance call. Continuing to emit either event would force a choice between steady-state noise and increasingly arbitrary "is this transition meaningful" gating. Once the path collapses, the offset carries no observer-actionable signal anyway. The accounting consumers care about (self-minting + allocator-minting = expected) is already covered by IssuanceDistributed, IssuanceSelfMintAllowance / IssuanceSelfMintAllowanceAggregate, IssuancePerBlockUpdated, TargetAllocationUpdated, and Pausable's Paused / Unpaused. No tests reference the offset events.
Four tests covering the case where the governor calls distributePendingIssuance() with selfMintingOffset == 0 and a non-empty undistributed range with totalSelfMintingRate > 0: - DefaultGetsOwnRateOnly: full flush over-mints to default by totalSelfMintingRate * blocks. - MixedTargets: same with mixed allocator/self-minting allocations. - ToBlock_Partial: partial flush exceeds defaultRate * K; cumulative after subsequent flush should restore per-rate * total blocks. - StaysWithinAllocatorBudget fuzz: allocator-minted never exceeds (issuancePerBlock - totalSelfMintingRate) * blocks. All four fail. Fixed in the next commit.
…e distribution path The Self-Minting Accumulation invariant (selfMintingOffset = cumulative self-minting share over the undistributed range) was only conditionally maintained: _advanceSelfMintingBlock accumulated when paused() || 0 < offsetBefore. The pending math depends on it, and distributePendingIssuance reaches that math from states where the invariant silently fails - producing total minted > issuancePerBlock * blocks. Two coupled changes for full correctness: 1. Drop the conditional gate in _advanceSelfMintingBlock so the invariant holds in every reachable state. 2. Collapse _distributeIssuance's offset-vs-normal dispatch to delegate to _distributePendingIssuance, and delete _performNormalDistribution. It was reachable post-partial-flush (where the reconcile formula clamps offset to 0) and would mint per-rate without seeing that self-minting was already authorized for the post-partial range, under-distributing to allocator targets. The four bug-demonstration tests now pass.
Now a single ternary; the helper added no abstraction value. Inlining puts lastDistributionBlock and selfMintingOffset updates next to each other where the period accounting reads naturally end-to-end.
…gCollector Removes three errors that were declared but never thrown: - RecurringCollectorAgreementNotFound - RecurringCollectorInvalidPaymentType (and its IGraphPayments import) - RecurringCollectorZeroCollectionSeconds Not-collectable conditions go through RecurringCollectorAgreementNotCollectable with an AgreementNotCollectableReason; these three were stragglers from an earlier shape of the API.
RecurringCollector accept/update — describe the actual auth model: The empty-signature path on accept and update authorizes via stored offer hash (registered through IAgreementCollector.offer), not via a callback to a now-removed IAgreementOwner.approveAgreement. Update the natspec to describe the actual mechanism, capture the idempotent re-accept / re-apply paths, and move the deadline check out of the signature-conditional since it applies to both paths. Sweep a stale comment in the gas test along the way. Other correctness fixes: - IRecurringCollector.RecurringCollectorAgreementInvalidCollectionWindow: notice claimed 'elapsed endsAt' but the error fires on (max - min) below MIN_SECONDS_COLLECTION_WINDOW. Rewrite to match. - IRecurringCollector.cancel: notice said 'indexing agreement'; replace with 'Recurring Collection Agreement' and add a dev note covering caller, state preconditions, and pending-RCAU cleanup. - IRecurringAgreementManagement.ProviderRemoved: notice said 'balance zero', but tracking is dropped at balance < 2^minResidualEscrowFactor. Reword. - IRecurringAgreements.getTotalEscrowDeficit: described as a per-provider sum; it is per (collector, provider) pair. Match the storage docstring. Trim repetition: - RecurringCollector impls (collect/accept/cancel/update, pauseGuardians): drop @notice/@dev that just restated the interface; keep @inheritdoc plus any genuinely new @dev (only update has one — the pricing-applies-immediately note). pauseGuardians now uses @inheritdoc.
…eClaimsClaimNotFound revert Closes the two real coverage gaps from PR #1331: - RewardsManager.supportsInterface(IProviderEligibilityManagement.interfaceId) was the only OR-clause never asserted true. - StakeClaims.processStakeClaim's claim-not-found require was never exercised; reachable only via library misuse, so test directly through a thin harness with empty mappings.
Hardhat (production deploy): - Toolshed base sets viaIR=true, runs=100, evm_version=cancun, solc 0.8.35, hardhat network hardfork pinned to cancun. Hardhat 2.28.6 (catalog ^2.28.0) for the upstream solc resolver fix; older versions mis-resolve 0.8.35 to its pre.1 build. - interfaces, horizon, data-edge bumped from older solc pins (0.8.27 / 0.8.12). - horizon and subgraph-service hardhat configs simplified to inherit cleanly. - RecurringCollector deployed bytecode 22.029 KiB (2.5 KiB EIP-170 headroom); ~10-15% reduction across deployable contracts vs the prior runs=20/paris baseline. Foundry: fast default, opt-in production: - [profile.default]: optimizer/viaIR off, evm_version=cancun, solc=0.8.35. Forge build 2-9s instead of ~4 min. - [profile.prod]: optimizer + runs=100 + viaIR for prod-matching CI runs. Root `pnpm test:prod` sets FOUNDRY_PROFILE=prod for the recursive run. - lint_on_build = false: forge 1.7's auto-lint duplicates pnpm lint:forge and infinite-loops on testing's pnpm-hoisted node_modules symlink cycle. - ignored_warnings_from = ["node_modules"] silences third-party warnings. Test changes induced by the compiler config: - HorizonStakingShared._slash split into _expectSlashEmits and _assertSlashStateAfter; block-scoping in AgreementLifecycle and FullStackHarness — legacy pipeline stack relief for [profile.prod]. - RecurringCollectorCallbackGas warm-path moved forge → hardhat (forge default is unoptimized, exceeds the assertion's 500-gas tolerance). - getMaxNextClaim PastEndsAt fuzz tests use vm.getBlockTimestamp() instead of `block.timestamp` capture — viaIR treats TIMESTAMP as pure and CSE-eliminates the local across vm.warp, re-reading post-warp. - Solc warning cleanups across test files (unused locals / params, missing view/pure, dead isDelegationSlashingEnabled in _slash). 1509 forge tests pass across all four packages under both profiles.
Squash of the GIP-0088 deployment package introduction and its three foundational follow-ups: - feat: GIP-0088 deployment infrastructure (deploy/ scripts, configs, ABI generation) - fix: register governor as rocketh named account for local/test TX signing - fix(ignition): update HorizonStaking and SubgraphService modules for localNetwork - chore(tag-deployment): drop commit sha from tag annotation
…tions in RCA decoder)
Squash of 4 commits: - feat: read revertOnIneligible from network config - chore: remove vestigial RecurringCollector block from network configs - refactor: introduce ResolvedSettings to centralise config resolution - feat: bundle RM.setRevertOnIneligible into upgrade governance batch Wires the RM revertOnIneligible gate from the deployment network config through ResolvedSettings into the upgrade governance batch, and removes the now-vestigial RecurringCollector block from network configs.
…ation
When syncing a non-proxy address-book entry that has no rocketh deployment
record, sync was unconditionally seeding rocketh's record from the local
artifact's bytecode. If the artifact had drifted from on-chain (source
recompiled since the impl was deployed), this masked the change: rocketh's
native bytecode comparison then matched its (just-seeded) record against the
artifact and returned newlyDeployed=false. The address book never advanced,
proxy scripts saw no impl-address change, and shared-impl proxies
(DefaultAllocation, ReclaimedRewards) ended up with code-changed status but
no pendingImplementation set — so the upgrade orchestrator skipped them.
Mirror the gate the proxy path already uses for ${name}_Implementation:
seed only when the local artifact's bytecode hash matches the address
book's stored hash. When it doesn't, leave rocketh's record absent so the
next deployFn sees no prior bytecode and produces newlyDeployed=true,
propagating the new impl address through to proxy pending-upgrade detection
naturally.
Scope of the gate:
- Only registered registry names — proxy sync recurses with synthetic names
(e.g. RewardsManager_Implementation) that aren't real address-book
entries; the proxy path has its own hashMatches gate before recursing.
- Only non-prerequisites — externally-deployed contracts (L2GraphToken)
are never run through deployFn, so the dedup-masking concern doesn't
apply and they still need an env record for downstream reads.
Also replace the silent fall-through in deployProxyContract's shared-impl
branch with a throw when env.getOrNull(sharedImplementation.name) returns
null. With the sync fix in place, missing implDep is no longer a routine
state masking drift; it's a real signal that the impl deploy script didn't
run or didn't produce a record.
test(deployment): cover shouldSeedRocketh gate truth table
Extract the inline gate from syncContract's non-proxy seed path into a
pure shouldSeedRocketh helper and pin its truth table.
The helper already had three known failure modes during this fix's
development: it had to let unregistered synthetic names through (proxy
sync recurses with `${name}_Implementation` names), let prerequisites
through (L2GraphToken-style external deployments), and skip the seed
only on a *verified* mismatch — not whenever any signal is missing.
Each of those is a regression test: getting the gate wrong on any of
them broke a real deploy run.
Squash across deployment / horizon / subgraph-service of localNetwork config corrections so addresses match the deploy modules' hardcoded ACCOUNT1 convention. Previously caused OwnableUnauthorizedAccount reverts mid-batch when run-scripts signed with ACCOUNT1_SECRET. Squash of: - fix(deployment): align localNetwork migrate governor with ACCOUNT1 - chore(subgraph-service): align localNetwork migrate governor with horizon sibling - fix(subgraph-service): align localNetwork protocol governor with ACCOUNT1 - fix(horizon): correct localNetwork pauseGuardian and subgraphAvailabilityOracle addresses
Static unit test that catches drift between per-network Ignition
config files in packages/horizon/ignition/configs/ and
packages/subgraph-service/ignition/configs/.
Four checks:
1. Cross-package sibling agreement. For each (prefix, network) pair
where both packages have a file (e.g. both migrate.arbitrumOne.json5),
every overlapping non-empty $global field must match.
2. localNetwork all-files $global agreement. For localNetwork
specifically (one stack, one governor) every $global field
meaningfully declared in more than one of the four
{horizon,subgraph-service}/{migrate,protocol}.localNetwork.json5
files must match across all of them. Stricter than #1 — catches
same-package cross-prefix drift.
3. localNetwork same-package cross-prefix sub-object agreement. Each
package's per-contract config blocks (e.g. "DisputeManager": { ... },
"RecurringCollector": { ... }) must agree leaf-by-leaf between
migrate and protocol. Catches drift in fields like eip712Name /
eip712Version (signature verification breakers) and disputePeriod /
disputeDeposit parameters. Restricted to localNetwork because for
other networks (notably default) migrate and protocol are
intentionally different templates.
4. localNetwork mnemonic-index correctness. Lines like
"governor": "0x70997970…", // index 1
must have an address that derives from the hardhat default mnemonic
at the stated BIP44 index. Catches copy-paste mistakes where the
value updates but the comment doesn't, or vice versa.
Verified to fail on the SS protocol governor drift the test was
designed against: reverting governor in
subgraph-service/ignition/configs/protocol.localNetwork.json5 to
ACCOUNT0 produces a clean three-way mismatch report from check #2.
Adds json5 as a direct devDep (was transitively available via
toolshed but not declarable that way under pnpm strict mode).
Squash of: - fix: sync RewardsManager in mock REO integrate (otherwise stale on subsequent runs) - fix: namespace mock REO governance-tx label and document the integrate path
…-fix-3 dips publish
…aphService impl Switch SubgraphService implementation deploy from m.contract() to the deployImplementation() helper from @graphprotocol/horizon/ignition, matching the pattern every other core module already uses (HorizonStaking, GraphPayments, PaymentsEscrow, RecurringCollector, RewardsManager, Curation). SubgraphService was the odd one out. Functionally identical — the helper dispatches to m.contract(name, constructorArgs, options) when no artifact is supplied, so future IDs and bytecode resolution are unchanged. The win is a single seam for any future cross-cutting concern (verification metadata, build-info pinning, journaling) and visual consistency with the rest of the implementation deploys.
Deployment/testnet/2024-05-09/gip-0088
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1337 +/- ##
==========================================
- Coverage 91.16% 91.16% -0.01%
==========================================
Files 81 81
Lines 5342 5318 -24
Branches 975 1128 +153
==========================================
- Hits 4870 4848 -22
+ Misses 449 448 -1
+ Partials 23 22 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking as PR for visibility. This is likely not the final version prior to deployment, and subject to being replaced or force-pushed.
Basis of deployment for GIP-0088.
Once main contract changes are merged I intend to extract components into separate PRs.
Replaces: #1336