fix: loading salt into buffer in the cli (#2467)

This PR is fixes #2090. The `--salt` option will now be checked to be a valid hex string before being loaded into a `Buffer`. The cli will now reject salts containing invalid characters (outside of `[0-9a-f]`) and in case a in case a string with an add number of characters is passed then it will be left padded with a zero in order to for it to be loaded into a Buffer. # Checklist: Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge. - [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag. - [x] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code. - [x] Every change is related to the PR description. - [x] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist).
AztecProtocol · Sep 25, 2023 · 753ac49 · 753ac49
1 parent 714c727
commit 753ac49
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 32 deletions.
diff --git a/yarn-project/cli/src/index.ts b/yarn-project/cli/src/index.ts
@@ -30,20 +30,20 @@ import {
   getAbiFunction,
   getContractAbi,
   getExampleContractArtifacts,
+  getSaltFromHexString,
   getTxSender,
   prepTx,
+  stripLeadingHex,
 } from './utils.js';
 
 const accountCreationSalt = Fr.ZERO;
 
-const stripLeadingHex = (hex: string) => {
-  if (hex.length > 2 && hex.startsWith('0x')) {
-    return hex.substring(2);
-  }
-  return hex;
-};
-
-const { ETHEREUM_HOST, AZTEC_RPC_HOST, PRIVATE_KEY, API_KEY } = process.env;
+const {
+  ETHEREUM_HOST = 'http://localhost:8545',
+  AZTEC_RPC_HOST = 'http://localhost:8080',
+  PRIVATE_KEY,
+  API_KEY,
+} = process.env;
 
 /**
  * Returns commander program that defines the CLI.
@@ -62,13 +62,13 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
   program
     .command('deploy-l1-contracts')
     .description('Deploys all necessary Ethereum contracts for Aztec.')
-    .option(
+    .requiredOption(
       '-u, --rpc-url <string>',
       'Url of the ethereum host. Chain identifiers localhost and testnet can be used',
-      ETHEREUM_HOST || 'http://localhost:8545',
+      ETHEREUM_HOST,
     )
     .option('-a, --api-key <string>', 'Api key for the ethereum host', API_KEY)
-    .option('-p, --private-key <string>', 'The private key to use for deployment', PRIVATE_KEY)
+    .requiredOption('-p, --private-key <string>', 'The private key to use for deployment', PRIVATE_KEY)
     .option(
       '-m, --mnemonic <string>',
       'The mnemonic to use in deployment',
@@ -129,7 +129,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
       'Private key for note encryption and transaction signing. Uses random by default.',
       PRIVATE_KEY,
     )
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async options => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const privateKey = options.privateKey
@@ -155,19 +155,23 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
       "A compiled Aztec.nr contract's ABI in JSON format or name of a contract ABI exported by @aztec/noir-contracts",
     )
     .option('-a, --args <constructorArgs...>', 'Contract constructor arguments', [])
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .option(
       '-k, --public-key <string>',
       'Optional encryption public key for this address. Set this value only if this contract is expected to receive private notes, which will be encrypted using this public key.',
     )
-    .option('-s, --salt <string>', 'Optional deployment salt as a hex string for generating the deployment address.')
+    .option(
+      '-s, --salt <hex string>',
+      'Optional deployment salt as a hex string for generating the deployment address.',
+      getSaltFromHexString,
+    )
     .action(async (abiPath, options: any) => {
       const contractAbi = await getContractAbi(abiPath, log);
       const constructorAbi = contractAbi.functions.find(({ name }) => name === 'constructor');
 
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const publicKey = options.publicKey ? Point.fromString(options.publicKey) : undefined;
-      const salt = options.salt ? Fr.fromBuffer(Buffer.from(stripLeadingHex(options.salt), 'hex')) : undefined;
+
       const deployer = new ContractDeployer(contractAbi, client, publicKey);
 
       const constructor = getAbiFunction(contractAbi, 'constructor');
@@ -176,7 +180,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
       debugLogger(`Input arguments: ${options.args.map((x: any) => `"${x}"`).join(', ')}`);
       const args = encodeArgs(options.args, constructorAbi!.parameters);
       debugLogger(`Encoded arguments: ${args.join(', ')}`);
-      const tx = deployer.deploy(...args).send({ contractAddressSalt: salt });
+      const tx = deployer.deploy(...args).send({ contractAddressSalt: options.salt });
       debugLogger(`Deploy tx sent with hash ${await tx.getTxHash()}`);
       const deployed = await tx.wait();
       log(`\nContract deployed at ${deployed.contractAddress!.toString()}\n`);
@@ -186,7 +190,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .command('check-deploy')
     .description('Checks if a contract is deployed to the specified Aztec address.')
     .requiredOption('-ca, --contract-address <address>', 'An Aztec address to check if contract has been deployed to.')
-    .option('-u, --rpc-url <url>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <url>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async options => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const address = AztecAddress.fromString(options.contractAddress);
@@ -199,7 +203,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .command('get-tx-receipt')
     .description('Gets the receipt for the specified transaction hash.')
     .argument('<txHash>', 'A transaction hash to get the receipt for.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (_txHash, options) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const txHash = TxHash.fromString(_txHash);
@@ -215,7 +219,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .command('get-contract-data')
     .description('Gets information about the Aztec contract deployed at the specified address.')
     .argument('<contractAddress>', 'Aztec address of the contract.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .option('-b, --include-bytecode <boolean>', "Include the contract's public function bytecode, if any.", false)
     .action(async (contractAddress, options) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
@@ -248,7 +252,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .description('Gets all the unencrypted logs from L2 blocks in the range specified.')
     .option('-f, --from <blockNum>', 'Initial block number for getting logs (defaults to 1).')
     .option('-l, --limit <blockCount>', 'How many blocks to fetch (defaults to 100).')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async options => {
       const { from, limit } = options;
       const fromBlock = from ? parseInt(from) : 1;
@@ -270,7 +274,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .requiredOption('-a, --address <aztecAddress>', "The account's Aztec address.")
     .requiredOption('-p, --public-key <publicKey>', 'The account public key.')
     .requiredOption('-pa, --partial-address <partialAddress', 'The partially computed address of the account contract.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async options => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const address = AztecAddress.fromString(options.address);
@@ -284,7 +288,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
   program
     .command('get-accounts')
     .description('Gets all the Aztec accounts stored in the Aztec RPC.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (options: any) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const accounts = await client.getRegisteredAccounts();
@@ -302,7 +306,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .command('get-account')
     .description('Gets an account given its Aztec address.')
     .argument('<address>', 'The Aztec address to get account for')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (_address, options) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const address = AztecAddress.fromString(_address);
@@ -318,7 +322,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
   program
     .command('get-recipients')
     .description('Gets all the recipients stored in the Aztec RPC.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (options: any) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const recipients = await client.getRecipients();
@@ -336,7 +340,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     .command('get-recipient')
     .description('Gets a recipient given its Aztec address.')
     .argument('<address>', 'The Aztec address to get recipient for')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (_address, options) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const address = AztecAddress.fromString(_address);
@@ -359,8 +363,8 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
       "A compiled Aztec.nr contract's ABI in JSON format or name of a contract ABI exported by @aztec/noir-contracts",
     )
     .requiredOption('-ca, --contract-address <address>', 'Aztec address of the contract.')
-    .option('-k, --private-key <string>', "The sender's private key.", PRIVATE_KEY)
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-k, --private-key <string>', "The sender's private key.", PRIVATE_KEY)
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
 
     .action(async (functionName, options) => {
       const { contractAddress, functionArgs, contractAbi } = await prepTx(
@@ -399,7 +403,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
     )
     .requiredOption('-ca, --contract-address <address>', 'Aztec address of the contract.')
     .option('-f, --from <string>', 'Aztec address of the caller. If empty, will use the first account from RPC.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (functionName, options) => {
       const { contractAddress, functionArgs, contractAbi } = await prepTx(
         options.contractAbi,
@@ -447,7 +451,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
   program
     .command('block-number')
     .description('Gets the current Aztec L2 block number.')
-    .option('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async (options: any) => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const num = await client.getBlockNumber();
@@ -478,7 +482,7 @@ export function getProgram(log: LogFn, debugLogger: DebugLogger): Command {
   program
     .command('get-node-info')
     .description('Gets the information of an aztec node at a URL.')
-    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST || 'http://localhost:8080')
+    .requiredOption('-u, --rpc-url <string>', 'URL of the Aztec RPC', AZTEC_RPC_HOST)
     .action(async options => {
       const client = await createCompatibleClient(options.rpcUrl, debugLogger);
       const info = await client.getNodeInfo();

diff --git a/yarn-project/cli/src/test/utils.test.ts b/yarn-project/cli/src/test/utils.test.ts
@@ -1,10 +1,11 @@
 import { AztecAddress, Fr } from '@aztec/aztec.js';
 import { AztecRPC, CompleteAddress } from '@aztec/types';
 
+import { InvalidArgumentError } from 'commander';
 import { MockProxy, mock } from 'jest-mock-extended';
 
 import { encodeArgs } from '../encoding.js';
-import { getTxSender } from '../utils.js';
+import { getSaltFromHexString, getTxSender, stripLeadingHex } from '../utils.js';
 import { mockContractAbi } from './mocks.js';
 
 describe('CLI Utils', () => {
@@ -128,4 +129,34 @@ describe('CLI Utils', () => {
       'Invalid value passed for integerParam. Could not parse foo as an integer.',
     );
   });
+
+  describe('stripLeadingHex', () => {
+    it.each([
+      ['0x1', '1'],
+      ['1', '1'],
+      ['0x00', '00'],
+      ['00', '00'],
+    ])('removes optional leading hex', (hex, expected) => {
+      expect(stripLeadingHex(hex)).toEqual(expected);
+    });
+  });
+
+  describe('getSaltFromHex', () => {
+    it.each([
+      ['0', Fr.ZERO],
+      ['0x0', Fr.ZERO],
+      ['00', Fr.ZERO],
+      ['1', new Fr(1)],
+      ['0x1', new Fr(1)],
+      ['0x01', new Fr(1)],
+      ['0xa', new Fr(0xa)],
+      ['fff', new Fr(0xfff)],
+    ])('correctly generates salt from a hex string', (hex, expected) => {
+      expect(getSaltFromHexString(hex)).toEqual(expected);
+    });
+
+    it.each(['foo', '', ' ', ' 0x1', '01foo', 'foo1', '0xfoo'])('throws an error for invalid hex strings', str => {
+      expect(() => getSaltFromHexString(str)).toThrow(InvalidArgumentError);
+    });
+  });
 });
diff --git a/yarn-project/cli/src/utils.ts b/yarn-project/cli/src/utils.ts
@@ -1,8 +1,9 @@
-import { AztecAddress, AztecRPC } from '@aztec/aztec.js';
+import { AztecAddress, AztecRPC, Fr } from '@aztec/aztec.js';
 import { createEthereumChain, deployL1Contracts } from '@aztec/ethereum';
 import { ContractAbi } from '@aztec/foundation/abi';
 import { DebugLogger, LogFn } from '@aztec/foundation/log';
 
+import { InvalidArgumentError } from 'commander';
 import fs from 'fs';
 import { mnemonicToAccount, privateKeyToAccount } from 'viem/accounts';
 
@@ -141,3 +142,36 @@ export async function prepTx(
 
   return { contractAddress, functionArgs, contractAbi };
 }
+
+/**
+ * Removes the leading 0x from a hex string. If no leading 0x is found the string is returned unchanged.
+ * @param hex - A hex string
+ * @returns A new string with leading 0x removed
+ */
+export const stripLeadingHex = (hex: string) => {
+  if (hex.length > 2 && hex.startsWith('0x')) {
+    return hex.substring(2);
+  }
+  return hex;
+};
+
+/**
+ * Parses a hex encoded string to an Fr integer to be used as salt
+ * @param str - Hex encoded string
+ * @returns A integer to be used as salt
+ */
+export function getSaltFromHexString(str: string): Fr {
+  const hex = stripLeadingHex(str);
+
+  // ensure it's a hex string
+  if (!hex.match(/^[0-9a-f]+$/i)) {
+    throw new InvalidArgumentError('Invalid hex string');
+  }
+
+  // pad it so that we may read it as a buffer.
+  // Buffer needs _exactly_ two hex characters per byte
+  const padded = hex.length % 2 === 1 ? '0' + hex : hex;
+
+  // finally, turn it into an integer
+  return Fr.fromBuffer(Buffer.from(padded, 'hex'));
+}