Skip to content

Commit

Permalink
Derivation Warnings (#389)
Browse files Browse the repository at this point in the history
### Description

Add warnings about the derivation path default changing in the future.
as per
https://forum.celo.org/t/deprecating-the-celo-derivation-path/9229


### Other changes

add derivationPath to the output

### Tested

yes new `account:new` tests

https://app.warp.dev/block/bYdy5uQ3WsAUT7rj1bT19l

### Related issues

Prework for #352  

### Backwards compatibility

yes unless you count the output of the command which we do not as it is
designed for humans to read.

### Documentation

improved!

<!-- start pr-codex -->

---

## PR-Codex overview
This PR focuses on updates to the `@celo/celocli` and
`@celo/cryptographic-utils` packages, including the addition of a
warning about the new default ETH derivation path, improvements to the
`NewAccount` command, and modifications to key generation functions.

### Detailed summary
- `@celo/celocli`: Allow `account:new` command to run without a node.
- Added warning about default ETH derivation path in future versions.
- Updated `generateKeys`, `generateSeed`, and related functions with
parameter documentation.
- Adjusted `NewAccount` command to reflect new derivation path logic.
- Updated test cases to verify new behavior with derivation paths.

> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your
question}`

<!-- end pr-codex -->
  • Loading branch information
aaronmgdr authored Oct 11, 2024
1 parent 576f902 commit 5a0a922
Show file tree
Hide file tree
Showing 8 changed files with 212 additions and 19 deletions.
6 changes: 6 additions & 0 deletions .changeset/fifty-roses-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@celo/cryptographic-utils': patch
'@celo/celocli': patch
---

Add warning that ETH derivation path will be the default in a future major breaking change.
5 changes: 5 additions & 0 deletions .changeset/polite-pets-push.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@celo/celocli': patch
---

Fix: account:new can now be called without a node
3 changes: 3 additions & 0 deletions docs/command-line-interface/account.md
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,9 @@ DESCRIPTION
command has been tested swapping mnemonics with the Ledger successfully (only supports
english)
WARN: In 7.0 the default derivation path will be Eth ("m/44'/60'/0'")
forum.celo.org/t/deprecating-the-celo-derivation-path/9229
EXAMPLES
new
Expand Down
10 changes: 5 additions & 5 deletions docs/sdk/cryptographic-utils/modules/account.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ base/lib/account.d.ts:18

#### Defined in

[cryptographic-utils/src/account.ts:455](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L455)
[cryptographic-utils/src/account.ts:468](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L468)

___

Expand Down Expand Up @@ -179,7 +179,7 @@ ___

#### Defined in

[cryptographic-utils/src/account.ts:403](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L403)
[cryptographic-utils/src/account.ts:410](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L410)

___

Expand All @@ -204,7 +204,7 @@ ___

#### Defined in

[cryptographic-utils/src/account.ts:390](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L390)
[cryptographic-utils/src/account.ts:397](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L397)

___

Expand Down Expand Up @@ -233,7 +233,7 @@ ___

#### Defined in

[cryptographic-utils/src/account.ts:431](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L431)
[cryptographic-utils/src/account.ts:444](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L444)

___

Expand Down Expand Up @@ -278,7 +278,7 @@ ___

#### Defined in

[cryptographic-utils/src/account.ts:416](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L416)
[cryptographic-utils/src/account.ts:423](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/cryptographic-utils/src/account.ts#L423)

___

Expand Down
145 changes: 145 additions & 0 deletions packages/cli/src/commands/account/new.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import { testWithAnvilL2 } from '@celo/dev-utils/lib/anvil-test'
import fs from 'node:fs'
import path from 'node:path'
import Web3 from 'web3'
import {
stripAnsiCodesAndTxHashes,
stripAnsiCodesFromNestedArray,
testLocallyWithWeb3Node,
} from '../../test-utils/cliUtils'
import NewAccount from './new'

process.env.NO_SYNCCHECK = 'true'

testWithAnvilL2('account:set-name cmd', (web3: Web3) => {
const writeMock = jest.spyOn(NewAccount.prototype, 'log')
const consoleMock = jest.spyOn(console, 'log')

beforeEach(() => {
writeMock.mockClear()
consoleMock.mockClear()
})
it('generates mnemonic and lets people know which derivation path is being used when called with no flags', async () => {
await testLocallyWithWeb3Node(NewAccount, [], web3)

expect(stripAnsiCodesFromNestedArray(writeMock.mock.calls)).toMatchInlineSnapshot(`
[
[
"
Using celo-legacy path (m/44'/52752'/0') for derivation. This will be switched to eth derivation path (m/44'/60'/0') next major version.
",
],
[
"
This is not being stored anywhere. Save the mnemonic somewhere to use this account at a later point.
",
],
]
`)

expect(deRandomize(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: *** ***
derivationPath: m/44'/52752'/0'
accountAddress: ADDRESS
privateKey: PUBLIC_KEY
publicKey: PRIVATE_KEY
address: ADDRESS"
`)
})
it("when called with --derivationPath eth flag generates mnemonic using m/44'/60'/0'", async () => {
await testLocallyWithWeb3Node(NewAccount, ['--derivationPath', 'eth'], web3)

expect(deRandomize(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: *** ***
derivationPath: m/44'/60'/0'
accountAddress: ADDRESS
privateKey: PUBLIC_KEY
publicKey: PRIVATE_KEY
address: ADDRESS"
`)
})

describe('when called with --mnemonicPath', () => {
const MNEMONIC_PATH = path.join(__dirname, 'public_mnemonic')
const TEST_mnemonic =
'hamster label near volume denial spawn stable orbit trade only crawl learn forest fire test feel bubble found angle also olympic obscure fork venue'
beforeEach(() => {
fs.writeFileSync(MNEMONIC_PATH, TEST_mnemonic, {
flag: 'w',
})
})
afterEach(async () => {
fs.rmSync(MNEMONIC_PATH)
})

it('generates using celo derivation path', async () => {
await testLocallyWithWeb3Node(NewAccount, [`--mnemonicPath`, MNEMONIC_PATH], web3)

expect(stripAnsiCodesAndTxHashes(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: hamster label near volume denial spawn stable orbit trade only crawl learn forest fire test feel bubble found angle also olympic obscure fork venue
derivationPath: m/44'/52752'/0'
accountAddress: 0x0a85BeCD036C86faD4Db5519634904be2021fb7d
privateKey: 6346d0cd7cfdb7904f08df48e442169d3333643de0351682f8b79cf714395471
publicKey: 02269b3efc9c4c6b81037d06e73e936078e625fb0f12b9ea1e0fd14d2cd45775f2
address: 0x0a85BeCD036C86faD4Db5519634904be2021fb7d"
`)
})

it("and --derivationPath m/44'/60'/0' generates using eth derivation path", async () => {
await testLocallyWithWeb3Node(
NewAccount,
[`--mnemonicPath`, MNEMONIC_PATH, '--derivationPath', "m/44'/60'/0'"],
web3
)

expect(stripAnsiCodesAndTxHashes(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: hamster label near volume denial spawn stable orbit trade only crawl learn forest fire test feel bubble found angle also olympic obscure fork venue
derivationPath: m/44'/60'/0'
accountAddress: 0x35A4d54B541fc7b2047fb5357cC706191E105cd3
privateKey: e4816cbb93346760921264ea38a7fc54903f4dd688ae0923fefd89a43c5f58cc
publicKey: 034b3036d657a6dc2f322db52cca29ae72101a9cf56de4765d17b0507ea1e87b7c
address: 0x35A4d54B541fc7b2047fb5357cC706191E105cd3"
`)
})
it("and --derivationPath m/44'/60'/0' and --changeIndex generates using eth derivation path", async () => {
await testLocallyWithWeb3Node(
NewAccount,
[`--mnemonicPath`, MNEMONIC_PATH, '--derivationPath', 'eth', '--changeIndex', '2'],
web3
)

expect(stripAnsiCodesAndTxHashes(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: hamster label near volume denial spawn stable orbit trade only crawl learn forest fire test feel bubble found angle also olympic obscure fork venue
derivationPath: m/44'/60'/0'
accountAddress: 0xb3492799c55141e0B3507302F241f1c34c08E1e2
privateKey: 3abc861ef3e9e31a6a7dc23e5903e41b3fe4a381d4fbb8f9db14e6730abd1c43
publicKey: 0280df4d09cf8ebcad418327287be3f4ba0054112544ffa290ab3cc0a87949b32a
address: 0xb3492799c55141e0B3507302F241f1c34c08E1e2"
`)
})
it('and --derivationPath eth and --addressIndex generates using eth derivation path', async () => {
await testLocallyWithWeb3Node(
NewAccount,
[`--mnemonicPath`, MNEMONIC_PATH, '--derivationPath', 'eth', '--addressIndex', '3'],
web3
)

expect(stripAnsiCodesAndTxHashes(consoleMock.mock.lastCall?.[0])).toMatchInlineSnapshot(`
"mnemonic: hamster label near volume denial spawn stable orbit trade only crawl learn forest fire test feel bubble found angle also olympic obscure fork venue
derivationPath: m/44'/60'/0'
accountAddress: 0x336E523118B6091e033F9715257e2E793002964c
privateKey: aa324b2efd0ebe6387c3bcf03387c78047d82a1d2e8b4e132e9e1b9fb93529d0
publicKey: 0394cc7cc524079c545aef2067c9ea7e69decb4815004afecf215ea1e6f370ce6c
address: 0x336E523118B6091e033F9715257e2E793002964c"
`)
})
})
})

