-
Notifications
You must be signed in to change notification settings - Fork 224
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-474 Migrate to native tracer #233
Conversation
@@ -26,16 +27,17 @@ type LogTracerFunc = () => LogTracer | |||
* @param provider the network node to trace on | |||
* @param tx the transaction to trace | |||
* @param options the trace options | |||
* @param nativeTracerProvider if set, submit only preStateTracer to the network provider, and use this (second) provider with native tracer. | |||
* @param prestateTracerProvider if set, submit only preStateTracer to the network provider, and use this (second) provider with native tracer. |
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.
The comment is not very clear or correct.
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 is going to be removed completely
let traceOptions: TraceOptions | ||
if (prestateTracerProvider != null) { | ||
traceOptions = tracer2string(options) | ||
// there is a prestateTracerProvider: use it for the native tracer, but first we need preStateTracer from the main provider: |
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 is getting weird.
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.
Dror added the prestate tracer usage. It's an interim solution until the native tracer is out. What is getting weird?
} | ||
requireCond(isStaked(entStake), | ||
failureMessage, ValidationErrors.OpcodeValidation, { [entityTitle]: entStakes?.addr }) | ||
// TODO: check real minimum stake values |
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.
New TODO?
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.
No, it was there
// console.log('validation res', res) | ||
// todo fix | ||
this.convertTracerResult(tracerResult, userOp) | ||
// console.log('tracer res') | ||
// console.dir(tracerResult, { depth: null }) |
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.
Clean up
@@ -327,4 +333,147 @@ export class ValidationManager implements IValidationManager { | |||
async getOperationHash (userOp: OperationBase): Promise<string> { | |||
return await this.entryPoint.getUserOpHash(packUserOp(userOp as UserOperation)) | |||
} | |||
|
|||
// todo fix rest of the code to work with the new tracer result instead of adjusting it here |
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.
New TODO?
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.
Well yes. Now, that all tests pass and the changes were already made to the tracer in geth, and to the parser here, it would be easier to add the remaining changes to the parser here instead of converting the return type. Not sure we need it right now though...
|
||
// todo fix rest of the code to work with the new tracer result instead of adjusting it here | ||
convertTracerResult (tracerResult: any, userOp: UserOperation): BundlerTracerResult { | ||
const SENDER_CREATOR = '0xefc2c1444ebcc4db75e7613d20c6a62ff67a167c'.toLowerCase() |
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.
SENDER_CREATOR
really should be configurable, there may be sub-versions of EntryPoint and rebuilding the bundler for that will become a pain.
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.
Yes I'll move it to config
// TODO: This is a hardcoded address of SenderCreator immutable member in EntryPoint. Any change in EntryPoint's code | ||
// requires a change of this address. | ||
// TODO check why the filter fails test_ban_user_op_access_other_ops_sender_in_bundle | ||
tracerResult.callsFromEntryPoint = tracerResult.calls // .filter((call: { from: string }) => call.from.toLowerCase() === this.entryPoint.address.toLowerCase() || call.from.toLowerCase() === SENDER_CREATOR) | ||
|
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 is no longer the case in with eth-infinitism/account-abstraction#514
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.
Well this PR is working against the current version of AA. Once we introduce the next PRs for v0.8, we'll update the bundler as well.
}, []) | ||
} | ||
|
||
getOpcodeName (opcodeNumber: number): string | 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.
Is there no library for that?
No description provided.