Add Create Account fromDerivePath #2073

daoauth · 2022-07-19T07:43:17Z

Description

Create Account with hdpath 637

Test Plan

test('generates derive path accounts', () => {
  const address = '0x07968dab936c1bad187c60ce4082f307d030d780e91e694ae03aef16aba73f30';
  const a1 = AptosAccount.fromDerivePath('m/44\'/637\'/0\'/0\'/0\'', mnemonic);
  expect(a1.address().hex()).toBe(address);
});

test('generates derive path accounts', () => {
  expect(() => { AptosAccount.fromDerivePath('', mnemonic); }).toThrow(new Error('Invalid derivation path'));
});

This change is

davidiw · 2022-07-20T16:22:19Z

Great stuff! Lemme get some typescript ninjas on this.

ecosystem/typescript/sdk/package.json

ecosystem/typescript/sdk/src/aptos_account.test.ts

ecosystem/typescript/sdk/package.json

jjleng · 2022-07-20T18:04:58Z

ecosystem/typescript/sdk/src/aptos_account.ts

+  static isValidPath = (path: string): boolean => {
+    if (
+      // eslint-disable-next-line prefer-regex-literals
+      !new RegExp('^m\\/44+\'\\/637+\'\\/[0-9]+\'\\/[0-9]+\'\\/[0-9]+\'+$').test(


This doesn't look right:

First of all, rebase and start using double quotes

Let's follow the BIP44 standard, https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#path-levels. The change and address_index do not follow by a '. In the link, a path looks like m / purpose' / coin_type' / account' / change / address_index.

The regex has issues. 44+ would match 44444 and 637+ would match 63777777. 44 and 637 should be an exact match. 44 is the current BIP number and 637 is Apto's coin type.

Because it uses the ed25519-hd-key module, the regex check uses a ' for all numbers. And other chains, such as near and solana are using a ' for all numbers.

ecosystem/typescript/sdk/src/aptos_account.ts

jjleng · 2022-07-20T18:09:36Z

ecosystem/typescript/sdk/src/aptos_account.ts

+      .some(isNaN as any);
+  };
+
+  static fromDerivePath(path: string, mnemonics: string): AptosAccount {


Also, check if mnemonics are empty and throw meaningful errors, just in case developers forgot to pass in the mnemonics.

You still need to check if the mnemonics are empty. Why should we allow that to happen? Throw an error in such case.

Add ts-doc for this method

ecosystem/typescript/sdk/src/types/bip39-light.d.ts

jjleng

Please squash your commits and make sure your final commit message follows the Angular Conventional Commits. This is to help the SDK tooling find your commit and add it to release notes.

For your final commit, the message should look like this:
feat: add BIP44 to allow the creation of multiple accounts with the same key

jjleng · 2022-07-21T17:06:25Z

ecosystem/typescript/sdk/src/aptos_account.ts

+    if (
+      // eslint-disable-next-line prefer-regex-literals
+      !new RegExp("^m\\/44'\\/637'\\/[0-9]+'\\/[0-9]+'\\/[0-9]+'+$").test(
+        path,
+      )


Why not one line?

I'll fix it

jjleng · 2022-07-21T17:08:12Z

ecosystem/typescript/sdk/src/aptos_account.ts

+  static isValidPath = (path: string): boolean => {
+    if (
+      // eslint-disable-next-line prefer-regex-literals
+      !new RegExp("^m\\/44'\\/637'\\/[0-9]+'\\/[0-9]+'\\/[0-9]+'+$").test(


You can either:
new RegExp("^m/44'/637'/[0-9]+'/[0-9]+'/[0-9]+'+$").test Or /^m\/44'\/637'\/[0-9]+'\/[0-9]+'\/[0-9]+'+$/.test

The \\ is not needed

I'll fix it

jjleng · 2022-07-21T17:11:23Z

ecosystem/typescript/sdk/src/aptos_account.ts

+      .some(isNaN as any);
+  };
+
+  static fromDerivePath(path: string, mnemonics: string): AptosAccount {


You still need to check if the mnemonics are empty. Why should we allow that to happen? Throw an error in such case.

jjleng · 2022-07-21T17:14:23Z

ecosystem/typescript/sdk/src/aptos_account.ts

@@ -30,6 +32,34 @@ export class AptosAccount {
    return new AptosAccount(HexString.ensure(obj.privateKeyHex).toUint8Array(), obj.address);
  }

+  static isValidPath = (path: string): boolean => {


Add @ts-doc for this method

jjleng · 2022-07-21T17:14:41Z

ecosystem/typescript/sdk/src/aptos_account.ts

+      .some(isNaN as any);
+  };
+
+  static fromDerivePath(path: string, mnemonics: string): AptosAccount {


Add ts-doc for this method

daoauth · 2022-08-10T09:04:12Z

@JJeng @scure/bip39

function normalize(str: string) {
  const norm = nfkd(str);
  const words = norm.split(' ');
  if (![12, 15, 18, 21, 24].includes(words.length)) throw new Error('Invalid mnemonic');
  return { nfkd: norm, words };
}

Length check of mnemonics is in the @scure/bip39 module.

jjleng

Thanks

jjleng · 2022-08-10T19:12:49Z

please rebase your PR

gregnazario · 2022-08-11T06:19:32Z

I think you need to rebase, there's a ton of commits here that are already in the codebase

github-actions · 2022-08-12T21:01:07Z

❌ Forge test failure on `24aec8cdf906c37ce9fd5f9f14c670b6057be803`

Forge test runner terminated

davidiw · 2022-08-23T05:36:49Z

please update, it seems like this is stale

daoauth · 2022-08-23T13:26:28Z

@davidiw update was done thanks.

ecosystem/typescript/sdk/package.json

https://github.com/satoshilabs/slips/pull/1364/files remove bip39-light Update aptos_account.ts Update aptos_account.ts Fix comma

github-actions · 2022-08-24T05:23:17Z

Forge is running with `ac434610e3b7c8f47be9c6991f201f7a47e3d637`

✅ Forge test success on `ac434610e3b7c8f47be9c6991f201f7a47e3d637`

performance benchmark with full nodes : 6944 TPS, 4297 ms latency, 7050 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `3f015828e9d64ecc32951c0ed63ea5e64012e78d`

❌ Forge test failure on `3f015828e9d64ecc32951c0ed63ea5e64012e78d`

Test Failed: Failed to mint accounts: API error Error(VmError): Invalid transaction: Type: Validation Code: SENDING_ACCOUNT_DOES_NOT_EXIST

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `cb62b285235461a63141c09f0c05e5e19d309188`

✅ Forge test success on `cb62b285235461a63141c09f0c05e5e19d309188`

performance benchmark with full nodes : 6596 TPS, 4523 ms latency, 6050 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

Forge is running with `e060b8dd9e54f4fe729059578d878681b54c2271`

✅ Forge test success on `e060b8dd9e54f4fe729059578d878681b54c2271`

performance benchmark with full nodes : 6582 TPS, 4547 ms latency, 6000 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
(Deprecated) OpenSearch Logs
Test runner output
Test run 1 is land-blocking
Triggered by pull_request_target

davidiw requested review from CapCap, jjleng and hariria and removed request for CapCap July 20, 2022 16:22

hariria reviewed Jul 20, 2022

View reviewed changes

ecosystem/typescript/sdk/package.json Outdated Show resolved Hide resolved

jjleng suggested changes Jul 20, 2022

View reviewed changes

daoauth requested review from hariria and jjleng July 21, 2022 14:54

jjleng reviewed Jul 21, 2022

View reviewed changes

jjleng mentioned this pull request Aug 1, 2022

[docs] Add an overview for system-integrators #2397

Closed

daoauth requested a review from jjleng August 10, 2022 15:01

jjleng approved these changes Aug 10, 2022

View reviewed changes

jjleng enabled auto-merge (squash) August 10, 2022 17:33

jjleng disabled auto-merge August 10, 2022 19:11

jjleng enabled auto-merge (rebase) August 10, 2022 19:12

auto-merge was automatically disabled August 11, 2022 05:58
Head branch was pushed to by a user without write access

daoauth requested review from msmouse, zekun000, JoshLind, a team, banool, gregnazario, alinush, davidiw and rajkaramchedu as code owners August 11, 2022 05:58

davidiw enabled auto-merge (squash) August 12, 2022 20:26

davidiw disabled auto-merge August 23, 2022 05:28

jjleng reviewed Aug 23, 2022

View reviewed changes

ecosystem/typescript/sdk/package.json Outdated Show resolved Hide resolved

jjleng enabled auto-merge (rebase) August 23, 2022 21:27

auto-merge was automatically disabled August 23, 2022 23:33
Rebase failed

jjleng requested review from movekevin and wrwg as code owners August 24, 2022 03:59

Add Create Account fromDerivePath

e060b8d

https://github.com/satoshilabs/slips/pull/1364/files remove bip39-light Update aptos_account.ts Update aptos_account.ts Fix comma

jjleng enabled auto-merge (rebase) August 24, 2022 04:42

jjleng merged commit a074e4c into aptos-labs:main Aug 24, 2022

stevenatcrypto mentioned this pull request Sep 11, 2022

[Feature Request][Rust SDK] Create account from derive path (as typescript SDK) #4089

Closed

stevenatcrypto mentioned this pull request Sep 20, 2022

[sdk] account recovery from derive path #4374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Create Account fromDerivePath #2073

Add Create Account fromDerivePath #2073

daoauth commented Jul 19, 2022 •

edited by geekflyer

Loading

davidiw commented Jul 20, 2022

jjleng Jul 20, 2022

daoauth Jul 21, 2022

jjleng Jul 20, 2022 •

edited

Loading

jjleng Jul 21, 2022

jjleng Jul 21, 2022

jjleng left a comment

jjleng Jul 21, 2022

daoauth Aug 10, 2022

jjleng Jul 21, 2022

jjleng Jul 21, 2022

daoauth Aug 10, 2022

jjleng Jul 21, 2022

jjleng Jul 21, 2022

jjleng Jul 21, 2022

daoauth commented Aug 10, 2022

jjleng left a comment

jjleng commented Aug 10, 2022

gregnazario commented Aug 11, 2022

github-actions bot commented Aug 12, 2022

davidiw commented Aug 23, 2022

daoauth commented Aug 23, 2022

github-actions bot commented Aug 24, 2022

Add Create Account fromDerivePath #2073

Add Create Account fromDerivePath #2073

Conversation

daoauth commented Jul 19, 2022 • edited by geekflyer Loading

Description

Test Plan

davidiw commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjleng Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjleng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daoauth commented Aug 10, 2022

jjleng left a comment

Choose a reason for hiding this comment

jjleng commented Aug 10, 2022

gregnazario commented Aug 11, 2022

github-actions bot commented Aug 12, 2022

❌ Forge test failure on 24aec8cdf906c37ce9fd5f9f14c670b6057be803

davidiw commented Aug 23, 2022

daoauth commented Aug 23, 2022

github-actions bot commented Aug 24, 2022

Forge is running with ac434610e3b7c8f47be9c6991f201f7a47e3d637

✅ Forge test success on ac434610e3b7c8f47be9c6991f201f7a47e3d637

Forge is running with 3f015828e9d64ecc32951c0ed63ea5e64012e78d

❌ Forge test failure on 3f015828e9d64ecc32951c0ed63ea5e64012e78d

Forge is running with cb62b285235461a63141c09f0c05e5e19d309188

✅ Forge test success on cb62b285235461a63141c09f0c05e5e19d309188

Forge is running with e060b8dd9e54f4fe729059578d878681b54c2271

✅ Forge test success on e060b8dd9e54f4fe729059578d878681b54c2271

daoauth commented Jul 19, 2022 •

edited by geekflyer

Loading

jjleng Jul 20, 2022 •

edited

Loading

❌ Forge test failure on `24aec8cdf906c37ce9fd5f9f14c670b6057be803`

Forge is running with `ac434610e3b7c8f47be9c6991f201f7a47e3d637`

✅ Forge test success on `ac434610e3b7c8f47be9c6991f201f7a47e3d637`

Forge is running with `3f015828e9d64ecc32951c0ed63ea5e64012e78d`

❌ Forge test failure on `3f015828e9d64ecc32951c0ed63ea5e64012e78d`

Forge is running with `cb62b285235461a63141c09f0c05e5e19d309188`

✅ Forge test success on `cb62b285235461a63141c09f0c05e5e19d309188`

Forge is running with `e060b8dd9e54f4fe729059578d878681b54c2271`

✅ Forge test success on `e060b8dd9e54f4fe729059578d878681b54c2271`