From 9c76d8a476013a238400d0121d88c5a498d5c747 Mon Sep 17 00:00:00 2001 From: Omar Garcia Date: Wed, 15 Jan 2025 16:31:45 +0100 Subject: [PATCH] Fix: Remove recipients from the array --- contracts/payments/PaymentRouterNative.sol | 12 ++ package.json | 3 +- test/hardhatTests/paymentRouterNative.test.ts | 118 +++++++++++++++++- 3 files changed, 131 insertions(+), 2 deletions(-) diff --git a/contracts/payments/PaymentRouterNative.sol b/contracts/payments/PaymentRouterNative.sol index 42ec537..5d4e409 100644 --- a/contracts/payments/PaymentRouterNative.sol +++ b/contracts/payments/PaymentRouterNative.sol @@ -254,9 +254,21 @@ contract PaymentRouterNative is function removeFeeRecipient(address recipient) external onlyRole(MANAGER_ROLE) { if (!feeRecipients[recipient].active) revert FeeRecipientDoesNotExist(); + // Remove from mapping feeRecipients[recipient].active = false; feeRecipients[recipient].percentage = 0; + // Remove from array by replacing with last element and popping + for (uint256 i = 0; i < feeRecipientAddresses.length; i++) { + if (feeRecipientAddresses[i] == recipient) { + if (i != feeRecipientAddresses.length - 1) { + feeRecipientAddresses[i] = feeRecipientAddresses[feeRecipientAddresses.length - 1]; + } + feeRecipientAddresses.pop(); + break; + } + } + emit FeeRecipientRemoved(recipient); } diff --git a/package.json b/package.json index 4810e88..edc7911 100644 --- a/package.json +++ b/package.json @@ -102,5 +102,6 @@ "ethers": "^6.13.4", "hardhat-contract-sizer": "^2.10.0", "viem": "^2.21.51" - } + }, + "packageManager": "pnpm@9.15.2+sha512.93e57b0126f0df74ce6bff29680394c0ba54ec47246b9cf321f0121d8d9bb03f750a705f24edc3c1180853afd7c2c3b94196d0a3d53d3e069d9e2793ef11f321" } diff --git a/test/hardhatTests/paymentRouterNative.test.ts b/test/hardhatTests/paymentRouterNative.test.ts index df35c5f..dfd6d10 100644 --- a/test/hardhatTests/paymentRouterNative.test.ts +++ b/test/hardhatTests/paymentRouterNative.test.ts @@ -244,7 +244,7 @@ describe('PaymentRouterNative', function () { }); it('Should revert when non-manager tries to pause', async function () { - const { paymentRouter, deployer, user1 } = await loadFixture(deployPaymentRouterFixture); + const { paymentRouter, deployer, managerWallet, user1 } = await loadFixture(deployPaymentRouterFixture); await paymentRouter.connect(deployer).setPaymentConfig(PAYMENT_ID, PAYMENT_AMOUNT, PAYMENT_URI); @@ -401,5 +401,121 @@ describe('PaymentRouterNative', function () { ).to.be.revertedWithCustomError(paymentRouter, 'TotalPercentageExceedsLimit'); }); }); + + it('Should remove fee recipient from array and prevent duplicates when re-adding', async function () { + const { paymentRouter, managerWallet, recipient1 } = await loadFixture(deployPaymentRouterFixture); + + // Get initial array length + const [initialRecipients] = await paymentRouter.getFeeRecipients(); + const initialLength = initialRecipients.length; + + // Remove recipient1 + await paymentRouter.connect(managerWallet).removeFeeRecipient(recipient1.address); + + // Verify array length decreased + const [afterRemovalRecipients] = await paymentRouter.getFeeRecipients(); + expect(afterRemovalRecipients.length).to.equal(initialLength - 1); + expect(afterRemovalRecipients).to.not.include(recipient1.address); + + // Re-add the same recipient + await paymentRouter.connect(managerWallet).setFeeRecipient(recipient1.address, 3000); + + // Verify array length increased by 1 and recipient is present + const [finalRecipients] = await paymentRouter.getFeeRecipients(); + expect(finalRecipients.length).to.equal(initialLength); + expect(finalRecipients).to.include(recipient1.address); + + // Verify only one instance exists + const recipientCount = finalRecipients.filter(r => r === recipient1.address).length; + expect(recipientCount).to.equal(1); + }); + + it('Should handle multiple recipient removals and additions correctly', async function () { + const { paymentRouter, managerWallet, multiSigWallet, recipient1 } = await loadFixture(deployPaymentRouterFixture); + + // Remove both initial recipients + await paymentRouter.connect(managerWallet).removeFeeRecipient(recipient1.address); + await paymentRouter.connect(managerWallet).removeFeeRecipient(multiSigWallet.address); + + // Verify array is empty + const [emptyRecipients] = await paymentRouter.getFeeRecipients(); + expect(emptyRecipients.length).to.equal(0); + + // Add three new recipients + const newRecipients = await Promise.all([ + ethers.Wallet.createRandom(), + ethers.Wallet.createRandom(), + ethers.Wallet.createRandom() + ]); + + for (let i = 0; i < newRecipients.length; i++) { + await paymentRouter.connect(managerWallet).setFeeRecipient( + newRecipients[i].address, + 3333 // ~33.33% each + ); + } + + // Verify all new recipients are present + const [finalRecipients, finalPercentages] = await paymentRouter.getFeeRecipients(); + expect(finalRecipients.length).to.equal(3); + + for (const recipient of newRecipients) { + expect(finalRecipients).to.include(recipient.address); + } + + // Remove middle recipient + await paymentRouter.connect(managerWallet).removeFeeRecipient(newRecipients[1].address); + + // Verify array is updated correctly + const [afterMiddleRemovalRecipients] = await paymentRouter.getFeeRecipients(); + expect(afterMiddleRemovalRecipients.length).to.equal(2); + expect(afterMiddleRemovalRecipients).to.include(newRecipients[0].address); + expect(afterMiddleRemovalRecipients).to.include(newRecipients[2].address); + expect(afterMiddleRemovalRecipients).to.not.include(newRecipients[1].address); + }); + + it('Should prevent duplicate payments when adding same recipient multiple times', async function () { + const { paymentRouter, managerWallet, deployer, user1, recipient1 } = await loadFixture(deployPaymentRouterFixture); + + // Setup a payment configuration + const paymentAmount = ethers.parseEther('1.0'); + await paymentRouter.connect(deployer).setPaymentConfig(PAYMENT_ID, paymentAmount, PAYMENT_URI); + + // Remove all existing recipients + const [initialRecipients] = await paymentRouter.getFeeRecipients(); + for (const recipient of initialRecipients) { + await paymentRouter.connect(managerWallet).removeFeeRecipient(recipient); + } + + // Add recipient1 twice with 100% fee + await paymentRouter.connect(managerWallet).setFeeRecipient(recipient1.address, 10000); // First addition + await paymentRouter.connect(managerWallet).setFeeRecipient(recipient1.address, 10000); // Second addition + + // Verify recipient appears only once in the array + const [recipients] = await paymentRouter.getFeeRecipients(); + const recipientCount = recipients.filter(r => r === recipient1.address).length; + expect(recipientCount).to.equal(1, 'Recipient address should appear exactly once in the array'); + + // Get recipient's initial balance + const initialBalance = await ethers.provider.getBalance(recipient1.address); + + // Generate signature for payment + const chainId = (await ethers.provider.getNetwork()).chainId; + const { seed, signature, nonce } = await generateRandomSeed({ + smartContractAddress: await paymentRouter.getAddress(), + chainId: chainId, + decode: true, + address: user1.address, + signer: deployer, + rawData: { type: 'string[]', data: [BOX_ID] }, + }); + + // Make payment + await paymentRouter.connect(user1).pay(PAYMENT_ID, nonce, seed, signature, { value: paymentAmount }); + + // Check recipient received exactly the payment amount once + const finalBalance = await ethers.provider.getBalance(recipient1.address); + expect(finalBalance - initialBalance).to.equal(paymentAmount, 'Recipient should receive payment exactly once'); + }); }); });