-
Notifications
You must be signed in to change notification settings - Fork 220
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
AA-398: Standardize 'preVerificationGas' verification & create 'debug_bundler_setConfiguration' #228
Conversation
/** | ||
* The size of the dummy 'signature' parameter to be used during estimation. | ||
*/ | ||
readonly estimationSignatureSize: number |
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 wonder if we should estimate signature size at all: there shouldn't be a "standard" sig size. The wallet should fill it in.
Same for paymaster: if there isn't a pmd, we should estimate based on no pmd, not provide a "hint" that there is a "suggested" value for it.
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.
This does not make sense to me. There absolutely is no "standard" sig size and this configuration parameter is exactly the result of that. In order to estimate the preVerificationGas
we must know what the size of the signature will be in the future.
When the wallet comes to a point where it needs to estimate the amount of preVerificationGas
, if it did not take into account the signature
size or the paymasterData
size, it would calculate a lower than necessary preVerificationGas
and its UserOps would be rejected.
NOTE that the "validation" step absolutely does not rely on estimation
-related configuration parameters and relies on the real value.
If you still disagree please elaborate how you expect preVerificationGas
to work alternatively.
Thanks.
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.
What I'm saying is that there is no "default" signature size: the wallet must fill in some value into the signature for the estimation to work, at the expected size, since no other component other than the wallet can "guess" a correct signature size.
/** | ||
* Only parameters in this object can be provided by a 'debug_bundler_setConfiguration' API. | ||
*/ | ||
export const BundlerConfigShape = { |
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.
Shadowing definition
/** | ||
* The size of the dummy 'paymasterData' parameter to be used during estimation. | ||
*/ | ||
readonly estimationPaymasterDataSize: number |
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.
What about initCodeSize?
@@ -278,7 +266,7 @@ export abstract class BaseAccountAPI { | |||
} | |||
return { | |||
...partialUserOp, | |||
preVerificationGas: await this.getPreVerificationGas(partialUserOp), | |||
preVerificationGas: 60000, |
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.
why is this a constant?
/** | ||
* The size of the dummy 'signature' parameter to be used during estimation. | ||
*/ | ||
readonly estimationSignatureSize: number |
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.
What I'm saying is that there is no "default" signature size: the wallet must fill in some value into the signature for the estimation to work, at the expected size, since no other component other than the wallet can "guess" a correct signature size.
No description provided.