-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feature/eip 7702 #1262
base: v2
Are you sure you want to change the base?
Feature/eip 7702 #1262
Conversation
… and verifyMessage
…7702 main.ts implementation
…n into feature/eip-7702
…which doesn't; a function to check whether the user has authrorized a network
…ate; bettet detection for 7702
this.#storage.get('accounts', []), | ||
this.#storage.get('accountPreferences', undefined) | ||
this.#storage.get('accountPreferences', undefined), | ||
this.#storage.get('signedMessages', {}) |
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.
signedMessages
should be read from ActivityController
instead of directly accessing localStorage.
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.
However, I've seen that we need #authorizations only in the case we call #updateAccountStates()
. If so, I would suggest to simply get it from ActivityController, in a similar way we do it here:
const entryPointAuthorizationMessageFromHistory = await this.activity.findMessage(
account.addr,
(message) =>
message.fromActionId === ENTRY_POINT_AUTHORIZATION_REQUEST_ID &&
message.networkId === meta.networkId
)
Doing this, there is not need to read the signed messages from localStorage and keeping #authorizations
variable up to date.
@@ -43,6 +49,9 @@ export class AccountsController extends EventEmitter { | |||
// Holds the initial load promise, so that one can wait until it completes | |||
initialLoadPromise: Promise<void> | |||
|
|||
// all SignedMessage type 7702-authorization the user has signed | |||
#authorizations: InternalSignedMessages |
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 above 7702 comment is great, but the first time I saw #authorizations in the code, I thought we stored all types of auth messages there.
* toggling, A/B testing, and gradual feature roll-outs. | ||
*/ | ||
export class FeatureFlagsController extends EventEmitter { | ||
flags = { ...featureFlags } |
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 guess this should be private, as we have a setter function and the same time we can mutate its value outside of the class
…ifferent paymaster API route endpoints because the new route is not live
src/controllers/main/main.ts
Outdated
actionsQueue: this.actions.actionsQueue | ||
}) | ||
|
||
this.actions.addOrUpdateAction(accountOpAction, 'first') |
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.
- BA -> add new txn on Odyssey
- The same BA -> add new txn on Odyssey
- Sign 7702 auth message.
- The first txn is removed and replaced with a new AccOp that contains the both transactions.
- But, the second transaction is still part of the Actions queue.
I would suggest in that case, if we sign an 7702 auth message, first to stack the existing txns into an acc op (as we did it), but the same time to remove all txns those are on the network we authorized.
) | ||
) | ||
) { | ||
// check if entry point auth is already visible |
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.
minor, I guess you reffer to 7702, not entry point
@@ -131,6 +132,9 @@ export class NetworksController extends EventEmitter { | |||
) | |||
if (!predefinedNetwork) { | |||
this.#networks[networkName].predefined = false | |||
|
|||
if (this.#networks[networkName].chainId === 911867n) |
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.
we could add the network as const here: src/legends/constants/networks.ts
? // https://eips.ethereum.org/EIPS/eip-6492 | ||
(wrapCounterfactualSign(signature, account.creation) as Hex) | ||
: (signature as Hex) | ||
} |
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.
6492 code is duplicated here and into getVerifyMessageSignature, we could reuse it
return concat([sig.r, sig.s, v]) as Hex | ||
} | ||
|
||
export function getSavedSignature( |
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 does 'saved' means here, as I could not get the context
…uthorization reject/sign
Everything necessary to enable eip-7702