Skip to content
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.

Sign up for GitHub

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

Improve validation and helper methods on QiHDWallet #356

Merged
merged 3 commits into from
Nov 19, 2024
+228 −114
Merged

Improve validation and helper methods on QiHDWallet #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab41c93
implement helper methods to get change and address HD node
alejoacosta74 Nov 8, 2024
0a9a3d5
implement custom address validator and harden addAddress method
alejoacosta74 Nov 13, 2024
0c62e01
fix: remove duplicated code
alejoacosta74 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 66 additions & 6 deletions src/wallet/hdwallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,30 @@ export abstract class AbstractHDWallet<T extends NeuteredAddressInfo = NeuteredA
return isCorrectShard && isCorrectLedger;
}

/**
* Gets the BIP44 change node for a given account and change flag.
*
* @param {number} account - The account number.
* @param {boolean} change - Whether to get the change node.
* @returns {HDNodeWallet} The change node.
*/
protected _getChangeNode(account: number, change: boolean): HDNodeWallet {
const changeIndex = change ? 1 : 0;
return this._root.deriveChild(account + HARDENED_OFFSET).deriveChild(changeIndex);
}

/**
* Gets the BIP44 address node for a given account, change flag, and address index.
*
* @param {number} account - The account number.
* @param {boolean} change - Whether to get the change node.
* @param {number} addressIndex - The address index.
* @returns {HDNodeWallet} The address node.
*/
protected _getAddressNode(account: number, change: boolean, addressIndex: number): HDNodeWallet {
return this._getChangeNode(account, change).deriveChild(addressIndex);
}

/**
* Derives the next valid address node for a specified account, starting index, and zone. The method ensures the
* derived address belongs to the correct shard and ledger, as defined by the Quai blockchain specifications.
Expand All @@ -122,9 +146,7 @@ export abstract class AbstractHDWallet<T extends NeuteredAddressInfo = NeuteredA
zone: Zone,
isChange: boolean = false,
): HDNodeWallet {
const changeIndex = isChange ? 1 : 0;
const changeNode = this._root.deriveChild(account + HARDENED_OFFSET).deriveChild(changeIndex);

const changeNode = this._getChangeNode(account, isChange);
let addrIndex = startingIndex;
let addressNode: HDNodeWallet;

Expand Down Expand Up @@ -354,23 +376,41 @@ export abstract class AbstractHDWallet<T extends NeuteredAddressInfo = NeuteredA
throw new Error('deserialize method must be implemented in the subclass');
}

/**
* Validates an address info object, including basic properties and address derivation.
*
* @param {T} info - The address info to validate
* @throws {Error} If validation fails
* @protected
*/
protected validateAddressInfo(info: T): void {
// Basic property validation
this.validateBaseAddressInfo(info);

// Validate address derivation
this.validateAddressDerivation(info);

// Allow subclasses to add their own validation
this.validateExtendedProperties(info);
}

/**
* Validates the NeuteredAddressInfo object.
*
* @param {NeuteredAddressInfo} info - The NeuteredAddressInfo object to be validated.
* @throws {Error} If the NeuteredAddressInfo object is invalid.
* @protected
*/
protected validateNeuteredAddressInfo(info: NeuteredAddressInfo): void {
protected validateBaseAddressInfo(info: NeuteredAddressInfo): void {
if (!/^(0x)?[0-9a-fA-F]{40}$/.test(info.address)) {
throw new Error(
`Invalid NeuteredAddressInfo: address must be a 40-character hexadecimal string: ${info.address}`,
`Invalid NeuteredAddressInfo: address must be a 40-character hexadecimal string prefixed with 0x: ${info.address}`,
);
}

if (!/^0x[0-9a-fA-F]{66}$/.test(info.pubKey)) {
throw new Error(
`Invalid NeuteredAddressInfo: pubKey must be a 32-character hexadecimal string with 0x prefix: ${info.pubKey}`,
`Invalid NeuteredAddressInfo: pubKey must be a 66-character hexadecimal string prefixed with 0x: ${info.pubKey}`,
);
}

Expand All @@ -387,6 +427,26 @@ export abstract class AbstractHDWallet<T extends NeuteredAddressInfo = NeuteredA
}
}

/**
* Validates that the address, pubKey and zone match what would be derived from the other properties.
*
* @param {T} info - The address info to validate
* @throws {Error} If validation fails
* @protected
*/
protected abstract validateAddressDerivation(info: T): void;

