Skip to content

refactor!: dropping PXE JSON RPC#17228

Merged
benesjan merged 1 commit intonextfrom
09-23-refactor_dropping_pxe_json_rpc
Sep 24, 2025
Merged

refactor!: dropping PXE JSON RPC#17228
benesjan merged 1 commit intonextfrom
09-23-refactor_dropping_pxe_json_rpc

Conversation

@benesjan
Copy link
Contributor

@benesjan benesjan commented Sep 23, 2025

PXE is becoming just a lib used by the wallet and for this reason I am dropping the corresponding JSON RPC Server.

This leads to a large refactor of basically all the setups that dealt with PXE and wallets.

This is a followup work of this PR in which I had this tasks planned:

  1. Drop PXE JSON RPC server,
  2. have Wallet instantiate PXE instead of having it passed in as an arg to a constructor,
  3. revisit the PXE methods exposed on the TestWallet.

In this PR I tackled points 1 and 2.

@benesjan benesjan changed the title refactor: dropping PXE JSON RPC refactor!: dropping PXE JSON RPC Sep 23, 2025
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan marked this pull request as ready for review September 23, 2025 12:29
Comment on lines +195 to +205
const ContractMetadataSchema = z.object({
contractInstance: z.union([ContractInstanceWithAddressSchema, z.undefined()]),
isContractInitialized: z.boolean(),
isContractPublished: z.boolean(),
}) satisfies ZodFor<ContractMetadata>;

const ContractClassMetadataSchema = z.object({
contractClass: z.union([ContractClassWithIdSchema, z.undefined()]),
isContractClassPubliclyRegistered: z.boolean(),
artifact: z.union([ContractArtifactSchema, z.undefined()]),
}) satisfies ZodFor<ContractClassMetadata>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved here from PXE as the PXESchema got dropped.

program = injectAztecNodeCommands(program, userLog, debugLogger);
program = injectMiscCommands(program, userLog);
program = injectDevnetCommands(program, userLog, debugLogger);
program = injectWalletCommands(program, userLog, debugLogger);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with Grego in DMs that it makes sense to drop the wallet commands from aztec command. Having it on aztec was just a tech debt.

@benesjan benesjan force-pushed the 09-23-refactor_dropping_pxe_json_rpc branch 2 times, most recently from 1ac248a to 7b5ec83 Compare September 23, 2025 17:11
@benesjan benesjan marked this pull request as draft September 24, 2025 05:46
@benesjan benesjan force-pushed the 09-23-refactor_dropping_pxe_json_rpc branch 2 times, most recently from a87ae53 to 545d3f3 Compare September 24, 2025 06:40
@benesjan benesjan marked this pull request as ready for review September 24, 2025 06:40
* @param testWalletOrPxe - Test wallet or pxe instance to use to get the registered accounts.
* @returns A set of key data for each of the initial accounts.
*/
export async function getDeployedTestAccounts(testWalletOrPxe: { getPxe(): PXE } | PXE): Promise<InitialAccountData[]> {
Copy link
Contributor Author

@benesjan benesjan Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced this with a registerInitialSandboxAccountsInWallet util that resides in the end-to-end package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd be fine with that utility living in the test-wallet package (people will connect the testwallet to sandbox)

Comment on lines +227 to +244
// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `checkTx` cli-wallet command.
getContracts(): Promise<AztecAddress[]> {
return this.pxe.getContracts();
}

// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `checkTx` cli-wallet command.
getNotes(filter: NotesFilter): Promise<UniqueNote[]> {
return this.pxe.getNotes(filter);
}

// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `bridge_fee_juice` cli-wallet command.
// TODO: This should most likely just be refactored to getProtocolContractAddresses.
getPXEInfo(): Promise<PXEInfo> {
return this.pxe.getPXEInfo();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about these commands? Needed to expose them as we had the corresponding commands exposed on the CLI (getNotes was required by inspectTx and it prints out notes emitted in the tx).

As a dev being able to print out contracts and notes when debugging seems pretty useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense in the CLIWallet, since it's just what it does. They do not belong in the public interface though.

@@ -1,2 +1,32 @@
# Aztec wallet Documentation

### Run tests locally
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grego sent me this through DMs so I made AI generate readme from it.

}
}

