code optimisation #2

livingrockrises · 2023-05-07T00:41:21Z

Remove pack() function and move everything to own implementation of getHash()
refactor this.exchangeRate -> exchangeRate by making function external -> public
aid contract FeeManager which might be helpful in future
dev notes : todo / review that should be checked on final stage (early stage of audits)
not using userOp.gasPrice() as bundlers would reject this (because it touches GASPRICE and BASEFEE opcodes)
NOTE: postOp may still use calculation based on above op-codes as max priority would be truth for accurate calcs
updated context accordingly so we still have needful information
commented allowance check and validateConstructor to open a discussion around this
EDIT: removed above
Added a test case for undeployed wallet

…uctor and allowance check

tomarsachin2271 · 2023-05-10T07:07:32Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -68,18 +51,12 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
    // receiver of withdrawn fee tokens
    address public feeReceiver;

-    // review offsets and define more if needed
-    // others in between if we just do concat and not abi.encode
    uint256 private constant VALID_PND_OFFSET = 21;


what is PND offset. Defined this in line comment above this line.

tomarsachin2271 · 2023-05-10T07:14:53Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -112,7 +89,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
        address indexed _actor
    );

-    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 cost);
+    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 charge, uint256 premium);


In Constructor, _transferOwnership(_owner) is already done in BasePaymaster.
No need to do this again in this constructor.

tomarsachin2271 · 2023-05-10T07:28:45Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -112,7 +89,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
        address indexed _actor
    );

-    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 cost);
+    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 charge, uint256 premium);



In setFeeReceiver() method, we are saving feeReceiver address in a slot variable.

assembly { sstore(feeReceiver.slot, _newFeeReceiver) }

But in constructor we are saving directly in variable.

feeReceiver = address(this);

Same is happening for oracleAggregator as well.

I din't get this. We can make this consistent, is that what you mean?

can use assembly in the constructor

yes need to make this consistent

this change is already done, pushed in last commit.

tomarsachin2271 · 2023-05-10T07:29:27Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -181,7 +158,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
        emit NewFeeTokenSupported(_token, msg.sender);
    }

