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.

Sign up for GitHub

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

finalUserOp replaced with userOp for overrides proper usage #239

Merged
merged 2 commits into from
Jul 24, 2023
Merged

finalUserOp replaced with userOp for overrides proper usage #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
024f1f7
finalUserOp replaced with userOp for overrides proper usage
talhamalik883 Jul 24, 2023
24f50db
conbvert to BN before checking < 0 (check 21k instead)
livingrockrises Jul 24, 2023
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
3 changes: 2 additions & 1 deletion packages/account/src/BiconomySmartAccount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,8 @@ export class BiconomySmartAccount extends SmartAccount implements IBiconomySmart
// Requesting to update gas limits again (especially callGasLimit needs to be re-calculated)
try {
finalUserOp = await this.estimateUserOpGas(finalUserOp)
if (finalUserOp.callGasLimit && finalUserOp.callGasLimit < 0) {
const cgl = ethers.BigNumber.from(finalUserOp.callGasLimit)
if (finalUserOp.callGasLimit && cgl.lt(ethers.BigNumber.from('21000'))) {
return {
...userOp,
callData: newCallData
Expand Down
2 changes: 1 addition & 1 deletion packages/account/src/SmartAccount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export abstract class SmartAccount implements ISmartAccount {
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
finalUserOp = await this.calculateUserOpGasValues(finalUserOp)
finalUserOp = await this.calculateUserOpGasValues(userOp)
Copy link
Contributor

Choose a reason for hiding this comment

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

If userOp already contains some keys here, calculateUserOpGasValues will skip local estimation and use provided key (aka override) here?

Copy link
Contributor

Choose a reason for hiding this comment

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

so essentially someone would have to pass overrides and skipBundlerEstimation true both in order for it to act as an override.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we make two override flags?
bundlerEstimationOverrides and overrides

Copy link
Contributor

Choose a reason for hiding this comment

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

if overrides is given then assume skipBundlerEstimation to be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can reinstate the previous logic. If all gas attributes are provided in buildUserOp, there is no need to use off-chain logic, and we can also avoid calling the bundler for estimation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah then overrides should be limited to only callGasLimit and verificationGasLimit.

preVerificationGas, maxFeePerGas, maxPriorityFeePerGas would need to still come from the bundler

Copy link
Contributor

Choose a reason for hiding this comment

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

and if we'd need to make bundler call and use some partial values provided in overrides, these overrides directly get applied to bundler call which might mess with estimated limits again..

Copy link
Contributor

Choose a reason for hiding this comment

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

there are two kinds of overrides which now requires discussion with @tomarsachin2271 and @arcticfloyd1984

Copy link
Contributor

Choose a reason for hiding this comment

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

I would merge this PR and then handle this differently.
current communication would be

  • send overrides and skipBundler flag true to just do the overrides
  • send overrides which will only be used for bundler estimation (for debugging some cases it helps)
  • no overrides and skip flag true which will be used to calculate values locally in the sdk
  • one can always replace keys in userop before it gets signed by paymaster (if involved) and the user

} else {
delete userOp.maxFeePerGas
delete userOp.maxPriorityFeePerGas
Expand Down