/**
* Hook for subclasses to add their own validation logic. Base implementation does nothing.
*
* @param {T} _info - The address info to validate
* @protected
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected validateExtendedProperties(_info: T): void {
// Base implementation does nothing
}

/**
* Validates the version and coinType of the serialized wallet.
*
Expand Down
184 changes: 117 additions & 67 deletions src/wallet/qi-hdwallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ export interface QiAddressInfo extends NeuteredAddressInfo {
/**
* @extends SerializedHDWallet
* @property {OutpointInfo[]} outpoints - Array of outpoint information.
* @property {QiAddressInfo[]} changeAddresses - Array of change addresses.
* @property {QiAddressInfo[]} gapAddresses - Array of gap addresses.
* @property {QiAddressInfo[]} gapChangeAddresses - Array of gap change addresses.
* @property {QiAddressInfo[]} addresses - Array of Qi address information.
* @property {Record<string, QiAddressInfo[]>} senderPaymentCodeInfo - Map of payment code to array ofQi address
* information.
* @interface SerializedQiHDWallet
*/
export interface SerializedQiHDWallet extends SerializedHDWallet {
Expand Down Expand Up @@ -261,18 +261,28 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
this._addressesMap.set(QiHDWallet.PRIVATE_KEYS_PATH, privateKeysArray);
}

const newAddrInfo = {
pubKey: addressNode.publicKey,
return this._createAndStoreQiAddressInfo(addressNode, account, zone, isChange);
}

private _createAndStoreQiAddressInfo(
addressNode: HDNodeWallet,
account: number,
zone: Zone,
isChange: boolean,
): QiAddressInfo {
const qiAddressInfo: QiAddressInfo = {
address: addressNode.address,
pubKey: addressNode.publicKey,
account,
index: addressNode.index,
change: isChange,
zone,
status: AddressStatus.UNUSED,
derivationPath: isChange ? 'BIP44:change' : 'BIP44:external',
};
this._addressesMap.get(isChange ? 'BIP44:change' : 'BIP44:external')?.push(newAddrInfo);
return newAddrInfo;

this._addressesMap.get(isChange ? 'BIP44:change' : 'BIP44:external')!.push(qiAddressInfo); // _addressesMap is initialized within the constructor
return qiAddressInfo;
}

/**
Expand Down Expand Up @@ -951,11 +961,7 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {

if (addressInfo.derivationPath === 'BIP44:external' || addressInfo.derivationPath === 'BIP44:change') {
// (BIP44 addresses)
const changeIndex = addressInfo.change ? 1 : 0;
const addressNode = this._root
.deriveChild(addressInfo.account + HARDENED_OFFSET)
.deriveChild(changeIndex)
.deriveChild(addressInfo.index);
const addressNode = this._getAddressNode(addressInfo.account, addressInfo.change, addressInfo.index);
return addressNode.privateKey;
} else {
// (BIP47 addresses)
Expand Down Expand Up @@ -1290,42 +1296,18 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
const root = HDNodeWallet.fromMnemonic(mnemonic, path);
const wallet = new this(_guard, root);

const validateQiAddressInfo = (addressInfo: QiAddressInfo): void => {
wallet.validateNeuteredAddressInfo(addressInfo);

if (!Object.values(AddressStatus).includes(addressInfo.status)) {
throw new Error(`Invalid QiAddressInfo: status '${addressInfo.status}' is not a valid AddressStatus`);
}

if (
addressInfo.derivationPath !== 'BIP44:external' &&
addressInfo.derivationPath !== 'BIP44:change' &&
!validatePaymentCode(addressInfo.derivationPath)
) {
throw new Error(
`Invalid QiAddressInfo: derivationPath '${addressInfo.derivationPath}' is not valid. It should be 'BIP44:external', 'BIP44:change', or a valid BIP47 payment code`,
);
}
};

// First, group addresses by derivation path
const addressesByPath = new Map<string, QiAddressInfo[]>();
// validate and import all the wallet addresses
for (const addressInfo of serialized.addresses) {
validateQiAddressInfo(addressInfo);
wallet.validateAddressInfo(addressInfo);
let key = addressInfo.derivationPath;
if (isHexString(key, 32)) {
key = QiHDWallet.PRIVATE_KEYS_PATH;
}

if (!addressesByPath.has(key)) {
addressesByPath.set(key, []);
const existingAddresses = wallet._addressesMap.get(key);
if (existingAddresses && existingAddresses.some((addr) => addr.address === addressInfo.address)) {
throw new Error(`Address ${addressInfo.address} already exists in the wallet`);
}
addressesByPath.get(key)!.push(addressInfo);
}

// Then, set all paths in the wallet's address map
for (const [key, addresses] of addressesByPath) {
wallet._addressesMap.set(key, addresses);
wallet._addressesMap.get(key)!.push(addressInfo);
}

// validate and import the counter party payment code info
Expand All @@ -1334,7 +1316,7 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
throw new Error(`Invalid payment code: ${paymentCode}`);
}
for (const pcInfo of paymentCodeInfoArray) {
validateQiAddressInfo(pcInfo);
wallet.validateAddressInfo(pcInfo);
}
wallet._paymentCodeSendAddressMap.set(paymentCode, paymentCodeInfoArray);
}
Expand All @@ -1350,6 +1332,90 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
return wallet;
}

protected validateAddressDerivation(info: QiAddressInfo): void {
const addressNode = this._getAddressNode(info.account, info.change, info.index);

// Validate derived address matches
if (addressNode.address !== info.address) {
throw new Error(`Address mismatch: derived ${addressNode.address} but got ${info.address}`);
}

// Validate derived public key matches
if (addressNode.publicKey !== info.pubKey) {
throw new Error(`Public key mismatch: derived ${addressNode.publicKey} but got ${info.pubKey}`);
}

// Validate zone
const zone = getZoneForAddress(addressNode.address);
if (!zone || zone !== info.zone) {
throw new Error(`Zone mismatch: derived ${zone} but got ${info.zone}`);
}

// Validate it's a valid Qi address
if (!isQiAddress(addressNode.address)) {
throw new Error(`Address ${addressNode.address} is not a valid Qi address`);
}
}

protected validateExtendedProperties(info: QiAddressInfo): void {
// Validate status
if (!Object.values(AddressStatus).includes(info.status)) {
throw new Error(`Invalid status: ${info.status}`);
}

// Validate derivation path
if (typeof info.derivationPath !== 'string' || !info.derivationPath) {
throw new Error(`Invalid derivation path: ${info.derivationPath}`);
}

// Validate derivation path format
this.validateDerivationPath(info.derivationPath, info.change);
}

/**
* Validates that the derivation path is either a BIP44 path or a valid payment code.
*
* @private
* @param {string} path - The derivation path to validate
* @param {boolean} isChange - Whether this is a change address
* @throws {Error} If the path is invalid
*/
private validateDerivationPath(path: string, isChange: boolean): void {
// Check if it's a BIP44 path
if (path === 'BIP44:external' || path === 'BIP44:change') {
// Validate that the path matches the change flag
const expectedPath = isChange ? 'BIP44:change' : 'BIP44:external';
if (path !== expectedPath) {
throw new Error(
`BIP44 path mismatch: address marked as ${isChange ? 'change' : 'external'} ` +
`but has path ${path}`,
);
}
return;
}

// Check if it's a private key path
if (path === QiHDWallet.PRIVATE_KEYS_PATH) {
if (isChange) {
throw new Error('Imported private key addresses cannot be change addresses');
}
return;
}

// If not a BIP44 path or private key, must be a valid payment code
if (!validatePaymentCode(path)) {
throw new Error(
`Invalid derivation path: must be 'BIP44:external', 'BIP44:change', ` +
`'${QiHDWallet.PRIVATE_KEYS_PATH}', or a valid payment code. Got: ${path}`,
);
}

// Payment code addresses cannot be change addresses
if (isChange) {
throw new Error('Payment code addresses cannot be change addresses');
}
}

/**
* Validates an array of OutpointInfo objects. This method checks the validity of each OutpointInfo object by
* performing the following validations:
Expand Down Expand Up @@ -1654,6 +1720,9 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
* @returns {QiAddressInfo} The address info for the new address.
*/
public addAddress(account: number, addressIndex: number): QiAddressInfo {
if (account < 0 || addressIndex < 0) {
throw new Error('Account and address index must be non-negative integers');
}
return this._addAddress(account, addressIndex, false);
}

Expand All @@ -1665,6 +1734,9 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
* @returns {QiAddressInfo} The address info for the new address.
*/
public addChangeAddress(account: number, addressIndex: number): QiAddressInfo {
if (account < 0 || addressIndex < 0) {
throw new Error('Account and address index must be non-negative integers');
}
return this._addAddress(account, addressIndex, true);
}

Expand All @@ -1675,11 +1747,7 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
if (existingAddresses.some((info) => info.index === addressIndex)) {
throw new Error(`Address index ${addressIndex} already exists in wallet under path ${derivationPath}`);
}

const addressNode = this._root
.deriveChild(account + HARDENED_OFFSET)
.deriveChild(isChange ? 1 : 0)
.deriveChild(addressIndex);
const addressNode = this._getAddressNode(account, isChange, addressIndex);
const zone = getZoneForAddress(addressNode.address);
if (!zone) {
throw new Error(`Failed to derive a Qi valid address zone for the index ${addressIndex}`);
Expand All @@ -1689,25 +1757,7 @@ export class QiHDWallet extends AbstractHDWallet<QiAddressInfo> {
throw new Error(`Address ${addressNode.address} is not a valid Qi address`);
}

const addressInfo: QiAddressInfo = {
pubKey: addressNode.publicKey,
address: addressNode.address,
account,
index: addressIndex,
change: isChange,
zone,
status: AddressStatus.UNUSED,
derivationPath,
};

const addresses = this._addressesMap.get(derivationPath);
if (!addresses) {
this._addressesMap.set(derivationPath, [addressInfo]);
} else {
addresses.push(addressInfo);
}

return addressInfo;
return this._createAndStoreQiAddressInfo(addressNode, account, zone, isChange);
}

/**
Expand Down
Loading
Loading