From 3114452ed7883cea174d9d417eeb5448d087ea7d Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Mon, 21 Oct 2024 17:26:54 +0400 Subject: [PATCH 1/3] feaT:refactor event + add more info in parse pm data --- .../IBiconomySponsorshipPaymaster.sol | 8 +++-- .../BiconomySponsorshipPaymaster.sol | 31 +++++++++++++------ .../concrete/TestSponsorshipPaymaster.t.sol | 8 +++-- .../TestFuzz_TestSponsorshipPaymaster.t.sol | 6 +++- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol index faf0665..0580d5e 100644 --- a/contracts/interfaces/IBiconomySponsorshipPaymaster.sol +++ b/contracts/interfaces/IBiconomySponsorshipPaymaster.sol @@ -9,9 +9,9 @@ interface IBiconomySponsorshipPaymaster { event FixedPriceMarkupChanged(uint256 indexed oldValue, uint256 indexed newValue); event VerifyingSignerChanged(address indexed oldSigner, address indexed newSigner, address indexed actor); 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 actualGasCost, uint256 indexed adjustedGasCost, bytes32 indexed userOpHash); + 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, uint256 indexed _premium); event Received(address indexed sender, uint256 value); event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor); @@ -50,6 +50,8 @@ interface IBiconomySponsorshipPaymaster { uint48 validUntil, uint48 validAfter, uint32 priceMarkup, + uint128 paymasterValidationGasLimit, + uint128 paymasterPostOpGasLimit, bytes calldata signature ); } diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index 8c00fff..5f008a6 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -235,6 +235,8 @@ contract BiconomySponsorshipPaymaster is uint48 validUntil, uint48 validAfter, uint32 priceMarkup, + uint128 paymasterValidationGasLimit, + uint128 paymasterPostOpGasLimit, bytes calldata signature ) { @@ -243,6 +245,8 @@ contract BiconomySponsorshipPaymaster is validUntil = uint48(bytes6(paymasterAndData[_PAYMASTER_ID_OFFSET + 20:_PAYMASTER_ID_OFFSET + 26])); validAfter = uint48(bytes6(paymasterAndData[_PAYMASTER_ID_OFFSET + 26:_PAYMASTER_ID_OFFSET + 32])); priceMarkup = uint32(bytes4(paymasterAndData[_PAYMASTER_ID_OFFSET + 32:_PAYMASTER_ID_OFFSET + 36])); + paymasterValidationGasLimit = uint128(bytes16(paymasterAndData[_PAYMASTER_VALIDATION_GAS_OFFSET:_PAYMASTER_POSTOP_GAS_OFFSET])); + paymasterPostOpGasLimit = uint128(bytes16(paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET])); signature = paymasterAndData[_PAYMASTER_ID_OFFSET + 36:]; } } @@ -261,8 +265,8 @@ contract BiconomySponsorshipPaymaster is override { unchecked { - (address paymasterId, uint32 priceMarkup, bytes32 userOpHash, uint256 prechargedAmount) = - abi.decode(context, (address, uint32, bytes32, uint256)); + (address paymasterId, uint32 priceMarkup, uint256 prechargedAmount) = + abi.decode(context, (address, uint32, uint256)); // Include unaccountedGas since EP doesn't include this in actualGasCost // unaccountedGas = postOpGas + EP overhead gas + estimated penalty @@ -270,16 +274,20 @@ contract BiconomySponsorshipPaymaster is // Apply the price markup uint256 adjustedGasCost = (actualGasCost * priceMarkup) / _PRICE_DENOMINATOR; + uint256 premium = adjustedGasCost - actualGasCost; + + // Add priceMarkup to fee collector balance + paymasterIdBalances[feeCollector] += premium; + if (prechargedAmount > adjustedGasCost) { // If overcharged refund the excess paymasterIdBalances[paymasterId] += (prechargedAmount - adjustedGasCost); + // here adjustedGasCost does not account for gasPenalty + emit GasBalanceDeducted(paymasterId, adjustedGasCost, premium); + } else { + // here chargedAmount accounts for penalty with maxGasPenalty + emit GasBalanceDeducted(paymasterId, prechargedAmount, premium); } - - // Add priceMarkup to fee collector balance - paymasterIdBalances[feeCollector] += adjustedGasCost - actualGasCost; - - // premium = adjustedGasCost - actualGasCost => do not need to emit it explicitly - emit GasBalanceDeducted(paymasterId, actualGasCost, adjustedGasCost, userOpHash); } } @@ -302,8 +310,11 @@ contract BiconomySponsorshipPaymaster is override returns (bytes memory context, uint256 validationData) { - (address paymasterId, uint48 validUntil, uint48 validAfter, uint32 priceMarkup, bytes calldata signature) = + (userOpHash); + (address paymasterId, uint48 validUntil, uint48 validAfter, uint32 priceMarkup, uint128 paymasterValidationGasLimit, uint128 paymasterPostOpGasLimit, bytes calldata signature) = parsePaymasterAndData(userOp.paymasterAndData); + (paymasterValidationGasLimit, paymasterPostOpGasLimit); + //ECDSA library supports both 64 and 65-byte long signatures. // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and // not "ECDSA" @@ -347,7 +358,7 @@ contract BiconomySponsorshipPaymaster is paymasterIdBalances[paymasterId] -= (effectiveCost + maxPenalty); - context = abi.encode(paymasterId, priceMarkup, userOpHash, effectiveCost); + context = abi.encode(paymasterId, priceMarkup, effectiveCost); //no need for other on-chain validation: entire UserOp should have been checked // by the external service prior to signing it. diff --git a/test/unit/concrete/TestSponsorshipPaymaster.t.sol b/test/unit/concrete/TestSponsorshipPaymaster.t.sol index b1f8cae..1388828 100644 --- a/test/unit/concrete/TestSponsorshipPaymaster.t.sol +++ b/test/unit/concrete/TestSponsorshipPaymaster.t.sol @@ -224,7 +224,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { // submit userops vm.expectEmit(true, false, false, false, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); + emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0); startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); stopPrank(); @@ -260,7 +260,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { // submit userops vm.expectEmit(true, false, false, false, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); + emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0); startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); @@ -403,6 +403,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { uint48 parsedValidUntil, uint48 parsedValidAfter, uint32 parsedPriceMarkup, + uint128 parsedPaymasterValidationGasLimit, + uint128 parsedPaymasterPostOpGasLimit, bytes memory parsedSignature ) = bicoPaymaster.parsePaymasterAndData(paymasterAndData); @@ -410,6 +412,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase { assertEq(validUntil, parsedValidUntil); assertEq(validAfter, parsedValidAfter); assertEq(priceMarkup, parsedPriceMarkup); + assertEq(pmData.validationGasLimit, parsedPaymasterValidationGasLimit); + assertEq(pmData.postOpGasLimit, parsedPaymasterPostOpGasLimit); assertEq(signature, parsedSignature); } } diff --git a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol index befa421..6b2e55f 100644 --- a/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol +++ b/test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol @@ -112,7 +112,7 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr); vm.expectEmit(true, false, false, false, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash); + emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0); startPrank(BUNDLER.addr); ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr)); @@ -156,6 +156,8 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { uint48 parsedValidUntil, uint48 parsedValidAfter, uint32 parsedPriceMarkup, + uint128 parsedPaymasterValidationGasLimit, + uint128 parsedPaymasterPostOpGasLimit, bytes memory parsedSignature ) = bicoPaymaster.parsePaymasterAndData(paymasterAndData); @@ -164,5 +166,7 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase { assertEq(validAfter, parsedValidAfter); assertEq(priceMarkup, parsedPriceMarkup); assertEq(signature, parsedSignature); + assertEq(pmData.validationGasLimit, parsedPaymasterValidationGasLimit); + assertEq(pmData.postOpGasLimit, parsedPaymasterPostOpGasLimit); } } From d2d96a53b703e2032334f725b99ee3a83feb7789 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:29:19 +0400 Subject: [PATCH 2/3] refactor:respond to PR comments --- contracts/sponsorship/BiconomySponsorshipPaymaster.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index 5f008a6..b27d5e7 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -285,6 +285,8 @@ contract BiconomySponsorshipPaymaster is // here adjustedGasCost does not account for gasPenalty emit GasBalanceDeducted(paymasterId, adjustedGasCost, premium); } else { + // deduct what needs to be deducted from paymasterId + paymasterIdBalances[paymasterId] -= (adjustedGasCost - prechargedAmount); // here chargedAmount accounts for penalty with maxGasPenalty emit GasBalanceDeducted(paymasterId, prechargedAmount, premium); } From fcc4c4671a3c387a91ae23c1615291c8c3975d95 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:44:48 +0400 Subject: [PATCH 3/3] respond to PR comments --- contracts/sponsorship/BiconomySponsorshipPaymaster.sol | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol index b27d5e7..cda8beb 100644 --- a/contracts/sponsorship/BiconomySponsorshipPaymaster.sol +++ b/contracts/sponsorship/BiconomySponsorshipPaymaster.sol @@ -282,14 +282,12 @@ contract BiconomySponsorshipPaymaster is if (prechargedAmount > adjustedGasCost) { // If overcharged refund the excess paymasterIdBalances[paymasterId] += (prechargedAmount - adjustedGasCost); - // here adjustedGasCost does not account for gasPenalty - emit GasBalanceDeducted(paymasterId, adjustedGasCost, premium); } else { // deduct what needs to be deducted from paymasterId - paymasterIdBalances[paymasterId] -= (adjustedGasCost - prechargedAmount); - // here chargedAmount accounts for penalty with maxGasPenalty - emit GasBalanceDeducted(paymasterId, prechargedAmount, premium); + paymasterIdBalances[paymasterId] -= (adjustedGasCost - prechargedAmount); } + // here adjustedGasCost does not account for gasPenalty. prechargedAmount accounts for penalty with maxGasPenalty + emit GasBalanceDeducted(paymasterId, adjustedGasCost, premium); } }