feat: add patch code from PR 23903 to keyringController.

[dawnseeker8]

This PR apply patch code from this PR to keyringController to fix the issue found in this issue

References

PR 23903
Fix Issue 23804

Changelog

@metamask/keyring-controlller

• Fixed: Fix creation of new QR keyrings
  • Previously the QR keyring would get initialized with invalid state when created for the first time

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

Thanks, the diff looks good! Had some questions about the changelog entry though.

It seems like this fixes a bug, but there is no description of what that bug is at the controller API level. The linked issue only shows the bug's impact upon the extension. Could you clarify, does this code prevent a crash? Or does the QR Keyring get instantiated correctly, but then crash later at runtime? Or does it get instantiated correctly and never crash, but fails to initialize?

We can omit the signature change to #newKeyring from the changelog; we typically only list public-facing changes, not internal changes.

[dawnseeker8]

Thanks, the diff looks good! Had some questions about the changelog entry though.

It seems like this fixes a bug, but there is no description of what that bug is at the controller API level. The linked issue only shows the bug's impact upon the extension. Could you clarify, does this code prevent a crash? Or does the QR Keyring get instantiated correctly, but then crash later at runtime? Or does it get instantiated correctly and never crash, but fails to initialize?

We can omit the signature change to #newKeyring from the changelog; we typically only list public-facing changes, not internal changes.

Hi, Mark, following is the bug detail from controller API level.

1. in the keyringController.#addQRKeyring() method, this function will be called when user want to import a QR account to metamask in mobile or extension app.

2. The #addQRKeyring() put a empty accounts array as extra argument to #newKeyring() method. this extra argument will be used in line 2004 in #newKeyring() method

      await keyring.deserialize(data);

3. if you look at the @keystonehq/base-eth-keyring BaseKeyring file, Their deserialize(opts) function

  deserialize(opts) {
    if (opts) {
      //common props;
      this.accounts = opts.accounts;
      this.currentAccount = opts.currentAccount;
      this.page = opts.page;
      this.perPage = opts.perPage;
      this.name = opts.name;
      this.initialized = opts.initialized;
      this.keyringMode = opts.keyringMode || KEYRING_MODE.hd;
      this.keyringAccount = opts.keyringAccount || KEYRING_ACCOUNT.standard;
      this.xfp = opts.xfp; //hd props;

      this.xpub = opts.xpub;
      this.hdPath = opts.hdPath;
      this.indexes = opts.indexes;
      this.paths = opts.paths;
      this.childrenPath = opts.childrenPath || DEFAULT_CHILDREN_PATH;
    }
  }

The empty accounts array will set the keystone QR keyring all the default properties to undefined except for accounts which will be empty array. this change will cause a lot of functions of keystone QR break due to default properties have been overrided by bug.

[Gudahtt]

OK, I have updated the PR description with this change entry:

• Fixed: Fix creation of new QR keyrings
  • Previously the QR keyring would get initialized with invalid state when created for the first time

Does that sound right? That was my interpretation of your explanation, considering that this entry is written for users of the package who don't understand the internals.

[dawnseeker8]

OK, I have updated the PR description with this change entry:

• Fixed: Fix creation of new QR keyrings

  • Previously the QR keyring would get initialized with invalid state when created for the first time

Does that sound right? That was my interpretation of your explanation, considering that this entry is written for users of the package who don't understand the internals.

yea, your explanation is better. :)

[Gudahtt]

Thanks for confirming that it was accurate!

[Gudahtt]

LGTM!