Browse Source

Do not emit an event when setSignerWeight is a no-op (#5775)

Hadrien Croubois 3 months ago
parent
commit
292b3542fc

+ 11 - 7
contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol

@@ -104,18 +104,22 @@ abstract contract MultiSignerERC7913Weighted is MultiSignerERC7913 {
         uint256 extraWeightRemoved = 0;
         for (uint256 i = 0; i < signers.length; ++i) {
             bytes memory signer = signers[i];
-            uint64 weight = weights[i];
-
             require(isSigner(signer), MultiSignerERC7913NonexistentSigner(signer));
+
+            uint64 weight = weights[i];
             require(weight > 0, MultiSignerERC7913WeightedInvalidWeight(signer, weight));
 
             unchecked {
-                // Overflow impossible: weight values are bounded by uint64 and economic constraints
-                extraWeightRemoved += _extraWeights[signer];
-                extraWeightAdded += _extraWeights[signer] = weight - 1;
+                uint64 oldExtraWeight = _extraWeights[signer];
+                uint64 newExtraWeight = weight - 1;
+
+                if (oldExtraWeight != newExtraWeight) {
+                    // Overflow impossible: weight values are bounded by uint64 and economic constraints
+                    extraWeightRemoved += oldExtraWeight;
+                    extraWeightAdded += _extraWeights[signer] = newExtraWeight;
+                    emit ERC7913SignerWeightChanged(signer, weight);
+                }
             }
-
-            emit ERC7913SignerWeightChanged(signer, weight);
         }
         unchecked {
             // Safe from underflow: `extraWeightRemoved` is bounded by `_totalExtraWeight` by construction

+ 14 - 10
test/account/AccountMultiSignerWeighted.test.js

@@ -158,6 +158,10 @@ describe('AccountMultiSignerWeighted', function () {
       await expect(this.mock.signerWeight(signer3)).to.eventually.equal(3); // unchanged
     });
 
+    it("no-op doesn't emit an event", async function () {
+      await expect(this.mock.$_setSignerWeights([signer1], [1])).to.not.emit(this.mock, 'ERC7913SignerWeightChanged');
+    });
+
     it('cannot set weight to non-existent signer', async function () {
       // Reverts when setting weight for non-existent signer
       await expect(this.mock.$_setSignerWeights([signer4], [1]))
@@ -186,28 +190,28 @@ describe('AccountMultiSignerWeighted', function () {
     });
 
     it('validates threshold is reachable when updating weights', async function () {
-      // First, lower the weights so the sum is exactly 6 (just enough for threshold=6)
-      await expect(this.mock.$_setSignerWeights([signer1, signer2, signer3], [1, 2, 3]))
+      // First, lower the weights so the sum is exactly 9 (just enough for threshold=9)
+      await expect(this.mock.$_setSignerWeights([signer1, signer2, signer3], [2, 3, 4]))
         .to.emit(this.mock, 'ERC7913SignerWeightChanged')
-        .withArgs(signer1, 1)
+        .withArgs(signer1, 2)
         .to.emit(this.mock, 'ERC7913SignerWeightChanged')
-        .withArgs(signer2, 2)
+        .withArgs(signer2, 3)
         .to.emit(this.mock, 'ERC7913SignerWeightChanged')
-        .withArgs(signer3, 3);
+        .withArgs(signer3, 4);
 
-      // Increase threshold to 6
-      await expect(this.mock.$_setThreshold(6)).to.emit(this.mock, 'ERC7913ThresholdSet').withArgs(6);
+      // Increase threshold to 9
+      await expect(this.mock.$_setThreshold(9)).to.emit(this.mock, 'ERC7913ThresholdSet').withArgs(9);
 
       // Now try to lower weights so their sum is less than the threshold
-      await expect(this.mock.$_setSignerWeights([signer1, signer2, signer3], [1, 1, 1])).to.be.revertedWithCustomError(
+      await expect(this.mock.$_setSignerWeights([signer1, signer2, signer3], [2, 2, 2])).to.be.revertedWithCustomError(
         this.mock,
         'MultiSignerERC7913UnreachableThreshold',
       );
 
       // Try to increase threshold to be larger than the total weight
-      await expect(this.mock.$_setThreshold(7))
+      await expect(this.mock.$_setThreshold(10))
         .to.be.revertedWithCustomError(this.mock, 'MultiSignerERC7913UnreachableThreshold')
-        .withArgs(6, 7);
+        .withArgs(9, 10);
     });
 
     it('reports default weight of 1 for signers without explicit weight', async function () {