feat: use alchemy_requestGasAndPaymasterAndData when using alchemy paymaster #1310
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd the label graphite-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
| * @param {UserOperationRequest<TEntryPointVersion>} userOperation the user operation request | ||
| * @returns {Hex | Multiplier | undefined} the overridden field value | ||
| */ | ||
| const overrideField = < |
There was a problem hiding this comment.
| @@ -144,6 +144,12 @@ export function createAlchemySmartAccountClient({ | |||
| const feeOptions = | |||
| opts?.feeOptions ?? getDefaultUserOperationFeeOptions(chain); | |||
|
|
|||
| // If we have a policyId, then it's safe to assume that | |||
There was a problem hiding this comment.
is this true? i think it is, but i'd like confirmation.
There was a problem hiding this comment.
not really. unless they use alchmey provider, otherwise a policyId only means they have created a policy with that paymaster.
There was a problem hiding this comment.
is it okay to do this here? the current implementation was also only including the gas manager middleware here based on the policyId, i.e.
...(policyId && alchemyGasManagerMiddleware(policyId))There was a problem hiding this comment.
wondering if it's actually better to include an "experimental" option (at least for now) instead of forcing everyone on to the other rpc method?
There was a problem hiding this comment.
is it okay to do this here? the current implementation was also only including the gas manager middleware here based on the policyId, i.e.
...(policyId && alchemyGasManagerMiddleware(policyId))
it is because without policyId, it doesn't make sense to involve a paymaster middleware, by any format.
jakehobbs
changed the title
jakehobbs
force-pushed
the
jake/use-alchemy_requestGasAndPaymasterAndData
branch
from
32be290 to
562f179
Compare
jakehobbs
requested review from
moldy530,
rthomare,
dancoombs,
mokok123,
dphilipson,
linnall,
adamegyed,
howydev,
Zer0dot,
jaypaik,
Blu-J,
noam-alchemy and
archit2407
as code owners
jakehobbs
force-pushed
the
jake/use-alchemy_requestGasAndPaymasterAndData
branch
from
74a918a to
a4f8465
Compare
jakehobbs
force-pushed
the
jake/use-alchemy_requestGasAndPaymasterAndData
branch
from
a4f8465 to
ea52e72
Compare
| return noopMiddleware(uo, args); | ||
| } | ||
|
|
||
| const paymaster = getAlchemyPaymasterAddress(args.client.chain); // throws if unsupported chain |
There was a problem hiding this comment.
❗ ❗ ❗ this is problematic b/c this function only supports 15 chains & throws if it's an unsupported chain.
@andysim3d do you think we should fall back to using pm_getPaymasterStubData here so we don't have to maintain this list of chains & paymaster addresses?
| * | ||
| * @deprecated This chain list in this function is no longer maintained since the ERC-7677 middleware is typically used to resolve the paymaster address |
There was a problem hiding this comment.
is this what's used in the new (old) middleware too?
There was a problem hiding this comment.
the old version of the middleware that used alchemy_requestGasAndPaymasterAndData was using this for building the dummy paymaster data.. i discovered it's an issue when the chains team was testing this in ws-tools, b/c it would fail for any other chain.
in order to prevent having to maintain this, the new version of the alchemy_requestGasAndPaymasterAndData middleware will fall back to using the 7677 dummyPaymasterAndData middleware (calling pm_getPaymasterStubData) if there is a gasEstimator or feeEstimator override, otherwise it just does a no-op since it's not actually needed.
| * ...alchemyGasAndPaymasterAndDataMiddleware({ | ||
| * policyId: "policyId", | ||
| * transport: alchemy({ apiKey: "your-api-key" }), | ||
| * }) |
There was a problem hiding this comment.
is this how we intend it to be used as well? Ideally the change to this middleware happens invisibily to users.
Currently, you just pass a policyId to createAlchemySmartAccountClient and under the hood that makes this call
There was a problem hiding this comment.
oh just saw the impl above does do that, then I'd update this example to use createSmartAccountClient instead of createAlchemySmartAccountClient
also worth calling out in the description that someone shouldn't have to use this method directly since the alchemy client will handle this for you
There was a problem hiding this comment.
oops, great call-out! def should just enable it here by passing the policyId. i missed that this example was using createAlchemySmartAccountClient and not just createSmartAccountClient. 👌
| * the `alchemy_requestGasAndPaymasterAndData` method instead of standard ERC-7677 methods. | ||
| * | ||
| * @example | ||
| * ```ts |
There was a problem hiding this comment.
nit:
| * ```ts | |
| * ```ts twoslash |
| } | ||
|
|
||
| // Fall back to the default 7677 dummyPaymasterAndData middleware. | ||
| return alchemyGasManagerMiddleware(policyId).dummyPaymasterAndData!( |
|
@moldy530 re: tests, we were not using the current i've almost got mocks working for it, but stumbling around a bit trying to find the best way to do the gas estimation. |
Pull Request Checklist
yarn test)sitefolder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change)yarn lint:check) and fix any issues? (yarn lint:write)PR-Codex overview
This PR introduces new functionality for handling gas requests and paymaster data in the
Alchemytransport layer, enhancing middleware capabilities and ensuring compliance with the ERC-7677 protocol.Detailed summary
alchemy_requestGasAndPaymasterAndDatamethod toalchemyTransport.AlchemyTransportConfigto include new methods.alchemyGasAndPaymasterAndDataMiddlewarefor custom gas and paymaster handling.