From 3c1f54305d8ab806542f4c8716119f76c215c9a2 Mon Sep 17 00:00:00 2001 From: benesjan Date: Wed, 23 Aug 2023 09:25:21 +0000 Subject: [PATCH 1/3] feat: not retrying unrecoverable errors --- .../canary/src/uniswap_trade_on_l1_from_l2.test.ts | 2 +- .../end-to-end/src/e2e_aztec_js_browser.test.ts | 10 +++++----- yarn-project/end-to-end/src/fixtures/utils.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/yarn-project/canary/src/uniswap_trade_on_l1_from_l2.test.ts b/yarn-project/canary/src/uniswap_trade_on_l1_from_l2.test.ts index 350a9ca2551..62e05bad643 100644 --- a/yarn-project/canary/src/uniswap_trade_on_l1_from_l2.test.ts +++ b/yarn-project/canary/src/uniswap_trade_on_l1_from_l2.test.ts @@ -53,7 +53,7 @@ const ethRpcUrl = ETHEREUM_HOST; const hdAccount = mnemonicToAccount(MNEMONIC); const privateKey = new PrivateKey(Buffer.from(hdAccount.getHdKey().privateKey!)); -const aztecRpcClient = createAztecRpcClient(aztecRpcUrl, makeFetch([1, 2, 3], false)); +const aztecRpcClient = createAztecRpcClient(aztecRpcUrl, makeFetch([1, 2, 3], true)); let wallet: Wallet; /** diff --git a/yarn-project/end-to-end/src/e2e_aztec_js_browser.test.ts b/yarn-project/end-to-end/src/e2e_aztec_js_browser.test.ts index d2d684a2fd5..c9427ae15fd 100644 --- a/yarn-project/end-to-end/src/e2e_aztec_js_browser.test.ts +++ b/yarn-project/end-to-end/src/e2e_aztec_js_browser.test.ts @@ -57,7 +57,7 @@ conditionalDescribe()('e2e_aztec.js_browser', () => { let page: Page; beforeAll(async () => { - testClient = AztecJs.createAztecRpcClient(SANDBOX_URL!, AztecJs.makeFetch([1, 2, 3], false)); + testClient = AztecJs.createAztecRpcClient(SANDBOX_URL!, AztecJs.makeFetch([1, 2, 3], true)); await AztecJs.waitForSandbox(testClient); app = new Koa(); @@ -111,7 +111,7 @@ conditionalDescribe()('e2e_aztec.js_browser', () => { const result = await page.evaluate( async (rpcUrl, privateKeyString) => { const { PrivateKey, createAztecRpcClient, makeFetch, getUnsafeSchnorrAccount } = window.AztecJs; - const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], false)); + const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], true)); const privateKey = PrivateKey.fromString(privateKeyString); const account = getUnsafeSchnorrAccount(client, privateKey); await account.waitDeploy(); @@ -136,7 +136,7 @@ conditionalDescribe()('e2e_aztec.js_browser', () => { const result = await page.evaluate( async (rpcUrl, contractAddress, PrivateTokenContractAbi) => { const { Contract, AztecAddress, createAztecRpcClient, makeFetch } = window.AztecJs; - const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], false)); + const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], true)); const owner = (await client.getAccounts())[0].address; const wallet = await AztecJs.getSandboxAccountsWallet(client); const contract = await Contract.at(AztecAddress.fromString(contractAddress), PrivateTokenContractAbi, wallet); @@ -156,7 +156,7 @@ conditionalDescribe()('e2e_aztec.js_browser', () => { async (rpcUrl, contractAddress, transferAmount, PrivateTokenContractAbi) => { console.log(`Starting transfer tx`); const { AztecAddress, Contract, createAztecRpcClient, makeFetch } = window.AztecJs; - const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], false)); + const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], true)); const accounts = await client.getAccounts(); const owner = accounts[0].address; const receiver = accounts[1].address; @@ -178,7 +178,7 @@ conditionalDescribe()('e2e_aztec.js_browser', () => { const txHash = await page.evaluate( async (rpcUrl, privateKeyString, initialBalance, PrivateTokenContractAbi) => { const { PrivateKey, DeployMethod, createAztecRpcClient, makeFetch, getUnsafeSchnorrAccount } = window.AztecJs; - const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], false)); + const client = createAztecRpcClient(rpcUrl!, makeFetch([1, 2, 3], true)); let accounts = await client.getAccounts(); if (accounts.length === 0) { // This test needs an account for deployment. We create one in case there is none available in the RPC server. diff --git a/yarn-project/end-to-end/src/fixtures/utils.ts b/yarn-project/end-to-end/src/fixtures/utils.ts index 91c7b619282..27f04ae7ff6 100644 --- a/yarn-project/end-to-end/src/fixtures/utils.ts +++ b/yarn-project/end-to-end/src/fixtures/utils.ts @@ -80,7 +80,7 @@ const createRpcServer = async ( ): Promise => { if (SANDBOX_URL) { logger(`Creating JSON RPC client to remote host ${SANDBOX_URL}`); - const jsonClient = createJsonRpcClient(SANDBOX_URL, makeFetch([1, 2, 3], false)); + const jsonClient = createJsonRpcClient(SANDBOX_URL, makeFetch([1, 2, 3], true)); await waitForRPCServer(jsonClient, logger); logger('JSON RPC client connected to RPC Server'); return jsonClient; From 30a479bc5b694d2601b44cb382fd04b07d70c046 Mon Sep 17 00:00:00 2001 From: benesjan Date: Wed, 23 Aug 2023 09:54:44 +0000 Subject: [PATCH 2/3] refactor: not throwing when adding a recipient or account twice --- .../src/aztec_rpc_server/aztec_rpc_server.ts | 21 ++++++++++++---- .../test/aztec_rpc_test_suite.ts | 24 ++++++++++++------- .../aztec-rpc/src/database/database.ts | 6 +++-- .../aztec-rpc/src/database/memory_db.ts | 10 +++++--- .../types/src/interfaces/aztec_rpc.ts | 1 - 5 files changed, 42 insertions(+), 20 deletions(-) diff --git a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts index ff7a7afb824..d4fb2b540d0 100644 --- a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts +++ b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts @@ -92,10 +92,17 @@ export class AztecRPCServer implements AztecRPC { } public async registerAccount(privKey: PrivateKey, partialAddress: PartialAddress) { - const pubKey = this.keyStore.addAccount(privKey); const completeAddress = await CompleteAddress.fromPrivateKeyAndPartialAddress(privKey, partialAddress); - await this.db.addCompleteAddress(completeAddress); - this.synchroniser.addAccount(pubKey, this.keyStore); + const wasAdded = await this.db.addCompleteAddress(completeAddress); + if (wasAdded) { + const pubKey = this.keyStore.addAccount(privKey); + this.synchroniser.addAccount(pubKey, this.keyStore); + this.log.info(`Added account: ${completeAddress.toReadableString()}`); + } else { + this.log.info( + `Skipped adding account "${completeAddress.toReadableString()}" because he/she was registered before.`, + ); + } } public async getAccounts(): Promise { @@ -114,8 +121,12 @@ export class AztecRPCServer implements AztecRPC { } public async registerRecipient(recipient: CompleteAddress): Promise { - await this.db.addCompleteAddress(recipient); - this.log.info(`Added recipient: ${recipient.toString()}`); + const wasAdded = await this.db.addCompleteAddress(recipient); + if (wasAdded) { + this.log.info(`Added recipient: ${recipient.toReadableString()}`); + } else { + this.log.info(`Skipped adding recipient "${recipient.toReadableString()}" because he/she was registered before.`); + } } public async getRecipients(): Promise { diff --git a/yarn-project/aztec-rpc/src/aztec_rpc_server/test/aztec_rpc_test_suite.ts b/yarn-project/aztec-rpc/src/aztec_rpc_server/test/aztec_rpc_test_suite.ts index 1e238c9cad9..0747e1149ef 100644 --- a/yarn-project/aztec-rpc/src/aztec_rpc_server/test/aztec_rpc_test_suite.ts +++ b/yarn-project/aztec-rpc/src/aztec_rpc_server/test/aztec_rpc_test_suite.ts @@ -1,4 +1,4 @@ -import { AztecAddress, CompleteAddress, Fr, FunctionData, TxContext } from '@aztec/circuits.js'; +import { AztecAddress, CompleteAddress, Fr, FunctionData, Point, TxContext } from '@aztec/circuits.js'; import { Grumpkin } from '@aztec/circuits.js/barretenberg'; import { ConstantKeyPair } from '@aztec/key-store'; import { @@ -57,7 +57,7 @@ export const aztecRpcTestSuite = (testName: string, aztecRpcSetup: () => Promise expect(recipient).toEqual(completeAddress); }); - it('cannot register the same account twice', async () => { + it('does not throw when registering the same account twice (just ignores the second attempt)', async () => { const keyPair = ConstantKeyPair.random(await Grumpkin.new()); const completeAddress = await CompleteAddress.fromPrivateKeyAndPartialAddress( await keyPair.getPrivateKey(), @@ -65,18 +65,24 @@ export const aztecRpcTestSuite = (testName: string, aztecRpcSetup: () => Promise ); await rpc.registerAccount(await keyPair.getPrivateKey(), completeAddress.partialAddress); - await expect(async () => - rpc.registerAccount(await keyPair.getPrivateKey(), completeAddress.partialAddress), - ).rejects.toThrow(`Complete address corresponding to ${completeAddress.address} already exists`); + await rpc.registerAccount(await keyPair.getPrivateKey(), completeAddress.partialAddress); }); - it('cannot register the same recipient twice', async () => { + it('cannot register a recipient with the same aztec address but different pub key or partial address', async () => { + const recipient1 = await CompleteAddress.random(); + const recipient2 = new CompleteAddress(recipient1.address, Point.random(), Fr.random()); + + await rpc.registerRecipient(recipient1); + await expect(() => rpc.registerRecipient(recipient2)).rejects.toThrow( + `Complete address with aztec address ${recipient1.address}`, + ); + }); + + it('does not throw when registering the same recipient twice (just ignores the second attempt)', async () => { const completeAddress = await CompleteAddress.random(); await rpc.registerRecipient(completeAddress); - await expect(() => rpc.registerRecipient(completeAddress)).rejects.toThrow( - `Complete address corresponding to ${completeAddress.address} already exists`, - ); + await rpc.registerRecipient(completeAddress); }); it('successfully adds a contract', async () => { diff --git a/yarn-project/aztec-rpc/src/database/database.ts b/yarn-project/aztec-rpc/src/database/database.ts index d1cae3100a8..e03467c5c9d 100644 --- a/yarn-project/aztec-rpc/src/database/database.ts +++ b/yarn-project/aztec-rpc/src/database/database.ts @@ -127,9 +127,11 @@ export interface Database extends ContractDatabase { /** * Adds complete address to the database. * @param address - The complete address to add. - * @returns Empty promise. + * @returns A promise resolving to true if the address was added, false if it already exists. + * @throws If we try to add a CompleteAddress with the same AztecAddress but different public key or partial + * address. */ - addCompleteAddress(address: CompleteAddress): Promise; + addCompleteAddress(address: CompleteAddress): Promise; /** * Retrieves the complete address corresponding to the provided aztec address. diff --git a/yarn-project/aztec-rpc/src/database/memory_db.ts b/yarn-project/aztec-rpc/src/database/memory_db.ts index c6b213159de..551569a2a42 100644 --- a/yarn-project/aztec-rpc/src/database/memory_db.ts +++ b/yarn-project/aztec-rpc/src/database/memory_db.ts @@ -121,15 +121,19 @@ export class MemoryDB extends MemoryContractDatabase implements Database { }); } - public addCompleteAddress(completeAddress: CompleteAddress): Promise { + public addCompleteAddress(completeAddress: CompleteAddress): Promise { const accountIndex = this.addresses.findIndex(r => r.address.equals(completeAddress.address)); if (accountIndex !== -1) { + if (this.addresses[accountIndex].equals(completeAddress)) { + return Promise.resolve(false); + } + throw new Error( - `Complete address corresponding to ${completeAddress.address.toString()} already exists in memory database`, + `Complete address with aztec address ${completeAddress.address.toString()} but different public key or partial key already exists in memory database`, ); } this.addresses.push(completeAddress); - return Promise.resolve(); + return Promise.resolve(true); } public getCompleteAddress(address: AztecAddress): Promise { diff --git a/yarn-project/types/src/interfaces/aztec_rpc.ts b/yarn-project/types/src/interfaces/aztec_rpc.ts index 337f171f19a..199ffe75788 100644 --- a/yarn-project/types/src/interfaces/aztec_rpc.ts +++ b/yarn-project/types/src/interfaces/aztec_rpc.ts @@ -81,7 +81,6 @@ export interface AztecRPC { * This is because we don't have the associated private key and for this reason we can't decrypt * the recipient's notes. We can send notes to this account because we can encrypt them with the recipient's * public key. - * @throws If the recipient is already registered. */ registerRecipient(recipient: CompleteAddress): Promise; From e6f57c0eede40a691dc0cb2055b26df3ff06bb28 Mon Sep 17 00:00:00 2001 From: benesjan Date: Wed, 23 Aug 2023 13:31:07 +0000 Subject: [PATCH 3/3] refactor: better error codes --- .../aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts index d4fb2b540d0..b5aeb0053ca 100644 --- a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts +++ b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts @@ -99,9 +99,7 @@ export class AztecRPCServer implements AztecRPC { this.synchroniser.addAccount(pubKey, this.keyStore); this.log.info(`Added account: ${completeAddress.toReadableString()}`); } else { - this.log.info( - `Skipped adding account "${completeAddress.toReadableString()}" because he/she was registered before.`, - ); + this.log.info(`Account "${completeAddress.toReadableString()}" already registered.`); } } @@ -125,7 +123,7 @@ export class AztecRPCServer implements AztecRPC { if (wasAdded) { this.log.info(`Added recipient: ${recipient.toReadableString()}`); } else { - this.log.info(`Skipped adding recipient "${recipient.toReadableString()}" because he/she was registered before.`); + this.log.info(`Recipient "${recipient.toReadableString()}" already registered.`); } }