From cdfa3d15d5d4a8261e9180958f56d5583f12bbf6 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Tue, 15 Oct 2024 15:12:22 +0300 Subject: [PATCH 01/11] add maxPenalty and emit A + U from Paymaster --- contracts/interfaces/IBiconomySponsorshipPaymaster.sol | 1 + contracts/sponsorship/BiconomySponsorshipPaymaster.sol | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol index 3ccc5b9..a364d14 100644 --- a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol +++ b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol @@ -15,6 +15,7 @@ interface IBiconomySponsorshipPaymaster { event PriceMarkupCollected(address indexed paymasterId, uint256 indexed priceMarkup); event Received(address indexed sender, uint256 value); event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor); + event ActualGasCostBeforePaymasterPremium(uint256 actualGasCost); function depositFor(address paymasterId) external payable; diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index 31737a2..6b1d2fa 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -267,6 +267,7 @@ contract BiconomySponsorshipPaymaster is // Include unaccountedGas since EP doesn't include this in actualGasCost // unaccountedGas = postOpGas + EP overhead gas + estimated penalty actualGasCost = actualGasCost + (unaccountedGas * actualUserOpFeePerGas); + emit ActualGasCostBeforePaymasterPremium(actualGasCost); // Apply the price markup uint256 adjustedGasCost = (actualGasCost * priceMarkup) / _PRICE_DENOMINATOR; @@ -337,9 +338,15 @@ contract BiconomySponsorshipPaymaster is revert InvalidPriceMarkup(); } + // callGasLimit + paymasterPostOpGas + uint256 maxPenalty = ( + uint128(uint256(userOp.accountGasLimits)) + + uint128(bytes16(userOp.paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET])) + ) * 10 / 100 * userOp.unpackMaxFeePerGas(); + // Deduct the max gas cost. uint256 effectiveCost = - ((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup) / _PRICE_DENOMINATOR; + ((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup / _PRICE_DENOMINATOR) + maxPenalty; if (effectiveCost > paymasterIdBalances[paymasterId]) { revert InsufficientFundsForPaymasterId(); From 500c63133c3079c20dbb5e8ed7e69dba3e52ffea Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Tue, 15 Oct 2024 18:33:36 +0400 Subject: [PATCH 02/11] fix:tests --- test/base/TestBase.sol | 135 +++++++++--------- test/base/TestHelper.sol | 35 +++++ .../concrete/TestSponsorshipPaymaster.t.sol | 52 ++++--- .../TestFuzz_TestSponsorshipPaymaster.t.sol | 19 ++- 4 files changed, 145 insertions(+), 96 deletions(-) diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol index 3cb77bc..0390d72 100644 --- a/test/base/TestBase.sol +++ b/test/base/TestBase.sol @@ -37,6 +37,15 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { Vm.Wallet internal PAYMASTER_FEE_COLLECTOR; Vm.Wallet internal DAPP_ACCOUNT; + struct PaymasterData { + uint128 validationGasLimit; + uint128 postOpGasLimit; + address paymasterId; + uint48 validUntil; + uint48 validAfter; + uint32 priceMarkup; + } + // Used to buffer user op gas limits // GAS_LIMIT = (ESTIMATED_GAS * GAS_BUFFER_RATIO) / 100 uint8 private constant GAS_BUFFER_RATIO = 110; @@ -80,11 +89,11 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); verificationGasUsed = gasleft(); IAccount(userOp.sender).validateUserOp(userOp, userOpHash, 0); - verificationGasUsed = verificationGasUsed - gasleft(); + verificationGasUsed = verificationGasUsed - gasleft(); //+ 21000; callGasUsed = gasleft(); bool success = Exec.call(userOp.sender, 0, userOp.callData, 3e6); - callGasUsed = callGasUsed - gasleft(); + callGasUsed = callGasUsed - gasleft(); //+ 21000; assert(success); verificationGasLimit = (verificationGasUsed * GAS_BUFFER_RATIO) / 100; @@ -108,68 +117,65 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // Estimate gas used validationGasUsed = gasleft(); (context,) = paymaster.validatePaymasterUserOp(userOp, userOpHash, requiredPreFund); - validationGasUsed = validationGasUsed - gasleft(); + validationGasUsed = validationGasUsed - gasleft(); //+ 21000; postopGasUsed = gasleft(); paymaster.postOp(IPaymaster.PostOpMode.opSucceeded, context, 1e12, 3e6); - postopGasUsed = (postopGasUsed - gasleft()); + postopGasUsed = (postopGasUsed - gasleft()); //+ 21000; validationGasLimit = (validationGasUsed * GAS_BUFFER_RATIO) / 100; postopGasLimit = (postopGasUsed * GAS_BUFFER_RATIO) / 100; } + // Note: we can use externally provided gas limits to override. + // Note: we can pass callData and callGasLimit as args to test with more tx types + // Note: we can pass Nexus instance instead of sender EOA and comuting counterfactual within buildUserOpWithCalldata + function createUserOp( Vm.Wallet memory sender, BiconomySponsorshipPaymaster paymaster, - uint32 priceMarkup + uint32 priceMarkup, + uint128 postOpGasLimitOverride ) internal returns (PackedUserOperation memory userOp, bytes32 userOpHash) { // Create userOp with no gas estimates - uint48 validUntil = uint48(block.timestamp + 1 days); - uint48 validAfter = uint48(block.timestamp); - userOp = buildUserOpWithCalldata(sender, "", address(VALIDATOR_MODULE)); - (userOp.paymasterAndData,) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, paymaster, 3e6, 8e3, DAPP_ACCOUNT.addr, validUntil, validAfter, priceMarkup + PaymasterData memory pmData = PaymasterData( + 24_000, + uint128(postOpGasLimitOverride), + DAPP_ACCOUNT.addr, + uint48(block.timestamp + 1 days), + uint48(block.timestamp), + priceMarkup ); + (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, paymaster, pmData); userOp.signature = signUserOp(sender, userOp); - (,, uint256 verificationGasLimit, uint256 callGasLimit) = estimateUserOpGasCosts(userOp); - // Estimate paymaster gas limits - (, uint256 postopGasUsed, uint256 validationGasLimit, uint256 postopGasLimit) = - estimatePaymasterGasCosts(paymaster, userOp, 5e4); - - // console2.log("postOpGasUsed"); - // console2.logUint(postopGasUsed); - - // uint256 prevValUnaccountedGas = paymaster.unaccountedGas(); - // console2.logUint(prevValUnaccountedGas); - - // Below may not be needed if unaccountedGas is set correctly - vm.startPrank(paymaster.owner()); - // Set unaccounted gas to be gas used in postop + 1000 for EP overhead and penalty - paymaster.setUnaccountedGas(postopGasUsed + 500); - vm.stopPrank(); - - // uint256 currentValUnaccountedGas = paymaster.unaccountedGas(); - // console2.logUint(currentValUnaccountedGas); - - // Ammend the userop to have new gas limits and signature - userOp.accountGasLimits = bytes32(abi.encodePacked(uint128(verificationGasLimit), uint128(callGasLimit))); - (userOp.paymasterAndData,) = generateAndSignPaymasterData( - userOp, - PAYMASTER_SIGNER, - paymaster, - uint128(validationGasLimit), - uint128(postopGasLimit), + // Estimate account gas limits + // (,, uint256 verificationGasLimit, uint256 callGasLimit) = estimateUserOpGasCosts(userOp); + // // Estimate paymaster gas limits + // (, uint256 postopGasUsed, uint256 validationGasLimit, uint256 postopGasLimit) = + // estimatePaymasterGasCosts(paymaster, userOp, 5e4); + + // vm.startPrank(paymaster.owner()); + // paymaster.setUnaccountedGas(postopGasUsed + 500); + // vm.stopPrank(); + + // Ammend the userop to have updated / overridden gas limits + userOp.accountGasLimits = bytes32(abi.encodePacked(uint128(55_000), uint128(0))); + PaymasterData memory pmDataNew = PaymasterData( + uint128(24_000), + uint128(postOpGasLimitOverride), DAPP_ACCOUNT.addr, - validUntil, - validAfter, + uint48(block.timestamp + 1 days), + uint48(block.timestamp), priceMarkup ); + + (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, paymaster, pmDataNew); userOp.signature = signUserOp(sender, userOp); userOpHash = ENTRYPOINT.getUserOpHash(userOp); } @@ -185,12 +191,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { PackedUserOperation memory userOp, Vm.Wallet memory signer, BiconomySponsorshipPaymaster paymaster, - uint128 paymasterValGasLimit, - uint128 paymasterPostOpGasLimit, - address paymasterId, - uint48 validUntil, - uint48 validAfter, - uint32 priceMarkup + PaymasterData memory pmData ) internal view @@ -199,12 +200,12 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // Initial paymaster data with zero signature bytes memory initialPmData = abi.encodePacked( address(paymaster), - paymasterValGasLimit, - paymasterPostOpGasLimit, - paymasterId, - validUntil, - validAfter, - priceMarkup, + pmData.validationGasLimit, + pmData.postOpGasLimit, + pmData.paymasterId, + pmData.validUntil, + pmData.validAfter, + pmData.priceMarkup, new bytes(65) // Zero signature ); @@ -212,7 +213,8 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { userOp.paymasterAndData = initialPmData; // Generate hash to be signed - bytes32 paymasterHash = paymaster.getHash(userOp, paymasterId, validUntil, validAfter, priceMarkup); + bytes32 paymasterHash = + paymaster.getHash(userOp, pmData.paymasterId, pmData.validUntil, pmData.validAfter, pmData.priceMarkup); // Sign the hash signature = signMessage(signer, paymasterHash); @@ -221,16 +223,18 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // Final paymaster data with the actual signature finalPmData = abi.encodePacked( address(paymaster), - paymasterValGasLimit, - paymasterPostOpGasLimit, - paymasterId, - validUntil, - validAfter, - priceMarkup, + pmData.validationGasLimit, + pmData.postOpGasLimit, + pmData.paymasterId, + pmData.validUntil, + pmData.validAfter, + pmData.priceMarkup, signature ); } + // Note: Token paymaster could also get into stack deep issues. + // TODO: Refactor to reduce stack depth /// @notice Generates and signs the paymaster data for a user operation. /// @dev This function prepares the `paymasterAndData` field for a `PackedUserOperation` with the correct signature. /// @param userOp The user operation to be signed. @@ -319,15 +323,11 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { uint256 totalGasFeesCharged = initialDappPaymasterBalance - resultingDappPaymasterBalance; - if (priceMarkup >= 1e6) { - //priceMarkup - expectedPriceMarkup = totalGasFeesCharged - ((totalGasFeesCharged * 1e6) / priceMarkup); - actualPriceMarkup = resultingFeeCollectorPaymasterBalance - initialFeeCollectorBalance; - } else { - revert("PriceMarkup must be more than 1e6"); - } + expectedPriceMarkup = totalGasFeesCharged - ((totalGasFeesCharged * 1e6) / priceMarkup); + actualPriceMarkup = resultingFeeCollectorPaymasterBalance - initialFeeCollectorBalance; } + // Note: can pack values into one struct function calculateAndAssertAdjustments( BiconomySponsorshipPaymaster bicoPaymaster, uint256 initialDappPaymasterBalance, @@ -350,9 +350,6 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { assertEq(expectedPriceMarkup, actualPriceMarkup); // Gas paid by dapp is higher than paymaster // Guarantees that EP always has sufficient deposit to pay back dapps - - // TODO - // Review: fix this properly. avoid out of stack errors assertGt(gasPaidByDapp, BUNDLER.addr.balance - initialBundlerBalance); // Ensure that max 1% difference between total gas paid + the adjustment premium and gas paid by dapp (from // paymaster) diff --git a/test/base/TestHelper.sol b/test/base/TestHelper.sol index 65945c8..58cff6a 100644 --- a/test/base/TestHelper.sol +++ b/test/base/TestHelper.sol @@ -251,6 +251,41 @@ contract TestHelper is CheatCodes, EventsAndErrors { userOp.signature = signature; } + function buildPackedUserOperation( + Vm.Wallet memory wallet, + Nexus account, + ExecType execType, + Execution[] memory executions, + address validator, + uint256 nonce + ) + internal + view + returns (PackedUserOperation[] memory userOps) + { + // Validate execType + require(execType == EXECTYPE_DEFAULT || execType == EXECTYPE_TRY, "Invalid ExecType"); + + // Initialize the userOps array with one operation + userOps = new PackedUserOperation[](1); + + uint256 nonceToUse; + if (nonce == 0) { + nonceToUse = getNonce(address(account), MODE_VALIDATION, validator); + } else { + nonceToUse = nonce; + } + + // Build the UserOperation + userOps[0] = buildPackedUserOp(address(account), nonceToUse); + userOps[0].callData = prepareERC7579ExecuteCallData(execType, executions); + + // Sign the operation + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]); + userOps[0].signature = signMessage(wallet, userOpHash); + return userOps; + } + /// @notice Retrieves the nonce for a given account and validator /// @param account The account address /// @param vMode Validation Mode diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index 94178a5..8fd986e 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -205,13 +205,16 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { bicoPaymaster.withdrawTo(payable(BOB_ADDRESS), 1 ether); } - function skip_test_ValidatePaymasterAndPostOpWithoutPriceMarkup() external prankModifier(DAPP_ACCOUNT.addr) { + function test_ValidatePaymasterAndPostOpWithoutPriceMarkup() external { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); - // No adjustment - uint32 priceMarkup = 1e6; + + startPrank(PAYMASTER_OWNER.addr); + bicoPaymaster.setUnaccountedGas(9e3); + stopPrank(); PackedUserOperation[] memory ops = new PackedUserOperation[](1); - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, priceMarkup); + // price markup of 1e6 + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 10_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; @@ -222,7 +225,9 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { // submit userops vm.expectEmit(true, false, true, true, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); + stopPrank(); // Calculate and assert price markups and gas payments calculateAndAssertAdjustments( @@ -231,17 +236,20 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { initialFeeCollectorBalance, initialBundlerBalance, initialPaymasterEpBalance, - priceMarkup + 1e6 ); } - function skip_test_ValidatePaymasterAndPostOpWithPriceMarkup() external { + function test_ValidatePaymasterAndPostOpWithPriceMarkup() external { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); - // 10% priceMarkup on gas cost - uint32 priceMarkup = 1e6 + 1e5; + startPrank(PAYMASTER_OWNER.addr); + bicoPaymaster.setUnaccountedGas(25_000); + stopPrank(); + + // 10% priceMarkup on gas cost PackedUserOperation[] memory ops = new PackedUserOperation[](1); - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, priceMarkup); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 35_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; @@ -254,7 +262,10 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0); vm.expectEmit(true, false, true, true, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + + startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); + stopPrank(); // Calculate and assert price markups and gas payments calculateAndAssertAdjustments( @@ -263,7 +274,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { initialFeeCollectorBalance, initialBundlerBalance, initialPaymasterEpBalance, - priceMarkup + 1_100_000 ); } @@ -274,9 +285,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint48 validAfter = uint48(block.timestamp); PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - (userOp.paymasterAndData,) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, bicoPaymaster, 3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6 - ); + PaymasterData memory pmData = PaymasterData(3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6); + (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); userOp.paymasterAndData = excludeLastNBytes(userOp.paymasterAndData, 2); userOp.signature = signUserOp(ALICE, userOp); @@ -293,9 +303,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint48 validAfter = uint48(block.timestamp); PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - (userOp.paymasterAndData,) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, bicoPaymaster, 3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6 - ); + PaymasterData memory pmData = PaymasterData(3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6); + (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); userOp.signature = signUserOp(ALICE, userOp); ops[0] = userOp; @@ -311,9 +320,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint48 validAfter = uint48(block.timestamp); PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - (userOp.paymasterAndData,) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, bicoPaymaster, 3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6 - ); + PaymasterData memory pmData = PaymasterData(3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, 1e6); + (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); userOp.signature = signUserOp(ALICE, userOp); ops[0] = userOp; @@ -386,9 +394,9 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint48 validAfter = uint48(block.timestamp); uint32 priceMarkup = 1e6; PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - (bytes memory paymasterAndData, bytes memory signature) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, bicoPaymaster, 3e6, 3e6, paymasterId, validUntil, validAfter, priceMarkup - ); + PaymasterData memory pmData = PaymasterData(3e6, 3e6, paymasterId, validUntil, validAfter, priceMarkup); + (bytes memory paymasterAndData, bytes memory signature) = + generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); ( address parsedPaymasterId, diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index c8bc619..c927dc5 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -92,12 +92,18 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { assertEq(token.balanceOf(ALICE_ADDRESS), mintAmount); } - function skip_testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { + // Review: fuzz with high markeup and current set values. + function testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { vm.assume(priceMarkup <= 2e6 && priceMarkup > 1e6); bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); + startPrank(PAYMASTER_OWNER.addr); + bicoPaymaster.setUnaccountedGas(30_000); + stopPrank(); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, priceMarkup); + (PackedUserOperation memory userOp, bytes32 userOpHash) = + createUserOp(ALICE, bicoPaymaster, priceMarkup, 45_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; @@ -110,7 +116,10 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0); vm.expectEmit(true, false, true, true, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + + startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); + stopPrank(); // Calculate and assert price markups and gas payments calculateAndAssertAdjustments( @@ -133,9 +142,9 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { view { PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - (bytes memory paymasterAndData, bytes memory signature) = generateAndSignPaymasterData( - userOp, PAYMASTER_SIGNER, bicoPaymaster, 3e6, 3e6, paymasterId, validUntil, validAfter, priceMarkup - ); + PaymasterData memory pmData = PaymasterData(3e6, 3e6, paymasterId, validUntil, validAfter, priceMarkup); + (bytes memory paymasterAndData, bytes memory signature) = + generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); ( address parsedPaymasterId, From 012a726933961353e2350aae97ea7c78919054ee Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Tue, 15 Oct 2024 19:09:36 +0400 Subject: [PATCH 03/11] refactor --- test/base/TestBase.sol | 10 ++++------ test/unit/concrete/TestSponsorshipPaymaster.t.sol | 8 ++++---- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol index 0390d72..e2cfaa8 100644 --- a/test/base/TestBase.sol +++ b/test/base/TestBase.sol @@ -198,7 +198,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { returns (bytes memory finalPmData, bytes memory signature) { // Initial paymaster data with zero signature - bytes memory initialPmData = abi.encodePacked( + userOp.paymasterAndData = abi.encodePacked( address(paymaster), pmData.validationGasLimit, pmData.postOpGasLimit, @@ -208,17 +208,15 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { pmData.priceMarkup, new bytes(65) // Zero signature ); - - // Update user operation with initial paymaster data - userOp.paymasterAndData = initialPmData; - + + { // Generate hash to be signed bytes32 paymasterHash = paymaster.getHash(userOp, pmData.paymasterId, pmData.validUntil, pmData.validAfter, pmData.priceMarkup); // Sign the hash signature = signMessage(signer, paymasterHash); - require(signature.length == 65, "Invalid Paymaster Signature length"); + } // Final paymaster data with the actual signature finalPmData = abi.encodePacked( diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index 8fd986e..2e0843a 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -209,12 +209,12 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); startPrank(PAYMASTER_OWNER.addr); - bicoPaymaster.setUnaccountedGas(9e3); + bicoPaymaster.setUnaccountedGas(1e4); stopPrank(); PackedUserOperation[] memory ops = new PackedUserOperation[](1); // price markup of 1e6 - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 10_000); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 15_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; @@ -244,12 +244,12 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); startPrank(PAYMASTER_OWNER.addr); - bicoPaymaster.setUnaccountedGas(25_000); + bicoPaymaster.setUnaccountedGas(27_000); stopPrank(); // 10% priceMarkup on gas cost PackedUserOperation[] memory ops = new PackedUserOperation[](1); - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 35_000); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 40_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; From 3ec8516cc58f7817ca19c23a54f2d85791bb466e Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Wed, 16 Oct 2024 14:29:46 +0300 Subject: [PATCH 04/11] fix events --- .../IBiconomySponsorshipPaymaster.sol | 4 +--- .../BiconomySponsorshipPaymaster.sol | 17 +++++------------ .../concrete/TestSponsorshipPaymaster.t.sol | 12 ++++++------ .../TestFuzz_TestSponsorshipPaymaster.t.sol | 5 +---- 4 files changed, 13 insertions(+), 25 deletions(-) diff --git a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol index a364d14..faf0665 100644 --- a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol +++ b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol @@ -11,11 +11,9 @@ interface IBiconomySponsorshipPaymaster { event FeeCollectorChanged(address indexed oldFeeCollector, address indexed newFeeCollector, address indexed actor); event GasDeposited(address indexed paymasterId, uint256 indexed value); event GasWithdrawn(address indexed paymasterId, address indexed to, uint256 indexed value); - event GasBalanceDeducted(address indexed paymasterId, uint256 indexed charge, bytes32 indexed userOpHash); - event PriceMarkupCollected(address indexed paymasterId, uint256 indexed priceMarkup); + event GasBalanceDeducted(address indexed paymasterId, uint256 actualGasCost, uint256 indexed adjustedGasCost, bytes32 indexed userOpHash); event Received(address indexed sender, uint256 value); event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor); - event ActualGasCostBeforePaymasterPremium(uint256 actualGasCost); function depositFor(address paymasterId) external payable; diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index 6b1d2fa..75cefc0 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -267,7 +267,6 @@ contract BiconomySponsorshipPaymaster is // Include unaccountedGas since EP doesn't include this in actualGasCost // unaccountedGas = postOpGas + EP overhead gas + estimated penalty actualGasCost = actualGasCost + (unaccountedGas * actualUserOpFeePerGas); - emit ActualGasCostBeforePaymasterPremium(actualGasCost); // Apply the price markup uint256 adjustedGasCost = (actualGasCost * priceMarkup) / _PRICE_DENOMINATOR; @@ -275,18 +274,12 @@ contract BiconomySponsorshipPaymaster is // If overcharged refund the excess paymasterIdBalances[paymasterId] += (prechargedAmount - adjustedGasCost); } + + // Add priceMarkup to fee collector balance + paymasterIdBalances[feeCollector] += adjustedGasCost - actualGasCost; - // Should always be true - // if (adjustedGasCost > actualGasCost) { - // Apply priceMarkup to fee collector balance - uint256 premium = adjustedGasCost - actualGasCost; - paymasterIdBalances[feeCollector] += premium; - // Review: if we should emit adjustedGasCost as well - emit PriceMarkupCollected(paymasterId, premium); - // } - - // Review: emit min required information - emit GasBalanceDeducted(paymasterId, adjustedGasCost, userOpHash); + // premium = adjustedGasCost - actualGasCost => do not need to emit it explicitly + emit GasBalanceDeducted(paymasterId, actualGasCost, adjustedGasCost, userOpHash); } } diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index 94178a5..7ab2909 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -220,8 +220,9 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); // submit userops - vm.expectEmit(true, false, true, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + // uncommenting this causes weird stack to deep Yul errors + //vm.expectEmit(true, false, true, true, address(bicoPaymaster)); + //emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); // Calculate and assert price markups and gas payments @@ -250,10 +251,9 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); // submit userops - vm.expectEmit(true, false, false, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0); - vm.expectEmit(true, false, true, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + // uncommenting this causes weird stack to deep Yul errors + //vm.expectEmit(true, false, true, true, address(bicoPaymaster)); + //emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); // Calculate and assert price markups and gas payments diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index c8bc619..d1a0df7 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -105,11 +105,8 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialDappPaymasterBalance = bicoPaymaster.getBalance(DAPP_ACCOUNT.addr); uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); - // submit userops - vm.expectEmit(true, false, false, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0); vm.expectEmit(true, false, true, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash); + emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); // Calculate and assert price markups and gas payments From a591bda28e635a60bec5cb4680e613dc8e946787 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Wed, 16 Oct 2024 14:37:39 +0300 Subject: [PATCH 05/11] fix maxPenalty deduction --- contracts/sponsorship/BiconomySponsorshipPaymaster.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index 75cefc0..8c00fff 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -335,17 +335,17 @@ contract BiconomySponsorshipPaymaster is uint256 maxPenalty = ( uint128(uint256(userOp.accountGasLimits)) + uint128(bytes16(userOp.paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET])) - ) * 10 / 100 * userOp.unpackMaxFeePerGas(); + ) * 10 * userOp.unpackMaxFeePerGas() / 100; // Deduct the max gas cost. uint256 effectiveCost = - ((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup / _PRICE_DENOMINATOR) + maxPenalty; + ((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup / _PRICE_DENOMINATOR); - if (effectiveCost > paymasterIdBalances[paymasterId]) { + if (effectiveCost + maxPenalty > paymasterIdBalances[paymasterId]) { revert InsufficientFundsForPaymasterId(); } - paymasterIdBalances[paymasterId] -= effectiveCost; + paymasterIdBalances[paymasterId] -= (effectiveCost + maxPenalty); context = abi.encode(paymasterId, priceMarkup, userOpHash, effectiveCost); From 93bb3f0705c1231327d757980c793d1af0965b85 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Wed, 16 Oct 2024 17:41:53 +0400 Subject: [PATCH 06/11] fix:coverage --- package.json | 4 ++-- test/base/TestBase.sol | 10 +++++----- test/unit/concrete/TestSponsorshipPaymaster.t.sol | 8 ++++---- test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index 95887fa..3bfb39d 100644 --- a/package.json +++ b/package.json @@ -60,9 +60,9 @@ "test": "pnpm run test:forge", "test:gas:forge": "forge test --gas-report", "test:gas": "pnpm test:gas:forge", - "coverage:forge": "forge coverage --ir-minimum", + "coverage:forge": "forge coverage --ir-minimum --gas-limit 50000000 --gas-price 1000000000", "coverage": "pnpm run coverage:forge", - "coverage:report": "forge coverage --ir-minimum --report lcov && genhtml lcov.info --branch-coverage --output-dir coverage/foundry && mv lcov.info coverage/foundry", + "coverage:report": "forge coverage --ir-minimum --gas-limit 50000000 --gas-price 1000000000 --report lcov && genhtml lcov.info --branch-coverage --output-dir coverage/foundry && mv lcov.info coverage/foundry", "deploy:forge": "forge script scripts/solidity/Deploy.s.sol --broadcast --rpc-url http://localhost:8545", "lint:sol": "pnpm solhint 'contracts/**/*.sol'", "format:check": "forge fmt --check", diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol index e2cfaa8..eb1c1b7 100644 --- a/test/base/TestBase.sol +++ b/test/base/TestBase.sol @@ -144,7 +144,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { userOp = buildUserOpWithCalldata(sender, "", address(VALIDATOR_MODULE)); PaymasterData memory pmData = PaymasterData( - 24_000, + 100_000, uint128(postOpGasLimitOverride), DAPP_ACCOUNT.addr, uint48(block.timestamp + 1 days), @@ -165,9 +165,9 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // vm.stopPrank(); // Ammend the userop to have updated / overridden gas limits - userOp.accountGasLimits = bytes32(abi.encodePacked(uint128(55_000), uint128(0))); + userOp.accountGasLimits = bytes32(abi.encodePacked(uint128(100_000), uint128(0))); PaymasterData memory pmDataNew = PaymasterData( - uint128(24_000), + uint128(100_000), uint128(postOpGasLimitOverride), DAPP_ACCOUNT.addr, uint48(block.timestamp + 1 days), @@ -349,9 +349,9 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // Gas paid by dapp is higher than paymaster // Guarantees that EP always has sufficient deposit to pay back dapps assertGt(gasPaidByDapp, BUNDLER.addr.balance - initialBundlerBalance); - // Ensure that max 1% difference between total gas paid + the adjustment premium and gas paid by dapp (from + // Ensure that max 2% difference between total gas paid + the adjustment premium and gas paid by dapp (from // paymaster) - assertApproxEqRel(totalGasFeePaid + actualPriceMarkup, gasPaidByDapp, 0.01e18); + assertApproxEqRel(totalGasFeePaid + actualPriceMarkup, gasPaidByDapp, 0.02e18); } function _toSingletonArray(address addr) internal pure returns (address[] memory) { diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index 2e0843a..dde4801 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -209,12 +209,12 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); startPrank(PAYMASTER_OWNER.addr); - bicoPaymaster.setUnaccountedGas(1e4); + bicoPaymaster.setUnaccountedGas(18_700); stopPrank(); PackedUserOperation[] memory ops = new PackedUserOperation[](1); // price markup of 1e6 - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 15_000); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 55_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; @@ -244,12 +244,12 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); startPrank(PAYMASTER_OWNER.addr); - bicoPaymaster.setUnaccountedGas(27_000); + bicoPaymaster.setUnaccountedGas(35_000); stopPrank(); // 10% priceMarkup on gas cost PackedUserOperation[] memory ops = new PackedUserOperation[](1); - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 40_000); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 100_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index c927dc5..a378352 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -93,17 +93,17 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { } // Review: fuzz with high markeup and current set values. - function testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { + function skip_testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { vm.assume(priceMarkup <= 2e6 && priceMarkup > 1e6); bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); startPrank(PAYMASTER_OWNER.addr); - bicoPaymaster.setUnaccountedGas(30_000); + bicoPaymaster.setUnaccountedGas(40_000); stopPrank(); PackedUserOperation[] memory ops = new PackedUserOperation[](1); (PackedUserOperation memory userOp, bytes32 userOpHash) = - createUserOp(ALICE, bicoPaymaster, priceMarkup, 45_000); + createUserOp(ALICE, bicoPaymaster, priceMarkup, 100_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; From c8fa3be450b4f65d378f97e79610632b5fb15755 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 17 Oct 2024 16:28:34 +0400 Subject: [PATCH 07/11] Update test/base/TestBase.sol Co-authored-by: filmakarov --- test/base/TestBase.sol | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol index eb1c1b7..e7e1e64 100644 --- a/test/base/TestBase.sol +++ b/test/base/TestBase.sol @@ -143,14 +143,14 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { // Create userOp with no gas estimates userOp = buildUserOpWithCalldata(sender, "", address(VALIDATOR_MODULE)); - PaymasterData memory pmData = PaymasterData( - 100_000, - uint128(postOpGasLimitOverride), - DAPP_ACCOUNT.addr, - uint48(block.timestamp + 1 days), - uint48(block.timestamp), - priceMarkup - ); + PaymasterData memory pmData = PaymasterData({ + validationGasLimit: 100_000, + postOpGasLimit: uint128(postOpGasLimitOverride), + paymasterId: DAPP_ACCOUNT.addr, + validUntil: uint48(block.timestamp + 1 days), + validAfter: uint48(block.timestamp), + priceMarkup: priceMarkup + }); (userOp.paymasterAndData,) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, paymaster, pmData); userOp.signature = signUserOp(sender, userOp); From 105c2ceb2209665e734667512a012586be3b6e4e Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 17 Oct 2024 16:28:39 +0400 Subject: [PATCH 08/11] Update test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol Co-authored-by: filmakarov --- test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index a378352..6331227 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -142,7 +142,14 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { view { PackedUserOperation memory userOp = buildUserOpWithCalldata(ALICE, "", address(VALIDATOR_MODULE)); - PaymasterData memory pmData = PaymasterData(3e6, 3e6, paymasterId, validUntil, validAfter, priceMarkup); + PaymasterData memory pmData = PaymasterData({ + validationGasLimit: 3e6, + postOpGasLimit: 3e6, + paymasterId: paymasterId, + validUntil: validUntil, + validAfter: validAfter, + priceMarkup: priceMarkup + }); (bytes memory paymasterAndData, bytes memory signature) = generateAndSignPaymasterData(userOp, PAYMASTER_SIGNER, bicoPaymaster, pmData); From eeff8172d3d8a71a746e7baed50cddaa26c453bd Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 17 Oct 2024 16:28:49 +0400 Subject: [PATCH 09/11] Update test/unit/concrete/TestSponsorshipPaymaster.t.sol Co-authored-by: filmakarov --- test/unit/concrete/TestSponsorshipPaymaster.t.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index dde4801..229c8eb 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -214,7 +214,12 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { PackedUserOperation[] memory ops = new PackedUserOperation[](1); // price markup of 1e6 - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 55_000); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp({ + sender: ALICE, + paymaster: bicoPaymaster, + pariceMarkup: 1e6, + postOpGasLimitOverride: 55_000 + }); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; From e2f786273a38f8ebd56165e9418cbb3403453bae Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 17 Oct 2024 16:39:59 +0400 Subject: [PATCH 10/11] fix:revert suggestion --- test/unit/concrete/TestSponsorshipPaymaster.t.sol | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index 229c8eb..dde4801 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -214,12 +214,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { PackedUserOperation[] memory ops = new PackedUserOperation[](1); // price markup of 1e6 - (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp({ - sender: ALICE, - paymaster: bicoPaymaster, - pariceMarkup: 1e6, - postOpGasLimitOverride: 55_000 - }); + (PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1e6, 55_000); ops[0] = userOp; uint256 initialBundlerBalance = BUNDLER.addr.balance; From 2ea2a95b71f7c630c172d8f9adfaa2b99f1e32b0 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Thu, 17 Oct 2024 18:42:35 +0300 Subject: [PATCH 11/11] fix tests after merge --- test/base/TestBase.sol | 37 +++++++++++++++---- .../concrete/TestSponsorshipPaymaster.t.sol | 10 +++-- .../TestFuzz_TestSponsorshipPaymaster.t.sol | 7 ++-- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol index e7e1e64..3dd4cbf 100644 --- a/test/base/TestBase.sol +++ b/test/base/TestBase.sol @@ -1,12 +1,12 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.27; - import { Test } from "forge-std/Test.sol"; import { Vm } from "forge-std/Vm.sol"; import { console2 } from "forge-std/console2.sol"; import "solady/utils/ECDSA.sol"; import "./TestHelper.sol"; +import "account-abstraction/core/UserOperationLib.sol"; import { IAccount } from "account-abstraction/interfaces/IAccount.sol"; import { Exec } from "account-abstraction/utils/Exec.sol"; @@ -26,10 +26,11 @@ import { } from "../../../contracts/token/BiconomyTokenPaymaster.sol"; abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { - address constant ENTRYPOINT_ADDRESS = address(0x0000000071727De22E5E9d8BAf0edAc6f37da032); - address constant WRAPPED_NATIVE_ADDRESS = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); + using UserOperationLib for PackedUserOperation; + address constant ENTRYPOINT_ADDRESS = address(0x0000000071727De22E5E9d8BAf0edAc6f37da032); + address constant WRAPPED_NATIVE_ADDRESS = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); address constant SWAP_ROUTER_ADDRESS = address(0xE592427A0AEce92De3Edee1F18E0157C05861564); Vm.Wallet internal PAYMASTER_OWNER; @@ -37,6 +38,9 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { Vm.Wallet internal PAYMASTER_FEE_COLLECTOR; Vm.Wallet internal DAPP_ACCOUNT; + uint256 internal constant _PAYMASTER_POSTOP_GAS_OFFSET = UserOperationLib.PAYMASTER_POSTOP_GAS_OFFSET; + uint256 internal constant _PAYMASTER_DATA_OFFSET = UserOperationLib.PAYMASTER_DATA_OFFSET; + struct PaymasterData { uint128 validationGasLimit; uint128 postOpGasLimit; @@ -310,7 +314,8 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { BiconomySponsorshipPaymaster paymaster, uint256 initialDappPaymasterBalance, uint256 initialFeeCollectorBalance, - uint32 priceMarkup + uint32 priceMarkup, + uint256 maxPenalty ) internal view @@ -320,11 +325,19 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { uint256 resultingFeeCollectorPaymasterBalance = paymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); uint256 totalGasFeesCharged = initialDappPaymasterBalance - resultingDappPaymasterBalance; + uint256 accountableGasFeesCharged = totalGasFeesCharged - maxPenalty; - expectedPriceMarkup = totalGasFeesCharged - ((totalGasFeesCharged * 1e6) / priceMarkup); + expectedPriceMarkup = accountableGasFeesCharged - ((accountableGasFeesCharged * 1e6) / priceMarkup); actualPriceMarkup = resultingFeeCollectorPaymasterBalance - initialFeeCollectorBalance; } + function getMaxPenalty(PackedUserOperation calldata userOp) public view returns (uint256) { + return ( + uint128(uint256(userOp.accountGasLimits)) + + uint128(bytes16(userOp.paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET])) + ) * 10 * userOp.unpackMaxFeePerGas() / 100; + } + // Note: can pack values into one struct function calculateAndAssertAdjustments( BiconomySponsorshipPaymaster bicoPaymaster, @@ -332,26 +345,34 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors { uint256 initialFeeCollectorBalance, uint256 initialBundlerBalance, uint256 initialPaymasterEpBalance, - uint32 priceMarkup + uint32 priceMarkup, + uint256 maxPenalty ) internal view { (uint256 expectedPriceMarkup, uint256 actualPriceMarkup) = - getPriceMarkups(bicoPaymaster, initialDappPaymasterBalance, initialFeeCollectorBalance, priceMarkup); + getPriceMarkups(bicoPaymaster, initialDappPaymasterBalance, initialFeeCollectorBalance, priceMarkup, maxPenalty); uint256 totalGasFeePaid = BUNDLER.addr.balance - initialBundlerBalance; uint256 gasPaidByDapp = initialDappPaymasterBalance - bicoPaymaster.getBalance(DAPP_ACCOUNT.addr); + console2.log("1"); // Assert that what paymaster paid is the same as what the bundler received assertEq(totalGasFeePaid, initialPaymasterEpBalance - bicoPaymaster.getDeposit()); + + console2.log("2"); // Assert that adjustment collected (if any) is correct assertEq(expectedPriceMarkup, actualPriceMarkup); + + console2.log("3"); // Gas paid by dapp is higher than paymaster // Guarantees that EP always has sufficient deposit to pay back dapps assertGt(gasPaidByDapp, BUNDLER.addr.balance - initialBundlerBalance); + + console2.log("4"); // Ensure that max 2% difference between total gas paid + the adjustment premium and gas paid by dapp (from // paymaster) - assertApproxEqRel(totalGasFeePaid + actualPriceMarkup, gasPaidByDapp, 0.02e18); + assertApproxEqRel(totalGasFeePaid + actualPriceMarkup + maxPenalty, gasPaidByDapp, 0.02e18); } function _toSingletonArray(address addr) internal pure returns (address[] memory) { diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index df343ed..b1f8cae 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -223,7 +223,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); // submit userops - vm.expectEmit(true, false, true, true, address(bicoPaymaster)); + vm.expectEmit(true, false, false, false, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); @@ -236,7 +236,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { initialFeeCollectorBalance, initialBundlerBalance, initialPaymasterEpBalance, - 1e6 + 1e6, // priceMarkup + this.getMaxPenalty(userOp) ); } @@ -258,7 +259,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); // submit userops - vm.expectEmit(true, false, true, true, address(bicoPaymaster)); + vm.expectEmit(true, false, false, false, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); startPrank(BUNDLER.addr); @@ -272,7 +273,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { initialFeeCollectorBalance, initialBundlerBalance, initialPaymasterEpBalance, - 1_100_000 + 1_100_000, //price markup, +10% paymaster fee + this.getMaxPenalty(userOp) ); } diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index 0cecb2e..befa421 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -93,7 +93,7 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { } // Review: fuzz with high markeup and current set values. - function skip_testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { + function testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external { vm.assume(priceMarkup <= 2e6 && priceMarkup > 1e6); bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr); @@ -111,7 +111,7 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialDappPaymasterBalance = bicoPaymaster.getBalance(DAPP_ACCOUNT.addr); uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); - vm.expectEmit(true, false, true, true, address(bicoPaymaster)); + vm.expectEmit(true, false, false, false, address(bicoPaymaster)); emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); startPrank(BUNDLER.addr); @@ -125,7 +125,8 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { initialFeeCollectorBalance, initialBundlerBalance, initialPaymasterEpBalance, - priceMarkup + priceMarkup, + this.getMaxPenalty(userOp) ); }