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.

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

fix userOp estimate gas overrides #227

Merged
merged 4 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions packages/account/src/BiconomySmartAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,18 @@ export class BiconomySmartAccount extends SmartAccount implements IBiconomySmart
return executeBatchCallData
}

getDummySignature(): string {
return '0x73c3ac716c487ca34bb858247b5ccf1dc354fbaabdd089af3b2ac8e78ba85a4959a2d76250325bd67c11771c31fccda87c33ceec17cc0de912690521bb95ffcb1b'
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 believe we should include an optional signature parameter in the buildOp function as well. This way, if anyone wants to override it, it can be easily done. If no value is supplied, we can use the getDummySignature method to retrieve the signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we give param signature it self it just accept param and returns the same?
what else could be input param for this method, can you think in terms of ownerless - I know lot of info there is being passed as part of userop signature cc @filmakarov

However this class is limited to ECDSA Biconomy Smart Account always so implementing abstract method is correct. If you know what param should be there for this method in abstract class, I can add it.
Anyway there might be more changes to the abstract class also when we implement ownerless. this is the only use case I can think of for your point

}

getDummyPaymasterData(): string {
return '0x'
}

async buildUserOp(
transactions: Transaction[],
overrides?: Overrides
overrides?: Overrides,
skipBundlerGasEstimation?: boolean
): Promise<Partial<UserOperation>> {
this.isInitialized()
// TODO: validate to, value and data fields
Expand Down Expand Up @@ -286,7 +295,10 @@ export class BiconomySmartAccount extends SmartAccount implements IBiconomySmart
callData: callData
}

userOp = await this.estimateUserOpGas(userOp, overrides)
// for this Smart Account dummy ECDSA signature will be used to estimate gas
userOp.signature = this.getDummySignature()
Copy link
Contributor

Choose a reason for hiding this comment

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

but even if sdk doesn’t send a dummy signature the bundler still appends a dummy signature right? do we need to add this @talhamalik883

Copy link
Contributor

Choose a reason for hiding this comment

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

bundler wouldn't know account information. I think it accepts a dummy sig and if not present defaults to ECDSA mock signature.


userOp = await this.estimateUserOpGas(userOp, overrides, skipBundlerGasEstimation)
Logger.log('userOp after estimation ', userOp)

// Do not populate paymasterAndData as part of buildUserOp as it may not have all necessary details
Expand Down
97 changes: 43 additions & 54 deletions packages/account/src/SmartAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,68 +63,57 @@ export abstract class SmartAccount implements ISmartAccount {
return true
}

abstract getDummySignature(): string

async calculateUserOpGasValues(userOp: Partial<UserOperation>): Promise<Partial<UserOperation>> {
if (!this.provider) throw new Error('Provider is not present for making rpc calls')
const feeData = await this.provider.getFeeData()
userOp.maxFeePerGas =
userOp.maxFeePerGas ??
feeData.maxFeePerGas ??
feeData.gasPrice ??
(await this.provider.getGasPrice())
userOp.maxPriorityFeePerGas =
userOp.maxPriorityFeePerGas ??
feeData.maxPriorityFeePerGas ??
feeData.gasPrice ??
(await this.provider.getGasPrice())
if (userOp.initCode)
userOp.verificationGasLimit =
userOp.verificationGasLimit ?? (await this.getVerificationGasLimit(userOp.initCode))
userOp.callGasLimit =
userOp.callGasLimit ??
(await this.provider.estimateGas({
from: this.smartAccountConfig.entryPointAddress,
to: userOp.sender,
data: userOp.callData
}))
userOp.preVerificationGas = userOp.preVerificationGas ?? this.getPreVerificationGas(userOp)
return userOp
}

