[SDK] sdk v2 account classes #10005

0xmigo · 2023-09-11T23:04:33Z

Description

Previous stack (Crypto classs): #9889

Account class updated to the following

Instance Method

sign
verifySignature

Getter

publicKey
privateKey
accountAddress

Static Creator Methods

create
fromPrivateKey
fromPrivateKeyAndAddress
fromDerivationPath

Static Helpers

isValidPath
authKey

Test Plan

PASS tests/unit/hex.test.ts
PASS tests/unit/deserializer.test.ts
PASS tests/unit/serializer.test.ts
PASS tests/unit/account_address.test.ts
PASS tests/unit/aptos_config.test.ts
PASS tests/e2e/api/account.test.ts
----------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files | 0 | 0 | 0 | 0 |
----------|---------|----------|---------|---------|-------------------

Test Suites: 6 passed, 6 total
Tests: 106 passed, 106 total
Snapshots: 0 total
Time: 1.748 s, estimated 34 s

0xmigo · 2023-09-11T23:06:55Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+    return authKey.data;
+  }
+
+  sign(data: Uint8Array): Hex {


I was hesitating whether I want this data input to be Uint8Array or HexInput

0xmigo · 2023-09-11T23:10:57Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+   * @returns Hex - public key of the account
+   */
+  get publicKey(): Hex {
+    return new Hex({ data: this._signingKey.publicKey });


I think using obj for argument for a single input param seems odd. Personally I would love to see single input param without obj

yes we all mentioned it at some point... decided to keep it as is and later, when we actually test the sdk_v2 and use it we can decide how it is from a dev user perspective

0xmigo · 2023-09-15T04:05:39Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+  static fromPrivateKey(privateKey: HexInput): Account {
+    const privatekeyHex = Hex.fromHexInput({ hexInput: privateKey });


I see there are many places where we have to build the Hex from HexInput. I wonder if we can just accept the Hex type as argument?

Hex is not a type but a Class. The whole idea is to use HexInput and then transform it to the desired format, this enables the greatest flexibility for the developer.

Beyond that, please make this take in an object as the argument. I know it's weird but we agreed we'd try this lol, let's see how it goes.

Yep gotcha!

banool

Looking good! I'd love to see examples in all of the doc comments on the public constructor methods.

banool · 2023-09-19T14:03:00Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+// SPDX-License-Identifier: Apache-2.0
+
+import nacl from "tweetnacl";
+import * as bip39 from "@scure/bip39";


Any reason we don't just import bip39 and do bip39.blah? I feel like it'd be more readable, I'd rather we avoid import * from x.

banool · 2023-09-19T14:03:17Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+import { HexInput } from "../types";
+
+/**
+ * Class for creating and managing account on Aptos network


Suggested change

* Class for creating and managing account on Aptos network

* Class for creating and managing an account on Aptos network

banool · 2023-09-19T14:04:11Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+import { HexInput } from "../types";
+
+/**
+ * Class for creating and managing account on Aptos network


Can you clarify that creating an instance of this account doesn't create an instance of the account on the network. Some more examples in the doc comment here would be nice, e.g. how to use this to create an account on the network, getting account address, etc.

banool · 2023-09-19T14:04:27Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+ */
+export class Account {
+  /**
+   * signing key of the account, which holds the public and private key


Suggested change

* signing key of the account, which holds the public and private key

* Signing key of the account, which holds the public and private key

banool · 2023-09-19T14:05:03Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+  private readonly _signingKey: nacl.SignKeyPair;
+
+  /**
+   * Account address associated with the account


Maybe some more information and a link to the docs site explaining why this is necessary and can't just be derived from the private / public keys (because of rotation).

banool · 2023-09-19T14:06:25Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+   *
+   * @returns Hex - private key of the account
+   */
+  get privateKey(): Hex {


Personally I think getters suck, I prefer just a good old function. Do we have guidelines on this Maayan?

Yeah I am thinking of using getter here b/c that private and public key came from _signingKey. Not sure if we want to make the signingKey as public. Any thoughts?

banool · 2023-09-19T14:07:20Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+  static fromPrivateKey(privateKey: HexInput): Account {
+    const privatekeyHex = Hex.fromHexInput({ hexInput: privateKey });


Beyond that, please make this take in an object as the argument. I know it's weird but we agreed we'd try this lol, let's see how it goes.

banool · 2023-09-19T14:07:53Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+   * Creates new account with provided private key and address
+   * This is intended to be used for account that has it's key rotated


Suggested change

* Creates new account with provided private key and address

* This is intended to be used for account that has it's key rotated

* Creates a new account with the provided private key and address.

* This is intended to be used for an account that has had it's key rotated.

banool · 2023-09-19T14:08:15Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+   * Creates new account with bip44 path and mnemonics,
+   * @param path. (e.g. m/44'/637'/0'/0'/0')


Suggested change

* Creates new account with bip44 path and mnemonics,

* @param path. (e.g. m/44'/637'/0'/0'/0')

* Creates new account with bip44 path and mnemonics.

*

* @param path. (e.g. m/44'/637'/0'/0'/0')

banool · 2023-09-19T14:08:43Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+   * Creates new account with bip44 path and mnemonics,
+   * @param path. (e.g. m/44'/637'/0'/0'/0')
+   * Detailed description: {@link https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki}
+   * @param mnemonics.


More information about the mnemonics argument would be very helpful. If you could include examples in the doc comment that'd be great too.

banool

Make sure to use objects as parameters across the board.

banool · 2023-09-19T14:09:46Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+  }
+
+  /**
+   * Check's if the derive path is valid


Suggested change

* Check's if the derive path is valid

* Checks if the derive path is valid.

banool · 2023-09-19T14:10:17Z

ecosystem/typescript/sdk_v2/src/core/account.ts

+    return authKey.data;
+  }
+
+  sign(data: HexInput): Hex {


Doc comments on this and veryifySignature please captain!

banool

There seems to be a lot of duplication with #9889. If this is a stack, could you stack them properly?

0xmigo · 2023-09-19T20:56:06Z

There seems to be a lot of duplication with #9889. If this is a stack, could you stack them properly?

Ah yes, this is actually the top stack of #9889 . I will do a rebase again once I fix all from comments on #9889

0xmigo · 2023-09-25T16:52:59Z

This is an old PR. for the new ones, please refer to #10210

0xmigo requested review from gregnazario, banool, 0xmaayan, xbtmatt and a team September 11, 2023 23:04

0xmigo commented Sep 11, 2023

View reviewed changes

Jin added 6 commits September 14, 2023 11:18

Migrate crypto classes over to sdk v2

cc371c7

remove Seq<T>

d9d7aca

Fix from comment and added authentication_key.test.ts

2a3fa07

add ed25519 test and multi ed25519

853ab46

update unit tests

83a7ecb

Add account class for sdk v2

109d06b

0xmigo force-pushed the jin/sdk_v2_account_classes branch from c0bf162 to 109d06b Compare September 14, 2023 22:45

add account unit test

dbf2e51

0xmigo commented Sep 15, 2023

View reviewed changes

0xmaayan mentioned this pull request Sep 18, 2023

[SDKv2] Add transaction fetch queries #10084

Closed

banool reviewed Sep 19, 2023

View reviewed changes

banool mentioned this pull request Sep 19, 2023

[SDKv2] Migrate crypto classes over to sdk v2 #9889

Closed

0xmigo added the Do not merge label Sep 22, 2023

0xmigo closed this Sep 25, 2023

0xmigo deleted the jin/sdk_v2_account_classes branch October 2, 2023 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] sdk v2 account classes #10005

[SDK] sdk v2 account classes #10005

0xmigo commented Sep 11, 2023 •

edited

Loading

0xmigo Sep 11, 2023

0xmigo Sep 11, 2023

0xmaayan Sep 15, 2023

0xmigo Sep 15, 2023

0xmaayan Sep 15, 2023

banool Sep 19, 2023

0xmigo Sep 19, 2023

banool left a comment

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

0xmigo Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool Sep 19, 2023

banool left a comment

banool Sep 19, 2023

banool Sep 19, 2023

banool left a comment

0xmigo commented Sep 19, 2023

0xmigo commented Sep 25, 2023

		static fromPrivateKey(privateKey: HexInput): Account {
		const privatekeyHex = Hex.fromHexInput({ hexInput: privateKey });

	* Class for creating and managing account on Aptos network
	* Class for creating and managing an account on Aptos network

	* signing key of the account, which holds the public and private key
	* Signing key of the account, which holds the public and private key

		* Creates new account with provided private key and address
		* This is intended to be used for account that has it's key rotated

		* Creates new account with bip44 path and mnemonics,
		* @param path. (e.g. m/44'/637'/0'/0'/0')

	* Check's if the derive path is valid
	* Checks if the derive path is valid.

[SDK] sdk v2 account classes #10005

[SDK] sdk v2 account classes #10005

Conversation

0xmigo commented Sep 11, 2023 • edited Loading

Description

Test Plan

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

banool 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banool left a comment

Choose a reason for hiding this comment

0xmigo commented Sep 19, 2023

0xmigo commented Sep 25, 2023

0xmigo commented Sep 11, 2023 •

edited

Loading