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

Adding ability to create accounts directly from plugin UI #2

Closed
wants to merge 6 commits into from

Conversation

dafuga
Copy link
Contributor

@dafuga dafuga commented Jul 16, 2024

No description provided.

@dafuga dafuga force-pushed the account-creation branch 6 times, most recently from 4d4a511 to 33ac9c4 Compare July 18, 2024 02:59
@dafuga dafuga force-pushed the account-creation branch 18 times, most recently from 61c15ae to 1e06181 Compare July 19, 2024 20:40
@dafuga dafuga force-pushed the account-creation branch 6 times, most recently from 03bd2ff to 119fdeb Compare July 19, 2024 22:39
@@ -36,7 +37,7 @@
"@typescript-eslint/eslint-plugin": "^5.20.0",
"@typescript-eslint/parser": "^5.20.0",
"@wharfkit/mock-data": "^1.2.0",
"@wharfkit/session": "^1.4.0-rc3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing some type mismatch errors when using the newer session kit version.

Copy link
Member

@aaroncox aaroncox Jul 24, 2024

Choose a reason for hiding this comment

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

Yeah I wouldn't use a RC version for the release version. Those are subject to change and might have bugs. It might be required here though?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - it requires the RC version to work. The 1.4.0 release introduces the key lookup mechanism that this plugin requires.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What key lookup mechanism? 😅 I think I might have missed that part lol.

Copy link
Member

Choose a reason for hiding this comment

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

The web-renderer (1.4.0-rc6) and session (1.4.x) does automatic key lookups now if a wallet provides a public key method:

https://github.com/wharfkit/web-renderer/blob/75b432fc77a57b455ffc72ee5384a988a66b4d6b/src/ui/login/Permission.svelte#L37-L45

That automatically retrieves the key and then does a lookup, displaying the accounts you can login with.

image

The metamask plugin implements this method, so it just automatically does lookups.

async retrievePublicKey(chainId: Checksum256Type): Promise<PublicKey> {
await this.initialize()
if (!this.provider) {
throw new Error('Metamask not found')
}
const result = (await this.invokeSnap({
method: 'antelope_getPublicKey',
params: {chainId: String(chainId)},
})) as string
return PublicKey.from(result)
}

This part of the web-renderer might be a place we could inject some sort of plugin to create an account?

Copy link
Contributor Author

@dafuga dafuga Jul 26, 2024

Choose a reason for hiding this comment

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

Oh sweet! And for the account creation, we would need to display a button to trigger that somewhere, right? I was thinking that we could have the session kit handle displaying that button when a compatible account creation plugin is available.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally yeah - if a creation plugin was available, we could show it in a few different spots.

I don't think the account creation plugin UI was developed like that though - and instead I think it was meant to be manually triggered by the developer 🤔

So we may need some web-renderer changes.

Copy link
Contributor Author

@dafuga dafuga Aug 5, 2024

Choose a reason for hiding this comment

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

Yeah, right now you need to trigger it with the sessionKit.createAccount() method. And yeah, we probably need to make some changes to the web renderer to make the UX better on that front.

@dafuga dafuga marked this pull request as ready for review July 24, 2024 00:39
return context.permissionLevel
}

const activeKey = await this.retrievePublicKey(chain.id, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any value in deriving the owner and active key from different parts of the mnemonic.

Both of those keys are derived from the same place, and stored in the same place... so if one gets stolen the other will too.

For these accounts I think we should just use the same key for both. Maybe in the future we could add a feature to generate an owner key certificate or something, so that they'd actually have two separate keys.

So here it'd just:

  • Make the keys both default to index 0.
  • Remove the index parameter from retrievePublicKey (since its unused)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm you may be right.. it's good practice to use different keys but in this case since the keys can't be exported I guess there is no risk of the active key being leaked somehow 🤔

})
}

context.ui.prompt({
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't have to prompt this, the web-renderer should do this 🤔

Copy link
Contributor Author

@dafuga dafuga Jul 24, 2024

Choose a reason for hiding this comment

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

I could be missing something, but as far as I could tell, there was no way to have the web renderer prompt the user to select a permission while also giving him the option to create a new account so that's why I ended up with something like this.

@dafuga
Copy link
Contributor Author

dafuga commented Jul 27, 2024

Closing this PR since we decided that the account creation logic shouldn't be in the wallet plugin.

@dafuga dafuga closed this Jul 27, 2024
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