export async function inspectTx(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the unpruned version to the cli-wallet. It became less useful here without the private info.

import { AztecAddress, type DeployOptions, Fr, type Wallet, publishContractClass } from '@aztec/aztec.js';
import type { SponsoredFPCContract } from '@aztec/noir-contracts.js/SponsoredFPC';
import type { TestWallet } from '@aztec/test-wallet';
import type { TestWallet } from '@aztec/test-wallet/server';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some e2e tests I started getting this error:

Image

so I just decided to have all the e2e tests use the newly introduced server version of TestWallet.

// should be set so that the only way for rollups to be built
// is if the txs are successfully gossiped around the nodes.
const contexts: NodeContext[] = [];
const txsSentViaDifferentNodes: SentTx[][] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in the tests that follow I dropped the NodeContext type:

export interface NodeContext {
  node: AztecNodeService;
  pxeService: PXEService;
  txs: SentTx[];
}

because only the txs from it were ever used.


import type { PXEServiceConfig } from '../config/index.js';
import { PXEService } from '../pxe_service/pxe_service.js';
import { pxeTestSuite } from './pxe_test_suite.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file I just merged the pxe_test_suite to it and moved the pxe service creation to the beforeAll block.

We had it done like this before because the pxe_test_suite was imported in e2e package.

@@ -1,422 +0,0 @@
import { L1_TO_L2_MSG_TREE_HEIGHT } from '@aztec/constants';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just testing the PXESchema and that schema no longer exists.

@benesjan benesjan requested a review from Thunkar September 24, 2025 14:26



{{- define "aztec-network.pxeUrl" -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very scared of this, ping someone from the networks team!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already let Mitch know about this 👍

Apparently all of this is anyway getting completely refactored soon so seems to be a good time to do this PR.

* @param testWalletOrPxe - Test wallet or pxe instance to use to get the registered accounts.
* @returns A set of key data for each of the initial accounts.
*/
export async function getDeployedTestAccounts(testWalletOrPxe: { getPxe(): PXE } | PXE): Promise<InitialAccountData[]> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd be fine with that utility living in the test-wallet package (people will connect the testwallet to sandbox)

Comment on lines +227 to +244
// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `checkTx` cli-wallet command.
getContracts(): Promise<AztecAddress[]> {
return this.pxe.getContracts();
}

// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `checkTx` cli-wallet command.
getNotes(filter: NotesFilter): Promise<UniqueNote[]> {
return this.pxe.getNotes(filter);
}

// Recently added when having the wallet instantiate PXE as PXE is now hidden. This forced me to expose this when
// refactoring `bridge_fee_juice` cli-wallet command.
// TODO: This should most likely just be refactored to getProtocolContractAddresses.
getPXEInfo(): Promise<PXEInfo> {
return this.pxe.getPXEInfo();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense in the CLIWallet, since it's just what it does. They do not belong in the public interface though.

salt: new Fr(SPONSORED_FPC_SALT),
});
await pxe.registerContract({ instance: sponsoredFPCInstance, artifact: SponsoredFPCContract.artifact });
await wallet.registerContract({ instance: sponsoredFPCInstance, artifact: SponsoredFPCContract.artifact });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useless for a short lived command, since the contract is just registered privately on a dead wallet. It might be useful to just print the address though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Addressed in 4a2b131

The sponsored fpc flag on the cli command was useless as we did not ever deploy the FPC there.

@benesjan benesjan force-pushed the 09-23-refactor_dropping_pxe_json_rpc branch from b317da3 to 4a2b131 Compare September 24, 2025 15:23
@benesjan benesjan requested a review from Thunkar September 24, 2025 15:26
@benesjan benesjan enabled auto-merge September 24, 2025 15:31
PXE is becoming just a lib used by the wallet and for this reason I am dropping the corresponding JSON RPC Server.

This leads to a large refactor of basically all the setups that dealt with PXE and wallets.

This is a followup work of [this PR](#17163) in which I had this tasks planned:
1. Drop PXE JSON RPC server,
2. have Wallet instantiate PXE instead of having it passed in as an arg to a constructor,
3. revisit the PXE methods exposed on the TestWallet.

In this PR I tackled points 1 and 2.
@AztecBot AztecBot force-pushed the 09-23-refactor_dropping_pxe_json_rpc branch from 4a2b131 to b9e53bf Compare September 24, 2025 15:57
@benesjan benesjan added this pull request to the merge queue Sep 24, 2025
Merged via the queue into next with commit ec201bf Sep 24, 2025
15 checks passed
@benesjan benesjan deleted the 09-23-refactor_dropping_pxe_json_rpc branch September 24, 2025 16:47
AztecBot pushed a commit that referenced this pull request Sep 26, 2025
Followup work of #17228

In this PR I drop `getPXEInfo` and `getContracts` from `TestWallet` + add random docs improvements.

`getPXEInfo` was just a tech debt as it was only used to check if node has the same protocol contract addresses which is quite non-sensical because those addresses are no literally 1, 2, 3, 4, 5... I assume this check is there from time when they didn't have these hardcoded addresses.

`getContracts` was used just in one test and the check there was not really that important so I just dropped it.
github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2025
Followup work of
#17228

In this PR I drop `getPXEInfo` and `getContracts` from `TestWallet` +
add random docs improvements.

`getPXEInfo` was just a tech debt as it was only used to check if node
has the same protocol contract addresses which is quite non-sensical
because those addresses are no literally 1, 2, 3, 4, 5... I assume this
check is there from time when they didn't have these hardcoded
addresses.

`getContracts` was used just in one test and the check there was not
really that important so I just dropped it.
mralj pushed a commit that referenced this pull request Oct 13, 2025
Followup work of #17228

In this PR I drop `getPXEInfo` and `getContracts` from `TestWallet` + add random docs improvements.

`getPXEInfo` was just a tech debt as it was only used to check if node has the same protocol contract addresses which is quite non-sensical because those addresses are no literally 1, 2, 3, 4, 5... I assume this check is there from time when they didn't have these hardcoded addresses.

`getContracts` was used just in one test and the check there was not really that important so I just dropped it.
ludamad pushed a commit that referenced this pull request Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants