-
Notifications
You must be signed in to change notification settings - Fork 598
refactor: un-exposing PXE from e2e tests #17163
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,21 @@ | ||
| import type { NamespacedApiHandlers } from '@aztec/foundation/json-rpc/server'; | ||
| import type { LogFn } from '@aztec/foundation/log'; | ||
| import { | ||
| type CliPXEOptions, | ||
| type PXEService, | ||
| type PXEServiceConfig, | ||
| allPxeConfigMappings, | ||
| createPXEService, | ||
| } from '@aztec/pxe/server'; | ||
| import { type CliPXEOptions, type PXEServiceConfig, allPxeConfigMappings, createPXEService } from '@aztec/pxe/server'; | ||
| import { type AztecNode, PXESchema, createAztecNodeClient } from '@aztec/stdlib/interfaces/client'; | ||
| import { makeTracedFetch } from '@aztec/telemetry-client'; | ||
| import { TestWallet } from '@aztec/test-wallet'; | ||
|
|
||
| import { extractRelevantOptions } from '../util.js'; | ||
| import { getVersions } from '../versioning.js'; | ||
|
|
||
| export type { PXEServiceConfig, CliPXEOptions }; | ||
| export type { CliPXEOptions, PXEServiceConfig }; | ||
|
|
||
| export async function startPXE( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would drop "pxe" from the name here and just go for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| export async function startPXEServiceGetWallet( | ||
| options: any, | ||
| signalHandlers: (() => Promise<void>)[], | ||
| services: NamespacedApiHandlers, | ||
| userLog: LogFn, | ||
| ): Promise<{ pxe: PXEService; config: PXEServiceConfig & CliPXEOptions }> { | ||
| return await addPXE(options, signalHandlers, services, userLog, {}); | ||
| } | ||
|
|
||
| export async function addPXE( | ||
| options: any, | ||
| _signalHandlers: (() => Promise<void>)[], | ||
| services: NamespacedApiHandlers, | ||
| userLog: LogFn, | ||
| deps: { node?: AztecNode } = {}, | ||
| ): Promise<{ pxe: PXEService; config: PXEServiceConfig & CliPXEOptions }> { | ||
| ): Promise<{ wallet: TestWallet; config: PXEServiceConfig & CliPXEOptions }> { | ||
| const pxeConfig = extractRelevantOptions<PXEServiceConfig & CliPXEOptions>(options, allPxeConfigMappings, 'pxe'); | ||
| const nodeUrl = pxeConfig.nodeUrl; | ||
|
|
||
|
|
@@ -42,8 +27,10 @@ export async function addPXE( | |
| const node = deps.node ?? createAztecNodeClient(nodeUrl!, getVersions(pxeConfig), makeTracedFetch([1, 2, 3], true)); | ||
| const pxe = await createPXEService(node, pxeConfig as PXEServiceConfig); | ||
|
|
||
| const wallet = new TestWallet(pxe, node); | ||
|
|
||
| // Add PXE to services list | ||
| services.pxe = [pxe, PXESchema]; | ||
|
|
||
| return { pxe, config: pxeConfig }; | ||
| return { wallet, config: pxeConfig }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,18 +2,18 @@ import { Fr } from '@aztec/foundation/fields'; | |
| import { createLogger } from '@aztec/foundation/log'; | ||
| import type { AztecAddress } from '@aztec/stdlib/aztec-address'; | ||
| import { deriveStorageSlotInMap } from '@aztec/stdlib/hash'; | ||
| import type { AztecNode, PXE } from '@aztec/stdlib/interfaces/client'; | ||
| import type { Note } from '@aztec/stdlib/note'; | ||
| import type { AztecNode } from '@aztec/stdlib/interfaces/client'; | ||
| import type { Note, NotesFilter, UniqueNote } from '@aztec/stdlib/note'; | ||
|
|
||
| /** | ||
| * A class that provides utility functions for interacting with the aztec chain. | ||
| */ | ||
| export class AztecCheatCodes { | ||
| constructor( | ||
| /** | ||
| * The PXE Service to use for interacting with the chain | ||
| * The test wallet or pxe to use for getting notes | ||
| */ | ||
| public pxe: PXE, | ||
| public testWalletOrPxe: { getNotes(filter: NotesFilter): Promise<UniqueNote[]> }, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using TestWallet here would make this unusable for externals as it's likely their wallet will not expose the getNotes method. Just having the method defined in the type makes this PR a non-breaking change. Everything else should not be facing externals.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is unused, I would drop the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if this is unused as this can be used by externals. Or do you expect externals to use our testing wallet?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or their own. But this is false advertisement, I would deprecate it even if they're using it (because it uses the wrong mental model)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say this being on a cheatcode class makes it ok to have this method. It might also be useful when debugging typescript tests. We should just instruct devs to not use it in production (implement a utility function for that if needed). WDYT? |
||
| /** | ||
| * The Aztec Node to use for interacting with the chain | ||
| */ | ||
|
|
@@ -71,7 +71,7 @@ export class AztecCheatCodes { | |
| * @returns The notes stored at the given slot | ||
| */ | ||
| public async loadPrivate(recipient: AztecAddress, contract: AztecAddress, slot: Fr | bigint): Promise<Note[]> { | ||
| const extendedNotes = await this.pxe.getNotes({ | ||
| const extendedNotes = await this.testWalletOrPxe.getNotes({ | ||
| recipient, | ||
| contractAddress: contract, | ||
| storageSlot: new Fr(slot), | ||
|
|
||

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.
Needed to do this ugly hack because currently TestWallet doesn't correctly load the accounts that were registered in PXE before the Wallet was instantiated. This will be easy to tackle once Wallets instantiate PXE instead of having it passed in as an arg.
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 utility should just die.
InitialAccountDatacontains everything theTestWalltneeds to create the accounts. If they're not deployed, well, the consumer should do 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.
I think it makes sense to address this in step 2 of followup work as then only wallet will have access to pxe making it a necessity that this is dropped.