From 25132a20e9796c112479d327efc13a9a1fcb5520 Mon Sep 17 00:00:00 2001 From: Lev Akhnazarov Date: Fri, 17 Apr 2026 17:39:50 +0400 Subject: [PATCH 1/4] refactor(wallet-registry): update withdrawRewards documentation and add tests for allowlist integration - Enhanced documentation for the function to clarify the beneficiary retrieval process based on the current authorization source. - Removed historical migration rationale as it is no longer relevant. - Added new tests to verify reward distribution when the allowlist is active, ensuring correct beneficiary handling and reward payouts. --- solidity/ecdsa/contracts/WalletRegistry.sol | 30 ++++----- .../ecdsa/test/WalletRegistry.Rewards.test.ts | 64 +++++++++++++++++++ 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/solidity/ecdsa/contracts/WalletRegistry.sol b/solidity/ecdsa/contracts/WalletRegistry.sol index 757f457566..21a0056b21 100644 --- a/solidity/ecdsa/contracts/WalletRegistry.sol +++ b/solidity/ecdsa/contracts/WalletRegistry.sol @@ -450,32 +450,24 @@ contract WalletRegistry is } /// @notice Withdraws application rewards for the given staking provider. - /// Rewards are withdrawn to the staking provider's beneficiary - /// address set in the staking contract. Reverts if staking provider - /// has not registered the operator address. + /// Rewards are sent to the beneficiary returned by + /// `_currentAuthorizationSource().rolesOf(stakingProvider)` — the + /// same authorization source used elsewhere after TIP-092 (Allowlist + /// when `allowlist != address(0)`, otherwise TokenStaking). Reverts + /// if the staking provider has not registered an operator address. /// @dev Emits `RewardsWithdrawn` event. /// - /// NOT MIGRATED: Beneficiary lookup remains on TokenStaking because - /// migrating dead code costs 50-100 bytes with zero benefit. + /// Allowlist vs TokenStaking: `Allowlist.rolesOf` follows TIP-092 semantics + /// (beneficiary is the staking provider address), while `TokenStaking.rolesOf` + /// can return a distinct delegated beneficiary. After `initializeV2(allowlist)`, + /// reward payout follows Allowlist, not legacy TokenStaking role resolution. /// /// Historical Context (TIP-092/100 - February 15, 2025): /// - Sortition pool DKG participation rewards HALTED Feb 15, 2025 /// - TokenStaking notification rewards HALTED for ECDSA/RandomBeacon /// - Only TACo application rewards continue (6-month transition) - /// - This function now returns 0 for all ECDSA operators (no rewards) - /// - /// Migration Decision Rationale: - /// - Bytecode cost: 50-100 bytes to migrate beneficiary lookup to Allowlist - /// - Benefit: Zero (function returns 0 - no rewards to withdraw) - /// - Preserved for historical compatibility and potential future reactivation - /// - /// Technical Note: If rewards are reactivated, Allowlist migration would - /// be required as Allowlist.rolesOf() always returns stakingProvider as - /// beneficiary (no delegation support), while TokenStaking.rolesOf() - /// returns configured beneficiary (supports owner != beneficiary delegation). - /// - /// Stakeholder Decision: Pragmatic choice to avoid bytecode cost for - /// dead code, predating TIP-092/100 implementation. + /// - For ECDSA, expect zero withdrawable amount unless application rewards + /// are reactivated. function withdrawRewards(address stakingProvider) external { address operator = stakingProviderToOperator(stakingProvider); if (operator == address(0)) revert UnknownOperator(); diff --git a/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts b/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts index 972f675a15..92d52f9e8f 100644 --- a/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts +++ b/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts @@ -243,4 +243,68 @@ describe("WalletRegistry - Rewards", () => { }) }) }) + + describe("withdrawRewards when allowlist != address(0)", () => { + const walletPublicKeyLocal: string = ecdsaData.group1.publicKey + + let tTokenLocal: T + let walletRegistryLocal: WalletRegistry + let sortitionPoolLocal: SortitionPool + let randomBeaconLocal: FakeContract + let walletOwnerLocal: FakeContract + let deployerLocal: SignerWithAddress + let membersLocal: Operator[] + + before(async () => { + await createSnapshot() + ;({ + tToken: tTokenLocal, + walletRegistry: walletRegistryLocal, + sortitionPool: sortitionPoolLocal, + randomBeacon: randomBeaconLocal, + walletOwner: walletOwnerLocal, + deployer: deployerLocal, + } = await walletRegistryFixture({ useAllowlist: true })) + expect(await walletRegistryLocal.allowlist()).to.not.equal( + ethers.constants.AddressZero + ) + ;({ members: membersLocal } = await createNewWallet( + walletRegistryLocal, + walletOwnerLocal.wallet, + randomBeaconLocal, + walletPublicKeyLocal + )) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should pay rewards to _currentAuthorizationSource().rolesOf beneficiary (Allowlist)", async () => { + const operator = membersLocal[0].signer.address + const stakingProvider = await walletRegistryLocal.operatorToStakingProvider( + operator + ) + const allowlistAddr = await walletRegistryLocal.allowlist() + const allowlist = (await ethers.getContractAt( + "Allowlist", + allowlistAddr + )) as Allowlist + + const { beneficiary: expectedBeneficiary } = + await allowlist.rolesOf(stakingProvider) + expect(expectedBeneficiary).to.equal(stakingProvider) + + await tTokenLocal + .connect(deployerLocal) + .mint(deployerLocal.address, rewardAmount) + await tTokenLocal + .connect(deployerLocal) + .approveAndCall(sortitionPoolLocal.address, rewardAmount, []) + + expect(await tTokenLocal.balanceOf(expectedBeneficiary)).to.equal(0) + await walletRegistryLocal.withdrawRewards(stakingProvider) + expect(await tTokenLocal.balanceOf(expectedBeneficiary)).to.be.gt(0) + }) + }) }) From 0ff7d79bc2ffcdd997fc4a73366ebd8260a1c5b0 Mon Sep 17 00:00:00 2001 From: Lev Akhnazarov Date: Fri, 17 Apr 2026 17:48:21 +0400 Subject: [PATCH 2/4] refactor(wallet-registry): improve test structure and readability - Updated import statements to include 'ethers' alongside 'helpers' for clarity. - Refactored test code for better readability by adjusting line breaks and formatting. - Ensured consistent handling of the 'stakingProvider' variable for improved code maintainability. --- solidity/ecdsa/test/WalletRegistry.Rewards.test.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts b/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts index 92d52f9e8f..6a0040b92b 100644 --- a/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts +++ b/solidity/ecdsa/test/WalletRegistry.Rewards.test.ts @@ -1,4 +1,4 @@ -import { helpers } from "hardhat" +import { ethers, helpers } from "hardhat" import { expect } from "chai" import { walletRegistryFixture } from "./fixtures" @@ -10,6 +10,7 @@ import type { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers" import type { FakeContract } from "@defi-wonderland/smock" import type { Operator, OperatorID } from "./utils/operators" import type { + Allowlist, SortitionPool, WalletRegistry, WalletRegistryGovernance, @@ -282,17 +283,17 @@ describe("WalletRegistry - Rewards", () => { it("should pay rewards to _currentAuthorizationSource().rolesOf beneficiary (Allowlist)", async () => { const operator = membersLocal[0].signer.address - const stakingProvider = await walletRegistryLocal.operatorToStakingProvider( - operator - ) + const stakingProvider = + await walletRegistryLocal.operatorToStakingProvider(operator) const allowlistAddr = await walletRegistryLocal.allowlist() const allowlist = (await ethers.getContractAt( "Allowlist", allowlistAddr )) as Allowlist - const { beneficiary: expectedBeneficiary } = - await allowlist.rolesOf(stakingProvider) + const { beneficiary: expectedBeneficiary } = await allowlist.rolesOf( + stakingProvider + ) expect(expectedBeneficiary).to.equal(stakingProvider) await tTokenLocal From 0c6f839fe9cc5598c98ebd81dea2edec4c68fc4e Mon Sep 17 00:00:00 2001 From: Lev Akhnazarov Date: Fri, 17 Apr 2026 19:45:34 +0400 Subject: [PATCH 3/4] fix(wallet-registry): update beneficiary retrieval in withdrawRewards function - Changed the beneficiary retrieval logic in the withdrawRewards function to use the current authorization source, improving accuracy in reward distribution. - This change ensures that the correct beneficiary is identified based on the latest authorization context. --- solidity/ecdsa/contracts/WalletRegistry.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/solidity/ecdsa/contracts/WalletRegistry.sol b/solidity/ecdsa/contracts/WalletRegistry.sol index 21a0056b21..dcdbfafc30 100644 --- a/solidity/ecdsa/contracts/WalletRegistry.sol +++ b/solidity/ecdsa/contracts/WalletRegistry.sol @@ -471,7 +471,9 @@ contract WalletRegistry is function withdrawRewards(address stakingProvider) external { address operator = stakingProviderToOperator(stakingProvider); if (operator == address(0)) revert UnknownOperator(); - (, address beneficiary, ) = staking.rolesOf(stakingProvider); + (, address beneficiary, ) = _currentAuthorizationSource().rolesOf( + stakingProvider + ); uint96 amount = sortitionPool.withdrawRewards(operator, beneficiary); // slither-disable-next-line reentrancy-events emit RewardsWithdrawn(stakingProvider, amount); From a1585cc47aa9afd2419ae101a64a311ed22ede47 Mon Sep 17 00:00:00 2001 From: Leonardo Saturnino Date: Thu, 23 Apr 2026 19:40:14 -0300 Subject: [PATCH 4/4] refactor(wallet-registry): remove stale NOT MIGRATED withdrawRewards test The "NOT MIGRATED Touchpoints" describe block in WalletRegistry.Authorization.test.ts asserted that withdrawRewards retained TokenStaking beneficiary lookup post-initializeV2. With withdrawRewards now routed through _currentAuthorizationSource(), that invariant no longer holds. The test still passed because it never invoked withdrawRewards -- it only read staking.rolesOf and walletRegistry.allowlist() -- but its name, rationale comment, and block-level documentation advertised behavior the code no longer exhibits and would mislead future maintainers. Positive coverage for post-upgrade Allowlist routing lives in WalletRegistry.Rewards.test.ts ("withdrawRewards when allowlist != address(0)"). The top-of-file coverage summary is updated so the remaining "NOT MIGRATED touchpoints" line continues to reflect the still-valid slashing path (challengeDkgResult) without the removed beneficiary claim. --- .../test/WalletRegistry.Authorization.test.ts | 70 +------------------ 1 file changed, 1 insertion(+), 69 deletions(-) diff --git a/solidity/ecdsa/test/WalletRegistry.Authorization.test.ts b/solidity/ecdsa/test/WalletRegistry.Authorization.test.ts index e34d2d60b6..4acb4ad45f 100644 --- a/solidity/ecdsa/test/WalletRegistry.Authorization.test.ts +++ b/solidity/ecdsa/test/WalletRegistry.Authorization.test.ts @@ -3714,7 +3714,7 @@ describe("WalletRegistry - Authorization", () => { * Test Coverage: * - Pre-upgrade mode: Authorization routes to TokenStaking (allowlist = address(0)) * - Post-upgrade mode: Authorization routes to Allowlist (allowlist != address(0)) - * - NOT MIGRATED touchpoints: Slashing and beneficiary queries stay on TokenStaking + * - NOT MIGRATED touchpoints: Slashing stays on TokenStaking * - Upgrade flow: Transition from TokenStaking to Allowlist via initializeV2() * - Edge cases: Zero address validation, re-initialization prevention * @@ -3975,74 +3975,6 @@ describe("WalletRegistry - Migration Scenario Tests (TIP-092)", () => { }) }) - /** - * NOT MIGRATED Touchpoint Tests - * - * Context: Some functions do NOT use _currentAuthorizationSource(). - * Expected: These functions always use staking contract, even after initializeV2(). - * - * Rationale: - * - withdrawRewards: Beneficiary roles remain in TokenStaking (WalletRegistry.sol:440-452) - * - challengeDkgResult: Stake custody and slashing remain in TokenStaking (WalletRegistry.sol:950-966) - */ - describe("NOT MIGRATED Touchpoints", () => { - let allowlist: FakeContract - - before(async () => { - await createSnapshot() - - // Setup: Use real TokenStaking for beneficiary roles (NOT migrated to Allowlist) - await setupRealStaking( - t, - staking, - walletRegistry, - deployer, - stakingProvider, - beneficiary, - minimumAuthorization - ) - - // Setup: Create allowlist fake and upgrade (but beneficiary still in TokenStaking) - allowlist = await smock.fake("IStaking") - allowlist.authorizedStake.returns(minimumAuthorization) - await walletRegistry.initializeV2(allowlist.address) - - // Setup: Trigger authorization callback from allowlist (post-upgrade) - await triggerAuthorizationCallback( - walletRegistry, - allowlist.address, - stakingProvider.address, - ethers.BigNumber.from(0), - minimumAuthorization - ) - - // Setup: Register operator with allowlist authorization - await walletRegistry - .connect(stakingProvider) - .registerOperator(operator.address) - }) - - after(async () => { - await restoreSnapshot() - }) - - /** - * Test: withdrawRewards always uses staking.rolesOf() for beneficiary lookup - * NOT using _currentAuthorizationSource() - * Direct call: staking.rolesOf() at line 456 - */ - it("should query TokenStaking for beneficiary in withdrawRewards (post-upgrade)", async () => { - // This test verifies that even after initializeV2, beneficiary lookup - // goes to TokenStaking, not Allowlist - expect(await walletRegistry.allowlist()).to.equal(allowlist.address) - - // Note: withdrawRewards requires actual rewards to test fully - // This test validates the pattern - beneficiary lookup stays on TokenStaking - const roles = await staking.rolesOf(stakingProvider.address) - expect(roles.beneficiary).to.equal(beneficiary.address) - }) - }) - /** * Upgrade Flow Tests *