feat: handle one off bypassing paymaster and data middleware case #606

denniswon · 2024-04-21T05:52:56Z

breaking change: - dummyPaymasterAndData middleware override used to be () => Hex, but in this PR, changed to ClientMiddlewareFn just like all other middlewares.

Update: breaking change avoided with dummyPaymasterAndData middleware override being changed to:

() =>
      | UserOperationRequest<"0.6.0">["paymasterAndData"]
      | Pick<UserOperationRequest<"0.7.0">, "paymaster" | "paymasterData">;

To make it not breaking change, and also make it not as confusing for entrypoint v0.7 operations.

on the next major version update, we (should) would change the override.dummyPaymasterAndData to be ClientMiddlewareFn as well like other middleware.

yarn test:run-e2e -- -t "simple account v7 should successfully execute with alchemy paymaster info"
yarn run v1.22.22
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ vitest run --config vitest.config.e2e.ts -t 'simple account v7 should successfully execute with alchemy paymaster info'

 RUN  v0.31.4 /Users/denniswon/alchemy/aa-sdk/packages/alchemy

 ↓ |aa-alchemy| e2e-tests/light-account.e2e.test.ts (13) [skipped]
 ↓ |aa-alchemy| e2e-tests/modular-account-multi-owner.e2e.test.ts (12) [skipped]
 ↓ |aa-alchemy| e2e-tests/multi-owner-light-account.e2e.test.ts (12) [skipped]
 ↓ |aa-alchemy| e2e-tests/multisig-account.e2e.test.ts (5) [skipped]
 ✓ |aa-alchemy| e2e-tests/simple-account-v7.e2e.test.ts (13) 19749ms

 Test Files  1 passed | 4 skipped (5)
      Tests  1 passed | 54 skipped (55)
   Start at  21:59:30
   Duration  21.82s (transform 56ms, setup 28ms, collect 1.39s, tests 19.75s, environment 0ms, prepare 53ms)

✨  Done in 22.41s.

e2e test for simple account with entrypoint v0.7 and alchemy smart account client + gas manager passes. cc: @dphilipson @moldy530

Pull Request Checklist

Did you add new tests and confirm existing tests pass? (yarn test)
Did you update relevant docs? (docs are found in the site folder, and guidelines for updating/adding docs can be found in the contribution guide)
Do your commits follow the Conventional Commits standard?
Does your PR title also follow the Conventional Commits standard?
If you have a breaking change, is it correctly reflected in your commit message? (e.g. feat!: breaking change)
Did you run lint (yarn lint:check) and fix any issues? (yarn lint:write)
Did you follow the contribution guidelines?

PR-Codex overview

This PR introduces enhancements to paymaster and data handling in various middleware functions and user operation structures.

Detailed summary

Added functions bypassPaymasterAndData and concatPaymasterAndData
Updated dummyPaymasterAndData type in multiple files
Added parsePaymasterAndData function
Updated UserOperationRequest types in multiple files

The following files were skipped due to too many changes: site/packages/aa-core/smart-account-client/index.md, packages/core/src/actions/smartAccount/internal/runMiddlewareStack.ts, packages/core/src/middleware/actions.ts, packages/core/src/utils/userop.ts, packages/alchemy/e2e-tests/simple-account-v7.e2e.test.ts, packages/alchemy/src/middleware/gasManager.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

packages/core/src/actions/smartAccount/internal/runMiddlewareStack.ts

moldy530 · 2024-04-22T15:31:58Z

packages/core/src/actions/smartAccount/internal/runMiddlewareStack.ts

@@ -51,7 +56,9 @@ export async function _runMiddlewareStack<
    client.middleware.feeEstimator,
    client.middleware.gasEstimator,
    client.middleware.customMiddleware,
-    client.middleware.paymasterAndData,
+    overrides && bypassPaymasterAndData(overrides)
+      ? bypassPaymasterMiddleware


nit: just use the noopMiddleware here

I cannot. @moldy530 because for bypassing case for entrypoint v7, I need to unset all 4 paymaster fields of user op.

see comment below, why are they ever set if the override is 0x then no code that handles paymaster should ever run and the gas estimator should never return paymaster fields. I would not expect to to have paymaster fields populated before the paymaster middleware runs.

yes it could, now that gas Estimator response includes paymasterVerificationGas, and this should be omitted for bypassing paymaster.

Thus, bypassmiddleware is not the same as noopmiddleware.

If paymaster is configured, gas Estimator response WILL include the paymasterVerificationGas field.

now that gas Estimator response includes paymasterVerificationGas, and this should be omitted for bypassing paymaster

Why is that? it should respect the overrides.paymasterAndData === "0x"?

I would expect that all middlewares that can return paymaster fields respect the paymasterAndData override if it is passed

okay, @dphilipson just to confirm, if paymaster address is not set on the user op struct sent for estimate user operation gas RPC, the bundler will never send the paymaster gas limits? Not just rundler, but all other bundlers. This is not clear on the 4337 standards.

Hm, the standard says, in the estimation response:

paymasterVerificationGasLimit value used for paymaster verification (if paymaster exists in the UserOperation)

paymasterPostOpGasLimit value used for paymaster post op execution (if paymaster exists in the UserOperation)

I would interpret that as saying that it doesn't return values if there's no paymaster, either omitting the fields entirely or returning null. Our bundler does one of these, don't remember which (I would guess returning null).

I'm inclined to say it's okay to not go out of the way to delete the fields here and just trust whatever the bundler is returning. For one thing, I would expect any bundler which is returning zeroes for the paymaster fields when there's no paymaster would also accept zeroes for the paymaster fields when there's no paymaster, so it should be fine.

if above where we have client.middleware.dummyPaymasterAndData run we add the same check for the override and just noop then we don't need this bypassMiddleware at all and we can just run the noop middleware

@moldy530 for entrypoint v0.6, don't we need to set paymasterAndData as '0x' for estimate gas step? or is that not needed? @dphilipson

packages/core/src/middleware/defaults/bypassPaymasterMiddleware.ts

packages/core/src/utils/userop.ts

denniswon · 2024-04-25T04:42:50Z

updated the PR with:
additional omit paymaster field from gas estimator for bypass case
Added a number of missing jsDocs

dphilipson

This looks good to me. Really appreciate all the doc comments. I'm not as familiar with the middleware stack as @moldy530 so I'd definitely appreciate a second look if he has a chance.

Question since I didn't notice it mentioned anywhere: do we also need to disable the dummyPaymasterAndData middleware when there's a "0x" paymaster override?

packages/alchemy/src/middleware/gasManager.ts

denniswon · 2024-04-25T14:47:47Z

@dphilipson we don't have control over the overridden dummy paymaster middleware. When the user sets the paymaster (for example gas manager), both dummy paymaster middleware and paymaster middleware is overridden.

The dummy paymaster middleware is supposed to set the paymaster address + dummy data for the dummy paymaster middleware. (we should add this to our doc of using custom paymaster)

denniswon · 2024-04-25T14:49:49Z

Asana task https://app.asana.com/0/1205598840815267/1207162808443182/f for "The dummy paymaster middleware is supposed to set the paymaster address + dummy data for the dummy paymaster middleware. (we should add this to our doc of using custom paymaster)" @dphilipson

moldy530 · 2024-04-25T18:10:14Z

we don't have control over the overridden dummy paymaster middleware. When the user sets the paymaster (for example gas manager), both dummy paymaster middleware and paymaster middleware is overridden

wait why don't we just do what we do with dummyPaymaster? we can skip running dummy paymaster if the override is 0x and just noop there. that means nothing downstream will get a paymaster field so doesn't matter if people override the gas estimator

…ase and added missing js docs

denniswon · 2024-04-25T22:37:01Z

packages/core/src/client/types.ts

@@ -42,7 +42,7 @@ export type ClientMiddlewareConfig<
  "dummyPaymasterAndData" | "paymasterAndData"
 > & {
  paymasterAndData?: {
-    dummyPaymasterAndData: () => Hex;
+    dummyPaymasterAndData: ClientMiddlewareFn<TContext>;


@dphilipson @moldy530 this would be a breaking change for devs who are overriding this dummy paymaster and data middleware.

But this is necessary because from dummy paymaster and data, we need to check the entrypoint version from the account (either hoisted or passed in).

@moldy530 let me know if you have an idea for preventing breaking change for this.

dphilipson

Had a question. If the answer is "no", then lgtm.

dphilipson · 2024-04-25T23:05:29Z

packages/alchemy/src/middleware/gasManager.ts

+    ...struct,
+    ...(account.getEntryPoint().version === "0.7.0"
+      ? {
+          paymaster: `${paymasterAddress}`,


Do you not need to set paymasterData: dummyData in this case?

denniswon · 2024-04-25T23:49:13Z

breaking change: - dummyPaymasterAndData middleware override used to be () => Hex, but in this PR, changed to ClientMiddlewareFn just like all other middlewares.

moldy530 · 2024-04-26T00:07:44Z

packages/core/src/middleware/defaults/bypassPaymasterMiddleware.ts

I thought we landed on the fact that you don't need this?

packages/core/src/middleware/actions.ts

dphilipson

Actually want to resolve the discussion in slack, taking back my approval for now. Sorry!

dphilipson

Think it needs a hex concat fix, but otherwise I like it!

packages/core/src/utils/userop.ts

dphilipson · 2024-04-26T05:08:29Z

packages/core/src/middleware/actions.ts

+                    overrides.paymasterAndData!.dummyPaymasterAndData()
+                  )
+                    ? parsePaymasterAndData(
+                        overrides.paymasterAndData!.dummyPaymasterAndData() as Hex


If you put overrides.paymasterAndData!.dummyPaymasterAndData() into a temp variable, you wouldn't need these casts because isHex would work as a type guard (plus you wouldn't need to keep writing this large expression over and over). I'd suggest:

overrides.paymasterAndData?.dummyPaymasterAndData ? async (struct, { account }) => { const data = overrides.paymasterAndData!.dummyPaymasterAndData(); return account.getEntryPoint().version === "0.7.0" ? { ...struct, ...(isHex(data) ? parsePaymasterAndData(data) : concatPayamsterAndData(data) ), } // ...

and maybe consider turning some of ternaries into if-else blocks for readability.

isHex would work as a type guard

Actually it doesn't which is why had to cast...

It didn't work before because you're evaluating dummyPaymasterAndData() multiple times, so the type system can't tell that it's the same value. isHex should work as a type guard if you extract the value, its signature is

function isHex( value: unknown, { strict = true }: { strict?: boolean } = {}, ): value is Hex;

Here's what it looks like without the casts using isHex as a type guard, if you want it. Verified locally.

dummyPaymasterAndData: overrides.paymasterAndData?.dummyPaymasterAndData ? async (struct, { account }) => { const data = overrides.paymasterAndData!.dummyPaymasterAndData(); const paymasterOverrides = account.getEntryPoint().version === "0.7.0" ? isHex(data) ? parsePaymasterAndData(data) : data : { paymasterAndData: isHex(data) ? data : concatPaymasterAndData(data), }; return { ...struct, ...paymasterOverrides }; } : defaultPaymasterAndData,

packages/core/src/actions/smartAccount/internal/runMiddlewareStack.ts

packages/core/src/middleware/defaults/bypassPaymasterMiddleware.ts

dphilipson

Think you have a hex conversion bug. If we get that fixed I think we're there.

dphilipson · 2024-04-26T07:37:00Z

packages/core/src/middleware/actions.ts

+                    overrides.paymasterAndData!.dummyPaymasterAndData()
+                  )
+                    ? parsePaymasterAndData(
+                        overrides.paymasterAndData!.dummyPaymasterAndData() as Hex


Here's what it looks like without the casts using isHex as a type guard, if you want it. Verified locally.

dummyPaymasterAndData: overrides.paymasterAndData?.dummyPaymasterAndData ? async (struct, { account }) => { const data = overrides.paymasterAndData!.dummyPaymasterAndData(); const paymasterOverrides = account.getEntryPoint().version === "0.7.0" ? isHex(data) ? parsePaymasterAndData(data) : data : { paymasterAndData: isHex(data) ? data : concatPaymasterAndData(data), }; return { ...struct, ...paymasterOverrides }; } : defaultPaymasterAndData,

packages/core/src/utils/userop.ts

moldy530

Gotta change the docs for he dummy paymaster and data to what it was before + the new union type

site/packages/aa-core/smart-account-client/index.md

site/packages/aa-core/smart-account-client/middleware/index.md

site/third-party/paymasters.md

denniswon · 2024-04-26T16:09:19Z

updated thanks for the thorough review! @dphilipson @moldy530

dphilipson

Looks good! I would still consider my suggested change with calling dummyPaymasterAndData once, not just because it doesn't need casts but also because calling a user-provided function once instead of multiple times can be a bit better e.g. if they add log statements to it for debugging, but I won't block because of that.

denniswon · 2024-04-26T18:37:20Z

@dphilipson udpated per your suggestion. Thanks!

changes requested made

…ypoint v7

denniswon requested review from dphilipson and alex-miao April 21, 2024 05:52

denniswon requested review from moldy530, rthomare, dancoombs, mokok123 and avasisht23 as code owners April 21, 2024 05:52

denniswon closed this Apr 21, 2024

denniswon deleted the denniswon/paymaster-override-revert branch April 21, 2024 16:06

denniswon restored the denniswon/paymaster-override-revert branch April 21, 2024 16:12

denniswon reopened this Apr 21, 2024

moldy530 reviewed Apr 21, 2024

View reviewed changes

packages/core/src/actions/smartAccount/internal/runMiddlewareStack.ts Outdated Show resolved Hide resolved

denniswon changed the title ~~feat: handle one off bypassing paymaster and data middleware case~~ fix: handle one off bypassing paymaster and data middleware case Apr 22, 2024

moldy530 reviewed Apr 22, 2024

View reviewed changes

packages/core/src/middleware/defaults/bypassPaymasterMiddleware.ts Outdated Show resolved Hide resolved

denniswon force-pushed the denniswon/paymaster-override-revert branch from 8623f5c to 27ffca4 Compare April 24, 2024 23:47

alchemy-bot reviewed Apr 24, 2024

View reviewed changes

packages/core/src/middleware/defaults/bypassPaymasterMiddleware.ts Outdated Show resolved Hide resolved

packages/core/src/utils/userop.ts Show resolved Hide resolved

denniswon requested review from moldy530 and alchemy-bot April 25, 2024 04:42

dphilipson previously approved these changes Apr 25, 2024

View reviewed changes

packages/alchemy/src/middleware/gasManager.ts Outdated Show resolved Hide resolved

denniswon dismissed dphilipson’s stale review via 628e769 April 25, 2024 17:02

denniswon force-pushed the denniswon/paymaster-override-revert branch from 628e769 to 7207a52 Compare April 25, 2024 22:30

denniswon added 2 commits April 25, 2024 15:30

feat: handle one off bypassing paymaster and data middleware case

298373a

feat: additional omit paymaster field from gas estimator for bypass c…

0d69e03

…ase and added missing js docs

denniswon force-pushed the denniswon/paymaster-override-revert branch from 7207a52 to 58ad449 Compare April 25, 2024 22:30

denniswon commented Apr 25, 2024

View reviewed changes

dphilipson previously approved these changes Apr 25, 2024

View reviewed changes

denniswon changed the title ~~fix: handle one off bypassing paymaster and data middleware case~~ fix!: handle one off bypassing paymaster and data middleware case Apr 25, 2024

moldy530 reviewed Apr 26, 2024

View reviewed changes

packages/core/src/middleware/actions.ts Outdated Show resolved Hide resolved

dphilipson reviewed Apr 26, 2024

View reviewed changes

denniswon dismissed dphilipson’s stale review via 6113ff8 April 26, 2024 02:59

denniswon force-pushed the denniswon/paymaster-override-revert branch 2 times, most recently from 6113ff8 to 18525a5 Compare April 26, 2024 04:54

denniswon changed the title ~~fix!: handle one off bypassing paymaster and data middleware case~~ feat: handle one off bypassing paymaster and data middleware case Apr 26, 2024

dphilipson reviewed Apr 26, 2024

View reviewed changes

moldy530 reviewed Apr 26, 2024

View reviewed changes

denniswon force-pushed the denniswon/paymaster-override-revert branch from 18525a5 to 5f0fd50 Compare April 26, 2024 05:56

dphilipson requested changes Apr 26, 2024

View reviewed changes

moldy530 previously requested changes Apr 26, 2024

View reviewed changes

site/packages/aa-core/smart-account-client/index.md Show resolved Hide resolved

site/packages/aa-core/smart-account-client/middleware/index.md Show resolved Hide resolved

site/third-party/paymasters.md Outdated Show resolved Hide resolved

denniswon requested review from moldy530 and dphilipson April 26, 2024 15:42

denniswon force-pushed the denniswon/paymaster-override-revert branch from 5f0fd50 to 03c179e Compare April 26, 2024 16:08

dphilipson previously approved these changes Apr 26, 2024

View reviewed changes

feat: alchemy gas manager fix for dummy paymaster middleware for entr…

9408c65

…ypoint v7

denniswon dismissed dphilipson’s stale review via 9408c65 April 26, 2024 18:38

denniswon force-pushed the denniswon/paymaster-override-revert branch from 03c179e to 9408c65 Compare April 26, 2024 18:38

denniswon merged commit b5d8110 into main Apr 26, 2024
3 checks passed

denniswon deleted the denniswon/paymaster-override-revert branch April 26, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: handle one off bypassing paymaster and data middleware case #606

feat: handle one off bypassing paymaster and data middleware case #606

denniswon commented Apr 21, 2024 •

edited by pr-codex bot

Loading

moldy530 Apr 22, 2024

denniswon Apr 22, 2024

moldy530 Apr 23, 2024

denniswon Apr 23, 2024 •

edited

Loading

moldy530 Apr 23, 2024

moldy530 Apr 23, 2024

denniswon Apr 23, 2024

dphilipson Apr 24, 2024

moldy530 Apr 26, 2024 •

edited

Loading

denniswon Apr 26, 2024

denniswon commented Apr 25, 2024

dphilipson left a comment

denniswon commented Apr 25, 2024

denniswon commented Apr 25, 2024

moldy530 commented Apr 25, 2024

denniswon Apr 25, 2024

dphilipson left a comment

dphilipson Apr 25, 2024

denniswon commented Apr 25, 2024

moldy530 Apr 26, 2024

dphilipson left a comment

dphilipson left a comment

dphilipson Apr 26, 2024

denniswon Apr 26, 2024

dphilipson Apr 26, 2024

dphilipson Apr 26, 2024

dphilipson left a comment

dphilipson Apr 26, 2024

moldy530 left a comment

denniswon commented Apr 26, 2024

dphilipson left a comment •

edited

Loading

denniswon commented Apr 26, 2024

feat: handle one off bypassing paymaster and data middleware case #606

feat: handle one off bypassing paymaster and data middleware case #606

Conversation

denniswon commented Apr 21, 2024 • edited by pr-codex bot Loading

Pull Request Checklist

PR-Codex overview

Detailed summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniswon Apr 23, 2024 • 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

moldy530 Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniswon commented Apr 25, 2024

dphilipson left a comment

Choose a reason for hiding this comment

denniswon commented Apr 25, 2024

denniswon commented Apr 25, 2024

moldy530 commented Apr 25, 2024

Choose a reason for hiding this comment

dphilipson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniswon commented Apr 25, 2024

Choose a reason for hiding this comment

dphilipson left a comment

Choose a reason for hiding this comment

dphilipson left a 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

dphilipson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moldy530 left a comment

Choose a reason for hiding this comment

denniswon commented Apr 26, 2024

dphilipson left a comment • edited Loading

Choose a reason for hiding this comment

denniswon commented Apr 26, 2024

denniswon commented Apr 21, 2024 •

edited by pr-codex bot

Loading

denniswon Apr 23, 2024 •

edited

Loading

moldy530 Apr 26, 2024 •

edited

Loading

dphilipson left a comment •

edited

Loading