async estimateUserOpGas(
userOp: Partial<UserOperation>,
overrides?: Overrides
overrides?: Overrides,
skipBundlerGasEstimation?: boolean
): Promise<Partial<UserOperation>> {
const requiredFields: UserOperationKey[] = ['sender', 'nonce', 'initCode', 'callData']
this.validateUserOp(userOp, requiredFields)

let finalUserOp = userOp
const skipBundlerCall = skipBundlerGasEstimation ?? false
// Override gas values in userOp if provided in overrides params
if (overrides) {
userOp = { ...userOp, ...overrides }
}

Logger.log('userOp in estimation', userOp)

// Defining the keys that are related that can be overrides
const overrideGasFields: UserOperationKey[] = [
'maxFeePerGas',
'maxPriorityFeePerGas',
'verificationGasLimit',
'callGasLimit',
'preVerificationGas'
]

// here we are verifying either all necessary gas properties are present in userOp.
let skipEstimations = true
for (const key of overrideGasFields) {
if (!userOp[key]) {
skipEstimations = false
break
}
}
// If all necessary properties are present in userOp. we will skip estimation and return userOp
if (skipEstimations) {
return userOp
}

if (!this.bundler) {
if (!this.bundler || skipBundlerCall) {
if (!this.provider) throw new Error('Provider is not present for making rpc calls')
// if no bundler url is provided run offchain logic to assign following values of UserOp
// maxFeePerGas, maxPriorityFeePerGas, verificationGasLimit, callGasLimit, preVerificationGas
const feeData = await this.provider.getFeeData()
userOp.maxFeePerGas =
userOp.maxFeePerGas ??
feeData.maxFeePerGas ??
feeData.gasPrice ??
(await this.provider.getGasPrice())
userOp.maxPriorityFeePerGas =
userOp.maxPriorityFeePerGas ??
feeData.maxPriorityFeePerGas ??
feeData.gasPrice ??
(await this.provider.getGasPrice())
if (userOp.initCode)
userOp.verificationGasLimit =
userOp.verificationGasLimit ?? (await this.getVerificationGasLimit(userOp.initCode))
userOp.callGasLimit =
userOp.callGasLimit ??
(await this.provider.estimateGas({
from: this.smartAccountConfig.entryPointAddress,
to: userOp.sender,
data: userOp.callData
}))
userOp.preVerificationGas = userOp.preVerificationGas ?? this.getPreVerificationGas(userOp)
finalUserOp = await this.calculateUserOpGasValues(finalUserOp)
} else {
// Making call to bundler to get gas estimations for userOp
const {
Expand All @@ -140,19 +129,19 @@ export abstract class SmartAccount implements ISmartAccount {
(!maxFeePerGas || !maxPriorityFeePerGas)
) {
const feeData = await this.provider.getFeeData()
userOp.maxFeePerGas =
finalUserOp.maxFeePerGas =
feeData.maxFeePerGas ?? feeData.gasPrice ?? (await this.provider.getGasPrice())
userOp.maxPriorityFeePerGas =
finalUserOp.maxPriorityFeePerGas =
feeData.maxPriorityFeePerGas ?? feeData.gasPrice ?? (await this.provider.getGasPrice())
} else {
userOp.maxFeePerGas = userOp.maxFeePerGas ?? maxFeePerGas
userOp.maxPriorityFeePerGas = userOp.maxPriorityFeePerGas ?? maxPriorityFeePerGas
finalUserOp.maxFeePerGas = maxFeePerGas ?? userOp.maxFeePerGas
finalUserOp.maxPriorityFeePerGas = maxPriorityFeePerGas ?? userOp.maxPriorityFeePerGas
}
userOp.verificationGasLimit = userOp.verificationGasLimit ?? verificationGasLimit
userOp.callGasLimit = userOp.callGasLimit ?? callGasLimit
userOp.preVerificationGas = userOp.preVerificationGas ?? preVerificationGas
finalUserOp.verificationGasLimit = verificationGasLimit ?? userOp.verificationGasLimit
finalUserOp.callGasLimit = callGasLimit ?? userOp.callGasLimit
finalUserOp.preVerificationGas = preVerificationGas ?? userOp.preVerificationGas
}
return userOp
return finalUserOp
}

async isAccountDeployed(address: string): Promise<boolean> {
Expand Down
17 changes: 3 additions & 14 deletions packages/bundler/src/Bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,9 @@ export class Bundler implements IBundler {
* @returns Promise<UserOpGasPricesResponse>
*/
async estimateUserOpGas(userOp: UserOperation): Promise<UserOpGasResponse> {
// bundler requires these dummy values for estimation
// TODO: dapp/wallet devs need to take dummy signature as well that we will pass to bundler. as signature depends on smart account implementation
const dummpyUserop = {
callGasLimit: '90000',
verificationGasLimit: '3000000',
preVerificationGas: '46856',
maxFeePerGas: '0',
maxPriorityFeePerGas: '0',
paymasterAndData: '0x',
signature:
'0x73c3ac716c487ca34bb858247b5ccf1dc354fbaabdd089af3b2ac8e78ba85a4959a2d76250325bd67c11771c31fccda87c33ceec17cc0de912690521bb95ffcb1b'
}
const userOperation = { ...dummpyUserop, ...userOp }
userOp = transformUserOP(userOperation)
// expected dummySig and possibly dummmy paymasterAndData should be provided by the caller
// bundler doesn't know account and paymaster implementation
userOp = transformUserOP(userOp)
Logger.log('userOp sending for fee estimate ', userOp)

const bundlerUrl = this.getBundlerUrl()
Expand Down
15 changes: 15 additions & 0 deletions packages/paymaster/src/BiconomyPaymaster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,19 @@ export class BiconomyPaymaster implements IHybridPaymaster<SponsorUserOperationD
}
throw new Error('Error in generating paymasterAndData')
}

/**
*
* @param userOp user operation
* @param paymasterServiceData optional extra information to be passed to paymaster service
* @returns paymasterAndData with valid length but mock signature
*/
async getDummyPaymasterAndData(
userOp: Partial<UserOperation>,
paymasterServiceData?: SponsorUserOperationDto // mode is necessary. partial context of token paymaster or verifying
): Promise<string> {
Logger.log('userOp is ', userOp)
Logger.log('paymasterServiceData is ', paymasterServiceData)
return '0x'
}
}
4 changes: 4 additions & 0 deletions packages/paymaster/src/interfaces/IHybridPaymaster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export interface IHybridPaymaster<T> extends IPaymaster {
userOp: Partial<UserOperation>,
paymasterServiceData?: T
): Promise<PaymasterAndDataResponse>
getDummyPaymasterAndData(
userOp: Partial<UserOperation>,
paymasterServiceData?: T
): Promise<string>
buildTokenApprovalTransaction(
tokenPaymasterRequest: BiconomyTokenPaymasterRequest,
provider: Provider
Expand Down
1 change: 1 addition & 0 deletions packages/paymaster/src/interfaces/IPaymaster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ import { PaymasterAndDataResponse } from '../utils/Types'
export interface IPaymaster {
// Implementing class may add extra parameter (for example paymasterServiceData with it's own type) in below function signature
getPaymasterAndData(userOp: Partial<UserOperation>): Promise<PaymasterAndDataResponse>
getDummyPaymasterAndData(userOp: Partial<UserOperation>): Promise<string>
}