-    function setUnaccountedEPGasOverhead(uint256 value) external onlyOwner {
+    function setUnaccountedEPGasOverhead(uint256 value) external payable onlyOwner {


why is this made payable?

All the methods that are not likely to be called (for example onlyOwner ones) will save gas if they are payable, because this removes the verification if there's value provided with the call

tomarsachin2271 · 2023-05-10T07:32:16Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -181,7 +158,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
        emit NewFeeTokenSupported(_token, msg.sender);
    }

-    function setUnaccountedEPGasOverhead(uint256 value) external onlyOwner {
+    function setUnaccountedEPGasOverhead(uint256 value) external payable onlyOwner {
        uint256 oldValue = UNACCOUNTED_COST;
        UNACCOUNTED_COST = value;
        emit EPGasOverheadChanged(oldValue, value);


Update this comment

from

/** * add a deposit for this paymaster, used for paying for transaction fees */

to

/** * Add a deposit in native currency for this paymaster, used for paying for transaction fees. * This is ideally done by the entity who is managing the received ERC20 gas tokens. */

tomarsachin2271 · 2023-05-10T07:56:25Z

contracts/token/BiconomyTokenPaymaster.sol

+                gasPriceUserOp = Math.min(maxFeePerGas, maxPriorityFeePerGas + block.basefee);
+            }
+        }
+
        uint256 actualTokenCost = ((actualGasCost + (UNACCOUNTED_COST * gasPriceUserOp)) * effectiveExchangeRate) / 1e18;


We should add the fee also during actualTokenCost calculation.

@tomarsachin2271 fee is in token terms, not gas execution cost or like fee multiplier even.

in below line check, no of tokens being transferred is actualTokenCost + fee

tomarsachin2271 · 2023-05-10T07:58:39Z

contracts/utils/Exec.sol

+
+// solhint-disable no-inline-assembly
+
+enum Operation {


Where is this enum used?

within the contract where Exec could be used.

this is utils contract which is not used directly in token paymaster. and won't be part of the audit

tomarsachin2271 · 2023-05-10T08:08:59Z

contracts/token/BiconomyTokenPaymaster.sol

            feeToken.safeTransferFrom(account, feeReceiver, actualTokenCost + fee);
-            emit TokenPaymasterOperation(account, address(feeToken), actualTokenCost + fee);
+            emit TokenPaymasterOperation(account, address(feeToken), actualTokenCost, fee);


Also emit effectiveExchangePrice , userOpHash, priceSource in the event for better tracking purpose.

If possible add another field which have value 'actualTokenCost + fee' also to simplify the accounting if it is to be used off-chain.

Need to pass userOpHash in the context from validatePaymasterUserOp method.

so what all we should emit?

userOpHash will anyways be emitted in UserOperationEvent (sender as well)

I am also emitting actualTokenCost and fee separately, do you want us to emit actualTokenCost, fee, and actualTokenCost + fee ?

and should all of these be in same event (I can index at max 3 values - all indexed is ideal)

Yes we need to also emit UserOpHash in this event, coz we want to co-relate the transaction when we update these transactions in the database in paymaster service.
Eg. When signing request comes to paymaster service, we save the data in db with userOpHash as primary key.
Now when we track the TokenPaymasterOperation event, then it becomes hard to corelate it with userOpHash already present in the database. Now we have to get all the logs of same transactino and try to search UserOperation event (which can be multiple btw in same transaction) and then try to corelate it with this event.

That's why its important to emit userOpHash also in this event, so we can corelate this event with actual transaction data in database.

Also its fine if there are more than 3 events, it can be decoded using the ABI, not a big issue.

So in all we need to emit
userOpHash, (actualTokenCost+fee), fee, account, tokenAddress, effectiveExchangePrice, priceSource

Where account, tokenAddress, actualTokenCost+fee are indexed.

ok, let me make single event and add more fields.

tomarsachin2271 · 2023-05-10T08:18:44Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -34,31 +37,11 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
    using UserOperationLib for UserOperation;


Contract level comment can be made more clear and simple.

/** * A token-based paymaster that allows user to pay gas fee in ERC20 tokens. The paymaster owner chooses which tokens to accept. * The payment manager (usually the owner) first deposits native gas into the EntryPoint. Then, for each transaction, it takes the gas fee from the user's ERC20 token balance. The manager must convert these collected tokens back to native gas and deposit it into the EntryPoint to keep the system running. * It is an extension of VerifyingPaymaster which trusts external signer to authorize the transaction, but also with an ability to withdraw tokens. * * The validatePaymasterUserOp function does not interact with external contracts but uses an externally provided exchange rate. * Based on the exchangeRate and requiredPrefund amount, the validation method checks if the user's account has enough token balance. This is done by only looking at the referenced storage. * All Withdrawn tokens are sent to a dynamic fee receiver address. * * Optionally a safe guard deposit may be used in future versions. */

tomarsachin2271 · 2023-05-10T08:19:46Z

contracts/token/BiconomyTokenPaymaster.sol

@@ -315,7 +277,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
        UserOperation calldata userOp,


Below line can be changed as paymasterAndData now contains more data.

The paymaster data is expected to be the paymaster and a signature over the entire request parameters.

livingrockrises · 2023-05-10T19:15:14Z

if rest of the things are fine you can approve this and I can merge.

code optimisation + dev notes

239f8d6

livingrockrises requested review from leekt, AmanRaj1608 and tomarsachin2271 May 7, 2023 00:41

livingrockrises added 6 commits May 8, 2023 19:20

remove unused struct PaymasterData

bc5d353

using util Exec + using maxPriorityFeePerGas + remove validate constr…

3b6d06f

…uctor and allowance check

remove approval from the test case and use in calldata instead

662fadd

failing test case ofr undeployed wallet

9717c2c

update deployment scripts

bb4fe95

separation of token charge and fee in the event + update test cases

713808a

tomarsachin2271 reviewed May 10, 2023

View reviewed changes

core-review/respond to PR comments

6adddfb

livingrockrises requested a review from talhamalik883 May 10, 2023 21:42

livingrockrises added 2 commits May 11, 2023 02:16

feat/event update to emit more information

ca020cf

placeholder folder for adapters

661cd02

tomarsachin2271 approved these changes May 11, 2023

View reviewed changes

tomarsachin2271 merged commit 126c9e0 into master May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code optimisation #2

code optimisation #2

livingrockrises commented May 7, 2023 •

edited

Loading

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023 •

edited

Loading

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

tomarsachin2271 May 10, 2023

livingrockrises May 10, 2023

livingrockrises commented May 10, 2023

		@@ -34,31 +37,11 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
		using UserOperationLib for UserOperation;

		@@ -315,7 +277,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
		UserOperation calldata userOp,

code optimisation #2

code optimisation #2

Conversation

livingrockrises commented May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises commented May 10, 2023

livingrockrises commented May 7, 2023 •

edited

Loading

livingrockrises May 10, 2023 •

edited

Loading