Skip to content

Commit 88ce412

Browse files
committed
fix: preserve notification state when changing default allocation
When changing default allocation address, allocation data is copied from oldAddress to newAddress, which was overwriting newAddress's lastChangeNotifiedBlock. - If newAddress was just notified, the notification record was lost - If newAddress had a future block set (via forceTargetNoChangeNotificationBlock), that restriction was lost - When changing from address(0) (which is never notified), newAddress's lastChangeNotifiedBlock was incorrectly set to 0 Fix: preserve newAddress's lastChangeNotifiedBlock when copying allocation data. Also clarifies that notification blocks are target-specific, not about the default in general.
1 parent cc652ae commit 88ce412

File tree

2 files changed

+45
-35
lines changed

2 files changed

+45
-35
lines changed

packages/issuance/contracts/allocate/IssuanceAllocator.sol

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -473,13 +473,18 @@ contract IssuanceAllocator is
473473
_notifyTarget(oldAddress);
474474
_notifyTarget(newAddress);
475475

476+
// Preserve the notification block of newAddress before copying old address data
477+
uint256 newAddressNotificationBlock = $.allocationTargets[newAddress].lastChangeNotifiedBlock;
478+
476479
// Update the default allocation address at index 0
477-
// Note this will also copy the lastChangeNotifiedBlock from old to new, which is relevant if
478-
// forceTargetNoChangeNotificationBlock was used to set a future block for the default address.
480+
// This copies allocation data from old to new, including allocatorMintingPPM and selfMintingPPM
479481
$.targetAddresses[0] = newAddress;
480482
$.allocationTargets[newAddress] = $.allocationTargets[oldAddress];
481483
delete $.allocationTargets[oldAddress];
482484

485+
// Restore the notification block for newAddress (regard as target-specific, not about default)
486+
$.allocationTargets[newAddress].lastChangeNotifiedBlock = newAddressNotificationBlock;
487+
483488
emit DefaultAllocationAddressUpdated(oldAddress, newAddress);
484489
return true;
485490
}

packages/issuance/test/tests/allocate/DefaultAllocation.test.ts

Lines changed: 38 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -540,39 +540,6 @@ describe('IssuanceAllocator - Default Allocation', () => {
540540
expect(target1Balance).to.equal(expectedTarget1)
541541
})
542542

543-
it('should inherit lastChangeNotifiedBlock when changing default address', async () => {
544-
// This test verifies the comment at IssuanceAllocator.sol:477-478
545-
// "Note this will also copy the lastChangeNotifiedBlock from old to new, which is relevant if
546-
// forceTargetNoChangeNotificationBlock was used to set a future block for the default address."
547-
548-
// Set default to target1
549-
await issuanceAllocator.connect(accounts.governor).setDefaultAllocationAddress(addresses.target1)
550-
551-
// Force a future notification block on target1 (the current default)
552-
const currentBlock = await ethers.provider.getBlockNumber()
553-
const futureBlock = currentBlock + 100
554-
await issuanceAllocator
555-
.connect(accounts.governor)
556-
.forceTargetNoChangeNotificationBlock(addresses.target1, futureBlock)
557-
558-
// Verify target1 has the future block set
559-
const target1Data = await issuanceAllocator.getTargetData(addresses.target1)
560-
expect(target1Data.lastChangeNotifiedBlock).to.equal(futureBlock)
561-
562-
// Change default from target1 to target2
563-
await issuanceAllocator
564-
.connect(accounts.governor)
565-
['setDefaultAllocationAddress(address,bool)'](addresses.target2, true)
566-
567-
// Verify target2 (new default) inherited the lastChangeNotifiedBlock from target1
568-
const target2Data = await issuanceAllocator.getTargetData(addresses.target2)
569-
expect(target2Data.lastChangeNotifiedBlock).to.equal(futureBlock)
570-
571-
// Verify old default (target1) no longer has data
572-
const oldDefaultData = await issuanceAllocator.getTargetData(addresses.target1)
573-
expect(oldDefaultData.lastChangeNotifiedBlock).to.equal(0)
574-
})
575-
576543
it('should handle changing default to address that previously had normal allocation', async () => {
577544
// Scenario: target1 has normal allocation → removed (0%) → set as default
578545
// This tests for stale data issues
@@ -679,6 +646,44 @@ describe('IssuanceAllocator - Default Allocation', () => {
679646
// Target1 should have accumulated tokens across multiple blocks
680647
const target1Balance = await graphToken.balanceOf(addresses.target1)
681648
expect(target1Balance).to.be.gt(0n) // Should have received something
649+
650+
// Verify lastChangeNotifiedBlock was preserved for the new default (not overwritten to 0 from address(0))
651+
const target2Data = await issuanceAllocator.getTargetData(addresses.target2)
652+
const currentBlock = await ethers.provider.getBlockNumber()
653+
expect(target2Data.lastChangeNotifiedBlock).to.be.gt(0n)
654+
expect(target2Data.lastChangeNotifiedBlock).to.be.lte(currentBlock)
655+
})
656+
657+
it('should not transfer future notification block from old default to new default', async () => {
658+
// Set initial default to target1
659+
await issuanceAllocator.connect(accounts.governor).setDefaultAllocationAddress(addresses.target1)
660+
661+
// Force a future notification block on target1 (the current default)
662+
const currentBlock = await ethers.provider.getBlockNumber()
663+
const futureBlock = currentBlock + 100
664+
await issuanceAllocator
665+
.connect(accounts.governor)
666+
.forceTargetNoChangeNotificationBlock(addresses.target1, futureBlock)
667+
668+
// Verify target1 has the future block set
669+
const target1DataBefore = await issuanceAllocator.getTargetData(addresses.target1)
670+
expect(target1DataBefore.lastChangeNotifiedBlock).to.equal(futureBlock)
671+
672+
// Change default from target1 to target2
673+
await issuanceAllocator.connect(accounts.governor).setDefaultAllocationAddress(addresses.target2)
674+
675+
// Verify target2 (new default) has its own notification block (current block), not the future block from target1
676+
const target2Data = await issuanceAllocator.getTargetData(addresses.target2)
677+
const blockAfterChange = await ethers.provider.getBlockNumber()
678+
679+
// target2 should have been notified at the current block, not inherit the future block
680+
expect(target2Data.lastChangeNotifiedBlock).to.equal(blockAfterChange)
681+
expect(target2Data.lastChangeNotifiedBlock).to.not.equal(futureBlock)
682+
expect(target2Data.lastChangeNotifiedBlock).to.be.lt(futureBlock)
683+
684+
// Old default (target1) should no longer have data (it was removed)
685+
const target1DataAfter = await issuanceAllocator.getTargetData(addresses.target1)
686+
expect(target1DataAfter.lastChangeNotifiedBlock).to.equal(0)
682687
})
683688
})
684689

0 commit comments

Comments
 (0)