Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.

BREAKING: TS Refactor - #202

Merged
gantunesr merged 111 commits into
mainfrom
refactor-typescript
Mar 28, 2023
Merged

BREAKING: TS Refactor#202
gantunesr merged 111 commits into
mainfrom
refactor-typescript

Conversation

@gantunesr

@gantunesrgantunesr commented Mar 1, 2023

Copy link
Copy Markdown
Member

BREAKING: TS Migration

This PR introduces the following changes,

  • Refactors the KeyringController code and tests to TypeScript.
  • Add guards logic to ensure the correct logic flow.
  • Updates the package configuration
  • Updates error messages

Description

Itemize the changes you have made into the categories below

  • BREAKING:

    • The addNewKeyring method now expects an object containing the property privateKeys of type string[] in the case the keyring to be added is a "Simple Keyring".
  • FIXED:

    • Method getKeyringForAccount
      • Add async keyword for method returning a Promise
      • Update conditional statements
  • CHANGED:

    • Method createNewVaultAndRestore
      • Add guard logic in case keyring is undefined.
    • Method addNewKeyring
      • Add guard logic in case keyring is undefined.
      • Add guard logic in case the method generateRandomMnemonic doesn't exist in the current keyring
    • Method exportAccount
      • Add guard logic in case the method exportAccount doesn't exist in the current keyring
    • Method signTransaction
      • Add guard logic in case the method signTransaction doesn't exist in the current keyring
    • Method signMessage
      • Add guard logic in case the method signMessage doesn't exist in the current keyring
    • Method signPersonalMessage
      • Add guard logic in case the method signPersonalMessage doesn't exist in the current keyring
    • Method getEncryptionPublicKey
      • Add guard logic in case the method getEncryptionPublicKey doesn't exist in the current keyring
    • Method decryptMessage
      • Add guard logic in case the method decryptMessage doesn't exist in the current keyring
    • Method getAppKeyAddress
      • Add guard logic in case the method getAppKeyAddress doesn't exist in the current keyring
    • Method unlockKeyrings
      • Add guard logic in case the encryptionKey data type is not a string
      • Add guard logic in case the password data type is not a string
      • Logic updated to handle errors
    • Method getKeyringForAccount
      • Update conditional statements
    • Method restoreKeyring
      • Now is a private method
      • Logic updated to handle errors
    • Method clearKeyrings
      • Now is a private method
    • Method newKeyring
      • Now is a private method

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

Issue

Progresses #195

Comment threadsrc/KeyringController.ts Outdated

@mcmiremcmire left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did a second pass comparing the JS and TS implementations. Looks like there's still one comment from me and @Gudahtt that is outstanding, but other than that this all seems to make sense to me, so nice job. I just found a few more things other than that, mostly minor.

One thing I noticed is that some of the methods that were previously public (but placed in a private section) are now actually private. Is it worth noting these in the PR description?

Comment threadtypes/@metamask/eth-hd-keyring.d.ts
Comment threadsrc/KeyringController.ts Outdated
Comment threadsrc/KeyringController.ts Outdated
Comment threadsrc/KeyringController.ts
Comment threadsrc/KeyringController.ts
Comment threadsrc/KeyringController.ts Outdated
Comment threadtypes/@metamask/eth-sig-util.d.ts Outdated
@gantunesr

gantunesr commented Mar 23, 2023

Copy link
Copy Markdown
MemberAuthor

Hey @mcmire,

Thanks for the review, I wasn't able to dedicate time to find the best solution for the unresolved comments,

@gantunesr
gantunesr requested a review from mcmireMarch 23, 2023 15:51
@mcmire

mcmire commented Mar 24, 2023

Copy link
Copy Markdown
Contributor

@gantunesr Okay no problem! Thanks for trying. Maybe we can address the encryptor one at least in the future.

@socket-security

socket-securityBot commented Mar 24, 2023

Copy link
Copy Markdown

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

    Pull request alert summary
    IssueStatus
    Install scripts✅ 0 issues
    Native code✅ 0 issues
    Bin script shell injection✅ 0 issues
    Unresolved require✅ 0 issues
    Invalid package.json✅ 0 issues
    HTTP dependency✅ 0 issues
    Git dependency✅ 0 issues
    Potential typo squat✅ 0 issues
    Known Malware✅ 0 issues
    Telemetry✅ 0 issues
    Protestware/Troll package✅ 0 issues

    📊 Modified Dependency Overview:

    ➕ Added PackageCapability Access+/- Transitive CountPublisher
    @metamask/utils@5.0.0None+30metamaskbot
    @metamask/eslint-config-typescript@11.1.0None+5metamaskbot
    @ethereumjs/tx@4.1.1None+26holgerd77
    depcheck@1.4.3filesystem+30rumpl
    ts-node@10.9.1filesystem, shell, environment+15cspotcode
    @types/sinon@10.0.13None+1types
    ts-jest@29.0.5filesystem, environment+77kul
    eslint-plugin-jsdoc@40.1.0None+2gajus
    @types/jest@29.5.0None+14types
    @typescript-eslint/parser@5.57.0None+0jameshenry
    @typescript-eslint/eslint-plugin@5.57.0None+3jameshenry
    typedoc@0.23.28filesystem, shell, environment+9typedoc-bot
    ⬆️ Updated PackageVersion DiffCapability Access+/- Transitive CountPublisher
    jest@29.5.027.5.1...29.5.0None+73/-103simenb
    sinon@15.0.311.1.2...15.0.3None+2/-3fatso83

    🚮 Removed packages:@metamask/eslint-config-commonjs@11.1.0

    mcmire
    mcmire previously approved these changes Mar 24, 2023

    @mcmiremcmire left a comment

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    Noticed some more typecasts in the tests. We should also try to remove those in future PRs. I was curious about one of them, but it isn't blocking. LGTM.

    Comment threadsrc/KeyringController.test.ts Outdated
    @gantunesr
    gantunesr requested a review from mcmireMarch 27, 2023 21:55
    @gantunesrgantunesr added the team-accounts This should be handled by the Accounts Team label Mar 27, 2023
    Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

    Labels

    team-accountsThis should be handled by the Accounts Team

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    5 participants

    @gantunesr@leotm@mcmire@Gudahtt@legobeat