From ec434d52baa1f06fe78c3c92a37169f081066fed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Mart=C3=ADn=20Alconada=20Verzini?= Date: Mon, 29 Jan 2024 09:37:31 +0000 Subject: [PATCH] feat: add user op max cost setter --- src/paymasters/SponsorPaymaster.sol | 70 ++++++++++++++++++----------- test/SponsorPaymaster.t.sol | 51 +++++++++++++++++++++ 2 files changed, 94 insertions(+), 27 deletions(-) diff --git a/src/paymasters/SponsorPaymaster.sol b/src/paymasters/SponsorPaymaster.sol index 10d146a4f..418603e58 100644 --- a/src/paymasters/SponsorPaymaster.sol +++ b/src/paymasters/SponsorPaymaster.sol @@ -23,18 +23,17 @@ import "../interfaces/IKintoWallet.sol"; contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, ReentrancyGuard, ISponsorPaymaster { using SafeERC20 for IERC20; - // ========== Events ============ - event AppRegistrySet(address appRegistry, address _oldRegistry); - // calculated cost of the postOp uint256 public constant COST_OF_POST = 200_000; uint256 public constant MAX_COST_OF_VERIFICATION = 230_000; uint256 public constant MAX_COST_OF_PREVERIFICATION = 110_000; - uint256 public constant MAX_COST_OF_USEROP = 0.03 ether; uint256 public constant RATE_LIMIT_PERIOD = 1 minutes; uint256 public constant RATE_LIMIT_THRESHOLD_TOTAL = 50; + uint256 public userOpMaxCost = 0.03 ether; + IKintoAppRegistry public override appRegistry; + mapping(address => uint256) public balances; mapping(address => uint256) public contractSpent; // keeps track of total gas consumption by contract mapping(address => uint256) public unlockBlock; @@ -46,7 +45,10 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen // rate limit across apps: user => RateLimitData mapping(address => ISponsorPaymaster.RateLimitData) public globalRateLimit; - IKintoAppRegistry public override appRegistry; + // ========== Events ============ + + event AppRegistrySet(address oldRegistry, address newRegistry); + event UserOpMaxCostSet(uint256 oldUserOpMaxCost, uint256 newUserOpMaxCost); // ========== Constructor & Upgrades ============ @@ -76,16 +78,6 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen (newImplementation); } - /** - * @dev Set the app registry - * @param _appRegistry address of the app registry - */ - function setAppRegistry(address _appRegistry) external override onlyOwner { - require(_appRegistry != address(0) && _appRegistry != address(appRegistry), "SP: appRegistry cannot be 0"); - emit AppRegistrySet(_appRegistry, address(appRegistry)); - appRegistry = IKintoAppRegistry(_appRegistry); - } - // ========== Deposit Mgmt ============ /** @@ -137,7 +129,7 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen entryPoint.withdrawTo(payable(target), amount); } - /* =============== Viewers & validation ============= */ + /* =============== Setters & Getters ============= */ /** * Return the deposit info for the account @@ -145,8 +137,7 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen * @return _unlockBlock - the block height at which the deposit can be withdrawn. */ function depositInfo(address account) external view returns (uint256 amount, uint256 _unlockBlock) { - amount = balances[account]; - _unlockBlock = unlockBlock[account]; + return (balances[account], unlockBlock[account]); } /** @@ -173,9 +164,30 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen } /** - * Validate the request from the sender to fund it. - * The sender should have enough txs and gas left to be gasless. - * The contract developer funds the contract for its users and rate limits the app. + * @dev Set the app registry + * @param _newRegistry address of the app registry + */ + function setAppRegistry(address _newRegistry) external override onlyOwner { + require(_newRegistry != address(0) && _newRegistry != address(appRegistry), "SP: new registry cannot be 0"); + emit AppRegistrySet(address(appRegistry), _newRegistry); + appRegistry = IKintoAppRegistry(_newRegistry); + } + + /** + * @dev Set the max cost of a user operation + * @param _newUserOpMaxCost max cost of a user operation + */ + function setUserOpMaxCost(uint256 _newUserOpMaxCost) external onlyOwner { + emit UserOpMaxCostSet(userOpMaxCost, _newUserOpMaxCost); + userOpMaxCost = _newUserOpMaxCost; + } + + /* =============== AA overrides ============= */ + + /** + * @notice Validates the request from the sender to fund it. + * @dev sender should have enough txs and gas left to be gasless. + * @dev contract developer funds the contract for its users and rate limits the app. */ function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 maxCost) internal @@ -196,7 +208,7 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen // use maxFeePerGas for conservative estimation of gas cost uint256 gasPriceUserOp = userOp.maxFeePerGas; uint256 ethMaxCost = (maxCost + COST_OF_POST * gasPriceUserOp); - require(ethMaxCost <= MAX_COST_OF_USEROP, "SP: gas too high for user op"); + require(ethMaxCost <= userOpMaxCost, "SP: gas too high for user op"); address sponsor = appRegistry.getSponsor(_decodeCallData(userOp.callData)); require(unlockBlock[sponsor] == 0, "SP: deposit not locked"); @@ -205,7 +217,7 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen } /** - * perform the post-operation to charge the account contract for the gas. + * @notice performs the post-operation to charge the account contract for the gas. */ function _postOp(PostOpMode, /* mode */ bytes calldata context, uint256 actualGasCost) internal override { (address account, address userAccount, uint256 maxFeePerGas, uint256 maxPriorityFeePerGas) = @@ -250,6 +262,8 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen _checkLimits(userAccount, account, ethCost); } + /* =============== Internal methods ============= */ + function _checkLimits(address sender, address targetAccount, uint256 ethMaxCost) internal view { // global rate limit check ISponsorPaymaster.RateLimitData memory globalData = globalRateLimit[sender]; @@ -277,10 +291,12 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen ); } - // @notice extracts `target` contract from callData - // @dev the last op on a batch MUST always be a contract whose sponsor is the one we want to - // bear with the gas cost of all ops - // @dev this is very similar to KintoWallet._decodeCallData, consider unifying + /** + * @notice extracts `target` contract from callData + * @dev the last op on a batch MUST always be a contract whose sponsor is the one we want to + * bear with the gas cost of all ops + * @dev this is very similar to KintoWallet._decodeCallData, consider unifying + */ function _decodeCallData(bytes calldata callData) private pure returns (address target) { bytes4 selector = bytes4(callData[:4]); // extract the function selector from the callData diff --git a/test/SponsorPaymaster.t.sol b/test/SponsorPaymaster.t.sol index 1d8e508c5..ba7db3165 100644 --- a/test/SponsorPaymaster.t.sol +++ b/test/SponsorPaymaster.t.sol @@ -36,6 +36,11 @@ contract SponsorPaymasterTest is SharedSetup { assertEq(_paymaster.COST_OF_POST(), 200_000); } + /* ============ Events ============ */ + + event AppRegistrySet(address oldRegistry, address newRegistry); + event UserOpMaxCostSet(uint256 oldUserOpMaxCost, uint256 newUserOpMaxCost); + /* ============ Upgrade ============ */ function testUpgradeTo() public { @@ -492,6 +497,52 @@ contract SponsorPaymasterTest is SharedSetup { assertRevertReasonEq("SP: Kinto Gas App limit exceeded"); } + function testSetAppRegistry() public { + address oldAppRegistry = address(_kintoAppRegistry); + address newAppRegistry = address(123); + + vm.expectEmit(true, true, true, true); + emit AppRegistrySet(oldAppRegistry, newAppRegistry); + + vm.prank(_owner); + _paymaster.setAppRegistry(newAppRegistry); + assertEq(address(_paymaster.appRegistry()), newAppRegistry); + } + + function testSetAppRegistry_RevertWhen_CallerIsNotOwner() public { + vm.expectRevert("Ownable: caller is not the owner"); + _paymaster.setAppRegistry(address(123)); + } + + function testSetAppRegistry_RevertWhen_AddressIsZero() public { + vm.expectRevert("SP: new registry cannot be 0"); + vm.prank(_owner); + _paymaster.setAppRegistry(address(0)); + } + + function testSetAppRegistry_RevertWhen_SameAddress() public { + vm.expectRevert("SP: new registry cannot be 0"); + vm.prank(_owner); + _paymaster.setAppRegistry(address(_kintoAppRegistry)); + } + + function testUserOpMaxCost() public { + uint256 oldUserOpMaxCost = _paymaster.userOpMaxCost(); + uint256 newUserOpMaxCost = 123; + + vm.expectEmit(true, true, true, true); + emit UserOpMaxCostSet(oldUserOpMaxCost, newUserOpMaxCost); + + vm.prank(_owner); + _paymaster.setUserOpMaxCost(newUserOpMaxCost); + assertEq(_paymaster.userOpMaxCost(), newUserOpMaxCost); + } + + function testUserOpMaxCost_RevertWhen_CallerIsNotOwner() public { + vm.expectRevert("Ownable: caller is not the owner"); + _paymaster.setUserOpMaxCost(123); + } + // TODO: // reset gas limits after periods // test doing txs in different days