Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feaT:refactor event + add more info in parse pm data #30

Merged
merged 3 commits into from
Oct 22, 2024
Merged

feaT:refactor event + add more info in parse pm data #30

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3114452
feaT:refactor event + add more info in parse pm data
livingrockrises Oct 21, 2024
d2d96a5
refactor:respond to PR comments
livingrockrises Oct 22, 2024
fcc4c46
respond to PR comments
livingrockrises Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions contracts/interfaces/IBiconomySponsorshipPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.27;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

Check warning on line 4 in contracts/interfaces/IBiconomySponsorshipPaymaster.sol

View workflow job for this annotation

GitHub Actions / Lint sources 

global import of path @openzeppelin/contracts/token/ERC20/IERC20.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
import { PackedUserOperation } from "account-abstraction/core/UserOperationLib.sol";

interface IBiconomySponsorshipPaymaster {
Expand All @@ -9,9 +9,9 @@
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);

Check warning on line 12 in contracts/interfaces/IBiconomySponsorshipPaymaster.sol

View workflow job for this annotation

GitHub Actions / Lint sources 

'_paymasterId' should not start with _

Check warning on line 12 in contracts/interfaces/IBiconomySponsorshipPaymaster.sol

View workflow job for this annotation

GitHub Actions / Lint sources 

'_value' should not start with _
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);

Expand Down Expand Up @@ -50,6 +50,8 @@
uint48 validUntil,
uint48 validAfter,
uint32 priceMarkup,
uint128 paymasterValidationGasLimit,
uint128 paymasterPostOpGasLimit,
bytes calldata signature
);
}
31 changes: 21 additions & 10 deletions contracts/sponsorship/BiconomySponsorshipPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ contract BiconomySponsorshipPaymaster is
uint48 validUntil,
uint48 validAfter,
uint32 priceMarkup,
uint128 paymasterValidationGasLimit,
uint128 paymasterPostOpGasLimit,
bytes calldata signature
)
{
Expand All @@ -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:];
}
}
Expand All @@ -261,25 +265,29 @@ 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
actualGasCost = actualGasCost + (unaccountedGas * actualUserOpFeePerGas);
// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it really happen?
if prechargedAmount < adjustedGasCost doesn't paymaster lose money?

if we only expect else to mean prechargedAmount == adjustedGasCost, we do need else at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it would never go into the else case, but if it does then want to emit different value in the event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we pre charged less than we want to charge more yeah. in that case otherwise paymaster loses money. so diff of that adjusted - pre charged should be deduced from paymasterId balance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Does it make sense to emit different event to easily recognize we charged less

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do it but it will be overhead on the paymaster service.
so, should I leave this

  1. as it is
  2. as it is but different event name
  3. same/different event name but in the else case doing
    paymasterIdBalances[paymasterId] += (adjustedGasCost - prechargedAmount);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filmakarov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

going with 3 with same event name.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you should emit adjustedGasCost in this case as it is now what in fact was charged after L289 right?
so emit can be moved out of if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. with this addition it makes sense
I wanted to emit different things that's the whole reason I added if else.
but it's ok.
knowing that charge can never be accurately emitted here and will need some off-chain component for penalties.

}

// 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);
}
}

Expand All @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
8 changes: 6 additions & 2 deletions test/unit/concrete/TestSponsorshipPaymaster.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -403,13 +403,17 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
uint48 parsedValidUntil,
uint48 parsedValidAfter,
uint32 parsedPriceMarkup,
uint128 parsedPaymasterValidationGasLimit,
uint128 parsedPaymasterPostOpGasLimit,
bytes memory parsedSignature
) = bicoPaymaster.parsePaymasterAndData(paymasterAndData);

assertEq(paymasterId, parsedPaymasterId);
assertEq(validUntil, parsedValidUntil);
assertEq(validAfter, parsedValidAfter);
assertEq(priceMarkup, parsedPriceMarkup);
assertEq(pmData.validationGasLimit, parsedPaymasterValidationGasLimit);
assertEq(pmData.postOpGasLimit, parsedPaymasterPostOpGasLimit);
assertEq(signature, parsedSignature);
}
}
6 changes: 5 additions & 1 deletion test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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);

Expand All @@ -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);
}
}
Loading