Multi contract keystore

[amirsaran3]

Pre-flight checklist

• I have read the Contributing Guidelines on pull requests.
• Commit messages follow the conventional commits spec
• If this is a code change: I have written unit tests.
• If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
• If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

Related issues/PRs

[changeset-bot]

⚠️ No Changeset found

Latest commit: 474b673

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[denbite]

hey @amirsaran3
could you please clarify what are use cases that could benefit from this change?

[gtsonevv]

@andy-haynes This PR looks good, we'll just add a "getContracts" method and tests and move all changes to a separate PR. What do you think?

[andy-haynes]

What do you think?

Overall I’m not convinced this is a good fit for the repo. I saw the linked signInMulti Wallet Selector PR but I'm not quite understanding how this fits; I'd be interested to see an example of how this would be used in that PR.

First and foremost, it doubles down on the usage of plaintext key storage. This is not a practice we should endorse by providing new functionality for it. There are more secure approaches to key storage supported by major browsers, so we would need a pretty compelling reason to continue to support this approach.

Secondly, I don’t think it’s a good idea from an architectural perspective. It adds a new keystore abstract class just to modify the arity on some of the methods and support this specific implementation, which is largely duplicated from the current localstorage keystore. If this implementation exists solely to associate sets of accounts with a contract, it could be implemented in Wallet Selector as a one-off by mapping contracts to localstorage keystores instead of extending the keystore behavior, e.g.

class LocalStorageMultiContractKeyStore {
  contracts: { [contractId: string]: BrowserLocalStorageKeyStore } = [];
...
  setContractKeys(network: string, accountId: string, permissions: AccessKeyPermission[]) {
    for (let permission of permissions) {
      if (!this.contracts[permission.receiverId]) {
        this.contracts[permission.receiverId] = new BrowserLocalStorageKeyStore(localStorage, permission.receiverId);
      }

      this.contracts[permission.receiverId].setKey(network, accountId);
    }
  }
...
}

[gtsonevv]

Hey @andy-haynes, thank you for the feedback! This change will not be used directly in the wallet-selector, instead it should be applied by the wallets supported by the wallet-selector.
Related issue: #1153

[andy-haynes]

Thanks for the clarification and context @gtsonevv!

After thinking about this some more, I see two big impediments in the current implementation to supporting signInMulti:

• keystore cardinality, i.e. only supporting one key per account (and per contract in this PR)
• no awareness of the access key type

If we had a keystore capable of storing an arbitrary number of keys per account and had awareness of access key types, it would be trivial to implement signInMulti (storing multiple function call access keys) and allow options for signing with keys matching certain criteria (e.g. sign with function call access key for contract + method).

Do you think something like that could be a viable option for the original use case? Given the upcoming work, it's probably worth looking more critically at our keystore interfaces.

[gtsonevv]

Hey @andy-haynes, your suggestion sounds reasonable but it would require a general refactoring of the existing classes and would delay the release of this feature a lot I think. It will also introduce breaking changes that all wallets must adapt to. This can be added as an important thing to do in the future.

[andy-haynes]

@gtsonevv fair enough.

This PR looks good, we'll just add a "getContracts" method and tests and move all changes to a separate PR. What do you think?

If this is the entirety of the changes (e.g. we're not changing InMemorySigner to take MultiContractKeyStore) required to unblock signInMulti, then let's go ahead with your plan to fork from this and add the missing method.

[andy-haynes]

Merging into feature branch to retain history