-
Notifications
You must be signed in to change notification settings - Fork 226
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.
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
EREP-010: paymaster should have deposit to cover all userops #194
Changes from all commits
048397b
1dddf61
6a737cf
084da17
130689f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { BigNumber } from 'ethers' | ||
import { getUserOpMaxCost, IEntryPoint, requireCond, UserOperation, ValidationErrors } from '@account-abstraction/utils' | ||
import { MempoolManager } from './MempoolManager' | ||
|
||
/** | ||
* manage paymaster deposits, to make sure a paymaster has enough gas for all its pending transaction in the mempool | ||
* [EREP-010] | ||
*/ | ||
export class DepositManager { | ||
deposits: { [addr: string]: BigNumber } = {} | ||
|
||
constructor (readonly entryPoint: IEntryPoint, readonly mempool: MempoolManager) { | ||
} | ||
|
||
async checkPaymasterDeposit (userOp: UserOperation): Promise<void> { | ||
const paymaster = userOp.paymaster | ||
if (paymaster == null) { | ||
return | ||
} | ||
let deposit = await this.getCachedDeposit(paymaster) | ||
deposit = deposit.sub(getUserOpMaxCost(userOp)) | ||
|
||
for (const entry of this.mempool.getMempool()) { | ||
if (entry.userOp.paymaster === paymaster) { | ||
deposit = | ||
deposit.sub(BigNumber.from(getUserOpMaxCost(userOp))) | ||
} | ||
} | ||
|
||
// [EREP-010] paymaster is required to have balance for all its pending transactions. | ||
// on-chain AA31 checks the deposit for the current userop. | ||
// but submitting all these UserOps it will eventually abort on this error, | ||
// so it's fine to return the same code. | ||
requireCond(deposit.gte(0), 'paymaster deposit too low for all mempool UserOps', ValidationErrors.PaymasterDepositTooLow) | ||
} | ||
|
||
/** | ||
* clear deposits after some known change on-chain | ||
*/ | ||
clearCache (): void { | ||
this.deposits = {} | ||
} | ||
|
||
async getCachedDeposit (addr: string): Promise<BigNumber> { | ||
let deposit = this.deposits[addr] | ||
if (deposit == null) { | ||
deposit = this.deposits[addr] = await this.entryPoint.balanceOf(addr) | ||
} | ||
return deposit | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change supposed to be included in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated. will separate. |
||
const pm = await new TestStorageAccount__factory(ethersSigner).deploy() | ||
// await entryPoint.depositTo(pm.address, { value: parseEther('0.1') }) | ||
// await pm.addStake(entryPoint.address, { value: parseEther('0.1') }) | ||
|
@@ -345,9 +345,7 @@ describe('#ValidationManager', () => { | |
paymasterPostOpGasLimit: 1e6, | ||
paymasterData: Buffer.from('postOp-context') | ||
} | ||
expect(await vm.validateUserOp(userOp) | ||
.then(() => 'should fail', e => e.message)) | ||
.to.match(/unstaked paymaster must not return context/) | ||
await vm.validateUserOp(userOp) | ||
}) | ||
|
||
it('should fail if validation recursively calls handleOps', async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ export enum ValidationErrors { | |
InsufficientStake = -32505, | ||
UnsupportedSignatureAggregator = -32506, | ||
InvalidSignature = -32507, | ||
PaymasterDepositTooLow = -32508, | ||
UserOperationReverted = -32521 | ||
} | ||
|
||
|
@@ -189,3 +190,19 @@ export async function runContractScript<T extends ContractFactory> (provider: Pr | |
if (parsed == null) throw new Error('unable to parse script (error) response: ' + ret) | ||
return parsed.args | ||
} | ||
|
||
/** | ||
* sum the given bignumberish items (numbers, hex, bignumbers) | ||
*/ | ||
export function sum (...args: BigNumberish[]): BigNumber { | ||
return args.reduce((acc: BigNumber, cur) => acc.add(cur), BigNumber.from(0)) | ||
} | ||
|
||
/** | ||
* calculate the maximum verification cost of a UserOperation. | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like Go code. Shouldn't we make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, should pack all userop related utils (resemble UserOperationLib in solidity - but we don't have "using" in TS..) |
||
return sum(userOp.preVerificationGas, userOp.verificationGasLimit, userOp.paymasterVerificationGasLimit ?? 0).mul(userOp.maxFeePerGas).toNumber() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
|
||
curl -s https://raw.githubusercontent.com/ethereum/ERCs/master/ERCS/erc-7562.md > /tmp/erc-7562.md | ||
`dirname $0`/checkRulesCoverage /tmp/erc-7562.md `dirname $0`/../packages/validation-manager | grep unmatched|grep -v -i -w ALT | grep -v REMOVED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a bad idea to have the cache stored in
DepositManager
but trigger the clearing of the cache from theEventsManager
. I suggest you keep the entire logic of the cache inside this class, i.e. let thegetCachedDeposit()
function do theclearCache
once every X blocks or whatever.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.