Strongly typed crypto proposal #294

hardsetting · 2024-02-16T11:01:28Z

Simplified and cleaned up #147

Notably, I rolled back renaming the Account class into Signer to avoid a breaking change and following some naming discussions.

Will add a better description in the AM.

0xmaayan

Very nice!

CHANGELOG updates is missing

tests/e2e/api/account.test.ts

tests/e2e/transaction/helper.ts

src/core/account/account.ts

src/core/account/singleKey.ts

src/core/crypto/multiEd25519.ts

src/core/crypto/multiKey.ts

src/transactions/transactionBuilder/transactionBuilder.ts

src/transactions/types.ts

examples/typescript/batch_mint.ts

examples/typescript/external_signing.ts

hariria

I think the naming of some of the types are a bit confusing. Some may have been there from before this PR, but it would be great to clarify them if possible.

src/core/account/account.ts

src/core/crypto/publicKey.ts

src/core/crypto/signature.ts

src/core/crypto/multiEd25519.ts

src/core/crypto/multiKey.ts

src/core/crypto/singleKey.ts

src/core/authenticationKey.ts

.eslintrc.js

0xmaayan · 2024-02-23T04:38:53Z

src/core/account/ed25519.ts

+ *
+ * Note: Generating a signer instance does not create the account on-chain.
+ */
+export class Ed25519Account implements Account {


this is representation of the "legacy" ed25519 right? I wonder if we should mention it and encourage using the new single key scheme

Honestly, the more I think of it, the more I think we should just always use the "legacy" ed25519 and possibly even discourage using the ed25519 variant of the single key authentication scheme.

We have to keep the legacy authentication scheme forever anyway. And if that's the case why should we over-complicate things and have two types of Ed25519 accounts around that can cause confusion and bugs.

Or maybe not? And maybe we can drop support in a year or two, and push users to rotate their authentication key.
And then we remove the code that handles the legacy authentication schemes..

I personally don't know what is the right call and I'm open to any of the solutions.
But the "laziest" for everyone would be what I proposed, where the only downside is to have to keep around the legacy authentication keys and authenticators.

I guess this is an implementation decision question and @davidiw can help with it.

I assume we eventually would want to support by default single key scheme, for future new authentication schemes also.

Also, if aptos-core supports both schemes, I think the sdk should probably support it also. We already need to support single key scheme for secp and probably some in the future so why not support all single key schemes?

src/core/crypto/singleKey.ts

src/core/account/singleKey.ts

src/core/crypto/singleKey.ts

src/types/index.ts

tests/e2e/transaction/transactionSubmission.test.ts

0xmaayan · 2024-02-23T05:09:43Z

src/core/account/account.ts

+ *
+ * Note: Generating an account instance does not create the account on-chain.
+ */
+export abstract class Account {


As part of the new wallet standard, we had the AccountInfo interface to hold an account of type Account

export interface AccountInfo = { account: Account, ansName?: string }

this was mostly done so it would be easier for us to resolve the account type (single signer/ multi signer) using the variant field - was this changed with this PR?

Also, where did AptosAccountVariant enum go?

I got some feedback regarding having both Signer and Account from @heliuchuan .
The argument is that it would be a breaking change, and the Signer name wouldn't be as intuitive.

So in this PR we only have Account and it has signing capabilities.
I think that for AccountInfo we can have the following struct, which would be functionally equivalent and similar to what we had before:

export interface AccountInfo = { address: AccountAddress, publicKey: AccountPublicKey, ansName?: string }

would also be nice to make it BCS serializable.

Let's update the standard AIP then. The AIP currently relies on that change we planned to have

0xmaayan · 2024-02-26T23:39:01Z

@hardsetting once it lands, we might need to update the docs on aptos.dev if there is anything specific to call out

hardsetting · 2024-02-27T00:57:05Z

once it lands, we might need to update the docs on aptos.dev if there is anything specific to call out

there's no breaking change at this point

hardsetting · 2024-02-27T00:58:10Z

Gathered all the feedback, and just rolled back more changes from the original PR.
The only notable change in this PR is now just splitting the monolithic Account class into abstract class + implementations

hariria

LGTM pending Oliver's feedback

0xmaayan

LGTM!

src/core/account/Account.ts

src/core/crypto/multiKey.ts

src/core/account/Account.ts

…ation

hardsetting requested a review from heliuchuan February 16, 2024 11:01

hardsetting self-assigned this Feb 16, 2024

hardsetting requested a review from a team as a code owner February 16, 2024 11:01

0xmaayan reviewed Feb 16, 2024

View reviewed changes

hardsetting force-pushed the gabriele/strongly-typed-crypto-account branch from 583b038 to f8c3caf Compare February 16, 2024 19:48

heliuchuan reviewed Feb 19, 2024

View reviewed changes

examples/typescript/batch_mint.ts Outdated Show resolved Hide resolved

heliuchuan reviewed Feb 19, 2024

View reviewed changes

examples/typescript/external_signing.ts Show resolved Hide resolved

hardsetting force-pushed the gabriele/strongly-typed-crypto-account branch 6 times, most recently from ad77ec3 to ededd97 Compare February 22, 2024 19:20

hariria reviewed Feb 22, 2024

View reviewed changes

hardsetting mentioned this pull request Feb 22, 2024

Strongly typed crypto proposal #147

Closed

0xmaayan reviewed Feb 23, 2024

View reviewed changes

[project] Updated .eslintrc.js to allow for..of

7ee1df2

hardsetting force-pushed the gabriele/strongly-typed-crypto-account branch 2 times, most recently from 9639916 to 46bff83 Compare February 26, 2024 22:34

hardsetting force-pushed the gabriele/strongly-typed-crypto-account branch 2 times, most recently from b9828a4 to 117c3c7 Compare February 27, 2024 00:56

hariria approved these changes Feb 27, 2024

View reviewed changes

0xmaayan approved these changes Feb 27, 2024

View reviewed changes

heliuchuan approved these changes Feb 27, 2024

View reviewed changes

src/core/account/Account.ts Show resolved Hide resolved

src/core/account/Account.ts Show resolved Hide resolved

src/core/account/Account.ts Show resolved Hide resolved

src/core/crypto/multiKey.ts Outdated Show resolved Hide resolved

src/core/account/Account.ts Show resolved Hide resolved

hardsetting added 3 commits February 27, 2024 13:33

[core] crypto revamp

8b9b1cd

[core] Added unit tests for MultiEd25519PublicKey signature verific…

d774e81

…ation

[core] Split Account into separate files

51f4043

hardsetting added 3 commits February 27, 2024 13:33

Fixed imports

d240a0f

Fixed build issues

8cf03c3

Updated changelog

bb8d2e7

hardsetting force-pushed the gabriele/strongly-typed-crypto-account branch from 117c3c7 to bb8d2e7 Compare February 27, 2024 21:36

heliuchuan enabled auto-merge (squash) February 27, 2024 21:39

0xaptosj mentioned this pull request Feb 27, 2024

Bump version to 1.9.0 #313

Merged

heliuchuan merged commit 9bf8d28 into main Feb 27, 2024
12 checks passed

heliuchuan deleted the gabriele/strongly-typed-crypto-account branch February 27, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strongly typed crypto proposal #294

Strongly typed crypto proposal #294

hardsetting commented Feb 16, 2024

0xmaayan left a comment

hariria left a comment

0xmaayan Feb 23, 2024

hardsetting Feb 26, 2024 •

edited

Loading

0xmaayan Feb 26, 2024

0xmaayan Feb 23, 2024

0xmaayan Feb 23, 2024

hardsetting Feb 23, 2024

0xmaayan Feb 23, 2024

0xmaayan commented Feb 26, 2024 •

edited

Loading

hardsetting commented Feb 27, 2024

hardsetting commented Feb 27, 2024

hariria left a comment

0xmaayan left a comment

Strongly typed crypto proposal #294

Strongly typed crypto proposal #294

Conversation

hardsetting commented Feb 16, 2024

0xmaayan left a comment

Choose a reason for hiding this comment

hariria left a comment

Choose a reason for hiding this comment

0xmaayan Feb 23, 2024

Choose a reason for hiding this comment

hardsetting Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

0xmaayan Feb 26, 2024

Choose a reason for hiding this comment

0xmaayan Feb 23, 2024

Choose a reason for hiding this comment

0xmaayan Feb 23, 2024

Choose a reason for hiding this comment

hardsetting Feb 23, 2024

Choose a reason for hiding this comment

0xmaayan Feb 23, 2024

Choose a reason for hiding this comment

0xmaayan commented Feb 26, 2024 • edited Loading

hardsetting commented Feb 27, 2024

hardsetting commented Feb 27, 2024

hariria left a comment

Choose a reason for hiding this comment

0xmaayan left a comment

Choose a reason for hiding this comment

hardsetting Feb 26, 2024 •

edited

Loading

0xmaayan commented Feb 26, 2024 •

edited

Loading