EREP-010: paymaster should have deposit to cover all userops #194
Conversation
drortirosh
force-pushed
the
erep-10-paymaster-deposit
branch
from
d091c26 to
1dddf61
Compare
| /** | ||
| * clear deposits after some known change on-chain | ||
| */ | ||
| clearCache (): void { |
There was a problem hiding this comment.
I think it is a bad idea to have the cache stored in DepositManager but trigger the clearing of the cache from the EventsManager. I suggest you keep the entire logic of the cache inside this class, i.e. let the getCachedDeposit() function do the clearCache once every X blocks or whatever.
There was a problem hiding this comment.
the DepositManager manages the deposits. the simplest impl is to read from the blockchain everytime.
Instead, I wanted some optimization, and cache it until there is a network change - which is always signaled by a message.
| if (userOp.paymaster != null) { | ||
| this.incrementEntryCount(userOp.paymaster) | ||
| } | ||
| this.incrementEntryCount(userOp.sender) |
There was a problem hiding this comment.
Why move the incrementEntryCount(sender) to be below incrementEntryCount(paymaster)? Seems like an unnecessary change to me.
There was a problem hiding this comment.
don't increment before you solved all cases where it can crash (otherwise, you need to catch to undo increments)
| @@ -331,7 +331,7 @@ describe('#ValidationManager', () => { | |||
| await testExistingUserOp('balance-self', undefined) | |||
| }) | |||
|
|
|||
| it('should fail with unstaked paymaster returning context', async () => { | |||
| it('should accept unstaked paymaster returning context', async () => { | |||
There was a problem hiding this comment.
Is this change supposed to be included in this PR?
There was a problem hiding this comment.
unrelated. will separate.
packages/utils/src/Utils.ts
Outdated
| @@ -33,7 +33,7 @@ export enum ValidationErrors { | |||
| SimulatePaymasterValidation = -32501, | |||
| OpcodeValidation = -32502, | |||
| NotInTimeRange = -32503, | |||
| Reputation = -32504, | |||
| BannedOrThrottledPaymaster = -32504, | |||
There was a problem hiding this comment.
Actually, this error is also returned for the "underfunded" paymaster, meaning there are now THREE separate issues that can cause this same ErrorID.
I would rather split these into THREE DIFFERENT error codes.
| * the cost is the sum of the verification gas limits, multiplied by the maxFeePerGas. | ||
| * @param userOp | ||
| */ | ||
| export function getUserOpMaxCost (userOp: UserOperation): number { |
There was a problem hiding this comment.
This looks like Go code. Shouldn't we make UserOperation a class?
There was a problem hiding this comment.
yes, should pack all userop related utils (resemble UserOperationLib in solidity - but we don't have "using" in TS..)
submitting a new UserOp into the mempool should revert if its deposit is not high enough.