Enable quota-based instant delegation with era restrictions #447
Conversation
Add support for instant delegation with per-wallet, per-era quota limits to encourage delegation while preventing attacks: - Add quota tracking with auto-reset on era change using storage-efficient struct - Implement 70% era window restriction to prevent last-minute sniping - Support automatic splitting of large delegations into instant + pending portions - Reuse existing applyRedelegation() for instant rewards distribution - Add configuration parameters for quota amount and era window percentage Changes: - Staking.sol: Add instant quota tracking, token transfer, and parameter configuration - StakingManager.sol: Refactor delegate() with era progress calculation and quota logic - IStaking.sol: Add new interface methods for instant delegation - Test coverage for all instant delegation scenarios
- Move InstantQuotaUsage struct to IStaking interface for better organization - Remove delegateToIndexer function, use transferDelegationTokens + addDelegation instead - Extract common transferDelegationTokens call in delegate() to reduce code duplication
Fix YN0078 invalid resolution warning for @ethersproject/abi and other packages
Remove invalid resolution entry that pointed 5.0.7 to 5.1.2, let yarn regenerate the correct resolution
Fix TypeScript compilation errors caused by multiple versions of @ethersproject packages. Force all @ethersproject dependencies to use 5.5.x versions to match ethers 5.5.4
|
Tip For best results, initiate chat on the files or code changes. @ianhe8x: Hello! How can I assist you with this PR? I can help with:
Please let me know what you'd like me to help with! |
|
@coderabbitai review the changes |
|
@ianhe8x: I'll review the changes in this PR now, focusing on the quota-based instant delegation implementation, era restrictions, and the integration with the existing staking logic. I'll provide detailed feedback on the code quality, security considerations, and potential issues. ✅ Actions performedReview triggered.
|
WalkthroughStaking contracts add era-scoped instant-delegation quota tracking and era-window logic; delegation is refactored into a token transfer plus addDelegation with instant vs pending handling. Interface, ABI, tests, and publish metadata updated; Yarn upgraded to 4.10.3 and package resolutions added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StakingManager
participant Staking
participant Rewards
User->>StakingManager: delegate(delegator, indexer, amount)
activate StakingManager
StakingManager->>StakingManager: _calculateEraProgress()
alt inside instant window
StakingManager->>StakingManager: _getRemainingQuota(delegator)
alt quota available
StakingManager->>Staking: transferDelegationTokens(delegator, amount)
activate Staking
Staking-->>StakingManager: transfer OK
deactivate Staking
StakingManager->>Staking: addDelegation(delegator, indexer, instant_amt, true)
Staking-->>StakingManager: add OK
StakingManager->>Rewards: applyInstantDelegation(...)
Rewards-->>StakingManager: applied
StakingManager->>Staking: updateInstantQuotaUsed(delegator, era, instant_amt)
StakingManager->>Staking: addDelegation(... pending_amt ..., false)
else quota exhausted
StakingManager->>Staking: addDelegation(delegator, indexer, amount, false)
end
else outside window
StakingManager->>Staking: addDelegation(delegator, indexer, amount, false)
end
deactivate StakingManager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
publish/ABI/Staking.json (1)
24-26: Remove unused DelegationAdded event from contract and ABI.Verification confirms that only
DelegationAdded2is emitted (contracts/Staking.sol:361); the originalDelegationAddedevent (line 147) is defined but never used. This dead code creates unnecessary confusion for event indexers and off-chain consumers.Required actions:
- Remove the unused
DelegationAddedevent definition from contracts/Staking.sol (line 147)- Remove corresponding entry from publish/ABI/Staking.json (lines 24-26)
- Keep only
DelegationAdded2as the sole, active event
🧹 Nitpick comments (7)
test/Vesting.test.ts (1)
196-199: Good fix: await the revert expectationsThe async call is now awaited inside the Chai expectation. Consider asserting the revert reason for stronger guarantees (e.g., .to.be.revertedWith(...)).
contracts/StakingManager.sol (1)
21-22: Unused using directiveusing MathUtil for uint256; isn’t used in this file; either remove or use .mulDiv extensions consistently.
contracts/interfaces/IStaking.sol (1)
47-68: Document units and semantics in the interfaceAdd NatSpec notes: windowPercent is per‑mill (0–PER_MILL), perEraQuota in token wei, getInstantQuotaRemaining returns remaining for provided era (not auto-fetching current era). Helps integrators.
contracts/Staking.sol (3)
369-378: Token transfer gate is correctTransfer via SafeERC20 on onlyStakingManager path is appropriate. Update docstring to reflect it’s used for all delegations (instant and pending), not just instant.
514-529: Quota update trusts caller eraFunction relies on the manager to pass the correct era. Consider deriving the current era internally to avoid misuse, unless StakingManager is strictly trusted.
- function updateInstantQuotaUsed(address delegator, uint256 era, uint256 amount) + function updateInstantQuotaUsed(address delegator, uint256 /*era*/, uint256 amount) external onlyStakingManager { - InstantQuotaUsage storage usage = instantQuotaUsed[delegator]; - if (usage.era != era) { + InstantQuotaUsage storage usage = instantQuotaUsed[delegator]; + uint256 curEra = IEraManager(settings.getContractAddress(SQContracts.EraManager)).eraNumber(); + if (usage.era != curEra) { - usage.era = era; + usage.era = curEra; usage.amount = amount; } else { usage.amount += amount; } }
536-547: Standardize Parameter event encoding in setInstantDelegationParamsLines 544-545 use
abi.encode, while the entire codebase usesabi.encodePackedfor all other Parameter emissions. Change toabi.encodePackedto ensure consistent encoding/decoding behavior across the system.function setInstantDelegationParams( uint256 _perEraQuota, uint256 _windowPercent ) external onlyOwner { require(_windowPercent <= PER_MILL, 'S015'); instantDelegationQuota = _perEraQuota; instantEraWindowPercent = _windowPercent; emit Parameter('instantDelegationQuota', abi.encodePacked(_perEraQuota)); emit Parameter('instantEraWindowPercent', abi.encodePacked(_windowPercent)); }publish/ABI/Staking.json (1)
437-486: Add inline documentation for new instant delegation configuration getters.Three new getter functions expose instant delegation parameters (lines 437–486):
instantDelegationQuota()– but what are the units? Wei? Percentage?instantEraWindowPercent()– self-explanatory but could be clearer (0–100%?)instantQuotaUsed(address delegator)– returns (era, amount) with unnamed fieldsWhile ABIs don't natively support documentation, ensure the Solidity source includes comprehensive natspec comments (e.g.,
@return quotaPerEra The per-era quota limit in wei) and that external documentation explains the semantics.Verify that the source contract includes natspec documentation:
#!/bin/bash # Check for natspec comments on instant delegation functions rg -B 3 'function instantDelegationQuota|function instantEraWindowPercent|function instantQuotaUsed' --type solidity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
.yarn/plugins/@yarnpkg/plugin-interactive-tools.cjsis excluded by!**/.yarn/**.yarn/plugins/@yarnpkg/plugin-typescript.cjsis excluded by!**/.yarn/**.yarn/plugins/@yarnpkg/plugin-version.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-3.6.4.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-4.10.3.cjsis excluded by!**/.yarn/**verify.logis excluded by!**/*.logyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.yarnrc.yml(1 hunks)contracts/Staking.sol(3 hunks)contracts/StakingManager.sol(3 hunks)contracts/interfaces/IStaking.sol(1 hunks)package.json(1 hunks)publish/ABI/Staking.json(5 hunks)test/Staking.test.ts(1 hunks)test/Vesting.test.ts(1 hunks)
🔇 Additional comments (9)
package.json (1)
104-123: Verify the necessity of the resolutions block and Yarn major version upgrade.The resolutions block pins @ethersproject/* packages to ~5.x versions, but these packages are already direct devDependencies with specific versions (lines 36–46). This is unusual and may indicate an unresolved dependency conflict in the tree. Additionally, upgrading Yarn from 3.6.4 to 4.10.3 is a major version change with breaking changes (e.g., PnP defaults, plugin system changes). This scope seems orthogonal to the instant delegation feature and introduces significant risk.
Please clarify:
- Why is the resolutions block needed? Is there a dependency conflict that necessitates this?
- What is the justification for the Yarn 4 upgrade in the context of this PR about contract changes?
- Have all build scripts, CI/CD, and dependencies been tested with Yarn 4?
Consider addressing these in a separate infrastructure PR to keep this PR focused on the instant delegation logic.
.yarnrc.yml (1)
2-3: Verify plugin removal impact and confirm Yarn 4 compatibility.The quote style changes (lines 11, 13) and indentation adjustments (lines 2–3) are cosmetic and non-functional. However, the summary indicates that a plugins block was removed from this configuration file. Yarn 4 introduced a new plugin system, and removing plugins could affect build, linting, or CI/CD workflows.
Please confirm:
- What plugins were removed and what functionality did they provide?
- Are there any build scripts, linting, or CI/CD workflows that depend on those plugins?
- Have all development and CI environments been tested with Yarn 4 and the new plugin configuration?
If plugins were removed as part of intentional Yarn 4 migration, document this in the PR description for clarity.
Also applies to: 11-11, 13-13, 15-15
contracts/interfaces/IStaking.sol (1)
33-41: Interface addition for quota tracking looks fineStruct layout is simple and ABI‑safe.
contracts/Staking.sol (2)
131-141: State for instant delegation is clear and minimalGood separation: quota, window, and per‑delegator usage.
562-575: Getter logic is soundReset-on-era-change and floor-at-zero behavior are correct.
contracts/StakingManager.sol (1)
298-316: Current order already preserves atomicity; no reentrancy risk from applyRedelegation
getEraDelegationAmountis a view function and cannot trigger state-changing external calls or reentrancy intodelegate(). The existing code order—callingapplyRedelegationbefore quota consumption—is safe and maintains atomicity on revert. No hardening is required.publish/ABI/Staking.json (3)
661-678: All three state-mutating functions have appropriate access control:
setInstantDelegationParams(line 536): restricted toonlyOwnertransferDelegationTokens(line 369): restricted toonlyStakingManagerupdateInstantQuotaUsed(line 518): restricted toonlyStakingManagerThe access controls are correctly implemented in the source code.
305-325: No issues found — function signature change is correctly implemented across the codebase.All call sites already destructure the three-value tuple return and handle the new signature correctly:
- StakingManager.sol destructures as
(uint256 era, uint256 valueAt, uint256 valueAfter) = staking.delegation(...)- Test files access properties
.era,.valueAt,.valueAfterconsistently- Function is properly read-only (
view) throughout the codebase- No
delegateToIndexer()references exist; no deprecated functions need cleanup
296-303: The parameter naming in the ABI is correct and reflects standard Solidity conventions.The
delegation()function (lines 296–303) is an auto-generated public mapping getter. Solidity does not assign names to mapping key parameters in the generated getter function—this is standard ABI behavior formapping(address => mapping(address => StakingAmount)) public delegation. The unnamed address parameters represent the mapping indices, not a defect.The
getInstantQuotaRemaining()function has properly named input parameters (delegator,era) in both the ABI and source code. The unnamed return value matches the source code, which declaresreturns (uint256)without a name.The ABI is auto-generated during compilation from source code and cannot be manually edited to add semantic names. If parameter or return names need to be added, the changes must be made to the Solidity source contract, not the ABI file.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/Staking.test.ts (1)
691-714: Consider adding event assertion for pending portion.The test correctly verifies the split delegation logic and quota exhaustion. However, when delegation is split into instant and pending portions, two
DelegationAdded2events are emitted. Consider adding an assertion for the second event withinstant=falseto fully verify the split behavior.Apply this enhancement:
// Check DelegationAdded2 event with instant=true for the instant portion (quota amount) await expect(tx) .to.emit(staking, 'DelegationAdded2') .withArgs(delegator.address, runner.address, quota, true); + + // Check DelegationAdded2 event with instant=false for the pending portion + await expect(tx) + .to.emit(staking, 'DelegationAdded2') + .withArgs(delegator.address, runner.address, delegateAmount.sub(quota), false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/Staking.sol(3 hunks)contracts/StakingManager.sol(3 hunks)test/Staking.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/Staking.test.ts (1)
test/helper.ts (4)
etherParse(119-122)timeTravel(47-51)startNewEra(314-319)timeTravelTo(53-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (15)
contracts/StakingManager.sol (6)
14-14: LGTM! Import and library usage correctly added.The Constants.sol import and MathUtil library usage declaration properly address the compilation requirements for PER_MILL constant and safe math operations used in the new instant delegation logic.
Also applies to: 22-22
56-57: Two-step flow correctly implemented for stake.The refactoring to use
transferDelegationTokensfollowed byaddDelegationwithinstant=falsemaintains the existing stake semantics while aligning with the new delegation architecture.
74-83: Era window boundary logic correctly implemented.The condition
eraProgress <= windowPercentensures delegations are instant up to and including the configured window percentage (e.g., 70%). After that point, all delegations become pending, preventing last-minute sniping.
86-108: Three-case quota handling logic is sound.The implementation correctly handles all scenarios:
- Case A: Full amount within quota → fully instant with rewards redelegation and quota consumption
- Case B: Partial quota remaining → split into instant (up to quota) and pending portions
- Case C: Quota exhausted → all pending
The order of operations is correct, ensuring delegation is added, rewards are synchronized, and quota is consumed in the proper sequence.
265-279: Era progress calculation correctly implemented.The function properly calculates the current position within an era as a percentage in perMill units (0-1,000,000). The clamping logic ensures the result never exceeds 100% even if the current time has passed the era end.
286-317: Helper functions correctly encapsulate instant delegation operations.The three helper functions cleanly delegate to the Staking contract for quota queries and updates, and reuse the existing
applyRedelegationmechanism for instant reward synchronization. Good separation of concerns.test/Staking.test.ts (5)
664-689: Comprehensive test for fully instant delegation.This test properly verifies the happy path: delegation within quota and era window results in instant delegation with correct event emission, state updates, and quota consumption.
953-1001: Excellent coverage of disabled mode scenarios.These tests properly verify that instant delegation can be disabled by setting either quota or window to 0, ensuring all delegations become pending. This addresses the past review comment requesting disabled mode test coverage.
1004-1091: Comprehensive multi-delegator quota isolation tests.These tests thoroughly verify that quota tracking is independent per delegator, covering usage, exhaustion, and reset scenarios. This fully addresses the past review comment requesting multi-delegator isolation test coverage.
898-950: Era boundary tests correctly verify quota reset behavior.These tests properly validate that quotas reset at era boundaries and track consumption correctly within an era, even without pending rewards. Good coverage of the quota lifecycle.
840-874: Review comment is incorrect—the test logic is sound.The boundary test correctly validates the 70% window behavior. Through detailed code analysis:
Timing sequence:
timeTravelTo(start + 70% - 2)positions time at 70% - 2 secondstx1execution advances block time ~1 sec → occurs at ~70% - 1 sectx2execution advances block time ~1 sec → occurs at ~70% + 1 secContract logic verification:
- Contract uses
eraProgress <= windowPercentto determine instant delegation- At tx1 (~70% - 1):
eraProgress < 700_000→instant=true✓- At tx2 (~70% + 1):
eraProgress > 700_000→instant=false✓Test assertions match expected behavior:
- Line 857:
instant=truefor tx1 (before 70%) is correct- Line 869:
instant=falsefor tx2 (after 70%) is correctThe test name correctly states "delegate instant before 70% and pending after 70%," which is what the test validates. The inline comments at lines 855 and 867 are misleading, but they don't impact test correctness.
contracts/Staking.sol (4)
131-140: State variables correctly declared for instant delegation.The three new state variables provide the necessary storage for quota management:
instantDelegationQuota: Per-era, per-wallet limitinstantEraWindowPercent: Configurable time window (in perMill)instantQuotaUsed: Per-delegator tracking with era scopePublic visibility enables transparent monitoring of quota usage.
369-378: Token transfer function correctly implemented.The
transferDelegationTokensfunction properly handles the first step of the two-step delegation flow, using SafeERC20 for secure token transfers with appropriate access control viaonlyStakingManager.
514-529: Quota tracking logic correctly handles era transitions.The function properly resets quota usage when the era changes and accumulates usage within the same era. The era-based reset ensures quotas refresh each era as designed.
536-575: Parameter setter and quota query correctly implemented.
setInstantDelegationParamsproperly validates the window percentage (≤ 100%) and emits events for transparency.getInstantQuotaRemainingcorrectly returns full quota for new eras and calculates remaining quota with underflow protection.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/Staking.test.ts (1)
1005-1014: Clarify delegator2 usage to avoid confusion.The multi-delegator test suite locally redefines
delegator2(lines 1005-1008), which shadows the globaldelegator2already initialized in the mainbeforeEach(line 76). This creates confusion about which delegator instance is being used and whether the token transfers inconfigWallet()(line 70) are relevant.Consider one of these approaches:
Option 1 (preferred): Reuse the existing
delegator2- describe('Multi-Delegator Quota Isolation', () => { - let delegator2: SignerWithAddress; - - beforeEach(async () => { - [delegator2] = await ethers.getSigners(); - - // Setup second delegator with tokens - await token.connect(root).transfer(delegator2.address, etherParse('10000')); + describe('Multi-Delegator Quota Isolation', () => { + beforeEach(async () => { + // delegator2 is already initialized in the main beforeEach + // Just ensure it has enough tokens and approval + await token.connect(root).transfer(delegator2.address, etherParse('10000'));Option 2: Use a distinct name for the third delegator
- describe('Multi-Delegator Quota Isolation', () => { - let delegator2: SignerWithAddress; + describe('Multi-Delegator Quota Isolation', () => { + let delegator3: SignerWithAddress; beforeEach(async () => { - [delegator2] = await ethers.getSigners(); + const signers = await ethers.getSigners(); + delegator3 = signers[5]; // Get a fresh signer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Staking.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/Staking.test.ts (1)
test/helper.ts (4)
etherParse(119-122)timeTravel(47-51)startNewEra(314-319)timeTravelTo(53-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (2)
test/Staking.test.ts (2)
27-27: LGTM: Imports and test setup are correct.The new imports (
timeTravelToandSignerWithAddress) are properly used throughout the test suite, and thebeforeEachsetup correctly initializes the instant delegation parameters.Also applies to: 30-30, 658-662
764-790: Verify pending-to-instant conversion after era boundary.This test verifies quota reset but doesn't test that pending delegations from the previous era convert to instant (increase
valueAt) after the era transition. The test only delegates instant amounts (within quota), so there are no pending amounts to verify conversion for.To comprehensively test era boundary behavior, consider adding a test case that:
- Delegates an amount that includes both instant and pending portions (or delegates after the era window)
- Advances to the next era
- Verifies that the pending portion from the previous era is now reflected in
valueAt(not justvalueAfter)This ensures the contract correctly converts pending→instant at era boundaries, which was flagged in the past review.
- make test more robust
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/Staking.test.ts (2)
853-853: Fix misleading comment.The comment states "Should be pending after 70%" but the test executes before the 70% boundary (line 848:
70% - 10 seconds) and correctly expectsinstant=true.Apply this diff:
- // Should be pending after 70% + // Should be instant before 70% boundary
691-714: Add event assertion for the pending portion in split delegation test.When a delegation is split (instant + pending), both portions should emit
DelegationAdded2events. Currently only the instant portion is verified (lines 700-702).Add an assertion for the pending portion after line 702:
// Check DelegationAdded2 event with instant=true for the instant portion (quota amount) await expect(tx) .to.emit(staking, 'DelegationAdded2') .withArgs(delegator.address, runner.address, quota, true); + + // Check DelegationAdded2 event with instant=false for the pending portion + await expect(tx) + .to.emit(staking, 'DelegationAdded2') + .withArgs(delegator.address, runner.address, delegateAmount.sub(quota), false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
publish/testnet.json(1 hunks)test/Staking.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/Staking.test.ts (1)
test/helper.ts (4)
etherParse(119-122)timeTravel(47-51)startNewEra(314-319)timeTravelTo(53-67)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/Staking.test.ts (1)
869-870: Clarify transaction timing comment.The comment states "this tx happens at exact 70% of era", but due to block mining time in
timeTravelTo, the transaction actually occurs in the next block, approximately 2 seconds after reaching 70%.Consider updating the comment for accuracy:
- // this tx happens at exact 70% of era + // Transaction executes at/after 70% boundary await timeTravelTo(start.add(eraPeriod.mul(70).div(100)).toNumber());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Staking.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/Staking.test.ts (1)
test/helper.ts (4)
etherParse(119-122)timeTravel(47-51)startNewEra(314-319)timeTravelTo(53-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (2)
test/Staking.test.ts (2)
27-30: LGTM! Imports are appropriate.The new imports (
timeTravelToandSignerWithAddress) are correctly used throughout the instant delegation test suite.
658-1093: Excellent test coverage for instant delegation feature!The test suite comprehensively covers all critical scenarios:
- Instant vs. pending delegation based on quota and era window
- Split delegations when quota is partially available
- Event emissions with correct
instantflags for both portions- Exact boundary tests at the era window threshold
- Quota reset behavior across era boundaries
- Disabled modes (quota=0, window=0)
- Multi-delegator quota isolation
All issues from previous reviews have been successfully addressed, including event assertions for split delegations, boundary tests, disabled-mode configurations, and multi-delegator isolation.
| await timeTravelTo(start.add(eraPeriod.mul(70).div(100).sub(10)).toNumber()); | ||
|
|
||
| // this tx happens at 70% of era - 1 sec | ||
| const tx = await stakingManager.connect(delegator).delegate(runner.address, delegateAmount); |
There was a problem hiding this comment.
Fix comment/code mismatch.
The comment states "Travel to 70% of era - 5 sec" but the code subtracts 10 seconds (.sub(10)). Additionally, the comment on line 856 claims the transaction happens at "70% of era - 1 sec", but due to block mining time in timeTravelTo (~2 seconds per block), the actual transaction would occur closer to 70% - 8 seconds.
Apply this diff to align the comment with the code:
- // Travel to 70% of era - 5 sec to ensure we're within and close to the window
+ // Travel to 70% of era - 10 sec to ensure we're within and close to the window
await timeTravelTo(start.add(eraPeriod.mul(70).div(100).sub(10)).toNumber());
- // this tx happens at 70% of era - 1 sec
+ // Transaction executes shortly before 70% boundary (~8 sec before, accounting for block time)
const tx = await stakingManager.connect(delegator).delegate(runner.address, delegateAmount);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/Staking.test.ts around lines 854 to 857, the comments do not match the
code: timeTravelTo subtracts 10 seconds but the comment says "-5 sec", and the
subsequent comment claims "-1 sec" though block mining delay makes the tx occur
~-8 sec; update the first comment to "Travel to 70% of era - 10 sec" and update
the second comment to reflect the actual observed timing such as "this tx
happens at ~70% of era - 8 sec (approx due to block mining delay)" so comments
accurately describe the code behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
104-122: Document the@ethersprojectresolutions for maintainability.The resolutions block correctly pins
@ethersproject/*packages to~5.5.xto maintain compatibility with Hardhat 2.x and its plugins, which expect ethers v5. Without these constraints, peer-dependency warnings or runtime incompatibilities can occur when other dependencies require different@ethersprojectversions.Consider adding a brief comment above the
resolutionsblock explaining this Hardhat 2.x + ethers v5 compatibility requirement:"resolutions": { "/* Hardhat 2.x and plugins require ethers v5 (@ethersproject/*) to avoid peer-dependency conflicts */": null, "@ethersproject/abstract-provider": "~5.5.1", ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
hardhat.config.ts(2 hunks)package.json(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (3)
package.json (2)
47-47: LGTM: Standard Hardhat verification plugin migration.The migration from
@nomiclabs/hardhat-etherscanto@nomicfoundation/hardhat-verifyaligns with Hardhat's recommended upgrade path.
73-73: No breaking changes detected between Hardhat 2.18.0 and 2.26.3; upgrade is compatible with the codebase.The codebase uses standard Hardhat APIs that maintain stability without breaking changes without notice. All detected patterns—
hre.ethers.getSigners(),hre.run()for verification tasks, andtask()definitions—are stable across these versions. The dependency stack is consistent: ethers v5.5.4 pairs correctly with@nomiclabs/hardhat-ethers ^2.0.5and@nomicfoundation/hardhat-verify ^2.1.3. Mocha was upgraded in recent Hardhat versions to support parallel test execution, which should be mostly backward-compatible though some edge cases could break existing tests.Recommend running the full test and deployment task suite to verify behavior with this version before merging.
hardhat.config.ts (1)
2-2: LGTM: Plugin import updated consistently.The import change from
@nomiclabs/hardhat-etherscanto@nomicfoundation/hardhat-verifyis consistent with the package.json dependency update.
| etherscan: { | ||
| apiKey: { | ||
| polygonMumbai: process.env.POLYGONSCAN_API_KEY, | ||
| goerli: process.env.ETHERSCAN_API_KEY, | ||
| sepolia: process.env.ETHERSCAN_API_KEY, | ||
| base: process.env.BASESCAN_API_KEY, | ||
| 'base-sepolia': process.env.BASESCAN_API_KEY, | ||
| polygon: process.env.POLYGONSCAN_API_KEY, | ||
| mainnet: process.env.ETHERSCAN_API_KEY, | ||
| }, | ||
| customChains: [ | ||
| { | ||
| network: 'base-sepolia', | ||
| chainId: 84532, | ||
| urls: { | ||
| apiURL: 'https://api-sepolia.basescan.org/api', | ||
| browserURL: 'https://sepolia.basescan.org', | ||
| }, | ||
| }, | ||
| { | ||
| network: 'base', | ||
| chainId: 8453, | ||
| urls: { | ||
| apiURL: 'https://api.basescan.org/api', | ||
| browserURL: 'https://basescan.org', | ||
| }, | ||
| }, | ||
| ], | ||
| apiKey: process.env.ETHERSCAN_API_KEY, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify multi-network verification still works.
The etherscan configuration has been simplified from a multi-network apiKey object to a single string. According to the AI summary, customChains configuration for base-sepolia and base networks was also removed.
The scripts at lines 26-27 reference scan-verify tasks for testnet (base-sepolia) and mainnet (base) networks. Ensure these verification tasks still function correctly without the custom chain definitions.
🌐 Web query:
Does @nomicfoundation/hardhat-verify version 2.1.3 require customChains configuration for Base and Base Sepolia networks?
💡 Result:
Short answer: No — not always. The plugin doesn't always force a customChains entry, but Base Sepolia frequently needs one unless your installed hardhat-verify version already includes Basescan as a built-in explorer. Check what networks your installed plugin knows with:
npx hardhat verify --list-networks
If "base" / "base-sepolia" (or the exact network key you use) is not listed, add a customChains entry for Base / Base Sepolia in your etherscan config (examples and Base docs show adding customChains for base-sepolia). [1][2][3]
Sources:
- hardhat-verify docs (how to list supported networks / add custom chains). [1]
- Base docs showing example customChains for Base Sepolia. [2]
- Hardhat verification guide explaining customChains usage. [3]
I need to verify the repository's setup to determine if the removal of customChains is indeed problematic. Let me inspect the configuration and installed versions.
🏁 Script executed:
#!/bin/bash
# Check installed version of hardhat-verify and current hardhat.config.ts state
echo "=== Checking package.json for hardhat-verify version ==="
jq '.devDependencies["@nomicfoundation/hardhat-verify"]' package.json 2>/dev/null || rg '@nomicfoundation/hardhat-verify' package.json -A1 -B1
echo ""
echo "=== Current etherscan config in hardhat.config.ts (lines 450-480) ==="
sed -n '450,480p' hardhat.config.ts
echo ""
echo "=== Checking for customChains in hardhat.config.ts ==="
rg -n 'customChains' hardhat.config.ts -C3 || echo "No customChains found"
echo ""
echo "=== Lines 20-40 to see scan-verify task references ==="
sed -n '20,40p' hardhat.config.tsLength of output: 1773
I need to locate the scan-verify tasks referenced and search for more specific information about Base network support in v2.1.3.
🏁 Script executed:
#!/bin/bash
# Find scan-verify or verify task definitions
echo "=== Searching for scan-verify or verify tasks ==="
rg -n 'scan-verify|verify.*task' hardhat.config.ts -C5
echo ""
echo "=== Searching for any reference to base networks ==="
rg -n -i 'base.*sepolia|base-sepolia|base' hardhat.config.ts -C2
echo ""
echo "=== Checking git log to see what was changed ==="
git log --oneline -20 hardhat.config.ts 2>/dev/null || echo "Git history not available"
echo ""
echo "=== Checking git diff for recent changes to etherscan config ==="
git diff HEAD~5..HEAD hardhat.config.ts 2>/dev/null | grep -A10 -B10 'etherscan\|customChains' || echo "Git diff not available"Length of output: 2751
I need to search for specific information about Base and Base Sepolia support in @nomicfoundation/hardhat-verify v2.1.3, and whether the single apiKey string is compatible with multi-network verification.
🌐 Web query:
@nomicfoundation/hardhat-verify 2.1.3 Base Sepolia built-in support
💡 Result:
Short answer: No — @nomicfoundation/hardhat-verify v2.1.3 does not include built‑in support for Base Sepolia (chainId 84532). Use a customChains entry (or map the explorer to a supported provider) to verify on Base Sepolia. [1][2]
Sources:
- Hardhat verify docs (plugin uses built‑in Etherscan/Blockscout support; customChains available). [1]
- Community report showing IGN1002 error for "Base Sepolia" (chainId 84532) and instruction to add a custom chain. [2]
References:
[1] https://hardhat.org/docs/plugins/hardhat-verify
[2] https://forum.openzeppelin.com/t/error-in-plugin-hardhat-ignition-need-some-help/42142
I need to verify if Base mainnet has built-in support in v2.1.3, and clarify the apiKey configuration approach.
🌐 Web query:
@nomicfoundation/hardhat-verify Base mainnet chainId 8453 built-in explorer
💡 Result:
Yes — Base mainnet uses chainId 8453, and @nomicfoundation/hardhat-verify supports Blockscout-compatible explorers (so you can verify on Base’s Blockscout/Etherscan-style explorers). Sources: Hardhat Verify docs and Base docs. [1][2][3]
Minimal example (hardhat.config.js / ts) — verify using Blockscout for Base mainnet (chainId 8453):
- set your network entry with chainId: 8453
- run verify with provider "blockscout" (or use the verify task)
Example (JS):
{
networks: {
base: {
url: "https://mainnet.base.org",
chainId: 8453,
accounts: [ "0x..." ]
}
},
verify: {
// API key not required for some Blockscout instances; when needed put it here
blockscout: "<API_KEY_IF_REQUIRED>"
}
}
Verify command examples:
- npx hardhat verify --network base <DEPLOYED_ADDRESS> "constructorArg1"
- programmatic: verifyContract({ address: "...", constructorArgs: [...], provider: "blockscout" }, hre).
References:
- @nomicfoundation/hardhat-verify docs — usage and provider "blockscout". [1]
- Base docs — Base Mainnet chainId = 8453 and Blockscout explorer (base.blockscout.com). [2]
- Blockscout announcement / REST API for Base explorer. [3]
Would you like a ready-to-use hardhat.config.js snippet with an example verify command including an actual Blockscout API URL?
Multiple critical issues introduced by this configuration change.
The removal of customChains combined with the simplified apiKey configuration breaks multi-network verification:
-
Base Sepolia verification will fail — @nomicfoundation/hardhat-verify v2.1.3 does not have built-in Base Sepolia support; it requires a
customChainsentry for chainId 84532, which was removed. -
Multi-network API key structure is broken — The old configuration used an
apiKeyobject with network-specific keys (separateBASESCAN_API_KEYandETHERSCAN_API_KEY). The new configuration uses a singleETHERSCAN_API_KEYstring for all networks, which is incorrect. Basescan requires its own API key, and other networks (Polygon, etc.) require their respective keys. -
Base mainnet may work via Blockscout — Base mainnet (8453) appears to have Blockscout support, but only if explicitly configured; the current simplified config does not guarantee this.
Fix required: Either restore the customChains definitions for Base and Base Sepolia and the multi-network apiKey object structure, or properly configure the verify provider and API keys for each network according to v2.1.3 plugin requirements.
🤖 Prompt for AI Agents
hardhat.config.ts around lines 455-457: the etherscan config was simplified to a
single apiKey string and customChains removed, which breaks multi-network
verification (Base Sepolia needs a customChains entry for chainId 84532 and
different networks require distinct API keys). Restore the apiKey object with
per-network keys (e.g., ETHERSCAN_API_KEY, BASESCAN_API_KEY,
POLYGONSCAN_API_KEY, etc.) and re-add customChains entries for Base (chainId
8453) and Base Sepolia (chainId 84532) per @nomicfoundation/hardhat-verify
v2.1.3 requirements; alternatively, configure the verify provider explicitly for
Base and Base Sepolia with the correct explorer URLs and API key mappings so
each network uses its proper provider and key.
| "@ethersproject/transactions": "~5.5.0", | ||
| "@ethersproject/wallet": "~5.5.0" | ||
| }, | ||
| "packageManager": "[email protected]" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Yarn 4 compatibility across the project.
Upgrading from Yarn 3.6.4 to 4.10.3 is a major version change with potential breaking changes in:
- Plugin/extension APIs
- Configuration format (.yarnrc.yml)
- Command-line interface
- Workspace protocol handling
Ensure all CI/CD pipelines, developer environments, and scripts are compatible with Yarn 4.
🏁 Script executed:
#!/bin/bash
# Description: Verify yarn configuration and check for potential incompatibilities
# Expected: Find .yarnrc.yml and scripts that invoke yarn commands
echo "=== Checking .yarnrc.yml configuration ==="
fd -t f '.yarnrc.yml' --exec cat {}
echo -e "\n=== Finding scripts that invoke yarn ==="
rg -n 'yarn ' --type=sh --type=json
echo -e "\n=== Checking package.json scripts for yarn-specific features ==="
cat package.json | jq '.scripts'Length of output: 1873
I'll continue verification by checking for Yarn configuration directory, CI/CD pipelines, and researching Yarn 4 breaking changes.
🏁 Script executed:
#!/bin/bash
# Check for .yarn directory and CI/CD configuration
echo "=== Checking for .yarn directory ==="
fd -t d '^\.yarn$'
echo -e "\n=== Checking for CI/CD pipeline files ==="
fd -t f '\.(github|gitlab-ci|circle|travis)' -x echo {}
echo -e "\n=== Checking for yarn lock file and its format ==="
fd -t f 'yarn.lock' -x head -20 {}
echo -e "\n=== Searching for yarn plugin references ==="
rg -n '@yarn|yarnpkg' --type=json -C 2Length of output: 1072
🌐 Web query:
Yarn 4 major breaking changes from Yarn 3
💡 Result:
Major breaking changes when upgrading from Yarn 3.x → Yarn 4.x:
- Minimum Node.js raised to 18+. [1]
- New projects no longer enable Zero‑Install by default. [1]
- New projects created with yarn init use Corepack instead of a yarnPath (per‑project yarn binary). [1]
- Many official plugins (typescript, interactive-tools, etc.) are included by default. [1]
- yarn workspaces foreach has a slightly altered command syntax (may require script updates). [1]
Sources: Yarn 4 release notes / breaking changes. [1]
🏁 Script executed:
#!/bin/bash
# Check Node.js version requirement and .github workflows with corrected commands
echo "=== Checking Node.js version requirement in package.json ==="
cat package.json | jq '.engines'
echo -e "\n=== Checking for .nvmrc ==="
fd -t f '.nvmrc' -x cat {}
echo -e "\n=== Checking for .github directory (with -H flag) ==="
fd -t d -H '^\.github$'
echo -e "\n=== Checking for any github workflows ==="
fd -t f -H 'workflows' --exec ls -la {} \;
echo -e "\n=== Checking for script files that call yarn ==="
fd -t f '\.sh$' -x grep -l 'yarn' {} \;Length of output: 251
🌐 Web query:
Yarn 4 Node.js version requirement compatibility
💡 Result:
Short answer: Yarn 4 requires Node.js 18 or newer (the release notes state Node.js 18+; the changelog notes support dropped for versions lower than 18.12). [1][2]
Notes:
- Yarn 4 is distributed with Corepack-friendly defaults (use packageManager / corepack to install). [1].
Sources:
- Yarn 4 release post. [1]
- Yarn changelog (4.0.0: dropped support for Node < 18.12). [2]
🏁 Script executed:
#!/bin/bash
# Direct check for GitHub Actions workflows and .github contents
echo "=== Checking .github directory contents ==="
find .github -type f 2>/dev/null | head -20
echo -e "\n=== Checking for GitHub Actions workflow files ==="
fd -H -t f '\.(yml|yaml)$' '.github/workflows/'
echo -e "\n=== Checking .gitignore for node version hints ==="
cat .gitignore 2>/dev/null | grep -i 'node\|nvmrc' || echo "No node-related patterns found"
echo -e "\n=== Checking for Corepack configuration ==="
cat .npmrc 2>/dev/null || echo "No .npmrc found"Length of output: 638
🏁 Script executed:
#!/bin/bash
# Check Node.js and yarn version setup in workflows
echo "=== Checking workflow files for Node.js and yarn setup ==="
for workflow in .github/workflows/*.yml; do
echo -e "\n--- $workflow ---"
grep -E 'node-version|setup-node|yarn' "$workflow" | head -10
done
echo -e "\n=== Full check of pr.yml (main CI pipeline) ==="
cat .github/workflows/pr.yml | grep -A5 -B5 'node-version\|setup-node' || echo "Checking entire pr.yml structure..."
cat .github/workflows/pr.ymlLength of output: 2753
🌐 Web query:
GitHub Actions setup-node v1 Corepack packageManager Yarn 4 support
💡 Result:
Short answer: setup-node@v1 does not automatically enable or configure Corepack/Yarn‑4 for you — you must enable Corepack (or install Yarn 4) in your workflow before running yarn. See Corepack docs and examples (add a step: run: corepack enable). [1][2][3]
Recommended minimal workflow change:
- run: corepack enable
- run: corepack use yarn@4 # optional: pin a specific Yarn 4.x
- run: yarn install --frozen-lockfile
References
- actions/setup-node docs & issues (shows CI still has Yarn 1 until Corepack is enabled). [1]
- Corepack README (how to enable/use Corepack and supported commands). [2]
- Example CI usages recommending
corepack enablebefore Yarn 4 installs. [3]
Add corepack enable to all GitHub Actions workflows before running yarn commands.
The package.json upgrade to Yarn 4.10.3 is specified via the packageManager field, but GitHub Actions setup-node@v1 does not automatically enable Corepack. All 7 workflows (.github/workflows/{pr,deploy,docs-deploy,fuzz,prerelease,release,upgrade}.yml) will continue using the bundled Yarn 1.x instead of 4.10.3 unless explicitly enabled.
Add a step before each yarn invocation:
- run: corepack enableThis ensures CI uses the specified [email protected] from package.json rather than defaulting to Yarn 1.x.
🤖 Prompt for AI Agents
In package.json around line 123, CI will still use bundled Yarn 1.x because
setup-node@v1 does not enable Corepack; add a step to each GitHub Actions
workflow file (.github/workflows/pr.yml, deploy.yml, docs-deploy.yml, fuzz.yml,
prerelease.yml, release.yml, upgrade.yml) before any yarn commands to run
corepack enable so the runner uses the Yarn version declared in package.json
([email protected]); update each workflow to insert that single-step run: corepack
enable immediately prior to any yarn invocation.
Description
This pull request introduces a quota-based instant delegation feature with configurable limits to enhance staking security and efficiency. Key features include:
applyRedelegation()for instant rewards processing.Key Changes
delegate()logic with era progress and quota handling.Checklist
Summary by CodeRabbit
New Features
Tests
Chores
Bug Fixes