function deRandomize(rawOutput: string) {
return stripAnsiCodesAndTxHashes(rawOutput)
.replace(/0x[A-Fa-f0-9]{40}/g, 'ADDRESS')
.replace(/ [A-Fa-f0-9]{66}/g, ' PRIVATE_KEY')
.replace(/ [A-Fa-f0-9]{64}/g, ' PUBLIC_KEY')
.replace(/mnemonic: ([a-z]+\s)+/, 'mnemonic: *** *** \n')
}
33 changes: 27 additions & 6 deletions packages/cli/src/commands/account/new.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
CELO_DERIVATION_PATH_BASE,
generateKeys,
generateMnemonic,
MnemonicLanguages,
Expand All @@ -9,6 +10,7 @@ import {
import { privateKeyToAddress } from '@celo/utils/lib/address'
import { toChecksumAddress } from '@ethereumjs/util'
import { Flags } from '@oclif/core'
import chalk from 'chalk'
import * as fs from 'fs-extra'
import { BaseCommand } from '../../base'
import { printValueMap } from '../../utils/cli'
Expand All @@ -17,7 +19,8 @@ const ETHEREUM_DERIVATION_PATH = "m/44'/60'/0'"

export default class NewAccount extends BaseCommand {
static description =
"Creates a new account locally using the Celo Derivation Path (m/44'/52752'/0/changeIndex/addressIndex) and print out the key information. Save this information for local transaction signing or import into a Celo node. Ledger: this command has been tested swapping mnemonics with the Ledger successfully (only supports english)"
"Creates a new account locally using the Celo Derivation Path (m/44'/52752'/0/changeIndex/addressIndex) and print out the key information. Save this information for local transaction signing or import into a Celo node. Ledger: this command has been tested swapping mnemonics with the Ledger successfully (only supports english)" +
"\n\nWARN: In 7.0 the default derivation path will be Eth (\"m/44'/60'/0'\") forum.celo.org/t/deprecating-the-celo-derivation-path/9229"

static flags = {
...BaseCommand.flags,
Expand Down Expand Up @@ -66,9 +69,13 @@ export default class NewAccount extends BaseCommand {
'new --passphrasePath some_folder/my_passphrase_file --mnemonicPath some_folder/my_mnemonic_file --addressIndex 5',
]

async init() {
// Dont call super class init because this command does not need to connect to a node
}

static languageOptions(language: string): MnemonicLanguages | undefined {
if (language) {
// @ts-ignore
// @ts-expect-error
const enumLanguage = MnemonicLanguages[language]
return enumLanguage as MnemonicLanguages
}
Expand All @@ -79,7 +86,7 @@ export default class NewAccount extends BaseCommand {
if (derivationPath) {
derivationPath = derivationPath.endsWith('/') ? derivationPath.slice(0, -1) : derivationPath
}
return derivationPath !== 'eth' ? derivationPath : ETHEREUM_DERIVATION_PATH
return derivationPath === 'eth' ? ETHEREUM_DERIVATION_PATH : derivationPath
}

static readFile(file?: string): string | undefined {
Expand Down Expand Up @@ -111,7 +118,9 @@ export default class NewAccount extends BaseCommand {
NewAccount.languageOptions(res.flags.language!)
)
}
const derivationPath = NewAccount.sanitizeDerivationPath(res.flags.derivationPath)
const derivationPath = NewAccount.sanitizeDerivationPath(
res.flags.derivationPath ?? CELO_DERIVATION_PATH_BASE
)
const passphrase = NewAccount.readFile(res.flags.passphrasePath)
const keys = await generateKeys(
mnemonic,
Expand All @@ -122,9 +131,21 @@ export default class NewAccount extends BaseCommand {
derivationPath
)
const accountAddress = toChecksumAddress(privateKeyToAddress(keys.privateKey))

if (derivationPath === CELO_DERIVATION_PATH_BASE) {
this.log(
chalk.magenta(
`\nUsing celo-legacy path (${CELO_DERIVATION_PATH_BASE}) for derivation. This will be switched to eth derivation path (${ETHEREUM_DERIVATION_PATH}) next major version.\n`
)
)
}

printValueMap({ mnemonic, derivationPath, accountAddress, ...keys })

this.log(
'This is not being stored anywhere. Save the mnemonic somewhere to use this account at a later point.\n'
chalk.green.bold(
'\nThis is not being stored anywhere. Save the mnemonic somewhere to use this account at a later point.\n'
)
)
printValueMap({ mnemonic, accountAddress, ...keys })
}
}
2 changes: 1 addition & 1 deletion packages/sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ for detail info see [#9127](https://github.com/celo-org/celo-monorepo/pull/9127)

- Removes phone and country related functions from utils. Now in [phone-utils](https://github.com/celo-org/celo-monorepo/pull/8987)

- comment encryption, bls and mneumonic functions moved to @celo/cryptographic-utils
- comment encryption, bls and mnemonic functions moved to @celo/cryptographic-utils


Features
Expand Down
27 changes: 20 additions & 7 deletions packages/sdk/cryptographic-utils/src/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export async function generateMnemonic(

export function validateMnemonic(
mnemonic: string,
bip39ToUse = bip39Wrapper,
bip39ToUse: Bip39 = bip39Wrapper,
language?: MnemonicLanguages
) {
if (language !== undefined) {
Expand Down Expand Up @@ -386,13 +386,20 @@ function wordSuggestions(typo: string, language: MnemonicLanguages): Suggestions
return map
}, new Map<number, string[]>())
}

/*
* @param mnemonic 12 or 12 BIP39 words
* @param password
* @param changeIndex postion 4 from https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
* @param addressIndex postion 5 from https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
* bip39ToUse - bip39 library
* @param derivationPath - This will default to ETH_DERIVATION_PATH in 7.0 forum.celo.org/t/deprecating-the-celo-derivation-path/9229
*/
export async function generateKeys(
mnemonic: string,
password?: string,
changeIndex: number = 0,
addressIndex: number = 0,
bip39ToUse = bip39Wrapper,
bip39ToUse: Bip39 = bip39Wrapper,
derivationPath: string = CELO_DERIVATION_PATH_BASE
): Promise<{ privateKey: string; publicKey: string; address: string }> {
const seed: Buffer = await generateSeed(mnemonic, password, bip39ToUse)
Expand All @@ -410,13 +417,13 @@ export function generateDeterministicInviteCode(
const seed = Buffer.from(keccak_256(utf8ToBytes(recipientPhoneHash + recipientPepper)))
return generateKeysFromSeed(seed, changeIndex, addressIndex, derivationPath)
}

// keyByteLength truncates the seed. *Avoid its use*
// It was added only because a backwards compatibility bug
/*
* @param keyByteLength truncates the seed. *Avoid its use* It was added only because a backwards compatibility bug
*/
export async function generateSeed(
mnemonic: string,
password?: string,
bip39ToUse = bip39Wrapper,
bip39ToUse: Bip39 = bip39Wrapper,
keyByteLength: number = 64
): Promise<Buffer> {
let seed = Buffer.from(await bip39ToUse.mnemonicToSeed(mnemonic, password))
Expand All @@ -428,6 +435,12 @@ export async function generateSeed(
return seed
}

/*
* @param seed - Buffer created from mnemonic
* @param changeIndex postion 4 from https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
* @param addressIndex postion 5 from https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
* @param derivationPath - This will default to ETH_DERIVATION_PATH in 7.0 forum.celo.org/t/deprecating-the-celo-derivation-path/9229
*/
export function generateKeysFromSeed(
seed: Buffer,
changeIndex: number = 0,
Expand Down

0 comments on commit 5a0a922

Please sign in to comment.