feat: Check connection method #539
Conversation
RezaRahemtola
force-pushed
the
reza/check-connection-method
branch
from
4edc74a
to
df45590
Compare
| @@ -19,6 +19,11 @@ const init = (): void => { | |||
| const response = await executor.addEstablish(name); | |||
| return response; | |||
| }, | |||
| async CheckConnection(name: string): Promise<unknown> { | |||
There was a problem hiding this comment.
Is a parameter of name necessary?
There was a problem hiding this comment.
yes, to check if the wallet is connected with the site that has this name, like the name passed in AddEstablish no ?
Or is it passed in AddEstablish just to be displayed in the popup while the value used to check the connection is the hostname ?
There was a problem hiding this comment.
Yes, the name variable passed in AddEstablish will only display the service name in the popup.
It is not needed for functions that simply check for connections.
| @@ -82,6 +82,9 @@ export class MessageHandler { | |||
| case 'ADD_ESTABLISH': | |||
| HandlerMethod.addEstablish(message, sendResponse); | |||
| break; | |||
| case 'CHECK_CONNECTION': | |||
| HandlerMethod.checkEstablished(message, sendResponse); | |||
There was a problem hiding this comment.
checkEstablished does not respond with an event when it succeeds.
It will only callback a failure event if the site is not registered.
You can create a new function to respond to the success event.
Also, in my opinion, the case when the wallet is locked should be taken into account.
A failure response of type WALLET_LOCKED should be added.
There was a problem hiding this comment.
Okay, I'll look into this.
btw, how do you test locally the extension before publishing it ?
Didn't tested this if I remember correctly 😅
There was a problem hiding this comment.
Aha, We didn't explain enough about running extensions.
When you run the yarn build command, it creates a dist directory inside the packages/adena-extension directory.
You can then register the result of the build command as an extension, and in the case of Chrome, you can load your extension after enabling developer mode at chrome://extensions 😄
There was a problem hiding this comment.
Fixed and tested locally - sending WALLET_LOCKED if needed, and ALREADY_CONNECTED response on success
There was a problem hiding this comment.
I think it would be better to define a new success type instead of ALREADY_CONNECTED.
ALREADY_CONNECTED returns a non-zero code and returns a message that can be confusing to the user.
What type of PR is this?
What this PR does:
Basic implementation of #525