Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add KeyringRequest.origin #273

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Apr 2, 2025

TODO.

@ccharly ccharly changed the title feat: add KeyringRequest.origin feat!: add KeyringRequest.origin Apr 2, 2025
@ccharly ccharly marked this pull request as ready for review April 4, 2025 16:11
@ccharly ccharly requested a review from a team as a code owner April 4, 2025 16:11
@@ -1123,6 +1135,7 @@ export class SnapKeyring extends EventEmitter {
});

const signedTx = await this.#submitRequest({
origin: 'metamask',
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we are setting the origin to metamask here regardless of the real origin.

Do you have an idea how we are going to send the real origin? Also, will this new field break the existing Snaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 🤔 I wrongly assumed we were using those methods when interacting through the MetaMask UI, but this will also be called externally with call like eth_sign*, in which case, the true origin would not be metamask.

And yes, this new field might also break existing Snaps. I don't know else I could trick that. My only idea right now would be to "version the API", and have a runtime check on the version, something like:

getVersion() {
  const { version } = await this.#snapClient
    .withSnapId(snapId)
    .getVersion();
    
  if (!version) {
    return ...; // First version when we introduced the new `getVersion`?
  }
}

But even know, we cannot check which methods are being implemented on a Snap, which would result in a runtime error for this ⬆️.

And adding a version won't solve everything, cause we would have type-issue here to (here we need to pass origin, or we need to have another variant type for submitRequest with an optional origin, that won't scale well IMO..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants