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

Jump to bottom

Wallet improvements #180

Merged
merged 6 commits into from
Jun 10, 2024
Merged

Wallet improvements #180

merged 6 commits into from
Jun 10, 2024

Conversation

alejoacosta74
Copy link
Contributor

Improvements to Qi and Quai HD wallets:

  • Rename class to AbstractHDWallet
  • Semantic change in order of inputs to the addAddress method (account, addressIndex, zone)
  • Update validateZone method to use Enum zone byte
  • Replace "naked" address term with "gap"
  • Add the public modifier to all public methods
  • Remove the _parentPath variable in favor of calculating it in the parentPath

@alejoacosta74 alejoacosta74 marked this pull request as ready for review June 10, 2024 13:40
@alejoacosta74 alejoacosta74 linked an issue Jun 10, 2024 that may be closed by this pull request
[Quais] Improvements to quais HD wallets #179
Closed
rileystephens28
rileystephens28 reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@rileystephens28 rileystephens28 left a comment

Choose a reason for hiding this comment

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

Looks good. In addition to the few comments I left, there are some public methods in the AbstractHDWallet class missing explicit public modifier

src/wallet/hdwallet.ts Outdated
addrIndex++;
// put a hard limit on the number of addresses to derive
if (addrIndex - startingIndex > MAX_ADDRESS_DERIVATION_ATTEMPTS) {
throw new Error(`Failed to derive a valid address for the zone ${zone} after MAX_ADDRESS_DERIVATION_ATTEMPTS attempts.`);
Copy link
Member

Choose a reason for hiding this comment

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

Let's log the actual value of MAX_ADDRESS_DERIVATION_ATTEMPTS by wrapping in ${} block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/wallet/qi-hdwallet.ts Outdated
@@ -18,42 +21,40 @@ type OutpointInfo = {
account?: number;
};

export class QiHDWallet extends HDWallet {
export class QiHDWallet extends AbstractHDWallet {

protected static _GAP_LIMIT: number = 20;

protected static _cointype: number = 969;

protected static _parentPath = `m/44'/${this._cointype}'`;
Copy link
Member

Choose a reason for hiding this comment

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

Remove static _parentPath var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/wallet/qi-hdwallet.ts
const txobj = QiTransaction.from(<TransactionLike>tx);
if (!txobj.txInputs || txobj.txInputs.length == 0 || !txobj.txOutputs)
throw new Error('Invalid UTXO transaction, missing inputs or outputs');
public importOutpoints(outpoints: OutpointInfo[]): void {
Copy link
Member

Choose a reason for hiding this comment

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

Update zone string on OutpointInfo type and validate OutpointInfo[] values are valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/wallet/qi-hdwallet.ts
public async signTransaction(tx: QiTransactionRequest): Promise<string> {
const txobj = QiTransaction.from(<TransactionLike>tx);
if (!txobj.txInputs || txobj.txInputs.length == 0 || !txobj.txOutputs)
throw new Error('Invalid UTXO transaction, missing inputs or outputs');

const hash = keccak_256(txobj.unsignedSerialized);
Copy link
Member

Choose a reason for hiding this comment

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

Replace keccak_256 with keccak256 exported from crypto dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current keccak_256 is imported directly from @noble/hashes/sha3. Even though is the same algorithm used by the keccak function in crypto dir, the internnaly exported keccak256 has a different signature.
Let's address this suggestion on a different PR? The code needs a little refactoring, and mainly testing compatibility with the keccak256 from crypto dir.

@rileystephens28 rileystephens28 merged commit 5897a8c into dominant-strategies:master Jun 10, 2024
@rileystephens28 rileystephens28 mentioned this pull request Jun 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rileystephens28 rileystephens28 rileystephens28 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Quais] Improvements to quais HD wallets
2 participants
@alejoacosta74 @rileystephens28